I notice this in working towards bug 1386351. 1) Add a symlink for the pre-push hook (to ../../tools/lint/hook.py) 2) Checkout a wip branch and attempt to `git push try` Actual results: fatal: ambiguous argument 'try..HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' error running: /usr/local/Cellar/git/2.14.1/libexec/git-core/git log --name-only --diff-filter=AM --oneline --pretty=format: try..HEAD warning: no files linted ✖ 0 problems (0 errors, 0 warnings) Expected results: Successful ESLint run with either a push to try, or aborted due to errors.

I'm starting to think that: http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/tools/lint/hooks.py#25 shouldn't pass the args through to the lint command, and: http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/python/mozversioncontrol/mozversioncontrol/__init__.py#352 get_outgoing_files could fallback to seeing if it can find the parent branch, though it looks like that might be a little tricky. Thoughts?

This looks like a regression from bug 1401309. We used to fall back to finding the parent branch, but gps specifically asked me to remove that for being too magical as it was making assumptions on branch names. I think we need to pass in args there, at least for the mercurial case. Maybe we can only do it for mercurial.

I think we should copy what |mach try| does to get outgoing files on git: https://dxr.mozilla.org/mozilla-central/source/tools/tryselect/vcs.py#191 At the very least, using that method correctly finds outgoing files on my git-cinnabar setup (whereas the old method doesn't). I need this base revision functionality for some other stuff I'm doing anyway, so figured I could kill two birds with one stone by using it to fix this bug.

Rereading comment 0 I think I missed the problem here. The attached patches do fix an error I had using the push hook with my cinnabar setup, but I don't think it'll fix Standard8's issue. The good news is that with these patches, I think we can simply stop forwarding the arguments in the git hook and it should automagically work.

Sorry for the churn. I discovered that the 'git log' command used could sometimes contain an empty string which was causing mozlint to lint *all* files instead of just the outgoing ones. This is now fixed and running 'git push try' lints the correct files in my configuration. Mark, would you mind testing if the latest patch series also works for you? There are a lot of ways to configure cinnabar and any extra data points would be helpful.

(In reply to Andrew Halberstadt [:ahal] from comment #10) > Mark, would you mind testing if the latest patch series also works for you? > There are a lot of ways to configure cinnabar and any extra data points > would be helpful. I can do that, though I probably won't have time to pick it up until Monday.

Comment on attachment 8924307 [details] Bug 1405588 - [lint] Stop forwarding 'args' in the git pre-push hook, https://reviewboard.mozilla.org/r/195554/#review202462 This looks good to me. It works for my cases of pushing a couple of changesets (with the first one broken) to try, and also to autoland. It also seems to work fine for my patch in bug 1386351 to get `git mozreview push` working as well :-) I suggest we try it out on the real world and see what happens.

Comment on attachment 8924302 [details] Bug 1405588 - [mozversioncontrol] Add ability to get base revision, https://reviewboard.mozilla.org/r/195550/#review205084 ::: python/mozversioncontrol/mozversioncontrol/__init__.py:343 (Diff revision 1) > + refs = self._run('for-each-ref', 'refs/heads', 'refs/remotes', > + '--format=%(objectname)').splitlines() > + head = self.head_ref > + if head in refs: > + refs.remove(head) > + return self._run('merge-base', 'HEAD', *refs).strip() There is the potential for this to "overflow" allowed number of command arguments or total command string length. Let's see if it becomes a problem and address it if it does. FTR, this is a clever solution for the problem and one I hadn't fully considered. Previous attempts by me to implement this code involved running `git log` with custom output and then looking for the first commit with an associated ref that wasn't HEAD or the active ref. We'll likely have to do that if we run into length limitations with this approach.

Comment on attachment 8924303 [details] Bug 1405588 - [mozversioncontrol] Use base_ref instead of upstream as default outgoing comparison on git, https://reviewboard.mozilla.org/r/195552/#review205086

Comment on attachment 8924302 [details] Bug 1405588 - [mozversioncontrol] Add ability to get base revision, https://reviewboard.mozilla.org/r/195550/#review205084 > There is the potential for this to "overflow" allowed number of command arguments or total command string length. Let's see if it becomes a problem and address it if it does. > > FTR, this is a clever solution for the problem and one I hadn't fully considered. Previous attempts by me to implement this code involved running `git log` with custom output and then looking for the first commit with an associated ref that wasn't HEAD or the active ref. We'll likely have to do that if we run into length limitations with this approach. Re-implementing later if necessary sounds good to me. I can't take credit for this approach though, it was initially implemented for |mach try| (I assume by chmanchester). The good news is that this method has already been in use by cinnabar users for awhile. So hopefully that means the command string length won't be too much of a problem.

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8c9a03f57a4 [mozversioncontrol] Add ability to get base revision, r=gps https://hg.mozilla.org/integration/autoland/rev/64a007cd4277 [mozversioncontrol] Use base_ref instead of upstream as default outgoing comparison on git, r=gps https://hg.mozilla.org/integration/autoland/rev/dd760c249122 [lint] Stop forwarding 'args' in the git pre-push hook, r=standard8

https://hg.mozilla.org/mozilla-central/rev/a8c9a03f57a4 https://hg.mozilla.org/mozilla-central/rev/64a007cd4277 https://hg.mozilla.org/mozilla-central/rev/dd760c249122

Thanks for the work here, this is a great milestone for getting our hook support improved :-)