Re: [PATCH v2] Performance optimization for detection of modified submodules
- From
Jens Lehmann <jens.lehmann@web.de>
- Date
- Jan 19, 2010, 08:35 UTC
- Message-ID
- <4B556ED2.7020904@web.de>
- In-Reply-To
- <7v1vhmg975.fsf@alter.siamese.dyndns.org>
Am 19.01.2010 02:44, schrieb Junio C Hamano:
Show 11 quoted lines
> 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.
Show 16 quoted lines
>> + 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.
Show 15 quoted lines
>> @@ -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?
Show 17 quoted lines
>> 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.