Re: [PATCH 1/2 v3] mergetool: use path to mergetool in config var mergetool.<tool>.path
- From
Steffen Prohaska <prohaska@zib.de>
- Date
- Oct 18, 2007, 08:40 UTC
- Message-ID
- <4591BA15-EB6B-4058-A2D0-879556481E59@zib.de>
- In-Reply-To
- <20071018080049.GK14735@spearce.org>
On Oct 18, 2007, at 10:00 AM, Shawn O. Pearce wrote:
Show 22 quoted lines
> Steffen Prohaska <prohaska@zib.de> wrote:
>> On Oct 18, 2007, at 7:27 AM, Shawn O. Pearce wrote:
>>> ...
>>>> + test -n "$merge_tool" || valid_tool "$merge_tool" || {
>>>
>>> Wouldn't an empty $merge_tool string be caught above in the
>>> valid_tool function where it falls through and returns 1?
>>> So isn't test -n here redundant?
>>
>> Sharp eyes, thanks.
>>
>> Correct is
>>
>> test -z "$merge_tool" || valid_tool "$merge_tool" || {
>>
>> No merge tool or a valid merge tool is allowed at this place.
>> If the tool's already empty there's no need to tell the user
>> that it will be reset to empty.
>>
>> I'll send a v4 version.
>
> Thanks but its a little late.Sorry.
Show 5 quoted lines
> I'm about to push maint/master/next/pu > and this series is in next. While testing it I found another > bug related to init_tool_merge_tool_path() not having $merge_tool > initialized and thus causing it to never select a default tool if > the user didn't have one configured.
I see. Thanks for fixing this.
> Here's what I'm actually about to push out:
One remark ...
[...]
Show 9 quoted lines
> > if test -z "$merge_tool"; then > merge_tool=`git config merge.tool` > - case "$merge_tool" in > - kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | > gvimdiff | "") > - ;; # happy > - *) > + if ! valid_tool "$merge_tool"; then
I think its ok to have an empty tool here ...
> echo >&2 "git config option merge.tool set to unknown tool: > $merge_tool" > echo >&2 "Resetting to default..."
... So we should not print this message if the tool is empty.
But an empty tool is not valid. Therefore a check if "$merge_tool" is not empty should be added to the if.
Steffen