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