[PATCH] qpush --move: move the right patch even with comment lines

[Mads Kiilerich]

  Gilles Moris wrote, On 07/07/2010 09:05 AM:
> # HG changeset patch
> # User Gilles Moris<gilles.moris at free.fr>
> # Date 1278484536 -7200
> # Node ID bb6b602014b381955a4f9746bc9be586f4a16f0b
> # Parent  33175ca5db9136cfec76b7d28c2da7f876ca63c6
> qpush --move: move the right patch even with comment lines
>
> 88fc876a4833 caused that we find the index of the moving patch in self.series
> but look it up in self.full_series. The difference between these is that
> full_series also contains comment lines, and we thus moved the wrong patch.
>
> Use back self.full_series to find the moving patch, but take care of striping
> the patch guard markers before comparing the patch name. Test cases have been
> added for comments and empty lines in self.full_series, and for the case of
> guarded patches.
>
> Original patch contributed by Mads Kiilerich<mads at kiilerich.com>

That's an exaggeration ;-)

> diff -r 33175ca5db91 -r bb6b602014b3 hgext/mq.py
> --- a/hgext/mq.py	Thu Feb 12 22:55:51 2009 +0100
> +++ b/hgext/mq.py	Wed Jul 07 08:35:36 2010 +0200
> @@ -1047,7 +1047,12 @@
>
>               if move:
>                   try:
> -                    index = self.series.index(patch, start)
> +                    for i, rpn in enumerate(self.full_series[start:]):

Perhaps the simpler (but probably slightly less scalable)

for index, rpn in enumerate(self.full_series)[start:]:


> +                        # strip markers for patch guards
> +                        pn = rpn.split('#', 1)[0].rstrip()
> +                        if pn == patch:
> +                            break
> +                    index = start + i
>                       fullpatch = self.full_series[index]
>                       del self.full_series[index]
>                   except ValueError:

The try/except ValueError was a relatively elegant way to handle errors 
originally. But it looks a bit clumsy now. Couldn't it be done in a 
better way now with a simple if?

And so much happens inside the try that I wonder in which cases it could 
be trigged - and if we can reach "not found" at all and how that is 
distinguished from the specifying the last patch in the series. Perhaps 
we should add a test for "not found" and make sure we try to move that 
(and also the only) patch.

> diff -r 33175ca5db91 -r bb6b602014b3 tests/test-mq
> --- a/tests/test-mq	Thu Feb 12 22:55:51 2009 +0100
> +++ b/tests/test-mq	Wed Jul 07 08:35:36 2010 +0200
> @@ -104,6 +104,11 @@
>
>   hg qnew -m 'foo bar' test.patch
>
> +echo '# comment'>  .hg/patches/series.tmp
> +echo>>  .hg/patches/series.tmp # empty line
> +cat .hg/patches/series>>  .hg/patches/series.tmp
> +mv .hg/patches/series.tmp .hg/patches/series
> +
>   echo % qrefresh
>
>   echo a>>  a
> @@ -225,17 +230,28 @@
>
>   echo % qpush --move
>   hg qpop -a
> +hg qguard test1b.patch -- -negguard
> +hg qguard test2.patch -- +posguard
> +hg qpush --move test2.patch # can't move guarded patch
> +hg qselect posguard
>   hg qpush --move test2.patch # move to front
> -hg qpush --move test1b.patch
> +hg qpush --move test1b.patch # negative guard unselected
>   hg qpush --move test.patch # noop move
>   hg qseries -v
>   hg qpop -a
> -hg qpush --move test.patch # cleaning up
> +# cleaning up
> +hg qselect --none
> +hg qguard --none test1b.patch
> +hg qguard --none test2.patch
> +hg qpush --move test.patch
>   hg qpush --move test1b.patch
>   hg qpush --move bogus # nonexistent patch
>   hg qpush --move test.patch # already applied
>   hg qpush
>
> +echo % series after move
> +cat `hg root`/.hg/patches/series
> +
>   echo % pop, qapplied, qunapplied
>   hg qseries -v
>   echo % qapplied -1 test.patch
> diff -r 33175ca5db91 -r bb6b602014b3 tests/test-mq.out
> --- a/tests/test-mq.out	Thu Feb 12 22:55:51 2009 +0100
> +++ b/tests/test-mq.out	Wed Jul 07 08:35:36 2010 +0200
> @@ -204,6 +204,8 @@
>   popping test1b.patch
>   popping test.patch
>   patch queue now empty
> +cannot push 'test2.patch' - guarded by ['+posguard']

That output (revealing python list syntax) looks unfortunate. It seems 
like we found another bug?

> +number of unguarded, unapplied patches has changed from 2 to 3
>   applying test2.patch
>   now at: test2.patch
>   applying test1b.patch
> @@ -217,6 +219,8 @@
>   popping test1b.patch
>   popping test2.patch
>   patch queue now empty
> +guards deactivated
> +number of unguarded, unapplied patches has changed from 3 to 2
>   applying test.patch
>   now at: test.patch
>   applying test1b.patch
> @@ -225,6 +229,12 @@
>   abort: cannot push to a previous patch: test.patch
>   applying test2.patch
>   now at: test2.patch
> +% series after move
> +test.patch
> +test1b.patch
> +test2.patch
> +# comment
> +
>   % pop, qapplied, qunapplied
>   0 A test.patch
>   1 A test1b.patch
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel