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@xxxxxxxxxxx
www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
