Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2012, 00:48 UTC
Message-ID: <7vk3tht7yz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1352821367-3611-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 9 quoted lines

> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 6c01d0c..e401814 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -33,6 +33,7 @@ test_create_repo sm1 &&
>  add_file . foo >/dev/null
>  
>  head1=$(add_file sm1 foo1 foo2)
> +fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)

That looks like quite a roundabout way to say

	$(cd sm1; git rev-parse --verify HEAD)

doesn't it? I know this is just moved code from the original, so I am not saying this should be fixed in this commit.

Existing code in t7401 may want to be cleaned up, perhaps after this topic settles. The add_file() function, for example, has at least these points:

 - do we know 7 hexdigits is always the right precision?
 - what happens when it fails to create a commit in one of the steps
   while looping over "$@" in its loop?
 - the function uses the "cd there and then come back", not
   "go there in a subshell and do whatever it needs to" pattern.

> +test_expect_success 'added submodule, set diff.submodule' "

s/added/add/;

Shouldn't it test the base case where no diff.submodule is set? For those people, the patch should not change the output when they do or do not pass --submodule option, right?

Show 38 quoted lines

> +	git config diff.submodule log &&
> +	git add sm1 &&
> +	git diff --cached >actual &&
> +	cat >expected <<-EOF &&
> +Submodule sm1 0000000...$head1 (new submodule)
> +EOF
> +	git config --unset diff.submodule &&
> +	test_cmp expected actual
> +"
> +
> +test_expect_success '--submodule=short overrides diff.submodule' "
> +	git config diff.submodule log &&
> +	git add sm1 &&
> +	git diff --submodule=short --cached >actual &&
> +	cat >expected <<-EOF &&
> +diff --git a/sm1 b/sm1
> +new file mode 160000
> +index 0000000..a2c4dab
> +--- /dev/null
> ++++ b/sm1
> +@@ -0,0 +1 @@
> ++Subproject commit $fullhead1
> +EOF
> +	git config --unset diff.submodule &&
> +	test_cmp expected actual
> +"
> +
>  commit_file sm1 &&
>  head2=$(add_file sm1 foo3)
>  
> @@ -73,7 +102,6 @@ EOF
>  	test_cmp expected actual
>  "
>  
> -fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
>  fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)
>  test_expect_success 'modified submodule(forward) --submodule=short' "
>  	git diff --submodule=short >actual &&