On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:
> with the classic implementation is that mergetool is no invoked from the
now?
> I'm still not entirely sure where the check for stgit.autoimerge
> should be done. In the classic infrastructure, it is done in the
> merge function. With this patch, it is done in Transaction.push().
> Should we push this even further to stgit.commands.push? My opinion
> is not since by having it in Transaction we get the advantage not
> listing the conflicts if the mergetool succeeds and we don't need to
> abort the transaction.
Yes, one advantage of having it here is that if the user resolves the
conflict, we can just continue. I'm not sure I personally like that
mode of operation -- you might have guessed by now that I like
noninteractive mechanisms -- but I can see how it's useful to someone
who does.
Another advantage of having it here is that it automatically just
works for all commands, not just "stg push".
The disadvantage that I see is that we ask the user to put work into
resolving conflicts before we've made sure that we won't roll back the
whole transaction. If this is to become a dependable feature, we need
a way to make sure we'll never throw away the user's work.
> --- a/stgit/lib/git.py
> +++ b/stgit/lib/git.py
> @@ -842,6 +842,12 @@ class IndexAndWorktree(RunWithEnvCwd):
> raise MergeConflictException(conflicts)
> 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."""
> + err = os.system('git mergetool %s' % ' '.join(files))
> + if err:
> + raise MergeException('"git mergetool" failed, exit code: %d' % err)
> def changed_files(self, tree, pathlimits = []):
> """Return the set of files in the worktree that have changed with
> respect to C{tree}. The listing is optionally restricted to
This is the right place for this method. But what happens if "files"
isn't specified -- do we operate on all files then? The method
documentation should probably say this.
(Small style tip: In Python, you're free to mutate the default values
of your arguments, and those changes will be visible the next time you
call the funtction. You don't change "files" in this function, but
it's probably still a good idea to make the default value an immutable
type, such as tuple.)
The rest of the patch looks good to me (with the roll-back caveat I
mentioned above).
--
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
