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

Junio C Hamano wrote:

Ramkumar Ramachandra [off-list ref] writes:

quoted

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.

Exactly what I was thinking.

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.

Okay, will do.

quoted

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

s/added/add/;

I see that the topic is already in `next`.  Do you want to fix it up there?

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?

When no diff.submodule is set, `git diff --submodule` should just work
as before; isn't this tested by the other tests in the file?

Ram