[StGit PATCH] Convert "pop" to the lib infrastructure

[StGit PATCH] Convert "pop" to the lib infrastructure

[Catalin Marinas]

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/pop.py |   67 ++++++++++++++++++++-----------------------------
 t/t3101-reset-hard.sh |    4 +--
 t/t3103-undo-hard.sh  |    4 +--
 3 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/stgit/commands/pop.py b/stgit/commands/pop.py
index 2c78ac2..eace090 100644
--- a/stgit/commands/pop.py
+++ b/stgit/commands/pop.py
@@ -16,11 +16,10 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
+from stgit.commands import common
+from stgit.lib import transaction
+from stgit import argparse
 from stgit.argparse import opt
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit import argparse, stack, git
 
 help = 'Pop one or more patches from the stack'
 kind = 'stack'
@@ -40,50 +39,40 @@ options = [
     opt('-a', '--all', action = 'store_true',
         short = 'Pop all the applied patches'),
     opt('-n', '--number', type = 'int',
-        short = 'Pop the specified number of patches'),
-    opt('-k', '--keep', action = 'store_true',
-        short = 'Keep the local changes')]
+        short = 'Pop the specified number of patches')
+    ] + argparse.keep_option()
 
-directory = DirectoryGotoToplevel(log = True)
+directory = common.DirectoryHasRepositoryLib()
 
 def func(parser, options, args):
-    """Pop the topmost patch from the stack
-    """
-    check_conflicts()
-    check_head_top_equal(crt_series)
+    """Pop the given patches or the topmost one from the stack."""
+    stack = directory.repository.current_stack
+    iw = stack.repository.default_iw
+    clean_iw = (not options.keep and iw) or None
+    trans = transaction.StackTransaction(stack, 'pop',
+                                         check_clean_iw = clean_iw)
 
-    if not options.keep:
-        check_local_changes()
-
-    applied = crt_series.get_applied()
-    if not applied:
-        raise CmdException, 'No patches applied'
+    if not trans.applied:
+        raise common.CmdException('No patches applied')
 
     if options.all:
-        patches = applied
+        patches = trans.applied
     elif options.number:
         # reverse it twice to also work with negative or bigger than
         # the length numbers
-        patches = applied[::-1][:options.number][::-1]
-    elif len(args) == 0:
-        patches = [applied[-1]]
+        patches = trans.applied[::-1][:options.number][::-1]
+    elif not args:
+        patches = [trans.applied[-1]]
     else:
-        patches = parse_patches(args, applied, ordered = True)
+        patches = common.parse_patches(args, trans.applied, ordered = True)
 
     if not patches:
-        raise CmdException, 'No patches to pop'
-
-    # pop to the most distant popped patch
-    topop = applied[applied.index(patches[0]):]
-    # push those not in the popped range
-    topush = [p for p in topop if p not in patches]
-
-    if options.keep and topush:
-        raise CmdException, 'Cannot pop arbitrary patches with --keep'
-
-    topop.reverse()
-    pop_patches(crt_series, topop, options.keep)
-    if topush:
-        push_patches(crt_series, topush)
-
-    print_crt_patch(crt_series)
+        raise common.CmdException('No patches to pop')
+
+    applied = [p for p in trans.applied if not p in set(patches)]
+    unapplied = patches + trans.unapplied
+    try:
+        trans.reorder_patches(applied, unapplied, iw = iw)
+    except transaction.TransactionException:
+        pass
+    return trans.run(iw)
diff --git a/t/t3101-reset-hard.sh b/t/t3101-reset-hard.sh
index 2807ba3..bd97b3a 100755
--- a/t/t3101-reset-hard.sh
+++ b/t/t3101-reset-hard.sh
@@ -28,7 +28,7 @@ cat > expected.txt <<EOF
 C a
 EOF
 test_expect_success 'Pop middle patch, creating a conflict' '
-    conflict_old stg pop p2 &&
+    conflict stg pop p2 &&
     stg status a > actual.txt &&
     test_cmp expected.txt actual.txt &&
     test "$(echo $(stg series))" = "+ p1 > p3 - p2"
@@ -47,7 +47,7 @@ test_expect_success 'Try to reset with --hard' '
     stg reset --hard master.stgit^~1 &&
     stg status a > actual.txt &&
     test_cmp expected.txt actual.txt &&
-    test "$(echo $(stg series))" = "> p1 - p3 - p2"
+    test "$(echo $(stg series))" = "> p1 - p2 - p3"
 '
 
 test_done
diff --git a/t/t3103-undo-hard.sh b/t/t3103-undo-hard.sh
index 599aa43..ce71668 100755
--- a/t/t3103-undo-hard.sh
+++ b/t/t3103-undo-hard.sh
@@ -28,7 +28,7 @@ cat > expected.txt <<EOF
 C a
 EOF
 test_expect_success 'Pop middle patch, creating a conflict' '
-    conflict_old stg pop p2 &&
+    conflict stg pop p2 &&
     stg status a > actual.txt &&
     test_cmp expected.txt actual.txt &&
     test "$(echo $(stg series))" = "+ p1 > p3 - p2"
@@ -47,7 +47,7 @@ test_expect_success 'Try to undo with --hard' '
     stg undo --hard &&
     stg status a > actual.txt &&
     test_cmp expected.txt actual.txt &&
-    test "$(echo $(stg series))" = "> p1 - p3 - p2"
+    test "$(echo $(stg series))" = "> p1 - p2 - p3"
 '
 
 test_done

Re: [StGit PATCH] Convert "pop" to the lib infrastructure

[Karl Hasselström]

On 2009-03-31 12:30:27 +0100, Catalin Marinas wrote:

> @@ -47,7 +47,7 @@ test_expect_success 'Try to reset with --hard' '
>      stg reset --hard master.stgit^~1 &&
>      stg status a > actual.txt &&
>      test_cmp expected.txt actual.txt &&
> -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
> +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
>  '

Hmm, why this change in behavior? Something that should be noted in
the commit message?

> @@ -47,7 +47,7 @@ test_expect_success 'Try to undo with --hard' '
>      stg undo --hard &&
>      stg status a > actual.txt &&
>      test_cmp expected.txt actual.txt &&
> -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
> +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
>  '

And I guess this is the same.

Otherwise, this looks good.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "pop" to the lib infrastructure

[Catalin Marinas]

2009/4/1 Karl Hasselström <kha@treskal.com>:
> On 2009-03-31 12:30:27 +0100, Catalin Marinas wrote:
>
>> @@ -47,7 +47,7 @@ test_expect_success 'Try to reset with --hard' '
>>      stg reset --hard master.stgit^~1 &&
>>      stg status a > actual.txt &&
>>      test_cmp expected.txt actual.txt &&
>> -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
>> +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
>>  '
>
> Hmm, why this change in behavior? Something that should be noted in
> the commit message?
>
>> @@ -47,7 +47,7 @@ test_expect_success 'Try to undo with --hard' '
>>      stg undo --hard &&
>>      stg status a > actual.txt &&
>>      test_cmp expected.txt actual.txt &&
>> -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
>> +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
>>  '
>
> And I guess this is the same.

I think we now get a slightly different behaviour because of how the
transactions are generated with the new infrastructure. In the above
case, you have "pop p2 p3" and "push p3", the latter failing. The "pop
p2 p3" command results in the stack being "> p1 - p2 - p3" while "push
p3" performs a single step for pushing and reordering. The old push
caused a reorder followed by a push.

So I think I should place the push changes before the pop ones so that
pop itself doesn't fail.

I'll try to push them tonight as I'll go on holiday soon for two weeks.

-- 
Catalin

Re: [StGit PATCH] Convert "pop" to the lib infrastructure

[Karl Hasselström]

On 2009-04-02 17:20:45 +0100, Catalin Marinas wrote:

> 2009/4/1 Karl Hasselström <kha@treskal.com>:
>
> > On 2009-03-31 12:30:27 +0100, Catalin Marinas wrote:
> >
> > > @@ -47,7 +47,7 @@ test_expect_success 'Try to reset with --hard' '
> > >      stg reset --hard master.stgit^~1 &&
> > >      stg status a > actual.txt &&
> > >      test_cmp expected.txt actual.txt &&
> > > -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
> > > +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
> > >  '
> >
> > Hmm, why this change in behavior? Something that should be noted
> > in the commit message?
> >
> > > @@ -47,7 +47,7 @@ test_expect_success 'Try to undo with --hard' '
> > >      stg undo --hard &&
> > >      stg status a > actual.txt &&
> > >      test_cmp expected.txt actual.txt &&
> > > -    test "$(echo $(stg series))" = "> p1 - p3 - p2"
> > > +    test "$(echo $(stg series))" = "> p1 - p2 - p3"
> > >  '
> >
> > And I guess this is the same.
>
> I think we now get a slightly different behaviour because of how the
> transactions are generated with the new infrastructure. In the above
> case, you have "pop p2 p3" and "push p3", the latter failing. The
> "pop p2 p3" command results in the stack being "> p1 - p2 - p3"
> while "push p3" performs a single step for pushing and reordering.
> The old push caused a reorder followed by a push.

Ah, OK. Hmm, I guess either behavior has its pros and cons. (Though I
guess the new behavior -- not changing the order when the push failed
-- might be slightly more intuitive.)

Add that explanation to the commit message, and I'll award you a

Acked-by: Karl Hasselström <kha@treskal.com>

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle