threads / patch / 14666

patch, 3 partsFix uncommit with top != head

Subject: [StGit PATCH 0/3] Fix uncommit with top != head

## tl;dr

4 messages between Jul 25, 2008 and Jul 25, 2008. Diffs are folded; open one to read it.

replies: 3people: 1as markdown or json

Karl Hasselström· Jul 25, 2008, 00:52 UTC · lore

See http://gna.org/bugs/?12043

---

Karl Hasselström (3):
      Make sure that stg uncommit doesn't touch the branch head
      stg uncommit should never touch the branch head
      Test for exit code with command_error()

 stgit/commands/uncommit.py |    2 +-
 stgit/lib/transaction.py   |   19 ++++++++++---------
 t/t1300-uncommit.sh        |   13 ++++++++++++-
 t/t1303-commit.sh          |   20 ++++++++++++++++++++
 4 files changed, 43 insertions(+), 11 deletions(-)
 create mode 100755 t/t1303-commit.sh

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

Karl Hasselström· Jul 25, 2008, 00:52 UTC · re: Karl Hasselström · lore

[StGit PATCH 1/3] Test for exit code with command_error()

The helper function was made for occasions such as this, so use it.

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

---

 t/t1300-uncommit.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to t/t1300-uncommit.sh +1 −1

diff --git a/t/t1300-uncommit.sh b/t/t1300-uncommit.sh
index a906d13..a657ead 100755
--- a/t/t1300-uncommit.sh
+++ b/t/t1300-uncommit.sh
@@ -79,7 +79,7 @@ test_expect_success \
 '
 
 test_expect_success 'Uncommit a commit with not precisely one parent' '
-    stg uncommit -n 5 ; [ $? = 2 ] &&
+    command_error stg uncommit -n 5  &&
     [ "$(echo $(stg series))" = "" ]
 '

Karl Hasselström· Jul 25, 2008, 00:53 UTC · re: Karl Hasselström · lore

[StGit PATCH 2/3] stg uncommit should never touch the branch head

However, currently, it will set head to top, potentially losing data (which can always be recovered via the reflog, but still). See https://gna.org/bugs/index.php?12043. Add a test to demonstrate the bad behavior. (Bug discovered by Erik Sandberg <mandolaerik@gmail.com>.)

stg commit, on the other hand, should refuse to run if top != head, since the committed patches might otherwise be lost. Add a test to demonstrate that this is the case.

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

---

 t/t1300-uncommit.sh |   11 +++++++++++
 t/t1303-commit.sh   |   20 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)
 create mode 100755 t/t1303-commit.sh

Show changes to 2 files +31 −0

t/t1300-uncommit.sh, t/t1303-commit.sh

diff --git a/t/t1300-uncommit.sh b/t/t1300-uncommit.sh
index a657ead..d01eaaa 100755
--- a/t/t1300-uncommit.sh
+++ b/t/t1300-uncommit.sh
@@ -83,4 +83,15 @@ test_expect_success 'Uncommit a commit with not precisely one parent' '
     [ "$(echo $(stg series))" = "" ]
 '
 
+# stg uncommit should work even when top != head, and should not touch
+# the head.
+test_expect_failure 'Uncommit when top != head' '
+    stg new -m foo &&
+    git reset --hard HEAD^ &&
+    h=$(git rev-parse HEAD)
+    stg uncommit bar &&
+    test $(git rev-parse HEAD) = $h &&
+    test "$(echo $(stg series))" = "+ bar > foo"
+'
+
 test_done
diff --git a/t/t1303-commit.sh b/t/t1303-commit.sh
new file mode 100755
index 0000000..d53b9f2
--- /dev/null
+++ b/t/t1303-commit.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+test_description='Test stg commit'
+. ./test-lib.sh
+
+test_expect_success 'Initialize the StGIT repository' '
+    stg init
+'
+
+# stg commit with top != head should not succeed, since the committed
+# patches are poptentially lost.
+test_expect_success 'Commit when top != head (should fail)' '
+    stg new -m foo &&
+    git reset --hard HEAD^ &&
+    h=$(git rev-parse HEAD)
+    command_error stg commit &&
+    test $(git rev-parse HEAD) = $h &&
+    test "$(echo $(stg series))" = "> foo"
+'
+
+test_done

Karl Hasselström· Jul 25, 2008, 00:53 UTC · re: Karl Hasselström · lore

[StGit PATCH 3/3] Make sure that stg uncommit doesn't touch the branch head

Even if top != head. It used to set head to top; but with this patch, it doesn't anymore.

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

---

 stgit/commands/uncommit.py |    2 +-
 stgit/lib/transaction.py   |   19 ++++++++++---------
 t/t1300-uncommit.sh        |    2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

Show changes to 3 files +12 −11

stgit/commands/uncommit.py, stgit/lib/transaction.py, t/t1300-uncommit.sh

diff --git a/stgit/commands/uncommit.py b/stgit/commands/uncommit.py
index eb39fcc..9d2dba9 100644
--- a/stgit/commands/uncommit.py
+++ b/stgit/commands/uncommit.py
@@ -136,5 +136,5 @@ def func(parser, options, args):
     for commit, pn in zip(commits, patchnames):
         trans.patches[pn] = commit
     trans.applied = list(reversed(patchnames)) + trans.applied
-    trans.run()
+    trans.run(set_head = False)
     out.done()
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index e47997e..23321c7 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -138,21 +138,22 @@ class StackTransaction(object):
         # The only state we need to restore is index+worktree.
         if iw:
             self.__checkout(self.__stack.head.data.tree, iw)
-    def run(self, iw = None):
+    def run(self, iw = None, set_head = True):
         """Execute the transaction. Will either succeed, or fail (with an
         exception) and do nothing."""
         self.__check_consistency()
         new_head = self.__head
 
         # Set branch head.
-        if iw:
-            try:
-                self.__checkout(new_head.data.tree, iw)
-            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:
+            if iw:
+                try:
+                    self.__checkout(new_head.data.tree, iw)
+                except git.CheckoutException:
+                    # We have to abort the transaction.
+                    self.abort(iw)
+                    self.__abort()
+            self.__stack.set_head(new_head, self.__msg)
 
         if self.__error:
             out.error(self.__error)
diff --git a/t/t1300-uncommit.sh b/t/t1300-uncommit.sh
index d01eaaa..4a955f6 100755
--- a/t/t1300-uncommit.sh
+++ b/t/t1300-uncommit.sh
@@ -85,7 +85,7 @@ test_expect_success 'Uncommit a commit with not precisely one parent' '
 
 # stg uncommit should work even when top != head, and should not touch
 # the head.
-test_expect_failure 'Uncommit when top != head' '
+test_expect_success 'Uncommit when top != head' '
     stg new -m foo &&
     git reset --hard HEAD^ &&
     h=$(git rev-parse HEAD)