Mozlint has a --outgoing parameter for linting files touched by commits that would get pushed to mozilla-central. It's supposed to work with git too, but I've seen reports that it errors when using git-cinnabar. I want to start using this parameter in CI, vcs hooks and by default when no paths are specified, so it needs to be rock solid. This bug should add some tests for it too.

I see this currently: fatal: ambiguous argument 'origin/master..HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' Error running mach: ['lint', '--outgoing'] The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it. You should consider filing a bug for this issue. If filing a bug, please include the full output of mach, including this error message. The details of the failure are as follows: CalledProcessError: Command '['git', 'log', '--name-only', 'origin/master..HEAD']' returned non-zero exit status 128 File "/Users/mark/dev/gecko/tools/lint/mach_commands.py", line 43, in lint return cli.run(*runargs, **lintargs) File "/Users/mark/dev/gecko/python/mozlint/mozlint/cli.py", line 111, in run workdir=workdir) File "/Users/mark/dev/gecko/python/mozlint/mozlint/roller.py", line 121, in roll paths.extend(self.vcs.outgoing(outgoing)) File "/Users/mark/dev/gecko/python/mozlint/mozlint/vcs.py", line 67, in outgoing return self._run(['git', 'log', '--name-only', comparing]) File "/Users/mark/dev/gecko/python/mozlint/mozlint/vcs.py", line 47, in _run files = subprocess.check_output(cmd).split() File "/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 219, in check_output raise CalledProcessError(retcode, cmd, output=output)

Ah, I see the issue, it is trying to do: git log --name-only origin/master..HEAD however, git-cinnabar repositories don't tend to have an origin, and I think they might not always have a master. For instance, if you read through the guide for Mozilla, it doesn't say to create a master branch, or a particular branch: https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development Maybe a workaround would be to see if the branch has an upstream and use that, and if it doesn't fallback to assuming master exists.

In the ./mach lint --help, it says: "If you are using git please specify which remote you want to compare to." So does something like `./mach lint --outgoing mozilla/central` work? Maybe it's not broken, we just need to change the default.

(In reply to Andrew Halberstadt [:ahal] from comment #3) > So does something like `./mach lint --outgoing mozilla/central` work? Maybe > it's not broken, we just need to change the default. Yes, that works. I think we still might want to add some logic so that we get the defaults right in most instances for the hooks.

Agreed. I found: git for-each-ref --format='%(upstream:short)' $(git symbolic-ref -q HEAD) seems to do what we want in both the cinnabar and non-cinnabar cases. I have a bunch of --outgoing improvements queued up, I think I'll just tackled them all here.

Comment on attachment 8877274 [details] Bug 1369787 - [mozlint] Remove 'rev' option from |mach lint|, https://reviewboard.mozilla.org/r/148612/#review153490 Seems straight forward. ::: tools/lint/docs/usage.rst:27 (Diff revision 1) > > .. parsed-literal:: > > ./mach lint -l eslint path/to/files > > -Finally, ``mozlint`` can lint the files touched by a set of revisions or the working directory using > +Finally, ``mozlint`` can lint the files touched by a outgoing revisions or the working directory using s/a outgoing revisions/outgoing revisions/

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review153590 I think there are enough issues to revisit this one. ::: python/mozlint/mozlint/roller.py:103 (Diff revision 1) > :return: A dictionary with file names as the key, and a list of > :class:`~result.ResultContainer`s as the value. > """ > - paths = paths or [] > + paths = paths or set() > if isinstance(paths, basestring): > - paths = [paths] > + paths = set([paths]) We should check here that paths is input as a set and if it is a list, we should convert it to a set so we are consistent in its use later. ::: python/mozlint/mozlint/roller.py:112 (Diff revision 1) > > # Calculate files from VCS > if workdir: > - paths.extend(self.vcs.by_workdir()) > + paths.update(self.vcs.by_workdir()) > if outgoing: > - paths.extend(self.vcs.outgoing(outgoing)) > + paths.extend(self.vcs.by_outgoing(outgoing)) This will fail if paths is a set. We need to change it to be update once we make sure paths is a set. ::: python/mozlint/mozlint/roller.py:115 (Diff revision 1) > - paths.extend(self.vcs.by_workdir()) > + paths.update(self.vcs.by_workdir()) > if outgoing: > - paths.extend(self.vcs.outgoing(outgoing)) > + paths.extend(self.vcs.by_outgoing(outgoing)) > > paths = paths or ['.'] > paths = map(os.path.abspath, paths) Now we are a list again. This interchange between set and list is confusing. ::: python/mozlint/mozlint/vcs.py:58 (Diff revision 1) > +class HgHelper(VCSHelper): > + """A helper to find files to lint from Mercurial.""" > + > + def by_outgoing(self, dest='default'): > + return self.run(['hg', 'outgoing', '--quiet', '-r .', > + dest, '--template', '{files % "\n{file}"}']) Neat. ::: python/mozlint/mozlint/vcs.py:72 (Diff revision 1) > + def by_outgoing(self, dest='default'): > + if dest == 'default': > - comparing = 'origin/master..HEAD' > + comparing = 'origin/master..HEAD' > - else: > + else: > - comparing = '{}..HEAD'.format(destination) > - return self._run(['git', 'log', '--name-only', comparing]) > + comparing = '{}..HEAD'.format(dest) > + return self.run(['git', 'log', '--name-only', comparing]) In my local autophone tree where I have some patches queued up on a development branch I got: bclary@sophie /mozilla/projects/autophone/src/bclary-autophone (sisyphus-dev) $ git log --name-only origin/master..HEAD commit 68f50e2026e29e8b0b1af8a41df07ddc4ad86d5a Author: Bob Clary <bob@bclary.com> Date: Wed Jun 7 07:29:59 2017 -0700 Bug 1371291 - Autophone - use intent action to shutdown fennec and geckoview_example and eliminate quitter, r=jmaher. .gitignore .gitmodules files/base/initialize_profile.html files/ep1 files/s1s2/blank.html phonetest.py tests/s1s2geckoviewtest.py tests/s1s2test.py xpi/quitter.xpi commit 28e9c75f6f404eda1fefe409f5ceec1e6dcc37ea Author: Bob Clary <bob@bclary.com> Date: Fri Jun 2 12:58:45 2017 -0700 Bug xxxxxxx - Autophone - logging tweaks, r=self. * simplify saving original loggerdeco * add the test class name to the loggerdecos before the test begins running * do not truncate worker log during test setup if we are not submitting to Treeherder phonetest.py worker.py commit dbceb0f14279df3c9ff133c071049b0fd4bdf234 Author: Bob Clary <bob@bclary.com> Date: Fri Jun 2 12:46:44 2017 -0700 No Bug - Autophone - errors.sh - use xzgrep to support multiple compression types. errors.sh I found --pretty which almost works: bclary@sophie /mozilla/projects/autophone/src/bclary-autophone (sisyphus-dev) $ git log --name-only --pretty=format: origin/master..HEAD .gitignore .gitmodules files/base/initialize_profile.html files/ep1 files/s1s2/blank.html phonetest.py tests/s1s2geckoviewtest.py tests/s1s2test.py xpi/quitter.xpi phonetest.py worker.py errors.sh but we have blank lines which should probably be removed before returning.

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review153590 > Now we are a list again. This interchange between set and list is confusing. Agreed it's confusing. I'll add a comment and try to tidy it up a bit. Fwiw, I needed to use a set just for this section because if multiple commits modified the same file, that file would get returned (and therefore linted) twice. It got converted back to a list because that's what it was before (though maybe it can stay as a set?). > In my local autophone tree where I have some patches queued up on a development branch I got: > > bclary@sophie /mozilla/projects/autophone/src/bclary-autophone (sisyphus-dev) > $ git log --name-only origin/master..HEAD > commit 68f50e2026e29e8b0b1af8a41df07ddc4ad86d5a > Author: Bob Clary <bob@bclary.com> > Date: Wed Jun 7 07:29:59 2017 -0700 > > Bug 1371291 - Autophone - use intent action to shutdown fennec and geckoview_example and eliminate quitter, r=jmaher. > > .gitignore > .gitmodules > files/base/initialize_profile.html > files/ep1 > files/s1s2/blank.html > phonetest.py > tests/s1s2geckoviewtest.py > tests/s1s2test.py > xpi/quitter.xpi > > commit 28e9c75f6f404eda1fefe409f5ceec1e6dcc37ea > Author: Bob Clary <bob@bclary.com> > Date: Fri Jun 2 12:58:45 2017 -0700 > > Bug xxxxxxx - Autophone - logging tweaks, r=self. > > * simplify saving original loggerdeco > * add the test class name to the loggerdecos before the test begins running > * do not truncate worker log during test setup if we are not submitting to Treeherder > > phonetest.py > worker.py > > commit dbceb0f14279df3c9ff133c071049b0fd4bdf234 > Author: Bob Clary <bob@bclary.com> > Date: Fri Jun 2 12:46:44 2017 -0700 > > No Bug - Autophone - errors.sh - use xzgrep to support multiple compression types. > > errors.sh > > I found --pretty which almost works: > > bclary@sophie /mozilla/projects/autophone/src/bclary-autophone (sisyphus-dev) > $ git log --name-only --pretty=format: origin/master..HEAD > .gitignore > .gitmodules > files/base/initialize_profile.html > files/ep1 > files/s1s2/blank.html > phonetest.py > tests/s1s2geckoviewtest.py > tests/s1s2test.py > xpi/quitter.xpi > > phonetest.py > worker.py > > errors.sh > > but we have blank lines which should probably be removed before returning. Good catch that this was pretty broken on git before, but (I think) this gets fixed in the next commit (assigned to Standard8). The command being used ends up being: $ git log --name-only --diff-filter=AM --pretty=format: --oneline origin/master..HEAD Does that still print the spaces for you? The esoteric commands git has never cease to amaze me.

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review153974 ::: python/mozlint/mozlint/roller.py:116 (Diff revision 2) > > # Calculate files from VCS > if workdir: > - paths.extend(self.vcs.by_workdir()) > + paths.update(self.vcs.by_workdir()) > if outgoing: > - paths.extend(self.vcs.outgoing(outgoing)) > + paths.update(self.vcs.by_outgoing(outgoing)) Testing this locally, I was seeing something strange. If I have no commits/pending changes then it would go to linting absolutely everything. Adding some debug, after the paths.update showed that python thought it was: set([]) When the `paths = paths or ['.']` happens on the line below, then it seems to be thinking the path is empty and changes path to ['.']. I don't think that's intentional, we should probably exit quickly in that case (assuming --workdir or --outgoing have been specified).

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review153976 ::: python/mozlint/mozlint/roller.py:120 (Diff revision 2) > if outgoing: > - paths.extend(self.vcs.outgoing(outgoing)) > + paths.update(self.vcs.by_outgoing(outgoing)) > > + # This will convert paths back to a list, but that's ok since > + # we're done adding to it. > paths = paths or ['.'] Should this be `or set(['.'])` ?

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review153978 Heading in the right direction, but I'd like to see it again after the changes. ::: commit-message-ba3a1:8 (Diff revision 2) > +The underlying commands to mozlint's vcs.py had a few problems. This: > + > + 1. Ensures deleted files aren't considered in both hg and git > + 2. Automatically determines the default remote repo and branch when using --outgoing with git > + 3. Excludes metadata from output of the git command used with --outgoing > + 4. Adds --cached to the git command for --workdir, which lints all staged files Are you doing the "allow linter to specify more files/directories due to config file changes" in a separate bug? ::: python/mozlint/mozlint/vcs.py:71 (Diff revision 2) > > def by_outgoing(self, dest='default'): > if dest == 'default': > - comparing = 'origin/master..HEAD' > - else: > + ref = subprocess.check_output(['git', 'symbolic-ref', '-q', 'HEAD']).strip() > + dest = subprocess.check_output( > + ['git', 'for-each-ref', '--format=%(upstream:short)', ref]).strip() If there's no upstream reference then dest ends up empty. Can we have some sort of fallback so that we select default or master, depending on if they exist or not? This would seem to be better overall. ::: python/mozlint/mozlint/vcs.py:77 (Diff revision 2) > - comparing = '{}..HEAD'.format(dest) > + comparing = '{}..HEAD'.format(dest) > - return self.run(['git', 'log', '--name-only', comparing]) > + return self.run(['git', 'log', '--name-only', '--diff-filter=AM', > + '--oneline', '--pretty=format:', comparing]) > > def by_workdir(self): > - return self.run(['git', 'diff', '--name-only']) > + return self.run(['git', 'diff', '--cached', '--name-only', '--diff-filter=AM']) From the description by --help, and my experience using git, I'd expect this to list both cached and non-cached changes, as both are in the work directory.

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review154056 Seems ok to me if you resolve the git log issue in the following patch. The question about whether we should bail early if the paths is empty or if we should set it to ['.'], I'll leave to you and standard8. I think I would agree that bailing early would be the right choice though. ::: python/mozlint/mozlint/roller.py:119 (Diff revision 2) > - paths.extend(self.vcs.by_workdir()) > + paths.update(self.vcs.by_workdir()) > if outgoing: > - paths.extend(self.vcs.outgoing(outgoing)) > + paths.update(self.vcs.by_outgoing(outgoing)) > > + # This will convert paths back to a list, but that's ok since > + # we're done adding to it. This comment actually belongs to the map statement below and not to this path = path or ['.']. If we could put them together that would be great.

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review153976 > Should this be `or set(['.'])` ? It doesn't really matter either way, the map call on the next line is going to convert it to a list anyway.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review153978 > If there's no upstream reference then dest ends up empty. > > Can we have some sort of fallback so that we select default or master, depending on if they exist or not? > > This would seem to be better overall. Are you suggesting I simply set it to "master..HEAD"? Or is there some special command you had in mind? Maybe it would be better to abort with an error message about no remotes? > From the description by --help, and my experience using git, I'd expect this to list both cached and non-cached changes, as both are in the work directory. I'm far from a git expert, so you may be right.. but this is the same command that the tools/git/eslintvalidate.py hook uses. So at the very least, it will lint the files that people running that hook are used to (which is good enough for me).

(In reply to Mark Banner (:standard8) from comment #19) > Are you doing the "allow linter to specify more files/directories due to > config file changes" in a separate bug? Yeah, just filed bug 1373368.

Comment on attachment 8877277 [details] Bug 1369787 - [mozlint] Add a test for vcs operations, https://reviewboard.mozilla.org/r/148618/#review154362 Looks ok though I'm not familiar with the test framework unfortunately. It does pass however.

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review156234 ::: python/mozlint/mozlint/roller.py:123 (Diff revision 3) > + if not paths and (workdir or outgoing): > + print("no files linted") > + return {} > > paths = paths or ['.'] > + In testing out the patches, I just noticed that this line has empty whitespace which upsets flake8

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review156234 > In testing out the patches, I just noticed that this line has empty whitespace which upsets flake8 Oh, it has been removed in the final test patch though...

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review156248 Sorry for the delay in getting back to this. I like the new changes, much nicer. There's still a couple of things I think we need to address though. ::: python/mozlint/mozlint/vcs.py:87 (Diff revision 3) > + break > + > + self._default = dest > + return self._default > > def by_outgoing(self, dest='default'): Somewhere along the line, we seem to have lost the ability to ESLint single files: `./mach eslint toolkit/components/places/nsTaggingService.js` doesn't work `./mach eslint toolkit/components/places/` works As a result, if I include some .js changes, then --outgoing and --workdir return no issues even though I've created some in the patch/directory.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review156248 > Somewhere along the line, we seem to have lost the ability to ESLint single files: > > `./mach eslint toolkit/components/places/nsTaggingService.js` doesn't work > `./mach eslint toolkit/components/places/` works > > As a result, if I include some .js changes, then --outgoing and --workdir return no issues even though I've created some in the patch/directory. Huh, I think this is unrelated to this change and actually broken right now! I'm seeing the same behaviour even while running from central. I'll see if there's a quick fix here and include it in this series, or else file a new bug to tackle it.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review156248 > Huh, I think this is unrelated to this change and actually broken right now! I'm seeing the same behaviour even while running from central. I'll see if there's a quick fix here and include it in this series, or else file a new bug to tackle it. I spun this out into bug 1375166

Comment on attachment 8877275 [details] Bug 1369787 - [mozlint] Refactor vcs.py into separate classes for hg and git, https://reviewboard.mozilla.org/r/148614/#review156234 > Oh, it has been removed in the final test patch though... My mistake, must have amended to the wrong commit. Should be in the proper change now.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review153978 > I'm far from a git expert, so you may be right.. but this is the same command that the tools/git/eslintvalidate.py hook uses. So at the very least, it will lint the files that people running that hook are used to (which is good enough for me). I think there's a difference between the hook and the work directory. The commit hook is checking what is going to be commited - what is cached when the hook is run. So for example I can do `git commit -a` to commit all my changes, and they'll be cached and then committed and the hook will run on all of them. For the work directory, I think it is slightly different - the set of uncommitted changes are the set of cached and uncached changes. In my typical workflow (I realise this may be different for others), my changes are all uncached unless something has been a `git mv`, or I've needed to explicitly separate changes.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review156812 Looks good. I'm happy for the --workdir change (to include both cached & non-cached as per comment 38) to either be done before landing or in a separate bug.

Comment on attachment 8877276 [details] Bug 1369787 - [mozlint] Fix up version control commands, https://reviewboard.mozilla.org/r/148616/#review153978 > I think there's a difference between the hook and the work directory. The commit hook is checking what is going to be commited - what is cached when the hook is run. > > So for example I can do `git commit -a` to commit all my changes, and they'll be cached and then committed and the hook will run on all of them. > > For the work directory, I think it is slightly different - the set of uncommitted changes are the set of cached and uncached changes. In my typical workflow (I realise this may be different for others), my changes are all uncached unless something has been a `git mv`, or I've needed to explicitly separate changes. Thanks for the explanation, makes sense. Looks like we want `git diff HEAD` which should include both staged and unstaged changes.

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/866a8c1b11f4 [mozlint] Remove 'rev' option from |mach lint|, r=bc https://hg.mozilla.org/integration/autoland/rev/4d85d4d86ef4 [mozlint] Refactor vcs.py into separate classes for hg and git, r=bc https://hg.mozilla.org/integration/autoland/rev/41c2ad89a722 [mozlint] Fix up version control commands, r=standard8 https://hg.mozilla.org/integration/autoland/rev/6304c0ecb59d [mozlint] Add a test for vcs operations, r=bc

https://hg.mozilla.org/mozilla-central/rev/866a8c1b11f4 https://hg.mozilla.org/mozilla-central/rev/4d85d4d86ef4 https://hg.mozilla.org/mozilla-central/rev/41c2ad89a722 https://hg.mozilla.org/mozilla-central/rev/6304c0ecb59d