StGit commands resulting in a conflicting patch pushing record two
transactions in the log (with one of them being inconsistent with HEAD
!= top). Undoing such operations requires two "stg undo" (possibly with
--hard) commands which is unintuitive. This patch changes such
operations to only record one log entry and "stg undo" reverts the stack
to the state prior to the operation.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
Cc: Gustav Hållberg <gustav@virtutech.com>
Cc: Karl Wiberg <kha@treskal.com>
---
 stgit/lib/transaction.py |    5 +++--
 t/t3103-undo-hard.sh     |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 30a153b..fad5ab4 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -232,8 +232,9 @@ class StackTransaction(object):
             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:
+        if not self.__conflicting_push:
+            write(self.__msg)
+        else:
             self.__patches = _TransPatchMap(self.__stack)
             self.__conflicting_push()
             write(self.__msg + ' (CONFLICT)')
diff --git a/t/t3103-undo-hard.sh b/t/t3103-undo-hard.sh
index 2d0f382..df14b1f 100755
--- a/t/t3103-undo-hard.sh
+++ b/t/t3103-undo-hard.sh
@@ -46,11 +46,11 @@ 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 &&
-    test "$(echo $(stg series))" = "> p1 - p2 - p3" &&
+    test "$(echo $(stg series))" = "+ p1 + p2 > p3" &&
     test "$(stg id)" = "$(stg id $(stg top))"
 '
 

--

[ message continues ]

On Fri, Dec 18, 2009 at 12:22 AM, Catalin Marinas

Hmm, OK. It was convenient to be able to undo just the last
conflicting step, but I guess the increase in UI complexity wasn't
worth it.

I think your patch doesn't go quite far enough, though.
self.__conflicting_push is currently set to a function that will do
the extra updates that take us from the first to the second state to
save in the log; if we'll be saving at only one point, we might as
well run those updates immediately instead of deferring them. In other
words, the entire __conflicting_push variable could be removed.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle
--

[ message continues ]

See below for an updated patch:


Record a single transaction for conflicting push operations

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

StGit commands resulting in a conflicting patch pushing record two
transactions in the log (with one of them being inconsistent with HEAD
!= top). Undoing such operations requires two "stg undo" (possibly with
--hard) commands which is unintuitive. This patch changes such
operations to only record one log entry and "stg undo" reverts the stack
to the state prior to the operation.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
Cc: Gustav Hållberg <gustav@virtutech.com>
Cc: Karl Wiberg <kha@treskal.com>
---
 stgit/lib/transaction.py |   14 +++++---------
 t/t3101-reset-hard.sh    |    2 +-
 t/t3103-undo-hard.sh     |    4 ++--
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 30a153b..ea85d5d 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -90,7 +90,6 @@ class StackTransaction(object):
         self.__applied = list(self.__stack.patchorder.applied)
         self.__unapplied = list(self.__stack.patchorder.unapplied)
         self.__hidden = list(self.__stack.patchorder.hidden)
-        self.__conflicting_push = None
         self.__error = None
         self.__current_tree = self.__stack.head.data.tree
         self.__base = self.__stack.base
@@ -232,10 +231,9 @@ class StackTransaction(object):
             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:
-            self.__patches = _TransPatchMap(self.__stack)
-            self.__conflicting_push()
+        if not self.__conflicts:
+            write(self.__msg)
+        else:
             write(self.__msg + ' (CONFLICT)')
         if print_current_patch:
             ...
[ message continues ]

On Fri, Dec 18, 2009 at 4:49 PM, Catalin Marinas

Better. But couldn't you remove the update function completely and
just inline the code in it, since it's called immediately?

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle
--

[ message continues ]

Of course, I tried, but couldn't get it to work. I get HEAD and top
not equal unless I call update() between _TransPatchMap and
self.__halt(). For the non-conflicting case we need to call update
before or after this "if merge_conflict".

One solution is to split the "if merge_conflict" in two but maybe you
have a better idea.

Thanks,

-- 
Catalin
--

[ message continues ]

On Mon, Dec 21, 2009 at 12:21 AM, Catalin Marinas

Yes, duplicating the conditional was what I had in mind. But if you
don't find it to improve the readability of the code (as compared to
having a function), I certainly won't insist.

Thanks for working on this.

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.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle
--

[ message continues ]

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 ...
[ message continues ]

On Mon, Dec 21, 2009 at 12:48 PM, Catalin Marinas

Yes. So it won't be affected by whatever you do here. (Unless you
consider that refresh -p needs to reorder patches, which can result in

I've seen more than one complaint that the current behavior is
confusing even if we don't count the bug, so I thought this was part

I haven't seen anyone but me defent the current design, and it's not a
big deal for me either, so I'd say go with just one transaction.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle
--

[ message continues ]

I don't know if this would be better than the other suggested solutions, 
but if "stg log" would clearly identify multi-stage entries as such, the 
current confusion would probably mostly go away.

Currently this is done reasonably well for make_temp_patch(), which says 
"refresh (create temporary patch)" in the log, but I think this could be 
taken further.

For example, if such annotations said "foo: stage N" or similar, 
indicating that this was the Nth step in the "foo" command (think 
"rebase" or whatever), it would be good enough for me least.

- Gustav
--

[ message continues ]

Updated patch below:


Record a single transaction for conflicting push operations

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

StGit commands resulting in a conflicting patch pushing record two
transactions in the log (with one of them being inconsistent with HEAD
!= top). Undoing such operations requires two "stg undo" (possibly with
--hard) commands which is unintuitive. This patch changes such
operations to only record one log entry and "stg undo" reverts the stack
to the state prior to the operation.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
Cc: Gustav Hållberg <gustav@virtutech.com>
Cc: Karl Wiberg <kha@treskal.com>
---
 stgit/lib/transaction.py |   35 ++++++++++++++++-------------------
 t/t3101-reset-hard.sh    |    2 +-
 t/t3103-undo-hard.sh     |    4 ++--
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 30a153b..d82e724 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -90,7 +90,6 @@ class StackTransaction(object):
         self.__applied = list(self.__stack.patchorder.applied)
         self.__unapplied = list(self.__stack.patchorder.unapplied)
         self.__hidden = list(self.__stack.patchorder.hidden)
-        self.__conflicting_push = None
         self.__error = None
         self.__current_tree = self.__stack.head.data.tree
         self.__base = self.__stack.base
@@ -232,10 +231,9 @@ class StackTransaction(object):
             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:
-            self.__patches = _TransPatchMap(self.__stack)
-            self.__conflicting_push()
+        if not self.__conflicts:
+            write(self.__msg)
+        else:
             write(self.__msg + ' (CONFLICT)')
         if print_current_patch:
             ...
[ message continues ]