Re: [PATCH] Show submodules as modified when they contain a dirty work tree

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 15.01.2010 00:13, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Subject: Show a modified submodule directory as dirty even if the refs match
>>
>> When the submodules HEAD and the ref committed in the HEAD of the
>> superproject were the same, "git diff[-index] HEAD" did not show the
>> submodule as dirty when it should.
>>
>> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
>> ---
>>  diff-lib.c                |    3 ++-
>>  t/t4027-diff-submodule.sh |   35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+), 1 deletions(-)
>>
>> diff --git a/diff-lib.c b/diff-lib.c
>> index 5ce226b..9cdf6da 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -233,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,
>>  			return -1;
>>  		}
>>  		changed = ce_match_stat(ce, &st, 0);
>> -		if (changed) {
>> +		if (changed
>> +		    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {
> 
> You had a check in your previous patch that decides to call or skip
> diff_change() based on is_submodule_modified() for diff-files, but forgot
> to have the same for diff-index, which this patch does.  Perhaps we want
> to squash this into 4519d9c (Show submodules as modified when they contain
> a dirty work tree, 2010-01-13).

Of course you are right, the change you quoted should have been in my
patch in the first place. So squashing it seems to be the right thing to
do (but AFAICS the tests i added might be a problem, as they use
expect_from_to() which your intermediate patch added. Maybe squash these
tests into your patch and the diff you quoted above into mine?).


> The existing code is a bit unfortunate; by the time we come to the output
> routine, the information we found from is_submodule_modified() is lost;
> that is why my "would look like this" patch calls is_submodule_modified().
> 
> We may want to add one parameter to diff_change() and diff_addremove(), to
> tell them if the work-tree side (if we are comparing something with the
> work tree) is a modified submodule, and add one bit to the diff_filespec
> structure to record that in diff_change() and diff_addremove() (obviously
> only when adding).  That way, my "would looks like this" patch needs to
> check the result of is_submodule_modified() the front-ends left in the
> filespec, instead of running it again.

Good idea, i've been already exploring this line of thought too and came
to the same conclusion (i noticed that when calling plain "git diff" in a
repo with submodules, is_submodule_modified() gets called *three* times
for each submodule, which is not /that/ good for performance ;-). But i
intended to do this optimization in a subsequent patch (and in preparation
for "git diff --submodule" being able to print /how/ the submodule is
dirty without having to scan it again).