[StGIT PATCH 0/6] David's conflict series using my subprocess stuff

[StGIT PATCH 0/6] David's conflict series using my subprocess stuff

[Karl Hasselström]

This is David's conflict series, rebased on top of my subprocess
calling series. There was one place where it used to have to call
popen2.Popen3 directly, since none of our existing ten subprocess
functions could do that particular job. But the new subprocess
machinery can. (This was the original motivation for that refactoring,
by the way.)

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

[StGIT PATCH 1/6] Split git.merge into two functions

[Karl Hasselström]

From: David Kågedal <davidk@lysator.liu.se>

This only prepares for later simplifications.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/commands/pick.py |    2 +
 stgit/git.py           |   92 +++++++++++++++++++++++++++++++++++++-----------
 stgit/stack.py         |    2 +
 3 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/stgit/commands/pick.py b/stgit/commands/pick.py
index 1c3ef11..871879c 100644
--- a/stgit/commands/pick.py
+++ b/stgit/commands/pick.py
@@ -99,7 +99,7 @@ def func(parser, options, args):
 
         # try a direct git-apply first
         if not git.apply_diff(bottom, top):
-            git.merge(bottom, git.get_head(), top, recursive = True)
+            git.merge_recursive(bottom, git.get_head(), top)
 
         out.done()
     elif options.update:
diff --git a/stgit/git.py b/stgit/git.py
index 7962cdb..87551ee 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -634,36 +634,86 @@ def apply_diff(rev1, rev2, check_index = True, files = None):
 
     return True
 
-def merge(base, head1, head2, recursive = False):
+stages_re = re.compile('^([0-7]+) ([0-9a-f]{40}) ([1-3])\t(.*)$', re.S)
+
+def merge_recursive(base, head1, head2):
     """Perform a 3-way merge between base, head1 and head2 into the
     local tree
     """
     refresh_index()
 
     err_output = None
-    if recursive:
-        # this operation tracks renames but it is slower (used in
-        # general when pushing or picking patches)
-        try:
-            # discard output to mask the verbose prints of the tool
-            GRun('git-merge-recursive', base, '--', head1, head2
-                 ).discard_output()
-        except GitRunException, ex:
-            err_output = str(ex)
-            pass
-    else:
-        # the fast case where we don't track renames (used when the
-        # distance between base and heads is small, i.e. folding or
-        # synchronising patches)
-        try:
-            GRun('git-read-tree', '-u', '-m', '--aggressive',
-                 base, head1, head2).run()
-        except GitRunException:
-            raise GitException, 'git-read-tree failed (local changes maybe?)'
+    # this operation tracks renames but it is slower (used in
+    # general when pushing or picking patches)
+    try:
+        # discard output to mask the verbose prints of the tool
+        GRun('git-merge-recursive', base, '--', head1, head2).discard_output()
+    except GitRunException, ex:
+        err_output = str(ex)
+        pass
+
+    # check the index for unmerged entries
+    files = {}
+
+    for line in GRun('git-ls-files', '--unmerged', '--stage', '-z'
+                     ).raw_output().split('\0'):
+        if not line:
+            continue
+
+        mode, hash, stage, path = stages_re.findall(line)[0]
+
+        if not path in files:
+            files[path] = {}
+            files[path]['1'] = ('', '')
+            files[path]['2'] = ('', '')
+            files[path]['3'] = ('', '')
+
+        files[path][stage] = (mode, hash)
+
+    if err_output and not files:
+        # if no unmerged files, there was probably a different type of
+        # error and we have to abort the merge
+        raise GitException, err_output
+
+    # merge the unmerged files
+    errors = False
+    for path in files:
+        # remove additional files that might be generated for some
+        # newer versions of GIT
+        for suffix in [base, head1, head2]:
+            if not suffix:
+                continue
+            fname = path + '~' + suffix
+            if os.path.exists(fname):
+                os.remove(fname)
+
+        stages = files[path]
+        if gitmergeonefile.merge(stages['1'][1], stages['2'][1],
+                                 stages['3'][1], path, stages['1'][0],
+                                 stages['2'][0], stages['3'][0]) != 0:
+            errors = True
+
+    if errors:
+        raise GitException, 'GIT index merging failed (possible conflicts)'
+
+def merge(base, head1, head2):
+    """Perform a 3-way merge between base, head1 and head2 into the
+    local tree
+    """
+    refresh_index()
+
+    err_output = None
+    # the fast case where we don't track renames (used when the
+    # distance between base and heads is small, i.e. folding or
+    # synchronising patches)
+    try:
+        GRun('git-read-tree', '-u', '-m', '--aggressive', base, head1, head2
+             ).run()
+    except GitRunException:
+        raise GitException, 'git-read-tree failed (local changes maybe?)'
 
     # check the index for unmerged entries
     files = {}
-    stages_re = re.compile('^([0-7]+) ([0-9a-f]{40}) ([1-3])\t(.*)$', re.S)
 
     for line in GRun('git-ls-files', '--unmerged', '--stage', '-z'
                      ).raw_output().split('\0'):
diff --git a/stgit/stack.py b/stgit/stack.py
index 12c5091..a82392f 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1066,7 +1066,7 @@ class Series(PatchSet):
 
                 # merge can fail but the patch needs to be pushed
                 try:
-                    git.merge(bottom, head, top, recursive = True)
+                    git.merge_recursive(bottom, head, top)
                 except git.GitException, ex:
                     out.error('The merge failed during "push".',
                               'Use "refresh" after fixing the conflicts or'

[StGIT PATCH 2/6] Leave working dir and index alone after failed (conflicting) push

[Karl Hasselström]

From: David Kågedal <davidk@lysator.liu.se>

This leaves the index and working tree in the state that merge-recursive
left it, with unmerged files in different stages, and the non-conflicting
changes in the index.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/git.py   |   20 +-------------------
 stgit/stack.py |   18 +++++++++++++-----
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/stgit/git.py b/stgit/git.py
index 87551ee..82cb211 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -675,25 +675,7 @@ def merge_recursive(base, head1, head2):
         # error and we have to abort the merge
         raise GitException, err_output
 
-    # merge the unmerged files
-    errors = False
-    for path in files:
-        # remove additional files that might be generated for some
-        # newer versions of GIT
-        for suffix in [base, head1, head2]:
-            if not suffix:
-                continue
-            fname = path + '~' + suffix
-            if os.path.exists(fname):
-                os.remove(fname)
-
-        stages = files[path]
-        if gitmergeonefile.merge(stages['1'][1], stages['2'][1],
-                                 stages['3'][1], path, stages['1'][0],
-                                 stages['2'][0], stages['3'][0]) != 0:
-            errors = True
-
-    if errors:
+    if files:
         raise GitException, 'GIT index merging failed (possible conflicts)'
 
 def merge(base, head1, head2):
diff --git a/stgit/stack.py b/stgit/stack.py
index a82392f..eb0114e 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -734,6 +734,7 @@ class Series(PatchSet):
         config.unset(self.format_version_key())
 
     def refresh_patch(self, files = None, message = None, edit = False,
+                      empty = False,
                       show_patch = False,
                       cache_update = True,
                       author_name = None, author_email = None,
@@ -783,9 +784,16 @@ class Series(PatchSet):
 
         bottom = patch.get_bottom()
 
+        if empty:
+            tree_id = git.get_commit(bottom).get_tree()
+        else:
+            tree_id = None
+
         commit_id = git.commit(files = files,
                                message = descr, parents = [bottom],
                                cache_update = cache_update,
+                               tree_id = tree_id,
+                               set_head = True,
                                allowempty = True,
                                author_name = author_name,
                                author_email = author_email,
@@ -1088,11 +1096,11 @@ class Series(PatchSet):
                     log = 'push'
                 self.refresh_patch(cache_update = False, log = log)
             else:
-                # we store the correctly merged files only for
-                # tracking the conflict history. Note that the
-                # git.merge() operations should always leave the index
-                # in a valid state (i.e. only stage 0 files)
-                self.refresh_patch(cache_update = False, log = 'push(c)')
+                # We make the patch empty, with the merged state in
+                # the working tree and index just like after a failed
+                # git merge.
+                self.refresh_patch(cache_update = False, empty = True,
+                                   log = 'push(c)')
                 raise StackException, str(ex)
 
         return modified

[StGIT PATCH 3/6] Added a test case to check what happens when push finds a conflict

[Karl Hasselström]

From: David Kågedal <davidk@lysator.liu.se>

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 t/t1203-push-conflict.sh |   64 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/t/t1203-push-conflict.sh b/t/t1203-push-conflict.sh
new file mode 100755
index 0000000..57fb477
--- /dev/null
+++ b/t/t1203-push-conflict.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 David Kågedal
+#
+
+test_description='Exercise push conflicts.
+
+Test that the index has no modifications after a push with conflicts.
+'
+
+. ./test-lib.sh
+
+test_expect_success \
+	'Initialize the StGIT repository' \
+	'stg init
+'
+
+test_expect_success \
+	'Create the first patch' \
+	'
+	stg new foo -m foo &&
+	echo foo > test &&
+	echo fie > test2 &&
+	stg add test test2 &&
+	stg refresh &&
+        stg pop
+	'
+
+test_expect_success \
+	'Create the second patch' \
+	'
+	stg new bar -m bar &&
+	echo bar > test &&
+	stg add test &&
+	stg refresh
+	'
+
+test_expect_failure \
+	'Push the first patch with conflict' \
+	'
+	stg push foo
+	'
+
+test_expect_failure \
+	'Show the, now empty, first patch' \
+	'
+	stg show foo | grep -q -e "^diff "
+	'
+
+test_expect_success \
+	'Check that the index has the non-conflict updates' \
+	'
+	git diff --cached --stat | grep -q -e "^ test2 | *1 "
+	'
+
+test_expect_success \
+	'Resolve the conflict' \
+	'
+	echo resolved > test &&
+	git add test &&
+	stg refresh
+	'
+
+test_done

[StGIT PATCH 4/6] Simplify merge_recursive

[Karl Hasselström]

From: David Kågedal <davidk@lysator.liu.se>

Listing the unmerged files is unnecessary, since the information
isn't really used anyway. Just note if the merge failed or succeeded.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

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

diff --git a/stgit/git.py b/stgit/git.py
index 82cb211..173cc4b 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -642,40 +642,10 @@ def merge_recursive(base, head1, head2):
     """
     refresh_index()
 
-    err_output = None
-    # this operation tracks renames but it is slower (used in
-    # general when pushing or picking patches)
     try:
         # discard output to mask the verbose prints of the tool
         GRun('git-merge-recursive', base, '--', head1, head2).discard_output()
-    except GitRunException, ex:
-        err_output = str(ex)
-        pass
-
-    # check the index for unmerged entries
-    files = {}
-
-    for line in GRun('git-ls-files', '--unmerged', '--stage', '-z'
-                     ).raw_output().split('\0'):
-        if not line:
-            continue
-
-        mode, hash, stage, path = stages_re.findall(line)[0]
-
-        if not path in files:
-            files[path] = {}
-            files[path]['1'] = ('', '')
-            files[path]['2'] = ('', '')
-            files[path]['3'] = ('', '')
-
-        files[path][stage] = (mode, hash)
-
-    if err_output and not files:
-        # if no unmerged files, there was probably a different type of
-        # error and we have to abort the merge
-        raise GitException, err_output
-
-    if files:
+    except GitRunException:
         raise GitException, 'GIT index merging failed (possible conflicts)'
 
 def merge(base, head1, head2):

[StGIT PATCH 5/6] Use the output from merge-recursive to list conflicts

[Karl Hasselström]

From: David Kågedal <davidk@lysator.liu.se>

merge-recursive already has useful information about what the conflicts
were, so we reuse that when pushing.

Signed-off-by: David Kågedal <davidk@lysator.liu.se>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/git.py   |   22 +++++++++++++++++-----
 stgit/stack.py |    2 ++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/stgit/git.py b/stgit/git.py
index 173cc4b..a185f19 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -39,6 +39,14 @@ class GitRunException(GitException):
 class GRun(Run):
     exc = GitRunException
 
+class GitConflictException(GitException):
+    def __init__(self, conflicts):
+        GitException.__init__(self)
+        self.conflicts = conflicts
+    def __str__(self):
+        return "%d conflicts" % len(self.conflicts)
+    def list(self):
+        out.info(*self.conflicts)
 
 #
 # Classes
@@ -642,11 +650,15 @@ def merge_recursive(base, head1, head2):
     """
     refresh_index()
 
-    try:
-        # discard output to mask the verbose prints of the tool
-        GRun('git-merge-recursive', base, '--', head1, head2).discard_output()
-    except GitRunException:
-        raise GitException, 'GIT index merging failed (possible conflicts)'
+    p = GRun('git-merge-recursive', base, '--', head1, head2).returns([0, 1])
+    output = p.output_lines()
+    if p.exitcode == 0:
+        # No problems
+        return
+    else: # exitcode == 1
+        # There were conflicts
+        conflicts = [l for l in output if l.startswith('CONFLICT')]
+        raise GitConflictException(conflicts)
 
 def merge(base, head1, head2):
     """Perform a 3-way merge between base, head1 and head2 into the
diff --git a/stgit/stack.py b/stgit/stack.py
index eb0114e..d2ca0e2 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1075,6 +1075,8 @@ class Series(PatchSet):
                 # merge can fail but the patch needs to be pushed
                 try:
                     git.merge_recursive(bottom, head, top)
+                except git.GitConflictException, ex:
+                    ex.list()
                 except git.GitException, ex:
                     out.error('The merge failed during "push".',
                               'Use "refresh" after fixing the conflicts or'

[StGIT PATCH 6/6] Better error message if merge fails

[Karl Hasselström]

This message is no longer printed in case of conflicts, just in case
the merge really failed; so don't talk about conflicts in the error
message.

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

---

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

diff --git a/stgit/stack.py b/stgit/stack.py
index d2ca0e2..080231f 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -1079,8 +1079,7 @@ class Series(PatchSet):
                     ex.list()
                 except git.GitException, ex:
                     out.error('The merge failed during "push".',
-                              'Use "refresh" after fixing the conflicts or'
-                              ' revert the operation with "push --undo".')
+                              'Revert the operation with "push --undo".')
 
         append_string(self.__applied_file, name)