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

David Aguilar [off-list ref] 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.

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

On Tue, Mar 24, 2009 at 7:42 PM, Junio C Hamano [off-list ref] wrote:

David Aguilar [off-list ref] writes:

quoted

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

quoted

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

quoted

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