[StGit PATCH 0/6] Two bugfixes

[StGit PATCH 0/6] Two bugfixes

[Karl Hasselström]

This series fixes one rather benign bug (4/6) and one that caused
patches to become empty, which is rather worse (6/6). (The patch
contents could still be recovered via the patch log or reflog, but
it's still a major inconvenience.)

Both bugs were discovered by Erik Sandberg.

These patches are also available in the kha/safe branch.

---

Karl Hasselström (6):
      Handle failed pushes differently depending on cause
      New test: conflicting push in dirty worktree
      Make sure that we only uncommit commits with exactly one parent
      Try uncommitting a commit with not exactly one parent
      Make sure patches with no parents have an empty list of parents
      Use a special exit code for bugs


 stgit/commands/uncommit.py |   14 ++++++++++++--
 stgit/lib/git.py           |   23 +++++++++++++++--------
 stgit/lib/transaction.py   |    4 +++-
 stgit/main.py              |   11 +++++++----
 stgit/utils.py             |    1 +
 t/t1300-uncommit.sh        |    5 +++++
 t/t3000-dirty-merge.sh     |   35 +++++++++++++++++++++++++++++++++++
 7 files changed, 78 insertions(+), 15 deletions(-)
 create mode 100755 t/t3000-dirty-merge.sh

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

[StGit PATCH 1/6] Use a special exit code for bugs

[Karl Hasselström]

Use a special exit code (4) for any error condition that indicates a
bug in StGit. This will be useful in the test suite.

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

---

 stgit/main.py  |   11 +++++++----
 stgit/utils.py |    1 +
 2 files changed, 8 insertions(+), 4 deletions(-)


diff --git a/stgit/main.py b/stgit/main.py
index 79044b0..663fdec 100644
--- a/stgit/main.py
+++ b/stgit/main.py
@@ -18,7 +18,7 @@ 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
+import sys, os, traceback
 from optparse import OptionParser
 
 import stgit.commands
@@ -279,10 +279,13 @@ def main():
     except (StgException, IOError, ParsingError, NoSectionError), err:
         out.error(str(err), title = '%s %s' % (prog, cmd))
         if debug_level > 0:
-            raise
-        else:
-            sys.exit(utils.STGIT_COMMAND_ERROR)
+            traceback.print_exc()
+        sys.exit(utils.STGIT_COMMAND_ERROR)
     except KeyboardInterrupt:
         sys.exit(utils.STGIT_GENERAL_ERROR)
+    except:
+        out.error('Unhandled exception:')
+        traceback.print_exc()
+        sys.exit(utils.STGIT_BUG_ERROR)
 
     sys.exit(ret or utils.STGIT_SUCCESS)
diff --git a/stgit/utils.py b/stgit/utils.py
index b75c3b4..cd52382 100644
--- a/stgit/utils.py
+++ b/stgit/utils.py
@@ -327,6 +327,7 @@ STGIT_SUCCESS = 0        # everything's OK
 STGIT_GENERAL_ERROR = 1  # seems to be non-command-specific error
 STGIT_COMMAND_ERROR = 2  # seems to be a command that failed
 STGIT_CONFLICT = 3       # merge conflict, otherwise OK
+STGIT_BUG_ERROR = 4      # a bug in StGit
 
 def strip_leading(prefix, s):
     """Strip leading prefix from a string. Blow up if the prefix isn't

[StGit PATCH 2/6] Make sure patches with no parents have an empty list of parents

[Karl Hasselström]

They used to have None instead of an empty list, which was
inconsistent. (It went undetected for quite a while because StGit
seldom needs to handle initial commits.)

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

---

 stgit/lib/git.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 50dc4f1..cdfe885 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -204,7 +204,7 @@ class Commitdata(Repr):
                 ) % (tree, parents, self.author, self.committer, self.message)
     @classmethod
     def parse(cls, repository, s):
-        cd = cls()
+        cd = cls(parents = [])
         lines = list(s.splitlines(True))
         for i in xrange(len(lines)):
             line = lines[i].strip()

[StGit PATCH 3/6] Try uncommitting a commit with not exactly one parent

[Karl Hasselström]

This should fail cleanly -- and in fact it does. Except for printing
an assertion backtrace instead of a nice error message. (This is a
regression introduced by the conversion of "stg uncommit" to the new
infrastructure.)

Found by Erik Sandberg <mandolaerik@gmail.com>.

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

---

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


diff --git a/t/t1300-uncommit.sh b/t/t1300-uncommit.sh
index d86e579..0d952a7 100755
--- a/t/t1300-uncommit.sh
+++ b/t/t1300-uncommit.sh
@@ -78,4 +78,9 @@ test_expect_success \
     stg commit --all
 '
 
+test_expect_failure 'Uncommit a commit with not precisely one parent' '
+    stg uncommit -n 5 ; [ $? = 2 ] &&
+    [ "$(echo $(stg series))" = "" ]
+'
+
 test_done

[StGit PATCH 4/6] Make sure that we only uncommit commits with exactly one parent

[Karl Hasselström]

If we encounter a commit with 0, or 2 or more parents, fail with a
nice error message instead of crashing.

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

---

 stgit/commands/uncommit.py |   14 ++++++++++++--
 t/t1300-uncommit.sh        |    2 +-
 2 files changed, 13 insertions(+), 3 deletions(-)


diff --git a/stgit/commands/uncommit.py b/stgit/commands/uncommit.py
index 933ec60..272c5db 100644
--- a/stgit/commands/uncommit.py
+++ b/stgit/commands/uncommit.py
@@ -85,13 +85,23 @@ def func(parser, options, args):
         patchnames = args
         patch_nr = len(patchnames)
 
+    def get_parent(c):
+        next = c.data.parents
+        try:
+            [next] = next
+        except ValueError:
+            raise common.CmdException(
+                'Trying to uncommit %s, which does not have exactly one parent'
+                % c.sha1)
+        return next
+
     commits = []
     next_commit = stack.base
     if patch_nr:
         out.start('Uncommitting %d patches' % patch_nr)
         for i in xrange(patch_nr):
             commits.append(next_commit)
-            next_commit = next_commit.data.parent
+            next_commit = get_parent(next_commit)
     else:
         if options.exclusive:
             out.start('Uncommitting to %s (exclusive)' % to_commit)
@@ -103,7 +113,7 @@ def func(parser, options, args):
                     commits.append(next_commit)
                 break
             commits.append(next_commit)
-            next_commit = next_commit.data.parent
+            next_commit = get_parent(next_commit)
         patch_nr = len(commits)
 
     taken_names = set(stack.patchorder.applied + stack.patchorder.unapplied)
diff --git a/t/t1300-uncommit.sh b/t/t1300-uncommit.sh
index 0d952a7..a906d13 100755
--- a/t/t1300-uncommit.sh
+++ b/t/t1300-uncommit.sh
@@ -78,7 +78,7 @@ test_expect_success \
     stg commit --all
 '
 
-test_expect_failure 'Uncommit a commit with not precisely one parent' '
+test_expect_success 'Uncommit a commit with not precisely one parent' '
     stg uncommit -n 5 ; [ $? = 2 ] &&
     [ "$(echo $(stg series))" = "" ]
 '

[StGit PATCH 5/6] New test: conflicting push in dirty worktree

[Karl Hasselström]

When the result of a conflicting push can't be represented in the
worktree because the worktree is dirty, the push should be aborted.
Similarly, the push should be aborted if we have to do the merge in
the worktree, but can't because the worktree is dirty.

Add a new test that tests for this. It currently fails, in a bad way:
the contents of the pushed patch is lost.

(The test uses goto instead of push, because push doesn't use the new
infrastructure yet. And old-infrastructure commands never have this
bug, because they refuse to run with a dirty worktree.)

This bug was found by Erik Sandberg <mandolaerik@gmail.com>, who also
came up with the minimal test case that I turned into this new test.

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

---

 t/t3000-dirty-merge.sh |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100755 t/t3000-dirty-merge.sh


diff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh
new file mode 100755
index 0000000..dd81c9e
--- /dev/null
+++ b/t/t3000-dirty-merge.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description='Try a push that requires merging a file that is dirty'
+
+. ./test-lib.sh
+
+test_expect_success 'Initialize StGit stack with two patches' '
+    stg init &&
+    touch a &&
+    git add a &&
+    git commit -m a &&
+    echo 1 > a &&
+    git commit -a -m p1 &&
+    echo 2 > a &&
+    git commit -a -m p2 &&
+    stg uncommit -n 2
+'
+
+test_expect_success 'Pop one patch and update the other' '
+    stg goto p1 &&
+    echo 3 > a &&
+    stg refresh
+'
+
+test_expect_failure 'Push with dirty worktree' '
+    echo 4 > a &&
+    [ "$(echo $(stg applied))" = "p1" ] &&
+    [ "$(echo $(stg unapplied))" = "p2" ] &&
+    ! stg goto p2 &&
+    [ "$(echo $(stg applied))" = "p1" ] &&
+    [ "$(echo $(stg unapplied))" = "p2" ] &&
+    [ "$(echo $(cat a))" = "4" ]
+'
+
+test_done

[StGit PATCH 6/6] Handle failed pushes differently depending on cause

[Karl Hasselström]

Specifically, if the push failed because of a merge conflict, the
patch should be applied but empty; and if it fails for any other
reason (such as a too-dirty worktree), the patch should not be
applied.

This fixes the data loss bug tested for by t3000.

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

---

 stgit/lib/git.py         |   21 ++++++++++++++-------
 stgit/lib/transaction.py |    4 +++-
 t/t3000-dirty-merge.sh   |    2 +-
 3 files changed, 18 insertions(+), 9 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index cdfe885..35b9bbf 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -433,6 +433,9 @@ class Repository(RunWithEnv):
 class MergeException(exception.StgException):
     pass
 
+class MergeConflictException(MergeException):
+    pass
+
 class Index(RunWithEnv):
     def __init__(self, repository, filename):
         self.__repository = repository
@@ -517,14 +520,18 @@ class IndexAndWorktree(RunWithEnv):
         assert isinstance(ours, Tree)
         assert isinstance(theirs, Tree)
         try:
-            self.run(['git', 'merge-recursive', base.sha1, '--', ours.sha1,
-                      theirs.sha1],
-                     env = { 'GITHEAD_%s' % base.sha1: 'ancestor',
-                             'GITHEAD_%s' % ours.sha1: 'current',
-                             'GITHEAD_%s' % theirs.sha1: 'patched'}
-                     ).cwd(self.__worktree.directory).discard_output()
+            r = self.run(['git', 'merge-recursive', base.sha1, '--', ours.sha1,
+                          theirs.sha1],
+                         env = { 'GITHEAD_%s' % base.sha1: 'ancestor',
+                                 'GITHEAD_%s' % ours.sha1: 'current',
+                                 'GITHEAD_%s' % theirs.sha1: 'patched'}
+                         ).cwd(self.__worktree.directory)
+            r.discard_output()
         except run.RunException, e:
-            raise MergeException('Index/worktree dirty')
+            if r.exitcode == 1:
+                raise MergeConflictException()
+            else:
+                raise MergeException('Index/worktree dirty')
     def changed_files(self):
         return self.run(['git', 'diff-files', '--name-only']).output_lines()
     def update_index(self, files):
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 3613b15..2946a67 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -197,10 +197,12 @@ class StackTransaction(object):
                 tree = iw.index.write_tree()
                 self.__current_tree = tree
                 s = ' (modified)'
-            except git.MergeException:
+            except git.MergeConflictException:
                 tree = ours
                 merge_conflict = True
                 s = ' (conflict)'
+            except git.MergeException, e:
+                self.__halt(str(e))
         cd = cd.set_tree(tree)
         self.patches[pn] = self.__stack.repository.commit(cd)
         del self.unapplied[self.unapplied.index(pn)]
diff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh
index dd81c9e..d87bba1 100755
--- a/t/t3000-dirty-merge.sh
+++ b/t/t3000-dirty-merge.sh
@@ -22,7 +22,7 @@ test_expect_success 'Pop one patch and update the other' '
     stg refresh
 '
 
-test_expect_failure 'Push with dirty worktree' '
+test_expect_success 'Push with dirty worktree' '
     echo 4 > a &&
     [ "$(echo $(stg applied))" = "p1" ] &&
     [ "$(echo $(stg unapplied))" = "p2" ] &&

Re: [StGit PATCH 0/6] Two bugfixes

[Catalin Marinas]

On 20/03/2008, Karl Hasselström <kha@treskal.com> wrote:
> This series fixes one rather benign bug (4/6) and one that caused
>  patches to become empty, which is rather worse (6/6). (The patch
>  contents could still be recovered via the patch log or reflog, but
>  it's still a major inconvenience.)
>
>  Both bugs were discovered by Erik Sandberg.

Thanks to both Erik and Karl for finding and fixing this. As I wrote
on the patch system, I'd like to put back the explicit --keep option
in goto.

BTW, I have about 5 patches that apply to the stable and master
branch, mainly UI updates I needed recently (like picking multiple
patches at once). Since the master branch still needs some work (which
I'll try to help with), maybe it's worth releasing a 0.14.2, together
with some of the bugs reported so far.

-- 
Catalin

Re: [StGit PATCH 0/6] Two bugfixes

[Karl Hasselström]

On 2008-03-20 15:19:12 +0000, Catalin Marinas wrote:

> BTW, I have about 5 patches that apply to the stable and master
> branch, mainly UI updates I needed recently (like picking multiple
> patches at once). Since the master branch still needs some work
> (which I'll try to help with), maybe it's worth releasing a 0.14.2,
> together with some of the bugs reported so far.

Absolutely. I think it would be worthwhile to treat "stable" much like
Junio handles his "maint" -- apply minor and/or important fixes there,
and release minor releases from it somewhat frequently. And merge it
to master often, so that master always has everything stable has.

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

Re: [StGit PATCH 0/6] Two bugfixes

[Catalin Marinas]

On 24/03/2008, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-03-20 15:19:12 +0000, Catalin Marinas wrote:
>
>  > BTW, I have about 5 patches that apply to the stable and master
>  > branch, mainly UI updates I needed recently (like picking multiple
>  > patches at once). Since the master branch still needs some work
>  > (which I'll try to help with), maybe it's worth releasing a 0.14.2,
>  > together with some of the bugs reported so far.
>
>
> Absolutely. I think it would be worthwhile to treat "stable" much like
>  Junio handles his "maint" -- apply minor and/or important fixes there,
>  and release minor releases from it somewhat frequently. And merge it
>  to master often, so that master always has everything stable has.

I'll try to release 0.14.2 today or tomorrow as I'll be on holiday
afterwards for 2.5 weeks.

BTW, on the master branch, I noticed that
StackTransaction.push_patch() checks whether the patch is empty before
pushing it. The behaviour on stable is that it reports whether it's
empty after push (just like "modified", so that you know that a patch
no longer has data as a result of the merge).

On the merging side during push, we have to add the plain patch
applying first followed by a three-way merge. Do we ever need the
Index.merge() function (implemented with git-read-tree) or should we
should always uses the git-recursive-merge in
IndexAndWorktree.merge()?

-- 
Catalin

Re: [StGit PATCH 0/6] Two bugfixes

[Karl Hasselström]

On 2008-03-20 15:19:12 +0000, Catalin Marinas wrote:

> As I wrote on the patch system, I'd like to put back the explicit
> --keep option in goto.

There are three possible values of "keepiness":

  1. Make sure there are _no_ local changes. (Default for old
     infrastructure.)

  2. Make sure there are no local changes in the files we need to
     touch. (Default for new infrastructure.)

  3. Bring along local changes by means of a merge. (What the --keep
     option does.)

git defaults to doing (2), and optionally does (3). (1) is
significantly slower than (2); I don't know how slow (3) is.

There are two questions: what subset of these options do we support,
and which of the supported modes should be the default?

I think that (2) should be the default, because it's faster, it's what
git does, and I don't really see the point in complaining about local
changes in a file we won't need to touch anyway. Having an option for
(3) might be handy, though.

But I gather you want (1) to be the default (with (3) as an option).
Correct?

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

Re: [StGit PATCH 0/6] Two bugfixes

[Catalin Marinas]

On 24/03/2008, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-03-20 15:19:12 +0000, Catalin Marinas wrote:
>
>
> > As I wrote on the patch system, I'd like to put back the explicit
>  > --keep option in goto.
>
>
> There are three possible values of "keepiness":
>
>   1. Make sure there are _no_ local changes. (Default for old
>      infrastructure.)
>
>   2. Make sure there are no local changes in the files we need to
>      touch. (Default for new infrastructure.)
>
>   3. Bring along local changes by means of a merge. (What the --keep
>      option does.)
>
>  git defaults to doing (2), and optionally does (3). (1) is
>  significantly slower than (2); I don't know how slow (3) is.

With the current functionality (see below), I prefer (1) to be the
default and (3) as optional (on stable, it does a reverse-apply of the
diff and it's kind of combination between 2 and 3 but without the
conflicts you could get with only 3).

My reason for this is that usually you "goto" a patch to do some
changes followed by "refresh". If there were local changes, they would
be included in the refreshed patch since this is the default StGIT
behaviour. To avoid this, I find myself running "status" before any
"goto".

>  I think that (2) should be the default, because it's faster, it's what
>  git does, and I don't really see the point in complaining about local
>  changes in a file we won't need to touch anyway.

But in git, for committing, you usually need to run "git add" on the
files or specify "commit -a" explicitly. We would need to change the
"refresh" behaviour in the same way and, in this case, I would be OK
with (2) as the default.

I personally prefer the current "refresh" way but maybe because I'm
used to it. It would be useful to get other users' opinion on this UI
change. Might not be a bad change since git does this already, quilt
needs an explicit "add" (anyone knows about guilt?).

-- 
Catalin

Re: [StGit PATCH 0/6] Two bugfixes

[Karl Hasselström]

On 2008-03-25 10:46:00 +0000, Catalin Marinas wrote:

> My reason for this is that usually you "goto" a patch to do some
> changes followed by "refresh". If there were local changes, they
> would be included in the refreshed patch since this is the default
> StGIT behaviour. To avoid this, I find myself running "status"
> before any "goto".

Yeah, I can see how that would be irritating. Especially since the
conversion to the new infrastructure is only half done, so that not
all commands behave the same.

> But in git, for committing, you usually need to run "git add" on the
> files or specify "commit -a" explicitly. We would need to change the
> "refresh" behaviour in the same way and, in this case, I would be OK
> with (2) as the default.
>
> I personally prefer the current "refresh" way but maybe because I'm
> used to it. It would be useful to get other users' opinion on this
> UI change. Might not be a bad change since git does this already,
> quilt needs an explicit "add" (anyone knows about guilt?).

I think my preference would be to to what git does: let just "stg
refresh" commit what's in the index, and have "stg refresh -a" or
something to automatically update the index first. (This should be
easy to do, btw -- refresh already has an --index flag.)

In general, I think it's a bad idea to do things differently from git
without a good reason.

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

Re: [StGit PATCH 0/6] Two bugfixes

[Catalin Marinas]

On 25/03/2008, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-03-25 10:46:00 +0000, Catalin Marinas wrote:
>  > I personally prefer the current "refresh" way but maybe because I'm
>  > used to it. It would be useful to get other users' opinion on this
>  > UI change. Might not be a bad change since git does this already,
>  > quilt needs an explicit "add" (anyone knows about guilt?).
>
>
> I think my preference would be to to what git does: let just "stg
>  refresh" commit what's in the index, and have "stg refresh -a" or
>  something to automatically update the index first. (This should be
>  easy to do, btw -- refresh already has an --index flag.)
>
>  In general, I think it's a bad idea to do things differently from git
>  without a good reason.

We can give this a try on the master branch and see how it goes.

-- 
Catalin