{"thread":{"id":"17699","subject":"[StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure","startedAt":"2009-02-10T14:14:07Z","lastAt":"2009-02-11T13:11:16Z","messageCount":4,"participants":["Catalin Marinas","Karl Hasselström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"104010","messageId":"20090210141157.28782.10027.stgit@pc1117.cambridge.arm.com","threadId":"17699","inReplyTo":null,"subject":"[StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@arm.com","sentAt":"2009-02-10T14:14:07Z","receivedAt":"2009-02-10T14:14:07Z","isPatch":true,"sender":{"key":"catalin.marinas@arm.com","avatar":null},"body":"A subsequent patch will remove the i3merge and i2merge customisation\nfrom the classic infrastructure and config files. The main difference\nwith the classic implementation is that mergetool is no invoked from the\nTransaction.push_patch() function rather than directly from\nIndexAndWorktree.merge().\n\nSigned-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n---\n\nI'm still not entirely sure where the check for stgit.autoimerge should\nbe done. In the classic infrastructure, it is done in the merge\nfunction. With this patch, it is done in Transaction.push(). Should we\npush this even further to stgit.commands.push? My opinion is not since\nby having it in Transaction we get the advantage not listing the\nconflicts if the mergetool succeeds and we don't need to abort the\ntransaction.\n\n\n stgit/lib/git.py         |    6 ++++++\n stgit/lib/transaction.py |   18 +++++++++++++++---\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex 07079b8..04c1fa5 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -842,6 +842,12 @@ class IndexAndWorktree(RunWithEnvCwd):\n                 raise MergeConflictException(conflicts)\n         except run.RunException, e:\n             raise MergeException('Index/worktree dirty')\n+    def mergetool(self, files = []):\n+        \"\"\"Invoke 'git mergetool' on the current IndexAndWorktree to resolve\n+        any outstanding conflicts.\"\"\"\n+        err = os.system('git mergetool %s' % ' '.join(files))\n+        if err:\n+            raise MergeException('\"git mergetool\" failed, exit code: %d' % err)\n     def changed_files(self, tree, pathlimits = []):\n         \"\"\"Return the set of files in the worktree that have changed with\n         respect to C{tree}. The listing is optionally restricted to\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex 5a81f9d..c1c9125 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -8,6 +8,7 @@ from stgit import exception, utils\n from stgit.utils import any, all\n from stgit.out import *\n from stgit.lib import git, log\n+from stgit.config import config\n \n class TransactionException(exception.StgException):\n     \"\"\"Exception raised when something goes wrong with a\n@@ -324,10 +325,21 @@ class StackTransaction(object):\n                 self.__current_tree = tree\n                 s = ' (modified)'\n             except git.MergeConflictException, e:\n-                tree = ours\n                 merge_conflict = True\n-                self.__conflicts = e.conflicts\n-                s = ' (conflict)'\n+                if config.get('stgit.autoimerge') == 'yes':\n+                    try:\n+                        iw.mergetool()\n+                        merge_conflict = False\n+                    except git.MergeException:\n+                        pass\n+                if merge_conflict:\n+                    tree = ours\n+                    self.__conflicts = e.conflicts\n+                    s = ' (conflict)'\n+                else:\n+                    tree = iw.index.write_tree()\n+                    self.__current_tree = tree\n+                    s = ' (modified)'\n             except git.MergeException, e:\n                 self.__halt(str(e))\n         cd = cd.set_tree(tree)\n"},{"id":"104156","messageId":"20090211092028.GC26136@diana.vm.bytemark.co.uk","threadId":"17699","inReplyTo":"20090210141157.28782.10027.stgit@pc1117.cambridge.arm.com","subject":"Re: [StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-02-11T09:20:28Z","receivedAt":"2009-02-11T09:20:28Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:\n\n> with the classic implementation is that mergetool is no invoked from the\n\nnow?\n\n> I'm still not entirely sure where the check for stgit.autoimerge\n> should be done. In the classic infrastructure, it is done in the\n> merge function. With this patch, it is done in Transaction.push().\n> Should we push this even further to stgit.commands.push? My opinion\n> is not since by having it in Transaction we get the advantage not\n> listing the conflicts if the mergetool succeeds and we don't need to\n> abort the transaction.\n\nYes, one advantage of having it here is that if the user resolves the\nconflict, we can just continue. I'm not sure I personally like that\nmode of operation -- you might have guessed by now that I like\nnoninteractive mechanisms -- but I can see how it's useful to someone\nwho does.\n\nAnother advantage of having it here is that it automatically just\nworks for all commands, not just \"stg push\".\n\nThe disadvantage that I see is that we ask the user to put work into\nresolving conflicts before we've made sure that we won't roll back the\nwhole transaction. If this is to become a dependable feature, we need\na way to make sure we'll never throw away the user's work.\n\n> --- a/stgit/lib/git.py\n> +++ b/stgit/lib/git.py\n> @@ -842,6 +842,12 @@ class IndexAndWorktree(RunWithEnvCwd):\n>                  raise MergeConflictException(conflicts)\n>          except run.RunException, e:\n>              raise MergeException('Index/worktree dirty')\n> +    def mergetool(self, files = []):\n> +        \"\"\"Invoke 'git mergetool' on the current IndexAndWorktree to resolve\n> +        any outstanding conflicts.\"\"\"\n> +        err = os.system('git mergetool %s' % ' '.join(files))\n> +        if err:\n> +            raise MergeException('\"git mergetool\" failed, exit code: %d' % err)\n>      def changed_files(self, tree, pathlimits = []):\n>          \"\"\"Return the set of files in the worktree that have changed with\n>          respect to C{tree}. The listing is optionally restricted to\n\nThis is the right place for this method. But what happens if \"files\"\nisn't specified -- do we operate on all files then? The method\ndocumentation should probably say this.\n\n(Small style tip: In Python, you're free to mutate the default values\nof your arguments, and those changes will be visible the next time you\ncall the funtction. You don't change \"files\" in this function, but\nit's probably still a good idea to make the default value an immutable\ntype, such as tuple.)\n\nThe rest of the patch looks good to me (with the roll-back caveat I\nmentioned above).\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"104169","messageId":"b0943d9e0902110248n7aa14743p19079e3d967f77a9@mail.gmail.com","threadId":"17699","inReplyTo":"20090211092028.GC26136@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-02-11T10:48:00Z","receivedAt":"2009-02-11T10:48:00Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/2/11 Karl Hasselström <kha@treskal.com>:\n> On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:\n>> I'm still not entirely sure where the check for stgit.autoimerge\n>> should be done. In the classic infrastructure, it is done in the\n>> merge function. With this patch, it is done in Transaction.push().\n>> Should we push this even further to stgit.commands.push? My opinion\n>> is not since by having it in Transaction we get the advantage not\n>> listing the conflicts if the mergetool succeeds and we don't need to\n>> abort the transaction.\n>\n> Yes, one advantage of having it here is that if the user resolves the\n> conflict, we can just continue. I'm not sure I personally like that\n> mode of operation -- you might have guessed by now that I like\n> noninteractive mechanisms -- but I can see how it's useful to someone\n> who does.\n\nI find it useful when I prepare a kernel release and pick patches from\nmany branches, it saves some typing with having to run the mergetool\nand restart the pick or push command. It's also useful for \"sync\".\n\n> Another advantage of having it here is that it automatically just\n> works for all commands, not just \"stg push\".\n\nIt works for commands that use Transaction.push_patch(). Other\ncommands that use IndexAndWorktree.merge() via some other function\nwould not work. Will there be such functions? I suspect a \"sync\"\nimplementation would need additional support in Transaction.\n\nAny thoughts on calling mergetool from IndexAndWorktree.merge() (with\nan additional parameter to explicitly enable this rather than just\nreading the config option)?\n\n> The disadvantage that I see is that we ask the user to put work into\n> resolving conflicts before we've made sure that we won't roll back the\n> whole transaction. If this is to become a dependable feature, we need\n> a way to make sure we'll never throw away the user's work.\n\nMaybe push_patch() can receive a parameter on whether to invoke\nmergetool. The calling code should know the behaviour for aborting\ntransactions and only ask for interactivity if the command is expected\nto leave conflicts in the index.\n\n>> --- a/stgit/lib/git.py\n>> +++ b/stgit/lib/git.py\n>> @@ -842,6 +842,12 @@ class IndexAndWorktree(RunWithEnvCwd):\n>>                  raise MergeConflictException(conflicts)\n>>          except run.RunException, e:\n>>              raise MergeException('Index/worktree dirty')\n>> +    def mergetool(self, files = []):\n>> +        \"\"\"Invoke 'git mergetool' on the current IndexAndWorktree to resolve\n>> +        any outstanding conflicts.\"\"\"\n>> +        err = os.system('git mergetool %s' % ' '.join(files))\n>> +        if err:\n>> +            raise MergeException('\"git mergetool\" failed, exit code: %d' % err)\n>>      def changed_files(self, tree, pathlimits = []):\n>>          \"\"\"Return the set of files in the worktree that have changed with\n>>          respect to C{tree}. The listing is optionally restricted to\n>\n> This is the right place for this method. But what happens if \"files\"\n> isn't specified -- do we operate on all files then? The method\n> documentation should probably say this.\n\nYes, it operates on all, I'll add a comment.\n\n> (Small style tip: In Python, you're free to mutate the default values\n> of your arguments, and those changes will be visible the next time you\n> call the funtction. You don't change \"files\" in this function, but\n> it's probably still a good idea to make the default value an immutable\n> type, such as tuple.)\n\nI've bean hit by this problem in the past but haven't learned :-).\n\n-- \nCatalin\n"},{"id":"104181","messageId":"20090211131116.GA30272@diana.vm.bytemark.co.uk","threadId":"17699","inReplyTo":"b0943d9e0902110248n7aa14743p19079e3d967f77a9@mail.gmail.com","subject":"Re: [StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-02-11T13:11:16Z","receivedAt":"2009-02-11T13:11:16Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-02-11 10:48:00 +0000, Catalin Marinas wrote:\n\n> 2009/2/11 Karl Hasselström <kha@treskal.com>:\n>\n> > On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote:\n> >\n> > > I'm still not entirely sure where the check for stgit.autoimerge\n> > > should be done. In the classic infrastructure, it is done in the\n> > > merge function. With this patch, it is done in\n> > > Transaction.push(). Should we push this even further to\n> > > stgit.commands.push? My opinion is not since by having it in\n> > > Transaction we get the advantage not listing the conflicts if\n> > > the mergetool succeeds and we don't need to abort the\n> > > transaction.\n> >\n> > Yes, one advantage of having it here is that if the user resolves\n> > the conflict, we can just continue. I'm not sure I personally like\n> > that mode of operation -- you might have guessed by now that I\n> > like noninteractive mechanisms -- but I can see how it's useful to\n> > someone who does.\n>\n> I find it useful when I prepare a kernel release and pick patches\n> from many branches, it saves some typing with having to run the\n> mergetool and restart the pick or push command. It's also useful for\n> \"sync\".\n>\n> > Another advantage of having it here is that it automatically just\n> > works for all commands, not just \"stg push\".\n>\n> It works for commands that use Transaction.push_patch(). Other\n> commands that use IndexAndWorktree.merge() via some other function\n> would not work. Will there be such functions? I suspect a \"sync\"\n> implementation would need additional support in Transaction.\n\nYes, you're right.\n\n> Any thoughts on calling mergetool from IndexAndWorktree.merge()\n> (with an additional parameter to explicitly enable this rather than\n> just reading the config option)?\n\nThat could very well be a good idea -- I can't think of anything wrong\nwith it. (And it's a good idea to make this a parameter rather than\nmaking it read the config option.)\n\n> > The disadvantage that I see is that we ask the user to put work\n> > into resolving conflicts before we've made sure that we won't roll\n> > back the whole transaction. If this is to become a dependable\n> > feature, we need a way to make sure we'll never throw away the\n> > user's work.\n>\n> Maybe push_patch() can receive a parameter on whether to invoke\n> mergetool. The calling code should know the behaviour for aborting\n> transactions and only ask for interactivity if the command is\n> expected to leave conflicts in the index.\n\nThat sounds like a plan.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"}]}