Re: [PATCH v2] Performance optimization for detection of modified submodules

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

Am 19.01.2010 02:44, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> - Changed the type of the new dirty_submodule flags to unsigned.
> 
> Why?  An unsigned used in place of a bool raises an eyebrow as it is more
> common to use "int" (the most natural type on the platform).  Going
> against tradition makes readers waste time wondering if there were some
> other reason why the code couldn't use "int" and had to use "unsigned"
> (e.g. "Hmmpphh, it looked like a mere boolean but the use of 'unsigned'
> suggests there might be something deeper going on.  Is this used as a
> bitfield?  Does this count and cannot go negative?").

I am working on a subsequent patch where dirty_submodule is used as a
bitfield to store more detailed information about the dirtiness. That is
then used by "git diff --submodule" to tell the user if the submodule
contains untracked and/or modified files.

But i can revert that part if you want.


>> +		if (S_ISGITLINK(ce->ce_mode)
>> +		    && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH))
>> +		    && is_submodule_modified(ce->name)) {
>> +			changed = 1;
>> +			dirty_submodule = 1;
>> +		}
> 
> If the index records a submodule commit, and the commit checked out in the
> submodule is different from that commit, ce_compare_gitlink() called from
> ce_match_stat() would have already said "changed".  If we want "-dirty",
> we need to call is_submodule_modified(), but otherwise we don't.  Looks
> good.
> 
> Does DIFF_FORMAT_PATCH cover all cases where we may want a reliable value
> in "dirty_submodule" here?  Should use of "--submodule=log" affect this
> decision?

In my testing "git diff --submodule=log" has DIFF_FORMAT_PATCH set too.


>> @@ -204,16 +209,18 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>>  static void diff_index_show_file(struct rev_info *revs,
>>  				 const char *prefix,
>>  				 struct cache_entry *ce,
>> -				 const unsigned char *sha1, unsigned int mode)
>> +				 const unsigned char *sha1, unsigned int mode,
>> +				 unsigned dirty_submodule)
>>  {
>>  	diff_addremove(&revs->diffopt, prefix[0], mode,
>> -		       sha1, ce->name);
>> +		       sha1, ce->name, dirty_submodule);
> 
> Mental note to myself.  prefix[0] is either '-' (removed from the work
> tree) or '+' (added to the work tree).  Added submodule could be
> immediately dirty.

Should i add a comment there?


>> diff --git a/diffcore.h b/diffcore.h
>> index 5b63458..66687c3 100644
>> --- a/diffcore.h
>> +++ b/diffcore.h
>> @@ -42,6 +42,7 @@ struct diff_filespec {
>>  #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)
>>  	unsigned should_free : 1; /* data should be free()'ed */
>>  	unsigned should_munmap : 1; /* data should be munmap()'ed */
>> +	unsigned dirty_submodule : 1;  /* For submodules: its work tree is dirty */
> 
> By the way, we might want to consolidate these bitfields into a single 
> 
> 	unsigned should_free:1,
>         	 should_munmap:1,
>                  dirty_submodule:1;
> 
> in a separate clean-up patch, after all the dust settles.

Okay.