Am 19.01.2010 02:44, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@xxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html