Re: [StGit PATCH 3/5] Add automatic git-mergetool invocation to the new infrastructure

On 2009-03-12 12:09:07 +0000, Catalin Marinas wrote:

This patch adds the IndexAndWorktree.mergetool() function responsible
for calling 'git mergetool' to interactively solve conflicts. The
function may also be called from IndexAndWorktree.merge() if the
standard 'git merge-recursive' fails and 'interactive == True'. The
'allow_interactive' parameter is passed to Transaction.push_patch() from
the functions allowing interactive merging.

Nicely done with the "interactive" and "allow_interactive" arguments;
the policy and the implementation end up at the right levels.

                 # There were conflicts
-                conflicts = [l for l in output if l.startswith('CONFLICT')]
-                raise MergeConflictException(conflicts)
+                if interactive:
+                    self.mergetool()
+                else:
+                    conflicts = [l for l in output if l.startswith('CONFLICT')]
+                    raise MergeConflictException(conflicts)

Does the merge tool always resolve all conflicts? If it doesn't, the
two lines in the "else" branch should probably be run unconditionally.

         except run.RunException, e:
             raise MergeException('Index/worktree dirty')
+    def mergetool(self, files = ()):
+        """Invoke 'git mergetool' on the current IndexAndWorktree to resolve
+        any outstanding conflicts. If 'not files', all the files in an
+        unmerged state will be processed."""
+        err = os.system('git mergetool %s' % ' '.join(files))

Look at how the surrounding code calls git. os.system() will do nasty
things with filenames that require quoting, such as for example
"; rm -rf ~/".

+        # check for unmerged entries (prepend 'CONFLICT ' for consistency with
+        # merge())
+        conflicts = ['CONFLICT ' + f for f in self.index.conflicts()]
+        if conflicts:
+            raise MergeConflictException(conflicts)
+        elif err:
+            raise MergeException('"git mergetool" failed, exit code: %d' % err)

Ah, you take care of conflicts here too. Hmm. I guess that's fine too,
though there is some code duplication. Maybe a helper function that
takes output as a parameter, and raises MergeConflictException if
necessary?

+                interactive = allow_interactive and \
+                        config.get('stgit.autoimerge') == 'yes'

Small style nit: backslash line continuations are ugly. :-)

If you put parentheses around the expression, you can break the line
without a backslash.

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