Re: [PATCH 2/2] mergetools: Make tortoisemerge work with

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2013, 17:48 UTC
Message-ID: <7vzjzuwm7s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <5104F009.5020606@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

Show 13 quoted lines

> Am 26.01.2013 08:10 schrieb David Aguilar:
>> These patches look correct (I do not have the tool to test)
>> but I think we should fixup this commit message.
>> 
>> How about something like...
>> 
>> mergetools: Teach tortoisemerge about TortoiseGitMerge
>> 
>> TortoiseGitMerge improved its syntax to allow for file paths
>> with spaces.  Detect when it is installed and prefer it over
>> TortoiseMerge.
>
> This message implies, that I have to combine two patches again?!

We can see that [1/2] teaches mergetool to use tortoisegitmerge when the user tells it to use tortoisemerge and the former is available.

The change in [2/2] is to use -base "$BASE" instead of -base:"$BASE" when the real tool is tortoisegitmerge (when it is tortoisemerge, nothing changes).

By reading these two patches, I would imagine that tortoisegitmerge can accept both forms, i.e. -base:"$BASE" and -base "$BASE", but the patch [2/2] considers that the latter form is preferrable in some way. As you talked about "paths with SPs in them", I would imagine that is the difference? That is -base:"$BASE" form will not work if the varilable $BASE has a SP in it (even though it is encolosed in dq, which does not make much sense from my POSIXy point of view, but perhaps command line argument processing in the Windows land may have different rules) but if you write -base "$BASE" then "$BASE" will be taken as a single thing even it has a SP in it? Also I would guess that the reason why patch [2/2] does this only for tortoisegitmerge is either because tortoisemerge will break paths with SPs even if it is given -base "$BASE" form, or because it only accepts -base:"$BASE" form? I cannot read it from your description, but let's assume that is the reason.

If that is the case, then the log message for the second patch would be easier to understand if it says so in a more explicit way, perhaps like this:

	TortoiseGitMerge, unlike TortoiseMerge, can be told to
	handle paths with SPs in them by using -option "$FILE" (not
	-option:"$FILE", which does not work for such paths) syntax.

	Use it to allow such paths to be handled correctly.

But I cannot read exactly why the patch [2/2] considers -base "$BASE" is preferrable over -base:"$BASE" from your original description, so this may well be way off the mark.

In short, I think proposed log message for [2/2] was not clear what is being fixed and how.