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

From: Karl Hasselström <kha@treskal.com>
Date: Mar 13, 2009, 02:17 UTC
Message-ID: <20090313021751.GC15393@diana.vm.bytemark.co.uk>
In-Reply-To: <20090312120907.2992.81035.stgit@pc1117.cambridge.arm.com>

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

Show 6 quoted lines

> 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.

Show 8 quoted lines

>                  # 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.

Show 7 quoted lines

>          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 ~/".

Show 7 quoted lines

> +        # 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