Junio C Hamano wrote:
> Ramkumar Ramachandra <artag...@gmail.com> writes:
>> diff --git a/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' "
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?
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html