Closed Bug 1401309 Opened 7 years ago Closed 7 years ago

[mozlint] Merge vcs.py into mozversioncontrol

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(5 files, 2 obsolete files)

Bug 1401309 - Add python/mozversioncontrol to flake8 linter,
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1401309 - [mozversioncontrol] Merge get_modified_files and get_added_files into a single function,
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1401309 - [mozversioncontrol] Add ability to get outgoing files,
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1401309 - [mozlint] Remove vcs.py and use mozversioncontrol instead,
59 bytes, text/x-review-board-request
gps
: review+
Details 
There's some version control abstraction logic in python/mozlint/vcs.py. We'd like for all stuff like this to live in python/mozversioncontrol instead. So we should move the logic in vcs.py to mozversioncontrol and refactor mozlint to use mozversioncontrol.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment on attachment 8910342 [details]
Bug 1401309 - Add python/mozversioncontrol to flake8 linter,

https://reviewboard.mozilla.org/r/181796/#review187164
Attachment #8910342 - Flags: review?(gps) → review+
Comment on attachment 8910343 [details]
Bug 1401309 - [mozversioncontrol] Use Repository.create() for instantiating repository objects,

https://reviewboard.mozilla.org/r/181798/#review187176

I'd like to see another round of review on this.

The API naming really comes down to preference. I'm -0 to changing the API, mostly because we still have other ``create_repository_*`` functions. I also like the explicitness of separate APIs specifying how the repo is created. If you attempt to factor in the other ``create_repository`` functions, I think you'll find that the complexity of ``Repository.create()`` quickly grows out of hand.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:69
(Diff revision 1)
>      """
>  
>      __metaclass__ = abc.ABCMeta
>  
> -    def __init__(self, path, tool):
> -        self.path = os.path.abspath(path)
> +    def __init__(self, root, tool):
> +        self.root = os.path.abspath(root)

I suspect this rename broke callers not changed in this patch. While I like the new name, it is safer to not change it.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:85
(Diff revision 1)
> +    @classmethod
> +    def find_vcs(cls):
> +        # First check if we're in an hg repo, if not try git
> +        commands = (
> +            ['hg', 'root'],
> +            ['git', 'rev-parse', '--show-toplevel'],
> +        )
> +
> +        for cmd in commands:
> +            proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +            output = proc.communicate()[0].strip()
> +
> +            if proc.returncode == 0:
> +                return cmd[0], output
> +        return 'none', ''

Why do we need this? Can we do it without launching a process?

Could this at least be factored off to its own commit so it doesn't pollute this refactor?
Attachment #8910343 - Flags: review?(gps) → review-
Comment on attachment 8910343 [details]
Bug 1401309 - [mozversioncontrol] Use Repository.create() for instantiating repository objects,

https://reviewboard.mozilla.org/r/181798/#review187176

I guess you're right. With some extra modifications to mozlint, I think this entire change can just be discarded.

> Why do we need this? Can we do it without launching a process?
> 
> Could this at least be factored off to its own commit so it doesn't pollute this refactor?

This is how mozlint.vcs finds the repo. It's not really necessary I guess, I wrote mozlint to be useable outside of mozilla-central so thought this would be a better way to automatically determine the repo root.. but now that it depends on mozversioncontrol, it's pretty tied to m-c anyway. Might as well just pass in topsrcdir.
Blocks: 1401199
Attachment #8910343 - Attachment is obsolete: true
This blocks the |mach try fuzzy| parameter mismatch fix, as fixing that requires using the "upstream" property added here.
Comment on attachment 8910344 [details]
Bug 1401309 - [mozversioncontrol] Merge get_modified_files and get_added_files into a single function,

https://reviewboard.mozilla.org/r/181800/#review188366
Attachment #8910344 - Flags: review?(gps) → review+
Comment on attachment 8910345 [details]
Bug 1401309 - [mozversioncontrol] Add ability to get outgoing files,

https://reviewboard.mozilla.org/r/181802/#review188374

I'm not crazy about how this is implemented because there are non-trivial differences between Mercurial and Git.

With Mercurial, we're actually doing exchange with a remote server and seeing what would be transferred.

With Git, we're looking at local state of refs to see what commits to examine.

I think we should consistently examine local state or consistently examine remote state.

Unfortunately, it is non-trivial to get this remote state from Git. `git ls-remote` can get you the refs on the remote. But you need to know all ancestors so you can traverse to find a common ancestor in order to identify the base of the "outgoing" range. I don't think you can get that via a `git` command. So local it is.

I think this should be implemented as 2 primitives:

1. Find base revision identifying the start of a "relevant" range of commits
2. Find stats about a range of commits (identified by a revset or revspec)

For Mercurial, "relevant" can be approximated via phases. For Git, walk ancestors until you find another commit with an attached ref, ideally a ref attached to the upstream/remote of the current branch.

Thoughts?

::: python/mozversioncontrol/mozversioncontrol/__init__.py:307
(Diff revision 3)
> +    def upstream(self):
> +        if self._upstream:
> +            return self._upstream

This functionality is too complicated for a property. Please turn this into a regular method.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:314
(Diff revision 3)
> +        if not dest:
> +            branches = self._run('branch', '--list')
> +            for b in ('master', 'central', 'default'):
> +                if b in branches and not ref.endswith(b):
> +                    dest = b
> +                    break

I'm really tempted to delete this and see who complains. Unless your Git config is berserk (branch.autoSetupMerge=false) or you explicitly tell Git to not set upstream/tracking info, an upstream should always be set.

At the very least, the set of default remotes shouldn't have "central" in it because the module is generic and is used outside Firefox.

I could also make the case that we should never guess the remote name because we have no clue what it actually is or the relationship to a repo we care about.
Attachment #8910345 - Flags: review?(gps) → review-
Comment on attachment 8910346 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181804/#review188382

This should be a named argument to `_run()`, not a global setting on the repo instance.
Attachment #8910346 - Flags: review?(gps) → review-
Comment on attachment 8910347 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181806/#review188388

This seems reasonable. But I'll wait for prerequisite patches before granting r+.
Attachment #8910347 - Flags: review?(gps)
Comment on attachment 8910346 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181804/#review188382

I'll need this to be controllable from consumers of Repository. So this change can be made, but it'll mean adding a `fatal` argument to every method that uses `_run()`.
Comment on attachment 8910346 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181804/#review188382

As the commit message says, only some commands may exit 1 and still be good. So I'm not sure why this detail is abstracted away in the implementation of each method that invokes a command.
Comment on attachment 8910345 [details]
Bug 1401309 - [mozversioncontrol] Add ability to get outgoing files,

https://reviewboard.mozilla.org/r/181802/#review188374

I think this makes sense.

For context, my main motivation for even filing this bug in the first place was to fix bug 1401199. Basically all I need is the `base` revision's parent from step 1. I'm pretty sure that's all `mozlint` needs as well. However, my git knoweldge is severely lacking so I'd be tempted to ask if this can be done in a follow-up.

> I'm really tempted to delete this and see who complains. Unless your Git config is berserk (branch.autoSetupMerge=false) or you explicitly tell Git to not set upstream/tracking info, an upstream should always be set.
> 
> At the very least, the set of default remotes shouldn't have "central" in it because the module is generic and is used outside Firefox.
> 
> I could also make the case that we should never guess the remote name because we have no clue what it actually is or the relationship to a repo we care about.

I'd be ok deleting it. Standard8 originally asked me to include it, but I can't imagine it gets hit very often.
Comment on attachment 8910346 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181804/#review188382

That's fair.

Mozlint's use case is that no matter what error comes out of vcs it should recover and attempt to keep going. But I'll solve this by catching `CalledProcessError` directly from mozlint.
Attachment #8910346 - Attachment is obsolete: true
Comment on attachment 8910345 [details]
Bug 1401309 - [mozversioncontrol] Add ability to get outgoing files,

https://reviewboard.mozilla.org/r/181802/#review188788

I think there's still room to improve the API and implementation. But this is a good start.
Attachment #8910345 - Flags: review?(gps) → review+
Comment on attachment 8910347 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181806/#review188792

::: python/mozversioncontrol/mozversioncontrol/__init__.py:96
(Diff revision 4)
> +            if e.output:
> +                print(' '.join(cmd))
> +                print(e.output)

I'm not a fan of using ``print()`` here. This is library code and ``print()`` should not be used in library code unless the caller explicitly instructed us to print.

Please fix this (possibly in a follow-up) so the behavior isn't enabled by default.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:266
(Diff revision 4)
>  
>      def get_changed_files(self, diff_filter='ADM', mode='unstaged'):
>          df = self._format_diff_filter(diff_filter)
>  
>          # Use --no-status to print just the filename.
> -        return self._run('status', '--no-status', '-{}'.format(df)).splitlines()
> +        return self._run('status', '--no-status', '-{}'.format(df), fatal=False).splitlines()

Under what circumstances is `hg status` expected to fail?

::: python/mozversioncontrol/mozversioncontrol/__init__.py:279
(Diff revision 4)
>          return self._run('outgoing', '-r', '.', '--quiet',
> -                         '--template', template, upstream).split()
> +                         '--template', template, upstream, fatal=False).split()

This should distinguish between ``1`` (no outgoing changesets) and ``255`` (general command failure). We don't want a general failure to result in this returning an empty set!

::: python/mozversioncontrol/mozversioncontrol/__init__.py:341
(Diff revision 4)
>          if mode == 'staged':
>              cmd.append('--cached')
>          elif mode == 'all':
>              cmd.append('HEAD')
>  
> -        return self._run(*cmd).splitlines()
> +        return self._run(*cmd, fatal=False).splitlines()

Under what circumstances is `git diff` expected to fail?

::: python/mozversioncontrol/mozversioncontrol/__init__.py:351
(Diff revision 4)
>          if upstream == 'default':
>              upstream = self.get_upstream()
>  
>          compare = '{}..HEAD'.format(upstream)
>          return self._run('log', '--name-only', '--diff-filter={}'.format(diff_filter.upper()),
> -                         '--oneline', '--pretty=format:', compare).splitlines()
> +                         '--oneline', '--pretty=format:', compare, fatal=False).splitlines()

Under what circumstances is ``git log`` expected to fail?
Attachment #8910347 - Flags: review?(gps) → review-
Comment on attachment 8910347 [details]
Bug 1401309 - [mozversioncontrol] Add an option to make failed subprocess commands non-fatal,

https://reviewboard.mozilla.org/r/181806/#review189630

I just realized that we don't redirect stderr in ``_run()``. That's not ideal. But we can deal with that another time.
Attachment #8910347 - Flags: review?(gps) → review+
Comment on attachment 8911981 [details]
Bug 1401309 - [mozlint] Remove vcs.py and use mozversioncontrol instead,

https://reviewboard.mozilla.org/r/183392/#review189634

I'm giving r+ because I'm about to get on a plane and likely will disappear for a while. But please look at the exception handling and make sure we don't silently succeed when we're not supposed to.

::: python/mozlint/mozlint/roller.py:120
(Diff revision 2)
> +            if e.output:
> +                print(' '.join(e.cmd))
> +                print(e.output)

I would expect an error running a VCS process to turn into an error here. Otherwise we may silently pass when we shouldn't.

::: taskcluster/ci/source-test/python.yml:171
(Diff revision 2)
> +    when:
> +        files-changed:
> +            - 'python/mozversioncontrol/**'
> +            - 'python/mach_commands.py'

Can we not use SCHEDULES.exclusive yet?
Attachment #8911981 - Flags: review?(gps) → review+
Comment on attachment 8911981 [details]
Bug 1401309 - [mozlint] Remove vcs.py and use mozversioncontrol instead,

https://reviewboard.mozilla.org/r/183392/#review189634

> Can we not use SCHEDULES.exclusive yet?

We can, but I'd rather convert this along with the rest of the tasks in this file. It's hard enough as it is to divine why some tasks run and others don't without adding another exception to the rule.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f2cb38633e49
Add python/mozversioncontrol to flake8 linter, r=gps
https://hg.mozilla.org/integration/autoland/rev/bb1af5a946f9
[mozversioncontrol] Merge get_modified_files and get_added_files into a single function, r=gps
https://hg.mozilla.org/integration/autoland/rev/46f18abdbe32
[mozversioncontrol] Add ability to get outgoing files, r=gps
https://hg.mozilla.org/integration/autoland/rev/f148c6eeb1d9
[mozversioncontrol] Add an option to make failed subprocess commands non-fatal, r=gps
https://hg.mozilla.org/integration/autoland/rev/4a92efa8e87d
[mozlint] Remove vcs.py and use mozversioncontrol instead, r=gps
Depends on: 1405588
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: