Re: [PATCH 3/3] Add -k/--keep-going option to mergetool
- From
Jeff King <peff@peff.net>
- Date
- Nov 14, 2008, 06:47 UTC
- Message-ID
- <20081114064756.GB11907@coredump.intra.peff.net>
- In-Reply-To
- <1226580075-29289-4-git-send-email-charles@hashpling.org>
On Thu, Nov 13, 2008 at 12:41:15PM +0000, Charles Bailey wrote:
Show 7 quoted lines
> This option stops git mergetool from aborting at the first failed merge. > This allows some additional use patterns. Merge conflicts can now be > previewed one at time and merges can also be skipped so that they can be > performed in a later pass. > > There is also a mergetool.keepGoing configuration variable covering the > same behaviour.
I think this series addresses the questions I remember from the first posting, and it looks sane overall to me. My only two complaints are:
> Documentation/config.txt | 4 +++ > Documentation/git-mergetool.txt | 12 +++++++++- > git-mergetool.sh | 46 ++++++++++++++++++++++++++++++-------- > 3 files changed, 51 insertions(+), 11 deletions(-)
This patch in particular changes some of the innards of mergetool, and while the changes look good to me via reading, I would feel even more comfortable if there were some tests (it looks like your previous t7610 gives at least a basic sanity check, but it might be nice to test to make sure we detect merge failures, too).
Show 15 quoted lines
> - return > + return 0 > [...] > - return > + return 0 > [...] > - exit 1 > + return 1 > [...] > - exit 1 > + return 1 > [...] > - exit 1 > + cleanup_temp_files > + return 1
One of these is not like the others. Is there a reason to add a cleanup_temp_files here (and if so, should it be noted in the commit message, and/or be in a separate commit?).
-Peff