[PATCH] hgext/mq - idempotent operations should return success

[Alexis S. L. Carvalho]

Thus spake B Thomas:
> Excellent point, thank you.
> 
> Is the attached closer to what you'd like to see ?

Almost (I'll nitpick a bit on some minor stuff):


> # HG changeset patch
> # User bthomas at virtualiron.com

Please also set your name in ~/.hgrc

> # Date 1170863745 18000
> # Node ID bc46c75764c066ed9b0b1ae129f4a521df87db86
> # Parent  5b1f663ef86d68ce11d70de8e5ab61d93341a18c
> Modify qpush/qpop such that the idempotent versions of the operations return success.
> That is, repeated operations qpush -a, qpop -a, qpush patch-name or qpop patch-name
> will return success. The end goal of each of these operations is to reach a particular state.
> Whether or not the patches were already applied does not affect that state or operation
> status.  Likewise, be careful to retain the error status on a qpush without arguments
> when the end of the series has been reached.

Since hg uses the first line as a short description, it's usually better
to have a shorter first line, an empty line and some paragraphs with a
longer description.  Keeping the lines with less than 80 characters
would be better, too (even less than that for the first line).

> 
> Signed-off-by: Ben Thomas (ben at virtualiron.com)

We don't really require Signed-off-by, but I guess it doesn't hurt...

> 
> diff -r 5b1f663ef86d -r bc46c75764c0 hgext/mq.py
> --- a/hgext/mq.py	Tue Feb 06 16:12:22 2007 -0600
> +++ b/hgext/mq.py	Wed Feb 07 10:55:45 2007 -0500
> @@ -802,10 +802,28 @@ class queue:
>          if not wlock:
>              wlock = repo.wlock()
>          patch = self.lookup(patch)
> -        if patch and self.isapplied(patch):
> -            raise util.Abort(_("patch %s is already applied") % patch)
> +        # Suppose our series file is: A B C and the current 'top' patch is B.
> +        # qpush C should be performed (moving forward)
> +        # qpush B is a NOP (no change)
> +        # qpush A is an error (can't go backwards with qpush)
> +        if patch:
> +            info = self.isapplied(patch)
> +            if info:
> +                if info[0] < len(self.applied)-1:
> +                    raise util.Abort(_("Cannot push to a previous patch: %s") % patch)

This would be printed as "abort: Cannot push...", so it'd be better to
lowercase the sentence.  And the line is a bit over 80 characters :)

> +                self.ui.warn('All patches are currently applied\n')

Following the example above, if B is the top patch and you do hg qpush
B, you'll get this message, which is slightly confusing.  If there are
unapplied patches, I think it would be best to have something
symmetrical to qpop's "qpop: patch B is already at the top".  If all
patches really are applied, this message is ok, since we can't
distinguish "qpush -a" from "qpush <top-patch>".

> +                return
> +
> +        # Following the above example, starting at 'top' of B:
> +        #  qpush should be performed (pushes C), but a subsequent qpush without
> +        #  an argument is an error (nothing to apply). This allows a loop
> +        #  of "...while hg qpush..." to work as it detects an error when done
>          if self.series_end() == len(self.series):
> -            raise util.Abort(_("patch series fully applied"))
> +            if not patch:
> +                raise util.Abort("No more patches to apply, already at end of series")
> +            else:
> +                self.ui.warn('Patch series already fully applied\n')
> +                return

I'd prefer something like

    self.ui.warn(_("patch series already fully applied\n"))
    return not patch

which should result in a correct exit code and a message without "abort: ".

>          if not force:
>              self.check_localchanges(repo)
>  
> @@ -847,8 +865,15 @@ class queue:
>              info = self.isapplied(patch)
>              if not info:
>                  raise util.Abort(_("patch %s is not applied") % patch)
> +
>          if len(self.applied) == 0:
> -            raise util.Abort(_("no patches applied"))
> +            # Allow qpop -a or qpop patchname to work repeatedly,
> +            # but not qpop without an argument
> +            if patch or all:
> +                self.ui.warn('There are no patches currently applied\n')
> +                return
> +            else:
> +                raise util.Abort("Cannot pop - no patches are currently applied")

Here I'd also prefer a single self.ui.warn (probably even with the
original message) followed by a "return not (patch or all)".

>  
>          if not update:
>              parents = repo.dirstate.parents()
> @@ -1761,7 +1786,8 @@ def push(ui, repo, patch=None, **opts):
>  
>      if opts['all']:
>          if not q.series:
> -            raise util.Abort(_('no patches in series'))
> +            ui.warn('There are no patches in series\n')
> +            return

As a final general comment, even though mq is not completely consistent,
try to wrap these user-visible messages in _() to keep them marked for
translation.

Thanks for doing this and sorry for the nitpicking :)

Alexis