From: David Aguilar <davvid@gmail.com>
Date: Wed, 25 Mar 2009 03:57:32 GMT
Subject: Re: [PATCH] difftool: add various git-difftool tests
Message-ID: <402731c90903242057g47cdcb2an39bbd95ff80584c6@mail.gmail.com>
In-Reply-To: <7vr60m9vca.fsf@gitster.siamese.dyndns.org>

On Tue, Mar 24, 2009 at 7:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> +remove_config_vars()
>> +{
>> +     # Unset all config variables used by git-difftool
>> +     git config --unset diff.tool
>> +     git config --unset difftool.test-tool.cmd
>> +     git config --unset merge.tool
>> +     git config --unset mergetool.test-tool.cmd
>> +     return 0
>> +}
>> +
>> +restore_test_defaults()
>> +{
>> +     # Restores the test defaults used by several tests
>> +     remove_config_vars
>> +     unset GIT_DIFF_TOOL &&
>> +     unset GIT_MERGE_TOOL &&
>> +     unset GIT_DIFFTOOL_NO_PROMPT &&
>
> I thought some shells' "unset" returns non-zero status when is given an
> already unset variable.  I suspect you would want to drop the && chain
> just like you did for remove_config_vars for the same reason.


Ah, I learn something new everyday.  Thanks.
I've addressed these notes and am sending patch v2 now.



>
>> +     git config diff.tool test-tool &&
>> +     git config difftool.test-tool.cmd "cat \$LOCAL"
>> +}
>
> 'cat $LOCAL' would be much easier to read, wouldn't it?
>
>> + ...
>> +# Specify the diff tool using $GIT_DIFF_TOOL
>> +test_expect_success 'GIT_DIFF_TOOL variable' '
>> +     git config --unset diff.tool &&
>
> You might want to lose && here in case the user told an earlier test that
> sets the configuration to some value skipped (or such test failed), or
> perhaps later somebody adds tests before this one that leaves the config
> without this variable.
>
> Other than that I didn't see major breakages.
>
> Thanks.
>



-- 
    David