Jeff King wrote: >> 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). I had a little difficulty thinking of some decently robust tests. git mergetool is, by its nature, more interactive than many git commands. With --no-prompt it should be easier to make some tests that don't just hang when it goes wrong. I'll have a further think on this. >> [...] >> - 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?). Ah yes, it's definitely worthy of at least a comment. This is something that I changed right at the end of testing and I should really have made it into a separate commit. Previously, if you aborted a merge, you were left with the base/local/remote temporaries for the merge that you aborted. To be honest, I found this a little irritating. The base, local and remote temporaries are inputs so are accessible from slots 1,2 and 3 of the index, and any intermediate output will be in the target file. You can git clean, but if you have other temporaries you need to keep, you end up having to manually clean them up in any case. With --keep-going, the problem is compounded. If you make several passes through a list of complex merges you end up with fistfuls of these temporary trios in your working tree. It goes from slightly annoying to very irritating. Charles. -- 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
