[PATCH 1 of 1 V5] revert: improve hint on abort if no files were specified

[Matt Mackall]

On Fri, 2011-06-17 at 13:56 +0200, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian at cadifra.com>
> # Date 1308220115 -7200
> # Node ID 123f9a41a8a6d8a4b736af89cb7e437cc0cf4500
> # Parent  e21fd445c297dfff9c709380e09be58f23108a36
> revert: improve hint on abort if no files were specified
> 
> and provide some additional status info in two notable cases as a friendly
> reminder for the user (please see the examples below)

I want to make headway on this thing, but you keep fixing one thing and
then introducing a new wrinkle. Can I get you to break it into smaller
pieces?

- introduce the different case logic (with unchanged messages)
- message tweak 1
...

As you'll see below, I'm leaning toward a format that looks something
like this:

abort: no files or directories specified
([<working directory note>, ]use --all to discard all changes[, or 'hg
update <rev>' to update])


> uncommitted merge:
> 
>   BEFORE:
>     $ hg revert
>     abort: no files or directories specified
>     (use --all to revert all files)
> 
>   NEW:
>     $ hg revert
>     uncommitted merge

-1 on introducing a new stdout message

If anything, we should put this in the hint. Or maybe use ui.warn. But I
don't think it's really that critical.

>     abort: no files or directories specified
>     (use --all to discard all changes you might have made)

..and any changes anyone else might have made, and any changes hg made
for you..

How about:

(uncommitted merge, use --all to discard all changes)


> uncommitted changes, revert to non-parent (most complex use case):
> 
>   BEFORE:
>     $ hg revert -d 2011-05-05
>     Found revision 7423 from Thu May 05 19:56:05 2011 +0200
>     abort: no files or directories specified
>     (use --all to revert all files)
> 
>   NEW:
>     $ hg revert -d 2011-05-05
>     Found revision 7423 from Thu May 05 19:56:05 2011 +0200
>     uncommitted changes
>     abort: no files or directories specified
>     (use --all to discard all changes, or 'hg update -r 7423' to update)

(uncommitted changes, use --all to discard all changes...)

The -r for update is unneeded.

> clean, revert to non-parent:
> 
>   BEFORE:
>     $ hg revert -d 2011-05-05
>     Found revision 7423 from Thu May 05 19:56:05 2011 +0200
>     abort: no files or directories specified
>     (use --all to revert all files)
> 
>   NEW:
>     $ hg revert -d 2011-05-05
>     Found revision 7423 from Thu May 05 19:56:05 2011 +0200
>     abort: no files or directories specified
>     (use --all to revert all files, or 'hg update -r 7423' to update)

discard?

> uncommitted changes:
> 
>   BEFORE:
>     $ hg revert
>     abort: no files or directories specified
>     (use --all to revert all files)
> 
>   NEW:
>     $ hg revert
>     abort: no files or directories specified
>     (use --all to discard all changes)

Why do we not mention uncommitted changes here?

> nothing changed:
> 
>   BEFORE:
>     $ hg revert
>     abort: no files or directories specified
>     (use --all to revert all files)
> 
>   NEW:
>     $ hg revert
>     nothing changed

Ok. This probably wants to be a separate patch, and that patch should
give a rationale for its error code. My gut feeling is that:

hg revert -> always fails
hg revert -a -> always success

> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4181,10 +4181,6 @@
>  
>      parent, p2 = repo.dirstate.parents()
>  
> -    if not pats and not opts.get('all'):
> -        raise util.Abort(_('no files or directories specified'),
> -                         hint=_('use --all to revert all files'))
> -
>      ctx = scmutil.revsingle(repo, opts.get('rev'))
>      node = ctx.node()
>      mf = ctx.manifest()
> @@ -4193,6 +4189,27 @@
>      else:
>          pmf = None
>  
> +    if not pats and not opts.get('all'):
> +        msg = _("no files or directories specified")
> +        if p2 != nullid:
> +            ui.status(_("uncommitted merge\n"))
> +            hint = _("use --all to discard all changes you might have made")
> +            raise util.Abort(msg, hint=hint)
> +        dirty = util.any(repo.status())
> +        if node != parent:
> +            if dirty:
> +                ui.status(_("uncommitted changes\n"))
> +                hint = _("use --all to discard all changes,"
> +                         " or 'hg update -r %s' to update") % ctx.rev()
> +            else:
> +                hint = _("use --all to revert all files,"
> +                         " or 'hg update -r %s' to update") % ctx.rev()
> +            raise util.Abort(msg, hint=hint)
> +        elif dirty:
> +            hint = _("use --all to discard all changes")
> +            raise util.Abort(msg, hint=hint)
> +        ui.status(_("nothing changed\n"))
> +
>      # need all matching names in dirstate and manifest of target rev,
>      # so have to walk both. do not print errors if files exist in one
>      # but not other.
> diff --git a/tests/test-confused-revert.t b/tests/test-confused-revert.t
> --- a/tests/test-confused-revert.t
> +++ b/tests/test-confused-revert.t
> @@ -59,8 +59,9 @@
>  Revert should fail:
>  
>    $ hg revert
> +  uncommitted merge
>    abort: no files or directories specified
> -  (use --all to revert all files)
> +  (use --all to discard all changes you might have made)
>    [255]
>  
>  Revert should be ok now:
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -186,7 +186,7 @@
>  
>    $ hg revert -rtip
>    abort: no files or directories specified
> -  (use --all to revert all files)
> +  (use --all to revert all files, or 'hg update -r 1' to update)
>    [255]
>  
>  should succeed
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel


-- 
Mathematics is the supreme nostalgia of our time.