On Tue, Mar 24, 2009 at 7:42 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> David Aguilar <davvid@xxxxxxxxx> 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
--
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
