A subsequent patch will remove the i3merge and i2merge customisation
from the classic infrastructure and config files. The main difference
with the classic implementation is that mergetool is no invoked from the
Transaction.push_patch() function rather than directly from
IndexAndWorktree.merge().
Signed-off-by: Catalin Marinas <redacted>
---
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.
stgit/lib/git.py | 6 ++++++
stgit/lib/transaction.py | 18 +++++++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 07079b8..04c1fa5 100644
--- 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 todiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 5a81f9d..c1c9125 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -8,6 +8,7 @@ from stgit import exception, utils
from stgit.utils import any, all
from stgit.out import *
from stgit.lib import git, log
+from stgit.config import config
class TransactionException(exception.StgException):
"""Exception raised when something goes wrong with a@@ -324,10 +325,21 @@ class StackTransaction(object):
self.__current_tree = tree
s = ' (modified)'
except git.MergeConflictException, e:
- tree = ours
merge_conflict = True
- self.__conflicts = e.conflicts
- s = ' (conflict)'
+ if config.get('stgit.autoimerge') == 'yes':
+ try:
+ iw.mergetool()
+ merge_conflict = False
+ except git.MergeException:
+ pass
+ if merge_conflict:
+ tree = ours
+ self.__conflicts = e.conflicts
+ s = ' (conflict)'
+ else:
+ tree = iw.index.write_tree()
+ self.__current_tree = tree
+ s = ' (modified)'
except git.MergeException, e:
self.__halt(str(e))
cd = cd.set_tree(tree)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.
quoted hunk
--- 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@treskal.com
www.treskal.com/kalle
2009/2/11 Karl Hasselström [off-list ref]:
On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:
quoted
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.
I find it useful when I prepare a kernel release and pick patches from
many branches, it saves some typing with having to run the mergetool
and restart the pick or push command. It's also useful for "sync".
Another advantage of having it here is that it automatically just
works for all commands, not just "stg push".
It works for commands that use Transaction.push_patch(). Other
commands that use IndexAndWorktree.merge() via some other function
would not work. Will there be such functions? I suspect a "sync"
implementation would need additional support in Transaction.
Any thoughts on calling mergetool from IndexAndWorktree.merge() (with
an additional parameter to explicitly enable this rather than just
reading the config option)?
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.
Maybe push_patch() can receive a parameter on whether to invoke
mergetool. The calling code should know the behaviour for aborting
transactions and only ask for interactivity if the command is expected
to leave conflicts in the index.
quoted
--- 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.
Yes, it operates on all, I'll add a comment.
(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.)
I've bean hit by this problem in the past but haven't learned :-).
--
Catalin
On 2009-02-11 10:48:00 +0000, Catalin Marinas wrote:
2009/2/11 Karl Hasselström [off-list ref]:
quoted
On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:
quoted
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.
I find it useful when I prepare a kernel release and pick patches
from many branches, it saves some typing with having to run the
mergetool and restart the pick or push command. It's also useful for
"sync".
quoted
Another advantage of having it here is that it automatically just
works for all commands, not just "stg push".
It works for commands that use Transaction.push_patch(). Other
commands that use IndexAndWorktree.merge() via some other function
would not work. Will there be such functions? I suspect a "sync"
implementation would need additional support in Transaction.
Yes, you're right.
Any thoughts on calling mergetool from IndexAndWorktree.merge()
(with an additional parameter to explicitly enable this rather than
just reading the config option)?
That could very well be a good idea -- I can't think of anything wrong
with it. (And it's a good idea to make this a parameter rather than
making it read the config option.)
quoted
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.
Maybe push_patch() can receive a parameter on whether to invoke
mergetool. The calling code should know the behaviour for aborting
transactions and only ask for interactivity if the command is
expected to leave conflicts in the index.
That sounds like a plan.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle