[PATCH] hgext/mq - idempotent operations should return success
B Thomas
bjthomas3 at gmail.com
Tue Feb 13 14:11:06 UTC 2007
> Thanks for doing this and sorry for the nitpicking :)
I appreciate the nitpicking as the end goal is the best possible patch.
Patches, IMHO, should be "invisible" after application. I think that I've
addressed the items (with one exception, see below) that you've mentioned
and the new attempt is attached. Once we've reached some sort of final
version of this, I have additional patches that I've been using to implement
qstatus and qfiles commands. Assuming interest, I'll post them as well.
The single misunderstanding on my part is in the qpop change. You suggest:
> Here I'd also prefer a single self.ui.warn (probably even with the
> original message) followed by a "return not (patch or all)".
I was unclear on what you meant by "single". I modified the message and
return, but... what else ?
I appreciate your feedback. Thanks for your time and effort.
-b
On 2/13/07, Alexis S. L. Carvalho <alexis at cecm.usp.br> wrote:
>
> 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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20070213/08e274e5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mq-idempotent.patch
Type: text/x-patch
Size: 3660 bytes
Desc: not available
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20070213/08e274e5/attachment.bin>
More information about the Mercurial-devel
mailing list