Re: [StGit PATCH] Add automatic git-mergetool invocation to the new infrastructure
- From
Karl Hasselström <kha@treskal.com>
- Date
- Feb 11, 2009, 13:11 UTC
- Message-ID
- <20090211131116.GA30272@diana.vm.bytemark.co.uk>
- In-Reply-To
- <b0943d9e0902110248n7aa14743p19079e3d967f77a9@mail.gmail.com>
On 2009-02-11 10:48:00 +0000, Catalin Marinas wrote:
Show 31 quoted lines
> 2009/2/11 Karl Hasselström <kha@treskal.com>: > > > On 2009-02-10 14:14:07 +0000, Catalin Marinas wrote: > > > > > 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.
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.)
Show 10 quoted lines
> > 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