Re: [StGit PATCH] Convert "sink" to the new infrastructure

["Catalin Marinas" <catalin.marinas@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

2008/9/15 Karl Hasselström <kha@treskal.com>:
> On 2008-09-14 22:19:41 +0100, Catalin Marinas wrote:
>
>> I wasn't used to reading documentation in StGit files :-). Thanks
>> for the info, I'll repost.
>
> It was you who asked for in-code docs. :-) The new-infrastructure code
> actually looks half decent in epydoc nowadays.

Since we are talking about this, the transactions documentation
doesn't explain when to use a iw and when to pass allow_conflicts. I
kind of figured out but I'm not convinced. At a first look, passing
allow_conflicts = True would seem that it may allow conflicts and not
revert the changes, however, this only works if I pass an "iw". But
passing it doesn't allow the default case where I want the changes
reverted.

Please have a look at the attached patch which is my last version of
the sink command rewriting. I'm not that happy (or maybe I don't
understand the reasons) with setting iw = None if not options.conflict
but that's the way I could get it to work.

>> I'll make the default behaviour to cancel the transaction and revert
>> to the original state unless an option is given to allow conflicts.
>
> What I've always wanted is "sink this patch as far as it will go
> without conflicting". This comes awfully close.

But this means that sink would try several consecutive sinks until it
can't find one. Not that it is try to implement but I wouldn't
complicate "sink" for this. I would rather add support for patch
dependency tracking (which used to be on the long term wish list). It
might be useful for other things as well like mailing a patch together
with those on which it depends (like darcs).

> BTW, this kind of flag might potentially be useful in many commands
> (with default value on or off depending on the command). Maybe
>
>  --conflicts=roll-back|stop-before|allow

ATM, I only added a --conflict option which has the "allow" meaning.

-- 
Catalin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: convert-sink-to-the-new-infras.patch --]
[-- Type: text/x-diff; name=convert-sink-to-the-new-infras.patch, Size: 7706 bytes --]

Convert "sink" to the new infrastructure

From: Catalin Marinas <catalin.marinas@gmail.com>

This patch converts the sink command to use stgit.lib. By default, the
command doesn't allow conflicts and it cancels the operations if patches
cannot be reordered cleanly. With the --conflict options, the command
stops after the first conflict during the push operations.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/sink.py |   90 +++++++++++++++++++++++++++++++-----------------
 t/t1501-sink.sh        |   65 +++++++++++++++++++++++++++++------
 2 files changed, 112 insertions(+), 43 deletions(-)

diff --git a/stgit/commands/sink.py b/stgit/commands/sink.py
index d8f79b4..a799433 100644
--- a/stgit/commands/sink.py
+++ b/stgit/commands/sink.py
@@ -1,6 +1,6 @@
 
 __copyright__ = """
-Copyright (C) 2007, Yann Dirson <ydirson@altern.org>
+Copyright (C) 2008, Catalin Marinas <catalin.marinas@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 2 as
@@ -16,13 +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 optparse import OptionParser, make_option
-
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit import stack, git
+from optparse import make_option
 
+from stgit.commands import common
+from stgit.lib import transaction
 
 help = 'send patches deeper down the stack'
 usage = """%prog [-t <target patch>] [-n] [<patches>]
@@ -32,43 +29,72 @@ push the specified <patches> (the current patch by default), and
 then push back into place the formerly-applied patches (unless -n
 is also given)."""
 
-directory = DirectoryGotoToplevel()
+directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-n', '--nopush',
                        help = 'do not push the patches back after sinking',
                        action = 'store_true'),
            make_option('-t', '--to', metavar = 'TARGET',
-                       help = 'sink patches below TARGET patch')]
+                       help = 'sink patches below TARGET patch'),
+           make_option('-c', '--conflict',
+                       help = 'allow conflicts during the push operations',
+                       action = 'store_true')]
 
 def func(parser, options, args):
     """Sink patches down the stack.
     """
+    stack = directory.repository.current_stack
 
-    check_local_changes()
-    check_conflicts()
-    check_head_top_equal(crt_series)
-
-    oldapplied = crt_series.get_applied()
-    unapplied = crt_series.get_unapplied()
-    all = unapplied + oldapplied
-
-    if options.to and not options.to in oldapplied:
-        raise CmdException('Cannot sink below %s, since it is not applied'
-                           % options.to)
+    if options.to and not options.to in stack.patchorder.applied:
+        raise common.CmdException('Cannot sink below %s since it is not applied'
+                                  % options.to)
 
     if len(args) > 0:
-        patches = parse_patches(args, all)
+        patches = common.parse_patches(args, stack.patchorder.all)
     else:
-        current = crt_series.get_current()
-        if not current:
-            raise CmdException('No patch applied')
-        patches = [current]
+        # current patch
+        patches = list(stack.patchorder.applied[-1:])
 
-    if oldapplied:
-        crt_series.pop_patch(options.to or oldapplied[0])
-    push_patches(crt_series, patches)
+    if not patches:
+        raise common.CmdException('No patches to sink')
+    if options.to and options.to in patches:
+        raise common.CmdException('Cannot have a sinked patch as target')
+
+    if options.conflict:
+        iw = stack.repository.default_iw
+    else:
+        iw = None
+    trans = transaction.StackTransaction(stack, 'sink')
+
+    # pop any patches to be sinked in case they are applied
+    to_push = trans.pop_patches(lambda pn: pn in patches)
+
+    if options.to:
+        if options.to in to_push:
+            # this is the case where sinking actually brings some
+            # patches forward
+            for p in to_push:
+                if p == options.to:
+                    del to_push[:to_push.index(p)]
+                    break
+                trans.push_patch(p, iw)
+        else:
+            # target patch below patches to be sinked
+            to_pop = trans.applied[trans.applied.index(options.to):]
+            to_push = to_pop + to_push
+            trans.pop_patches(lambda pn: pn in to_pop)
+    else:
+        # pop all the remaining patches
+        to_push = trans.applied + to_push
+        trans.pop_patches(lambda pn: True)
 
+    # push the sinked and other popped patches
     if not options.nopush:
-        newapplied = crt_series.get_applied()
-        def not_reapplied_yet(p):
-            return not p in newapplied
-        push_patches(crt_series, filter(not_reapplied_yet, oldapplied))
+        patches.extend(to_push)
+    try:
+        for p in patches:
+            trans.push_patch(p, iw)
+    except transaction.TransactionHalted:
+        if not options.conflict:
+            raise
+
+    return trans.run(iw)
diff --git a/t/t1501-sink.sh b/t/t1501-sink.sh
index 32931cd..b3e2eb3 100755
--- a/t/t1501-sink.sh
+++ b/t/t1501-sink.sh
@@ -5,24 +5,67 @@ test_description='Test "stg sink"'
 . ./test-lib.sh
 
 test_expect_success 'Initialize StGit stack' '
-    echo 000 >> x &&
-    git add x &&
+    echo 0 >> f0 &&
+    git add f0 &&
     git commit -m initial &&
-    echo 000 >> y &&
-    git add y &&
-    git commit -m y &&
+    echo 1 >> f1 &&
+    git add f1 &&
+    git commit -m p1 &&
+    echo 2 >> f2 &&
+    git add f2 &&
+    git commit -m p2 &&
+    echo 3 >> f3 &&
+    git add f3 &&
+    git commit -m p3 &&
+    echo 4 >> f4 &&
+    git add f4 &&
+    git commit -m p4 &&
+    echo 22 >> f2 &&
+    git add f2 &&
+    git commit -m p22 &&
     stg init &&
-    stg uncommit &&
-    stg pop
+    stg uncommit p22 p4 p3 p2 p1 &&
+    stg pop -a
 '
 
-test_expect_success 'sink without applied patches' '
+test_expect_success 'sink default without applied patches' '
     command_error stg sink
 '
 
-test_expect_success 'sink a specific patch without applied patches' '
-    stg sink y &&
-    test $(echo $(stg series --applied --noprefix)) = "y"
+test_expect_success 'sink and reorder specified without applied patches' '
+    stg sink p2 p1 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p2 p1"
+'
+
+test_expect_success 'sink patches to the bottom of the stack' '
+    stg sink p4 p3 p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p4 p3 p2 p1"
+'
+
+test_expect_success 'sink current below a target' '
+    stg sink --to=p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p4 p3 p1 p2"
+'
+
+test_expect_success 'bring patches forward' '
+    stg sink --to=p2 p3 p4 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p3 p4 p2"
+'
+
+test_expect_success 'sink specified patch below a target' '
+    stg sink --to=p3 p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p2 p3 p4"
+'
+
+test_expect_success 'sink with conflict and restore the stack' '
+    command_error stg sink --to=p2 p22 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p2 p3 p4"
+'
+
+test_expect_success 'sink with conflict and do not restore the stack' '
+    conflict stg sink --conflict --to=p2 p22 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p22" &&
+    test "$(echo $(stg status --conflict))" = "f2"
 '
 
 test_done