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

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

[Catalin Marinas]

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))"
 '
 

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

[Karl Wiberg]

On Fri, Dec 18, 2009 at 12:22 AM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:

> 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.

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

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

[Catalin Marinas]

2009/12/18 Karl Wiberg <kha@treskal.com>:
> On Fri, Dec 18, 2009 at 12:22 AM, Catalin Marinas
> <catalin.marinas@gmail.com> wrote:
>
>> 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.
>
> 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.

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:
             _print_current_patch(old_applied, self.__applied)
@@ -371,12 +369,10 @@ class StackTransaction(object):
             # We've just caused conflicts, so we must allow them in
             # the final checkout.
             self.__allow_conflicts = lambda trans: True
-
-            # Save this update so that we can run it a little later.
-            self.__conflicting_push = update
+            self.__patches = _TransPatchMap(self.__stack)
+            update()
             self.__halt("%d merge conflict(s)" % len(self.__conflicts))
         else:
-            # Update immediately.
             update()

     def push_tree(self, pn):
diff --git a/t/t3101-reset-hard.sh b/t/t3101-reset-hard.sh
index bd97b3a..45e86dc 100755
--- a/t/t3101-reset-hard.sh
+++ b/t/t3101-reset-hard.sh
@@ -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 - p2 - p3"
+    test "$(echo $(stg series))" = "+ p1 + p2 > p3"
 '

 test_done
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))"
 '


-- 
Catalin

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

[Karl Wiberg]

On Fri, Dec 18, 2009 at 4:49 PM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:

> @@ -371,12 +369,10 @@ class StackTransaction(object):
>             # We've just caused conflicts, so we must allow them in
>             # the final checkout.
>             self.__allow_conflicts = lambda trans: True
> -
> -            # Save this update so that we can run it a little later.
> -            self.__conflicting_push = update
> +            self.__patches = _TransPatchMap(self.__stack)
> +            update()
>             self.__halt("%d merge conflict(s)" % len(self.__conflicts))
>         else:
> -            # Update immediately.
>             update()
>
>     def push_tree(self, pn):

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

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

[Catalin Marinas]

2009/12/19 Karl Wiberg <kha@treskal.com>:
> On Fri, Dec 18, 2009 at 4:49 PM, Catalin Marinas
> <catalin.marinas@gmail.com> wrote:
>
>> @@ -371,12 +369,10 @@ class StackTransaction(object):
>>             # We've just caused conflicts, so we must allow them in
>>             # the final checkout.
>>             self.__allow_conflicts = lambda trans: True
>> -
>> -            # Save this update so that we can run it a little later.
>> -            self.__conflicting_push = update
>> +            self.__patches = _TransPatchMap(self.__stack)
>> +            update()
>>             self.__halt("%d merge conflict(s)" % len(self.__conflicts))
>>         else:
>> -            # Update immediately.
>>             update()
>>
>>     def push_tree(self, pn):
>
> Better. But couldn't you remove the update function completely and
> just inline the code in it, since it's called immediately?

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

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

[Karl Wiberg]

On Mon, Dec 21, 2009 at 12:21 AM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:
> 2009/12/19 Karl Wiberg <kha@treskal.com>:
>
>> Better. But couldn't you remove the update function completely and
>> just inline the code in it, since it's called immediately?
>
> 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.

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

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

[Catalin Marinas]

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

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

[Karl Wiberg]

On Mon, Dec 21, 2009 at 12:48 PM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:

> 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.

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
conflicts---right now, refresh -p can result in three log entries.)

> 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.

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
of the motivation.

> 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.

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

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

[Gustav Hållberg]

On 2009-12-21 14:48, Karl Wiberg wrote:
> 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
> of the motivation.

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

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

[Catalin Marinas]

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:
             _print_current_patch(old_applied, self.__applied)
@@ -358,26 +356,25 @@ class StackTransaction(object):
         elif not merge_conflict and cd.is_nochange():
             s = 'empty'
         out.done(s)
-        def update():
-            if comm:
-                self.patches[pn] = comm
-            if pn in self.hidden:
-                x = self.hidden
-            else:
-                x = self.unapplied
-            del x[x.index(pn)]
-            self.applied.append(pn)
+
         if merge_conflict:
             # We've just caused conflicts, so we must allow them in
             # the final checkout.
             self.__allow_conflicts = lambda trans: True
+            self.__patches = _TransPatchMap(self.__stack)

-            # Save this update so that we can run it a little later.
-            self.__conflicting_push = update
-            self.__halt("%d merge conflict(s)" % len(self.__conflicts))
+        # Update the stack state
+        if comm:
+            self.patches[pn] = comm
+        if pn in self.hidden:
+            x = self.hidden
         else:
-            # Update immediately.
-            update()
+            x = self.unapplied
+        del x[x.index(pn)]
+        self.applied.append(pn)
+
+        if merge_conflict:
+            self.__halt("%d merge conflict(s)" % len(self.__conflicts))

     def push_tree(self, pn):
         """Push the named patch without updating its tree."""
diff --git a/t/t3101-reset-hard.sh b/t/t3101-reset-hard.sh
index bd97b3a..45e86dc 100755
--- a/t/t3101-reset-hard.sh
+++ b/t/t3101-reset-hard.sh
@@ -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 - p2 - p3"
+    test "$(echo $(stg series))" = "+ p1 + p2 > p3"
 '

 test_done
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))"
 '