Re: [RFC PATCH] Record a single transaction for conflicting push operations

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Dec 21, 2009, 11:48 UTC
Message-ID: <b0943d9e0912210348o37b71935x5fad4f1a4be4b70@mail.gmail.com>
In-Reply-To: <b8197bcb0912202308p296207av416cd5590a11251b@mail.gmail.com>

2009/12/21 Karl Wiberg <kha@treskal.com>:

> By the way, you do realize there's another command that requires two
> steps to undo completely: refresh? And that one is harder to get out
> of---undoing it all in one step would mean throwing away the updates
> to the patch.

But it looks to me like refresh does this by running separate transactions. The push command does this in a single transaction, so the quickest fix for the HEAD != top undo problem was to only record one log per transaction.

If we keep the current behaviour with two logs per transaction, we need to preserve the HEAD prior to the conflict so that logging doesn't get the wrong HEAD (which is the new conflicting HEAD currently). The patch below appears to fix this problem and still generate two logs per transaction. While I'm more in favour of a single log per transaction, if people find it useful I'm happy to keep the current behaviour.

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 30a153b..ba97c4f 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -197,18 +197,14 @@ class StackTransaction(object):
         exception) and do nothing."""
         self.__check_consistency()
         log.log_external_mods(self.__stack)
-        new_head = self.head
-
-        # Set branch head.
-        if set_head:
-            if iw:
-                try:
-                    self.__checkout(new_head.data.tree, iw, allow_bad_head)
-                except git.CheckoutException:
-                    # We have to abort the transaction.
-                    self.abort(iw)
-                    self.__abort()
-            self.__stack.set_head(new_head, self.__msg)
+
+        if set_head and iw:
+            try:
+                self.__checkout(self.head.data.tree, iw, allow_bad_head)
+            except git.CheckoutException:
+                # We have to abort the transaction.
+                self.abort(iw)
+                self.__abort()

         if self.__error:
             if self.__conflicts:
@@ -216,8 +212,11 @@ class StackTransaction(object):
             else:
                 out.error(self.__error)

-        # Write patches.
-        def write(msg):
+        # Write patches and update the branch head.
+        def write(msg, new_head):
+            # Set branch head.
+            if new_head:
+                self.__stack.set_head(new_head, self.__msg)
             for pn, commit in self.__patches.iteritems():
                 if self.__stack.patches.exists(pn):
                     p = self.__stack.patches.get(pn)
@@ -231,12 +230,16 @@ class StackTransaction(object):
             self.__stack.patchorder.unapplied = self.__unapplied
             self.__stack.patchorder.hidden = self.__hidden
             log.log_entry(self.__stack, msg)
+
         old_applied = self.__stack.patchorder.applied
-        write(self.__msg)
         if self.__conflicting_push != None:
+            write(self.__msg, set_head and self.head)
             self.__patches = _TransPatchMap(self.__stack)
             self.__conflicting_push()
-            write(self.__msg + ' (CONFLICT)')
+            write(self.__msg + ' (CONFLICT)', set_head and self.head)
+        else:
+            write(self.__msg, set_head and self.head)
+
         if print_current_patch:
             _print_current_patch(old_applied, self.__applied)

@@ -346,10 +349,10 @@ class StackTransaction(object):
             if merge_conflict:
                 # When we produce a conflict, we'll run the update()
                 # function defined below _after_ having done the
-                # checkout in run(). To make sure that we check out
-                # the real stack top (as it will look after update()
-                # has been run), set it hard here.
-                self.head = comm
+                # checkout in run(). Make sure that we have a consistent
+                # HEAD before the update function is called below (which
+                # sets the real HEAD).
+                self.head = self.top
         else:
             comm = None
             s = 'unmodified'
@@ -367,6 +370,8 @@ class StackTransaction(object):
                 x = self.unapplied
             del x[x.index(pn)]
             self.applied.append(pn)
+            # Set the real conflicting HEAD.
+            self.head = comm
         if merge_conflict:
             # We've just caused conflicts, so we must allow them in
             # the final checkout.
diff --git a/t/t3103-undo-hard.sh b/t/t3103-undo-hard.sh
index 2d0f382..a71cd32 100755
--- a/t/t3103-undo-hard.sh
+++ b/t/t3103-undo-hard.sh
@@ -46,7 +46,7 @@ test_expect_success 'Try to undo without --hard' '

 cat > expected.txt <<EOF
 EOF
-test_expect_failure 'Try to undo with --hard' '
+test_expect_success 'Try to undo with --hard' '
     stg undo --hard &&
     stg status a > actual.txt &&
     test_cmp expected.txt actual.txt &&

-- 
Catalin