1313194 - Replace mozlint's --rev with a --outgoing argument

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew Halberstadt [:ahal]]

Currently you can run |mach lint --rev <revision(s)>| and this will lint all the files touched by the specified revisions. The problem is that it will lint those files at whatever state your file system is in, whether or not you are actually updated to the proper revision.

This is bad UX and counter intuitive. We *could* make mozlint first update people to the proper revision and then run the lint, but this seems like it is opening the door for a lot of complexity. I like the idea of leaving version control to version control, and not getting mozlint too tied up in it.

The main purpose of --rev was to only lint files that were being touched by the current feature branch / revisions that were being pushed to the remote server. This can also be done with hg outgoing (or git log I believe). So I propose we just remove --rev altogther, and instead replace it with --outgoing. E.g:

./mach lint --outgoing central

(or leave blank to use default-push)

This would lint all files touched by all revisions which *would* be pushed to the specified remote. It will use whatever revision(s) are currently updated, so we don't need to worry about linting the wrong version of a file.

Comment 1

[Andrew Halberstadt [:ahal]]

Hm, though I guess for mozreview's use case it might need to do it on a revision by revision basis so that errors can be attached to the proper child review request.. Will we need to keep --rev?

Francesco, for now please focus on simply adding a --outgoing option instead of replacing --rev. We can decide whether --rev should be removed or not later. Thanks!

Comment 2

[Steven MacLeod [:smacleod]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Hm, though I guess for mozreview's use case it might need to do it on a
> revision by revision basis so that errors can be attached to the proper
> child review request.. Will we need to keep --rev?

Ya, it's going to be done once for every revision for MozReview. That being
said I could do that calculation in the task before calling the lint, just
passing in the files myself. Having --rev still would be nice though.

Comment 3

[Francesco Pischedda]

Hi,

I've restarted looking at this issue and I think I've found where to start; I'm lookign at roller.py, specifically the method def roll(self, paths=None, rev=None, workdir=None, num_procs=None) of the LintRoller class

I think that replicationg this code
if rev:
    paths.extend(self.vcs.by_rev(rev))

with something like
if outgoing:
    paths.extend(self.vcs.outgoing(rev))

should do the trick

Am I looking at the right code?

Comment 4

[Andrew Halberstadt [:ahal]]

Yep, you got it! Then in the vcs.py file you do the actual implementation that calls into version control (for both hg and git).

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Steven MacLeod [:smacleod] from comment #2)
> Ya, it's going to be done once for every revision for MozReview. That being
> said I could do that calculation in the task before calling the lint, just
> passing in the files myself. Having --rev still would be nice though.

This is off-topic for this bug, but will mozreview update to each revision before running the linter? Because keep in mind that not only could the files be different between revisions, but the configuration that affects the linters as well. I wonder if mozreview should just only run the linters on the last revision in the series :/

Comment 6

[Francesco Pischedda]

and you answered another question which is if it has to support both git and hg :)

Talking about hg outgoing, looking at its documentation I've seen that it support an optional DEST argument:
"Show changesets not found in the specified destination repository or the default push location. These are the changesets that would be pushed if a push was requested."

do you want to provide the possibility to specify DEST like --rev provides a specific revision? or going with the default destination could be enough?

Comment 7

[Andrew Halberstadt [:ahal]]

Yes, we'll need to be able to supply dest. Mercurial has a "default-push" concept, e.g if you just run `hg push` it'll push to your default repository, `hg outgoing` uses the same thing. However people might want to push elsewhere, e.g 'try', 'inbound', 'review', etc..

It would be nice if --outgoing worked both with or without an extra parameter. For example, `./mach lint --outgoing` would use default-push, but `./mach lint --outgoing inbound` would explicitly compare against the proper path. This can be accomplished with some argparse tricks, namely setting nargs='?' and const=None:
http://stackoverflow.com/questions/15301147/python-argparse-default-value-or-specified-value

Comment 8

[Andrew Halberstadt [:ahal]]

Or, in this case const='default' I guess.

Comment 9

[Francesco Pischedda]

Hello!

how can I skip the first two lines printed by hg outgoing? Here is an example:
~/works/firefox ᐅ hg outgoing --template '{files % "\\n{file}"}'
comparing with https://hg.mozilla.org/mozilla-central/
searching for changes
\npython/mozlint/mozlint/cli.py\npython/mozlint/mozlint/roller.py\npython/mozlint/mozlint/vcs.py

hg log, as used by vcs.py does not print any additional lines and I'm wandering if this could be replicated with hg outgoing otherwise I am thinging about a new parameter to the _run method that accepts the lines to skip from the output, something like

    def _run(self, cmd, skiplines=0):
        files = subprocess.check_output(cmd).split()
        return [os.path.join(self.root, f) for f in files][skiplines:]

Comment 10

[Francesco Pischedda]

I've been looking for a solution to replicate the hg outgoing functionality with git and it seems that the only way is to use git log comparing local and remote branches for ex. git log --name-only master..origin/master so to keep the interface of the function I'm thinking about something like this:


    def outgoing(self, destination='default'):
        if self.is_hg:
            return self._run(['hg', 'outgoing', destination, '--template', '{files % "\\n{file}"}'])
        elif self.is_git:
            if destination == 'default':
                comparing = 'master..origin/master'
            else:
                comparing = '{0}..origin/{0}'.format(destination)
            return self._run(['git', 'log', '--name-only', comparing])
        return []

if case of git if the fucntion receives the 'default' destination I assume that the caller want to compare local and remote master branches (see the comparing variable); this way we are restricted to compare the same local and remote branch even if it could be possible to compare different branches for example master..origin/review so I'd like to get your opinion on this.

Comment 11

[Francesco Pischedda]

thinking again about this issue it may be better to ask the user to specify directly which branches to compare, maybe writing this in the help of the --outgoing parameter so if someone is using git could write --outgoing master..origin/review or whatever he/she likes

if the user does not provide the branches to compare the destination parameter will still hold the 'default' value and in this case I can force the comparing branches to master..origin/master.

Comment 12

[Andrew Halberstadt [:ahal]]

Hey Francesco, sorry I've been away at the company all hands event so haven't been checking my bugmail very frequently.

Regarding `hg outgoing`, I think you can pass in --quiet and that should do what you want.

Regarding git, I think you have the right idea. However, the user should only specify the destination branch, not the local one. Locally we should use whatever revision is currently checkout (whether it's at the tip of a branch or not). But defaulting to origin/master seems sensible to me if a different branch isn't specified.

Thanks for your comments and thinking through some of these potential issues in advance! Looks like you're making good progress, and let me know if you have follow-up questions.

Comment 13

[Francesco Pischedda]

Attachment: Bug 1313194 - Add --outgoing argument to mozlint.

added outgoing support for mercurial; git WIP

Review commit: https://reviewboard.mozilla.org/r/99356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99356/

Comment 14

[Francesco Pischedda]

Hi Andrew,

sorry if this push to review is incomplete in the git side, but I thought that some kind of validation on the hg side would help me to focus on git commands.
Talking about git, since we need to use whatever revision, branch etc the user is on, do you have you some method/tip to get the current branch/revision without parsing the output of the git branch command?

Thanks,
Francesco

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

https://reviewboard.mozilla.org/r/99356/#review99998

Thanks this is really close! Just a couple fixes left for the commands themselves, but everything else looks good.

::: python/mozlint/mozlint/vcs.py:59
(Diff revision 1)
> +            return self._run(['hg', 'outgoing', '--quiet',
> +                              destination, '--template',
> +                              '{files % "\\n{file}"}'])

By default hg will find all revisions that exist locally but don't on the remote server unless -r is specified. In mercurial, the '.' character is a shortcut for the currently active revision, so you need to add "-r ." to the command.

Also, I'm not sure if the newline character needs to be escaped in the template. I think it should work without it.

::: python/mozlint/mozlint/vcs.py:63
(Diff revision 1)
> +            if destination == 'default':
> +                comparing = 'master..origin/master'
> +            else:
> +                comparing = destination
> +            return self._run(['git', 'log', '--name-only', comparing])

With git, the shorctut HEAD is equivalent to mercurial's "." character. So you can simply use HEAD as the source. Note the destination needs to come before the source, e.g:

git log --name-only origin/master..HEAD

By using HEAD and '.', we can guarantee that we will always use the current commit as the reference point, meaning the files will be linted at their proper state.

Comment 16

[Andrew Halberstadt [:ahal]]

Hey Francesco, was this still something you wanted to work on? No worries if you don't have time, I understand that stuff like this usually takes a back seat to life. If you don't have time do you mind if I finish this up for you? Just don't want to step on your toes.

Comment 17

[Francesco Pischedda]

Hello Andrew,

last time I have pushed to review I thought I had resolved the issues reported in your review but I have not noticed that something had gone wrong and now I have my local working directory in a very strange state; I'll try to revert everything (keeping my changes somewhere) and retry to push to review.

This explains why I haven't heard from you since my last push :)

I'll get back to you as soon as I have finished restoring my local working copy.

Comment 18

[Francesco Pischedda]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99356/diff/1-2/

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

https://reviewboard.mozilla.org/r/99356/#review108630

Thanks! And this did slip my mind a bit, otherwise I would have checked in sooner. Just a couple changes to comments, and your commit accidentally removed a change that recently landed. I'll give this an r+ once those are fixed and I get a chance to test this out on my machine.

::: python/mozlint/mozlint/cli.py:52
(Diff revision 2)
>                    "mercurial or git."
>            }],
> +        [['-o', '--outgoing'],
> +         {'const': 'default',
> +          'nargs': '?',
> +          'help': "Lint files that are not on remote repository."

I'd change this to "Lint files touched by commits that are not on the remote repository."

::: python/mozlint/mozlint/roller.py:101
(Diff revision 2)
> -    def roll(self, paths=None, rev=None, workdir=None, num_procs=None):
> +    def roll(self, paths=None, rev=None, outgoing=None, workdir=None, num_procs=None):
>          """Run all of the registered linters against the specified file paths.
>  
>          :param paths: An iterable of files and/or directories to lint.
>          :param rev: Lint all files touched by the specified revision.
>          :param workdir: Lint all files touched in the working directory.
>          :param num_procs: The number of processes to use. Default: cpu count

Please add `outgoing` to the docstring as well.

::: python/mozlint/mozlint/roller.py:149
(Diff revision 2)
> -        orig_sigint = signal.signal(signal.SIGINT, signal.SIG_IGN)
> +        signal.signal(signal.SIGINT, signal.SIG_IGN)
>          self.failed = []
>          for worker in workers:
>              # parent process blocks on worker.get()
>              results, failed = worker.get()
>              if failed:
>                  self.failed.extend(failed)
>              for k, v in results.iteritems():
>                  all_results[k].extend(v)
> -
> -        signal.signal(signal.SIGINT, orig_sigint)
> -        m.shutdown()

This is undoing a change that recently landed, you'll need to rebase your patch on top of the latest mozilla-central and make sure not to undo this in the merge.

Comment 20

[Francesco Pischedda]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99356/diff/2-3/

Comment 21

[Francesco Pischedda]

Hi Andrew,

instead of rebasing I've added the missing lines because I've cloned the latest version and copyed my old code to python/mozlint/mozlint/{cli,vcs,runner}.py in order to quickly recover from my working copy mess, hope this is as good as rebasing :)

Comment 22

[Andrew Halberstadt [:ahal]]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

https://reviewboard.mozilla.org/r/99356/#review108688

Thanks for the quick fix! I tested this locally and it works well (at least for the hg case). I noticed that we should be using a set instead of a list when building the paths, otherwise we can get duplicate results.. but that's a pre-existing bug we can fix separately from this.

I'll do a try run just to be safe. Once that's done, and you re-push with the newlines removed, I'll go ahead and land this!

::: python/mozlint/mozlint/roller.py:156
(Diff revision 3)
>          signal.signal(signal.SIGINT, orig_sigint)
> +
>          m.shutdown()
> +
>          return all_results

Looks like there are still two extra newlines here.. just delete them manually and I think we're good!

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

https://reviewboard.mozilla.org/r/99356/#review108698

Actually, could you also modify the commit message? We ended up 'adding a --outgoing' argument instead of replacing --rev.

Comment 24

[Francesco Pischedda]

Comment on attachment 8819656 [details]
Bug 1313194 - Add --outgoing argument to mozlint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99356/diff/3-4/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abf6177ec928
Add --outgoing argument to mozlint. r=ahal

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/abf6177ec928