Subject: [PATCH] annotate nonexistent file non zero exit code

Greg Ward greg at gerg.ca
Wed Mar 16 20:42:10 CDT 2011


On 16 March 2011, Loic Dachary said:
> This is a tentative patch for http://mercurial.selenic.com/bts/issue1590
> It is only related to hg annotate.

Thanks!  Good to see someone working on status codes; it is a bit of
mess that needs some attention.

> Because it is my first patch to
> mercurial  I would like to know if it is on the right track. I took
> great care to follow the instructions at
> http://mercurial.selenic.com/wiki/ContributingChanges and I hope you
> will be able to apply it without troubles.

You missed a couple of details:
  * "Using the patchbomb extension always works; see below for
    details." (Scroll down to section 5 of the wiki page for details
    on setting up patchbomb.)
  * please re-read the section on formatting commit messages (aka
    "Patch descriptons", section 2)

> P.S. I apologize for the attachment : I am quite sure my mailer would
> alter the patch and I was not able to figure out how to attach this
> message to the patchbomb output.

Once you have configured patchbomb, use "hg email --intro ..." to add
an intro message.  However, you should not need an intro message: the
patch should stand alone and be self descriptive.  

> # HG changeset patch
> # User loic at dachary.org

Fullname + address please, e.g. "Loic Dachary <loic at dachary.org>"
(just as in email headers).  Unless you are extremely paranoid about
keeping your identity private, that is.  ;-)

> # Date 1300297128 -3600
> # Node ID a3609d28f84355fff55519a49b924ea49b3fcb79
> # Parent  30a0e3519f69142288d3d5b6d64a966cf171163d
> introduce the context.py:walkwithfound function that is then used in command.py:annotate as a replacement of the walk function. It allows annotate to be notified of the patterns that did not match any file in the repository, in which case it will abort and hg will have a non zero exit status as expected

First line should be a brief introduction: what are you changing and,
if there is room in 75 columns, why?  Then a blank line.  Then the
longer explanation, which is where you can explain the problem in
greater detail, explain why you chose this particular solution, etc.

In this case, the first line should almost certainly be

  annotate: exit with non-zero status (abort) when bogus filename passed in

The rest of the message should explain the details of your fix.

> diff -r 30a0e3519f69 -r a3609d28f843 mercurial/commands.py
> --- a/mercurial/commands.py	Tue Mar 15 16:53:46 2011 -0500
> +++ b/mercurial/commands.py	Wed Mar 16 18:38:48 2011 +0100
> @@ -129,7 +129,10 @@
>      ctx = cmdutil.revsingle(repo, opts.get('rev'))
>      m = cmdutil.match(repo, pats, opts)
>      follow = not opts.get('no_follow')
> -    for abs in ctx.walk(m):
> +    for (abs, found) in ctx.walkwithfound(m):
> +        if not found:
> +            raise util.Abort(_('%s did not match any file') % abs)
> +
>          fctx = ctx[abs]
>          if not opts.get('text') and util.binary(fctx.data()):
>              ui.write(_("%s: binary file\n") % ((pats and m.rel(abs)) or abs))
> diff -r 30a0e3519f69 -r a3609d28f843 mercurial/context.py
> --- a/mercurial/context.py	Tue Mar 15 16:53:46 2011 -0500
> +++ b/mercurial/context.py	Wed Mar 16 18:38:48 2011 +0100
> @@ -203,6 +203,25 @@
>              if match.bad(fn, _('no such file in rev %s') % self) and match(fn):
>                  yield fn
>  
> +    def walkwithfound(self, match):
> +        fset = set(match.files())
> +        # for dirstate.walk, files=['.'] means "walk the whole tree".
> +        # follow that here, too
> +        fset.discard('.')
> +        for fn in self:
> +            for ffn in fset:
> +                # match if the file is the exact name or a directory
> +                if ffn == fn or fn.startswith("%s/" % ffn):
> +                    fset.remove(ffn)
> +                    break
> +            if match(fn):
> +                yield (fn, True)
> +        for fn in sorted(fset):
> +            if match.bad(fn, _('no such file in rev %s') % self) and match(fn):
> +                yield (fn, True)
> +            else:
> +                yield (fn, False)

Hmmm: except for the last 3 lines, this is a straight copy 'n paste
job from changectx.walk().  We don't do copy 'n paste coding around
here.  Please try again: don't copy, refactor.  (And don't forget to
consider performance!)

> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/test-issue1590.t	Wed Mar 16 18:38:48 2011 +0100
> @@ -0,0 +1,11 @@
> +http://mercurial.selenic.com/bts/issue1590
> +
> +The exit status of annotate when a file does not exist must
> +not be 0
> +
> +  $ hg init
> +
> +  $ hg annotate nonexistent
> +  nonexistent: no such file in rev 000000000000
> +  abort: nonexistent did not match any file
> +  [255]

We should print just one error message.

Thanks for your patch -- please clean it up and resubmit.

        Greg


More information about the Mercurial-devel mailing list