Junio C Hamano wrote:
> Ramkumar Ramachandra <artag...@gmail.com> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to