:glob ✱ Assignee
	Description • 7 years ago

we need to morph backouts from the servo/ directory on integration/autoland into pull requests on the servo/servo github repository.

the current plan is to:
- listen for backout commits via pulse
- for each backout that touches servo/
  - create a pull request with highest priority
  - link the backed out pull-request and bug with the new pr

	Gregory Szorc [:gps]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8868023 [details]
servo-vcs-sync: fix PEP8 warnings due to long lines (bug 1365181)

https://reviewboard.mozilla.org/r/139600/#review145166

	Gregory Szorc [:gps]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8868024 [details]
servo-vcs-sync: create the wheelhouse in a shared location (bug 1365181)

https://reviewboard.mozilla.org/r/139602/#review145174

The wheelhouse is a cache. So I'd prefer if we could share the cache across multiple virtualenvs.

If there is anything to do here, it is probably to make the micro-envs actually use the wheelhouse. I think I forgot to port that feature to the new environment :(

	Gregory Szorc [:gps]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8868025 [details]
servo-vcs-sync: add mozautomation dependancy (bug 1365181)

https://reviewboard.mozilla.org/r/139604/#review145176

This would get an r+ if it didn't get bit rotted by dropping the prior commit.

	Gregory Szorc [:gps]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8868026 [details]
servo-vcs-sync: add configuration for servo-backout-pr (bug 1365181)

https://reviewboard.mozilla.org/r/139606/#review145178

	Gregory Szorc [:gps]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8868027 [details]
WIP servo-vcs-sync: extract hg_converted handling into own method (bug 1365181)

https://reviewboard.mozilla.org/r/139608/#review145182

::: vcssync/mozvcssync/servo.py:98
(Diff revision 1)
> +            on_hg_converted(body)
> +
> -            message.ack()
> +        message.ack()
> -            return
>  
> +    def on_hg_converted(body):

It is really weird to see a reference to a function that occurs later in the same scope. I know this works, but still.

Would you mind moving the callbacks to global functions? They exist in the main function mostly out of laziness.

	Gregory Szorc [:gps]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review145210

Mix of high and low level feedback.

I have some additional questions based on what you just said in IRC. But I'm guessing those will be answered in follow-up work.

Overall, I like the direction and code architecture. There's a ton of details we need to flush out. If I had one request for v2 it would be for more docs explaining the overall architecture. Since it is still in flux, trying to assess the implementation against the desired end state is a bit difficult!

::: vcssync/mozvcssync/github_pr.py:21
(Diff revision 1)
> +        self.token = os.getenv('GITHUB_TOKEN')
> +        if self.token is None:
> +            raise Exception('GITHUB_TOKEN environmental var is not defined')

Please don't use environment variables from library functions. The thing closest to the new process invocation should read environment variables then pass them around as regular variables. Or use config files.

::: vcssync/mozvcssync/github_pr.py:28
(Diff revision 1)
> +            raise Exception('GITHUB_TOKEN environmental var is not defined')
> +        self._fork_repo = None
> +        self._upstream_repo = None
> +
> +        # login to github
> +        self.github = github3.login(token=self.token)

Actually, we may just want to pass a github client instance into the function, as reusing clients can result in perf wins, so that's a behavior we want to encourage.

::: vcssync/mozvcssync/github_pr.py:33
(Diff revision 1)
> +        self.github = github3.login(token=self.token)
> +        self.user = str(self.github.user())
> +        logger.info('logged in to github as %s' % self.user)
> +
> +    def git(self, *command):
> +        command = command[0] if type(command[0]) is list else list(command)

Nit: can this be `isinstance(command[0], list)`? I almost never see `type(x) is ...`

::: vcssync/mozvcssync/github_pr.py:80
(Diff revision 1)
> +            if not os.path.exists(os.path.dirname(self.repo_path)):
> +                os.makedirs(os.path.dirname(self.repo_path))
> +            logger.info('cloning %s to %s' % (fork_repo.clone_url,
> +                                              self.repo_path))
> +            self.git('clone', fork_repo.clone_url, self.repo_path)
> +        os.chdir(self.repo_path)

chdir is evil. Set the cwd argument of invoked processes instead. Alternatively, I believe you could also set GIT_WORK_TREE and GIT_DIR so Git uses explicit working directory and .git directories, respectively.

::: vcssync/mozvcssync/github_pr.py:83
(Diff revision 1)
> +                                              self.repo_path))
> +            self.git('clone', fork_repo.clone_url, self.repo_path)
> +        os.chdir(self.repo_path)
> +
> +        # setup remote upstream
> +        if 'upstream' not in self.git_output('remote'):

Nit: this will fail in the edge case where `upstream` is a substring of another remote. Not that this will ever happen...

::: vcssync/mozvcssync/github_pr.py:106
(Diff revision 1)
> +            except:
> +                pass
> +        self.git('checkout', '-b', branch_name)
> +
> +        # apply changes
> +        command = ['apply', patch_file]

I'm nervous about the use of patch files. At the very least, the Mercurial side needs to ensure the correct `diff` config options are set or there could be data loss. The important one being git-style diffs.

This is why I prefer to avoid patches completely and operate at the object level. Here, we could avoid checkouts completely and just write Git objects.

::: vcssync/mozvcssync/github_pr.py:107
(Diff revision 1)
> +        if include_paths is not None:
> +            for path in include_paths:

for path in include_paths or []

(or do `include_paths = include_paths or []` at the top of the function)

::: vcssync/mozvcssync/github_pr.py:119
(Diff revision 1)
> +        temp_file = tempfile.NamedTemporaryFile(delete=False)
> +        try:

You can use NamedTemporaryFile as a context manager. Just remember to .flush() the file handle so other processes can see data.

::: vcssync/mozvcssync/github_pr.py:136
(Diff revision 1)
> +
> +        # insert user and token into push url
> +        u = urlparse.urlsplit(fork_repo.clone_url)
> +        url = re.sub(r'^//', '',
> +                     urlparse.urlunsplit(['', u.netloc, u.path, u.query, '']))
> +        push_auth_url = 'https://%s:%s@%s' % (urllib.quote(self.user),

Must we put a private token in the URL? Surely there's a way we can configure Git to answer with that token for certain URLs?

::: vcssync/mozvcssync/servo_backout.py:31
(Diff revision 1)
> +MOZREVIEW_COMMIT_ID_RE = re.compile(
> +    r'^MozReview-Commit-ID: .+$',
> +    re.MULTILINE
> +)

I'm pretty sure we have code for this in mozautomation.

::: vcssync/mozvcssync/servo_backout.py:46
(Diff revision 1)
> +    # Initialise integration repo.
> +    if not os.path.exists(integration_repo_path):
> +        logger.warn('%s does not exist; cloning %s' % (integration_repo_path,
> +                                                       integration_repo_url))
> +        subprocess.check_call([hglib.HGPATH, b'clone', b'--noupdate',
> +                               b'--pull', integration_repo_url,

Why did you add --pull here? For local testing?

::: vcssync/mozvcssync/servo_backout.py:61
(Diff revision 1)
> +                            'specified' % tracking_file)
> +        with open(tracking_file) as f:
> +            starting_revision = f.read().strip()
> +
> +    # Iterate over each revision
> +    os.chdir(integration_repo_path)

Why do you chdir here? hglib should work without it.

::: vcssync/mozvcssync/servo_backout.py:65
(Diff revision 1)
> +    # Iterate over each revision
> +    os.chdir(integration_repo_path)
> +    github_pr = None
> +    processed_revision = None
> +    with hglib.open(integration_repo_path, 'utf-8') as hg_repo:
> +        for commit in hg_repo.log('%s:' % starting_revision, files=['servo/']):

This wants the :: operator, which follows the DAG (unlike : which follows storage order).

::: vcssync/mozvcssync/servo_backout.py:66
(Diff revision 1)
> +            node = commit[0]
> +            author = commit[4]
> +            desc = commit[5]

Does hglib not return a named tuple? Boo if not.

::: vcssync/mozvcssync/servo_backout.py:87
(Diff revision 1)
> +                raise Exception('failed to find merge id in %s'
> +                                % backed_out_node)

I'm not sure what your plans are for dealing with this, but since this error is fatal and there's no way to advanced the last processed changeset, this will permanently break the backout service.

As long as every changeset touching servo/ is a well-formed merge or backout, this won't come into play. Can we guarantee that?

::: vcssync/mozvcssync/servo_backout.py:91
(Diff revision 1)
> +            backed_out_url = '%s/pull/%s' % (github_pr.upstream_repo().html_url,
> +                                             backed_out_pr)

This /may/ fail... because if the source repo for the PR has been deleted, I believe GitHub's API stops reporting certain details about it. I can't remember specifics. But this is something we may want to test.

::: vcssync/mozvcssync/servo_backout.py:127
(Diff revision 1)
> +
> +        # PR created, mark tip as processed
> +        processed_revision = hg_repo.log(limit=1)[0][0]
> +
> +    if processed_revision:
> +        with open(tracking_file, b'w') as f:

Nit: you shouldn't need the b'' here.

::: vcssync/mozvcssync/servo_backout.py:130
(Diff revision 1)
> +
> +    if processed_revision:
> +        with open(tracking_file, b'w') as f:
> +            f.write(b'%s\n' % processed_revision)
> +
> +        # TODO so hacky. Relies on credentials in the environment.

heh. At some point we'll need to do this properly.

	:glob ✱ Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review145210

> Why did you add --pull here? For local testing?

it's there because the current code applies patches.

> Why do you chdir here? hglib should work without it.

i definitely hit a command that didn't work without chdir at some point during development (`hg files` maybe, don't recall exactly).  will check if it's still needed.

> I'm not sure what your plans are for dealing with this, but since this error is fatal and there's no way to advanced the last processed changeset, this will permanently break the backout service.
> 
> As long as every changeset touching servo/ is a well-formed merge or backout, this won't come into play. Can we guarantee that?

every changeset should be one or the other, however as long as we allow non-automation to touch servo/ we can't guarentee.

however this is only used for information purposes - so the original PR is linked to the backout.  it should be safe to change this to a warning.

> This /may/ fail... because if the source repo for the PR has been deleted, I believe GitHub's API stops reporting certain details about it. I can't remember specifics. But this is something we may want to test.

the only place this is used is in the commit description of the backout; if it's no longer valid there's no harm.

> Nit: you shouldn't need the b'' here.

pycharm's typechecker inspection throws a warning without it; unicode_literals is enabled, and open() expects a str.

	:glob ✱ Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review145210

> i definitely hit a command that didn't work without chdir at some point during development (`hg files` maybe, don't recall exactly).  will check if it's still needed.

it's still needed.  here's an example of the command:

~$ hg log -R ~/Desktop/servo-pr/autoland -r '3f985817021e::' --template '{rev}\n' servo/
abort: servo/ not under root '/Users/byron/Desktop/servo-pr/autoland'
(consider using '--cwd Desktop/servo-pr/autoland')

servo/ is a subdir under the specified root path.
hg's output suggests using --cwd, however that doesn't appear to be available when using the command server.

this command works when cwd is set:

~/Desktop/servo-pr/autoland$ hg log -R ~/Desktop/servo-pr/autoland -r '3f985817021e::' --template '{rev}\n' servo/
358354
358366
358375
...

	:glob ✱ Assignee
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review145210

> Must we put a private token in the URL? Surely there's a way we can configure Git to answer with that token for certain URLs?

the other viable method is to switch to ssh and use cert auth  (~/.netrc works, but that's a single per-user file not per-repo).

as a token is already configured and deployed for linearize this is the path of least resistance and simplifies the setup should anyone else consume this lib.

	:glob ✱ Assignee
	Comment 35 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review145210

> Actually, we may just want to pass a github client instance into the function, as reusing clients can result in perf wins, so that's a behavior we want to encourage.

as per our chat a while ago we need the token in order to authenticate this would mean consumers would have to provide both the token and a github3 object.  i'll drop this, but keep this in mind if github login ends up being slower than expected.

	Gregory Szorc [:gps]
	Comment 62 • 7 years ago
mozreview-review

Comment on attachment 8868024 [details]
servo-vcs-sync: create the wheelhouse in a shared location (bug 1365181)

https://reviewboard.mozilla.org/r/139602/#review148836

	Gregory Szorc [:gps]
	Comment 63 • 7 years ago
mozreview-review

Comment on attachment 8873474 [details]
testing: add hgext to vcssync and hgdev test suite (bug 1365181)

https://reviewboard.mozilla.org/r/144848/#review148840

::: testing/vcttesting/testing.py:45
(Diff revision 1)
> +    'hgext': {
> +        'venvs': {'hgdev', 'vcssync',},
> +    },

First, this dict is an alphabetical order.

More importantly, this dict defines directories containing Python unit tests to be executed with nose. I'm pretty sure that all tests in hgext/ are executed with Mercurial's test harness, not nose.

	Gregory Szorc [:gps]
	Comment 64 • 7 years ago
mozreview-review

Comment on attachment 8868025 [details]
servo-vcs-sync: add mozautomation dependancy (bug 1365181)

https://reviewboard.mozilla.org/r/139604/#review148842

::: ansible/roles/vcs-sync/defaults/main.yml:3
(Diff revision 5)
>  ---
>  mozvcssync_wheel: mozvcssync-0.1-py2-none-any.whl
> +mozautomation_wheel: mozautomation-0.2-py2-none-any.whl

The version number here is a bit dangerous because the following can happen:

1. Deploy current code, which generates a 0.2 wheel
2. Bump version to 0.3 in setup.py
3. Generate 0.3 wheel
4. Deploy picks up 0.2 wheel because we forgot to update this version

Can we get a comment in setup.py alerting us that we need to update this reference when the version is bumped?

Alternatively, we could potential do some version number trickery to avoid the multiple references. But that's scope bloat.

	Gregory Szorc [:gps]
	Comment 65 • 7 years ago
mozreview-review

Comment on attachment 8868026 [details]
servo-vcs-sync: add configuration for servo-backout-pr (bug 1365181)

https://reviewboard.mozilla.org/r/139606/#review148844

	Gregory Szorc [:gps]
	Comment 66 • 7 years ago
mozreview-review

Comment on attachment 8870458 [details]
servo-vcs-sync: add extra_data param to pulse consumers (bug 1365181)

https://reviewboard.mozilla.org/r/141898/#review148850

	Gregory Szorc [:gps]
	Comment 67 • 7 years ago
mozreview-review

Comment on attachment 8870459 [details]
servo-vcs-sync: pass config object as extra_data to pulse consumers (bug 1365181)

https://reviewboard.mozilla.org/r/141900/#review148852

	Gregory Szorc [:gps]
	Comment 68 • 7 years ago
mozreview-review

Comment on attachment 8870460 [details]
servo-vcs-sync: move pulse callbacks to global functions (bug 1365181)

https://reviewboard.mozilla.org/r/141902/#review148854

	Gregory Szorc [:gps]
	Comment 69 • 7 years ago
mozreview-review

Comment on attachment 8870461 [details]
servo-vcs-sync: extract hg_converted handling into own method (bug 1365181)

https://reviewboard.mozilla.org/r/141904/#review148856

::: vcssync/mozvcssync/servo.py:83
(Diff revision 4)
>      if repo_url != config['hg_converted']:
> +        on_hg_converted(body)

Calling the function when the current repo isn't the hg converted repo?

	Gregory Szorc [:gps]
	Comment 70 • 7 years ago
mozreview-review

Comment on attachment 8872893 [details]
servo-vcs-sync: add servo-backout-pr services (bug 1365181)

https://reviewboard.mozilla.org/r/144422/#review148858

	Gregory Szorc [:gps]
	Comment 71 • 7 years ago
mozreview-review

Comment on attachment 8873475 [details]
overlay: add test for revsets (bug 1365181)

https://reviewboard.mozilla.org/r/144850/#review148860

This isn't a great test because it isn't testing incremental overlay, which is the scenario we're using the feature for. But it's better than nothing. So r+.

	Gregory Szorc [:gps]
	Comment 72 • 7 years ago
mozreview-review

Comment on attachment 8873431 [details]
servo-vcs-sync: add --source_revs to overlay-hg-repos (bug 1365181)

https://reviewboard.mozilla.org/r/144840/#review148862

	Gregory Szorc [:gps]
	Comment 73 • 7 years ago
mozreview-review

Comment on attachment 8873432 [details]
servo-vcs-sync: set source_revs to the backout author (bug 1365181)

https://reviewboard.mozilla.org/r/144842/#review148864

	Gregory Szorc [:gps]
	Comment 74 • 7 years ago
mozreview-review

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review149438

This generally looks pretty good!

I need to spend some more time on the Git commands. But this certainly looks functional and in line with what we (mostly you) devised.

::: vcssync/mozvcssync/cli.py:301
(Diff revision 5)
> +    logging.getLogger('mozvcssync.servo_backout').setLevel(logging.INFO)
> +    logging.getLogger('mozvcssync.github_pr').setLevel(logging.INFO)

Can we not set the level universally for all of `mozvcssync`?

::: vcssync/mozvcssync/github_pr.py:1
(Diff revision 5)
> +from __future__ import unicode_literals

Missing license header.

::: vcssync/mozvcssync/github_pr.py:16
(Diff revision 5)
> +class GitCommand(object):
> +    """Helper class for running git commands"""
> +    def __init__(self, repo_path, token=None):
> +        self.repo_path = repo_path
> +        self.token = token
> +
> +    def cmd(self, *command):
> +        command = command[0] if isinstance(command[0], list) else list(command)
> +        command = ['git', '-C', self.repo_path] + command
> +        command_str = ' '.join(command)
> +        if self.token:
> +            command_str = command_str.replace(self.token, 'xxx')
> +        logger.info('$ %s' % command_str)
> +        subprocess.check_call(command)
> +
> +    def get(self, *command):
> +        command = command[0] if isinstance(command[0], list) else list(command)
> +        command = ['git', '-C', self.repo_path] + command
> +        return subprocess.check_output(command)

This should go in gitutil.py.

::: vcssync/mozvcssync/github_pr.py:24
(Diff revision 5)
> +        self.repo_path = repo_path
> +        self.token = token
> +
> +    def cmd(self, *command):
> +        command = command[0] if isinstance(command[0], list) else list(command)
> +        command = ['git', '-C', self.repo_path] + command

It's probably best to execute the process with cwd set instead of using -C.

::: vcssync/mozvcssync/github_pr.py:77
(Diff revision 5)
> +            if self._upstream_repo is None:
> +                raise Exception('failed to find github repo: %s/%s'
> +                                % (self.upstream_user, self.repo_name))
> +        return self._upstream_repo
> +
> +    def create_pr(self,

Let's rename this create_pr_from_patch.

::: vcssync/mozvcssync/github_pr.py:128
(Diff revision 5)
> +        # Setup remote upstream.
> +        if 'upstream' not in git.get('remote').splitlines():
> +            git.cmd('remote', 'add', 'upstream', upstream_repo.clone_url)

This isn't strictly necessary. You can do something like `git fetch <URL> +refs/heads/master:refs/upstream/master` to fetch a specific ref from a URL and map it to a local ref. This avoids complexity for managing remotes. It also avoids fetching refs not relevant to the operation at hand.

In some of the Git code I'm writing for pull request ingest, I actually generate a UUID and use that as the ref name because the actual names don't mean anything: they are just human-friendly labels (that also exist for garbage collection reasons).

::: vcssync/mozvcssync/github_pr.py:134
(Diff revision 5)
> +        if 'upstream' not in git.get('remote').splitlines():
> +            git.cmd('remote', 'add', 'upstream', upstream_repo.clone_url)
> +
> +        # reset
> +        git.cmd('checkout', 'master')
> +        git.cmd('reset', '--hard', 'origin/master')

`origin` here assumes the original clone `origin` is the same as the current one, which may not always be true.

::: vcssync/mozvcssync/github_pr.py:149
(Diff revision 5)
> +        if not reuse_branch and git.get('branch', '--list', branch_name):
> +            git.cmd('branch', '--delete', '--force', branch_name)
> +            # noinspection PyBroadException
> +            try:
> +                git.cmd('push', 'origin', '--delete', branch_name)
> +            except:

Why bareword except? Do you need to swallow KeyboardInterrupt and SystemExit? 99% of the time you want `except Exception`.

::: vcssync/mozvcssync/servo.py:97
(Diff revision 5)
> +    """Trigger backout service when a commit is backed out of the integration
> +    repo."""

Strictly speaking, this won't just fire for just backouts: it fires for all changes to the repo.

It's still fine to trigger the service: it just no-ops if nothing to do. But the comments should be clear what's going on here.

::: vcssync/mozvcssync/servo_backout.py:134
(Diff revision 5)
> +                            'specified' % tracking_file)
> +        with open(tracking_file) as f:
> +            starting_revision = f.read().strip()
> +        # As the tracking file stores the last processed revision, we need
> +        # work on its children.
> +        starting_revision = 'children(%s)' % starting_revision

Are you asserting there is only a single child? If so, that assertion may not be true.

::: vcssync/mozvcssync/servo_backout.py:139
(Diff revision 5)
> +        starting_revision = 'children(%s)' % starting_revision
> +
> +    configs = ['ui.interactive=False']
> +    with hglib.open(integration_repo_path, 'utf-8', configs) as hg_repo:
> +        # Update repo.
> +        hg_repo.update()

Always pass a revision to update, even if it is `default` or `tip`. Semantics for the default update target are not static and have changed in Mercurial versions.

Note: this may require you to look up the revision on the remote. You can use `hg -R <URL> identify` for that IIRC.

::: vcssync/mozvcssync/servo_backout.py:161
(Diff revision 5)
> +            for backed_out_node in nodes:
> +                pr_url = _create_pr(integration_repo_path, hg_repo, github_pr,
> +                                    backed_out_node, commit.desc, commit.node,
> +                                    author)

So, this means if we're backing out multiple commits that we'll create the PR then update it N times in rapid order. This works. But there may be consequences, such as entities listening for PRs getting overwhelmed (e.g. scheduling multiple runs of automation against each PR update).

::: vcssync/mozvcssync/servo_backout.py:172
(Diff revision 5)
> +                                                           pr_url))
> +
> +        # PRs created, mark tip as processed.
> +        processed_revision = hg_repo.log(limit=1)[0][0]
> +
> +    if processed_revision:

We currently ignore commits that don't have a backout. So in the common case where there is a push that isn't a servo backout, we never advance the last processed revision. This leads to redundant work.

I think we should advance the pointer so it is always current.

	:glob ✱ Assignee
	Comment 75 • 7 years ago
mozreview-review-reply

Comment on attachment 8873474 [details]
testing: add hgext to vcssync and hgdev test suite (bug 1365181)

https://reviewboard.mozilla.org/r/144848/#review148840

> First, this dict is an alphabetical order.
> 
> More importantly, this dict defines directories containing Python unit tests to be executed with nose. I'm pretty sure that all tests in hgext/ are executed with Mercurial's test harness, not nose.

get_test_files() uses UNIT_TEST_DIRS to find both types of tests; populating both unit_test (test*.py) and hook_tests (test*.t).

	:glob ✱ Assignee
	Comment 76 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review149438

> We currently ignore commits that don't have a backout. So in the common case where there is a push that isn't a servo backout, we never advance the last processed revision. This leads to redundant work.
> 
> I think we should advance the pointer so it is always current.

`processed_revision` is always set even if no backouts were found.  i'll remove the redundant and misleading `if processed_revision`.

	:glob ✱ Assignee
	Comment 77 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review149438

> `origin` here assumes the original clone `origin` is the same as the current one, which may not always be true.

i put some time against this, but it looks like it's trickier that i expected assuming we don't want to get into the "managing remotes" game.  this isn't a problem for our automation, so i'll drop this and add a comment that if the origin is changed after cloning things may break.

> Are you asserting there is only a single child? If so, that assertion may not be true.

within the context of servo backouts this shouldn't be a problem - we're only supporting backouts on the autoland repo.

	Gregory Szorc [:gps]
	Comment 91 • 7 years ago
mozreview-review-reply

Comment on attachment 8873474 [details]
testing: add hgext to vcssync and hgdev test suite (bug 1365181)

https://reviewboard.mozilla.org/r/144848/#review148840

> get_test_files() uses UNIT_TEST_DIRS to find both types of tests; populating both unit_test (test*.py) and hook_tests (test*.t).

Please read https://hg.mozilla.org/hgcustom/version-control-tools/rev/d1f6cc40b04b and let me know if things make more sense.

	Gregory Szorc [:gps]
	Comment 92 • 7 years ago
mozreview-review

Comment on attachment 8870461 [details]
servo-vcs-sync: extract hg_converted handling into own method (bug 1365181)

https://reviewboard.mozilla.org/r/141904/#review153122

	Gregory Szorc [:gps]
	Comment 93 • 7 years ago
mozreview-review

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153270

I haven't yet reviewed `github_pr.py`. I may or may not get to that tonight and wanted to make sure I left you with some review to act on.

::: vcssync/mozvcssync/gitutil.py:31
(Diff revision 6)
> +        self.repo_path = repo_path
> +        self.logger = logger
> +        self.secret = secret
> +
> +    def cmd(self, *command):
> +        command = command[0] if isinstance(command[0], list) else list(command)

This employs just enough magic to warrant a comment, preferably in a docstring. Unless you need to use `*args`, I'd just accept a single argument which is an iterable of command arguments. Although I do concede that `*args` makes code more concise at the call site.

::: vcssync/mozvcssync/gitutil.py:34
(Diff revision 6)
> +
> +    def cmd(self, *command):
> +        command = command[0] if isinstance(command[0], list) else list(command)
> +        command = ['git'] + command
> +        if self.logger:
> +            command_str = ' '.join(command)

`command_str = ' '.join(map(pipes.quote, command))`

If you do that, it should be possible to copy and paste the command, even if arguments contain whitespace, etc.

::: vcssync/mozvcssync/servo_backout.py:46
(Diff revision 6)
> +    r'^servo: Merge #([0-9]+)'
> +)
> +
> +
> +def _find_backout_commits(hg_repo, starting_revision):
> +    LightWeightCommit = namedtuple('LightWeightCommit', 'node desc author')

TIL the 2nd value accepts a space delimited list of fields. Can you change that to a tuple?

Also, please move the type assignment outside of the function, otherwise you create a type on every invocation and the types will actually be different! I don't think this really matters in this use case. But if it ever does, you don't want to track down that bug :)

::: vcssync/mozvcssync/servo_backout.py:48
(Diff revision 6)
> +
> +
> +def _find_backout_commits(hg_repo, starting_revision):
> +    LightWeightCommit = namedtuple('LightWeightCommit', 'node desc author')
> +    commits = []
> +    for commit in hg_repo.log('%s::' % starting_revision,

Per video conversation, this wants a `- merge()` because we never care about merges.

::: vcssync/mozvcssync/servo_backout.py:52
(Diff revision 6)
> +                                             commit.desc.decode('utf-8'),
> +                                             commit.author.decode('utf-8')))

Why the convert to Unicode? Can't we keep as bytes?

::: vcssync/mozvcssync/servo_backout.py:61
(Diff revision 6)
> +    return commits
> +
> +
> +def _find_backed_out_urls(hg_repo, github_url, commit_desc, commit_node):
> +    backed_out_urls = []
> +    revs, bugs = commitparser.parse_backouts(commit_desc)

We do this parsing twice. Do you think it is worth storing result from `_find_backout_commits` in the returned data structure?

::: vcssync/mozvcssync/servo_backout.py:91
(Diff revision 6)
> +        assert filename.startswith('servo/')
> +        return filename[len('servo/'):]
> +
> +    # Grab a list of files under the servo/ directory touched by this
> +    # change.
> +    args = cmdbuilder('log', template='{join(files,"\n")}',

Can `\0` be used as a separator?

::: vcssync/mozvcssync/servo_backout.py:115
(Diff revision 6)
> +    if len(github_repo_name.split('/')) != 2:
> +        raise Exception('"%s" is not in the form "user/repo"'
> +                        % github_repo_name)

You could do `user, repo = github_repo_name.split('/')` and count on that to raise an exception if it doesn't split into exactly 2 pieces.

::: vcssync/mozvcssync/servo_backout.py:134
(Diff revision 6)
> +    if not starting_revision:
> +        # We never want to walk all commits.
> +        if not os.path.exists(tracking_file):
> +            raise Exception('%s does not exist, and a revision was not '
> +                            'specified' % tracking_file)
> +        with open(tracking_file) as f:

I always forget what the default mode to `open()` is. So please specify it. And make the mode `rb`, which is compatible with Python 3. (`r` in Python 3 opens in text mode which involves Unicode - eww.)

::: vcssync/mozvcssync/servo_backout.py:140
(Diff revision 6)
> +            starting_revision = f.read().strip()
> +        # As the tracking file stores the last processed revision, we need
> +        # work on its children.
> +        # This assumes that backouts to servo files will only happen on the
> +        # integration/autoland repository.
> +        starting_revision = 'children(%s)' % starting_revision

Per video chat, I think I convinced you to use `{{rev}}:: - {{rev}}`.

::: vcssync/mozvcssync/servo_backout.py:142
(Diff revision 6)
> +        # work on its children.
> +        # This assumes that backouts to servo files will only happen on the
> +        # integration/autoland repository.
> +        starting_revision = 'children(%s)' % starting_revision
> +
> +    configs = ['ui.interactive=False']

Out of curiosity, what prompted you to add this?

::: vcssync/mozvcssync/servo_backout.py:147
(Diff revision 6)
> +    configs = ['ui.interactive=False']
> +    with hglib.open(integration_repo_path, 'utf-8', configs) as hg_repo:
> +        # Update repo if required.
> +        local_tip = hg_repo.identify(id=True).strip()
> +        remote_tip = subprocess.check_output([
> +            hglib.HGPATH, 'identify', integration_repo_url]).strip()

Please humor me by adding `-r tip` to this.

::: vcssync/mozvcssync/servo_backout.py:150
(Diff revision 6)
> +        local_tip = hg_repo.identify(id=True).strip()
> +        remote_tip = subprocess.check_output([
> +            hglib.HGPATH, 'identify', integration_repo_url]).strip()
> +        if local_tip != remote_tip:
> +            logger.info('updating %s to %s' % (integration_repo_path, remote_tip))
> +            hg_repo.pull(source=integration_repo_url)

Since we only need to use remote tip, let's only pull that revision.

::: vcssync/mozvcssync/servo_backout.py:156
(Diff revision 6)
> +        if backout_commits:
> +            github_pr = GitHubPR(github_token, github_repo_name,
> +                                 github_repo_path)

Consider pulling this block into a separate function to aid readability of this long function.

::: vcssync/mozvcssync/servo_backout.py:182
(Diff revision 6)
> +                # Checkout the backout commit to ensure we're copying the
> +                # correct file contents.
> +                hg_repo.update(rev=commit.node)

Somewhere in here we'll also need to purge/clean the working directory. Call `clean_hg_repo()` in util.py.

::: vcssync/mozvcssync/servo_backout.py:193
(Diff revision 6)
> +                        if not os.path.exists(source_file):
> +                            logger.info('removing %s' % filename)
> +                            try:
> +                                os.unlink(dest_file)
> +                            except OSError as e:
> +                                # ignore FileNotFound
> +                                if e.errno != errno.ENOENT:
> +                                    raise
> +                        else:
> +                            logger.info('updating %s' % filename)
> +                            shutil.copy(source_file, dest_file)

I feel like there is a bug involving deleted/added files here. I need to think about this some more. Keep in mind that the `{files}` returned by `hg log` is a union of all modifies, adds, deletes, copies, and renames (copies and deletes being special cases of adds and deletes).

Please be sure to write tests covering file add, remove, copy, and rename scenarios.

::: vcssync/mozvcssync/servo_backout.py:215
(Diff revision 6)
> +        # PRs created, mark tip as processed.
> +        processed_revision = hg_repo.log(limit=1)[0].node

We never commit to the hg repo. So I think it is semantically better to set this to `remote_tip`. If so, do we need this variable at all?

::: vcssync/mozvcssync/servo_backout.py:219
(Diff revision 6)
> +
> +        # PRs created, mark tip as processed.
> +        processed_revision = hg_repo.log(limit=1)[0].node
> +
> +    if processed_revision != starting_revision:
> +        with open(tracking_file, b'w') as f:

`'wb'`

You don't need to pass bytes to the mode argument. And I think this will actually fail on Python 3!

	:glob ✱ Assignee
	Comment 94 • 7 years ago
mozreview-review-reply

Comment on attachment 8873474 [details]
testing: add hgext to vcssync and hgdev test suite (bug 1365181)

https://reviewboard.mozilla.org/r/144848/#review148840

> Please read https://hg.mozilla.org/hgcustom/version-control-tools/rev/d1f6cc40b04b and let me know if things make more sense.

you said "it isn't necessary to list directories here that are scanned for Mercurial tests".
this isn't correct.  get_test_files() uses UNIT_TEST_DIRS *only* to find tests, and with the vcssync env active, hgext currently isn't scanned for tests.

i want to be able to run hgext tests with the vcssync venv active using run-tests.  this isn't possible without this change.

unpatched:
$ ./run-tests hgext/overlay/tests/test-overlay-basic.t 
......
this is running the whole test suite, which does not include hgext/overlay/tests

patched:
$ ./run-tests hgext/overlay/tests/test-overlay-basic.t
.
(runs just the test i requested)

	Gregory Szorc [:gps]
	Comment 95 • 7 years ago
mozreview-review

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153300

Still need to look at this in more detail. My eyes are starting to glaze over because I'm tired. But some useful feedback before I sleep.

::: vcssync/mozvcssync/github_pr.py:36
(Diff revision 6)
> +        self.github = github or github3.GitHub()
> +        self.github = github3.login(token=self.token)

Eh?

::: vcssync/mozvcssync/github_pr.py:150
(Diff revision 6)
> +        git.cmd('checkout', 'master')
> +        # Note this assumes the origin remote hasn't been changed since the
> +        # initial clone.
> +        git.cmd('reset', '--hard', 'origin/master')

This does 2 checkouts. First, it updates the working directory to the `master` branch then it forcefully resets `master` to `origin/master` and updates the working directory to match. This could be quite expensive.

Try running `git update-ref` to forcefully set local `master` to `origin/master` then do `git checkout master` to do a working directory update.

::: vcssync/mozvcssync/github_pr.py:157
(Diff revision 6)
> +        # initial clone.
> +        git.cmd('reset', '--hard', 'origin/master')
> +        git.cmd('clean', '-d', '--force')
> +
> +        # update master
> +        git.cmd('fetch', upstream_repo.clone_url, 'master:refs/upstream/master')

The refspec should be prefixed with `+` to allow non-fast-forward changes. In other words, allow any changes on the remote to be reflected to the local ref. Basically a force fetch.

::: vcssync/mozvcssync/github_pr.py:192
(Diff revision 6)
> +
> +        if git.get('status', '--short'):
> +
> +            # Commit changes.
> +            with tempfile.NamedTemporaryFile() as temp_file:
> +                temp_file.write(description.encode('utf-8'))

Not sure why this variable is unicode.

	:glob ✱ Assignee
	Comment 96 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153270

> Why the convert to Unicode? Can't we keep as bytes?

needs to be unicode so it can be treated correctly later on (eg. when writing to a file for the git commit desc).

> We do this parsing twice. Do you think it is worth storing result from `_find_backout_commits` in the returned data structure?

i don't think it's worth doing that.

the performance of parse_backouts() is pretty good, keeping these split makes testing easier, and it would be clumsy to do avoid the duplicate call because parse_backouts() returns None when there's no match.

> Can `\0` be used as a separator?

it could, but i don't see the value.  \n makes this easier to debug, and filenames won't contain \n.

> You could do `user, repo = github_repo_name.split('/')` and count on that to raise an exception if it doesn't split into exactly 2 pieces.

i think this comes down to style.  i prefer to avoid using exceptions when a simple comparison will do.
doubly so in this case where we just do validation here; the split values are used in another class.

> Per video chat, I think I convinced you to use `{{rev}}:: - {{rev}}`.

i can't use that exact syntax:

$ hg log -r '6010f4b0f4fd:: - 6010f4b0f4fd'
abort: unknown revision '-6010f4b0f4fd'!

looks like it's parsing the negation as a revision.  explicitly defining the default value fort the second part of the dag range works:

hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd'

> Out of curiosity, what prompted you to add this?

to guard against hg blocking waiting for stdin.  this should never happen with the current commands, but i see value in raising an error should that happen instead of blocking.

> `'wb'`
> 
> You don't need to pass bytes to the mode argument. And I think this will actually fail on Python 3!

passing in str here is to keep linting happy; it's complaining that the definition for open() is (unicode, str), while i'm passing in (unicode, unicode) due to unicode_literals.

you're right that this will fail on py3, so i'll change to 'wb'.
i'll add a `noinspection PyTypeChecker` override to this and other open statements to keep pycharm happy.

	:glob ✱ Assignee
	Comment 97 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153300

> Eh?

this makes testing with betamax possible.  will add a comment.

	:glob ✱ Assignee
	Comment 98 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153270

> Somewhere in here we'll also need to purge/clean the working directory. Call `clean_hg_repo()` in util.py.

will do, but to be honest i'm not sure why it's required as we don't make changes to the hg repo.

	Steven MacLeod [:smacleod]
	Comment 99 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153270

> i can't use that exact syntax:
> 
> $ hg log -r '6010f4b0f4fd:: - 6010f4b0f4fd'
> abort: unknown revision '-6010f4b0f4fd'!
> 
> looks like it's parsing the negation as a revision.  explicitly defining the default value fort the second part of the dag range works:
> 
> hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd'

Um, I thought you were attempting to identify a single revision with this (Since there will only ever be a single child)? The new revset will identify all descendants of the revision. In fact, `hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd'` should be the exact same as `hg log -r 'descendants(6010f4b0f4fd)'` (well, order *might* be different...)

	Steven MacLeod [:smacleod]
	Comment 100 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153300

> this makes testing with betamax possible.  will add a comment.

I think gps' is confused because there is a botched merge here with my changes. this should look something like:
```
self.github = github or github3.GitHub()
self.github.login(token=self.token)
```

you're calling login on `github3`, not `github`. The tests should fail with the state you currently have.

	:glob ✱ Assignee
	Comment 101 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153300

> I think gps' is confused because there is a botched merge here with my changes. this should look something like:
> ```
> self.github = github or github3.GitHub()
> self.github.login(token=self.token)
> ```
> 
> you're calling login on `github3`, not `github`. The tests should fail with the state you currently have.

oops; well spotted. fixed.

	Steven MacLeod [:smacleod]
	Comment 102 • 7 years ago
mozreview-review-reply

Comment on attachment 8868028 [details]
servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181)

https://reviewboard.mozilla.org/r/139610/#review153270

> This employs just enough magic to warrant a comment, preferably in a docstring. Unless you need to use `*args`, I'd just accept a single argument which is an iterable of command arguments. Although I do concede that `*args` makes code more concise at the call site.

Why not just have callers use `cmd(*im_a_list)` when they call `cmd()` with a pre-existing list?

	:glob ✱ Assignee
	Comment 119 • 7 years ago
mozreview-review

Comment on attachment 8878022 [details]
servo-vcs-sync: add basic testing for apply_changes_from_list (bug 1365181)

https://reviewboard.mozilla.org/r/149436/#review154050

::: vcssync/mozvcssync/util.py:170
(Diff revision 1)
> +if __name__ == '__main__':
> +    if not sys.argv:
> +        sys.exit()
> +
> +    if sys.argv[1] == 'apply_changes_from_list':
> +        logger = logging.getLogger('mozvcssync')
> +        logger.addHandler(logging.StreamHandler(sys.stdout))
> +        logger.setLevel(logging.INFO)
> +        source_path, dest_path, filelist = sys.argv[2::]
> +        apply_changes_from_list(logger,
> +                                source_path, dest_path, filelist.split(','))

yes, this is eww.

it's just something quick and dirty so i can push up the rest of the changes along with the simple test this supports.

i'll rework this to move it to a test-only cli script.

	:glob ✱ Assignee
	Comment 120 • 7 years ago
mozreview-review

Comment on attachment 8876807 [details]
vcssync: install nose in test environment (bug 1365181)

https://reviewboard.mozilla.org/r/148130/#review154286