Re: [PATCH] difftool: add various git-difftool tests

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2009, 02:42 UTC
Message-ID: <7vr60m9vca.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1237797676-32047-2-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 17 quoted lines

> +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.

> +	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.