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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Nov 22, 2012, 09:40 UTC
Message-ID: <CALkWK0kZ0p4ptmAySXKspU2qJm9gm6Wg4uyBhRnXQ4qdSC34FA@mail.gmail.com>
In-Reply-To: <7vk3tht7yz.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 18 quoted lines

> Ramkumar Ramachandra <artagnon@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.

Show 9 quoted lines

> 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