Junio C Hamano wrote: > Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes: > >> 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. >> +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 -- 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