Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2015, 07:20 UTC
Message-ID: <xmqqk2s9p9rb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqtwrdrk1k.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +static int module_list_compute(int argc, const char **argv,
>> +				const char *prefix,
>> +				struct pathspec *pathspec)
>> +{
>> ...
>> +	for (i = 0; i < active_nr; i++) {
>> +		const struct cache_entry *ce = active_cache[i];
>> +
>> +		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
>> +				    max_prefix_len, ps_matched,
>> +				    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))
>> +			continue;

Another minor thing I noticed here is that ce->ce_mode would never be S_ISDIR().

It is not immediately clear if it is necessary to pass is_dir=true upon S_ISGITLINK(ce->ce_mode) to match_pathspec(), but I think that is probably a right thing to do. The only difference this makes, I think, is when a pathspec ends with a slash. E.g. when you have a submodule at path ce->ce_name == "dir" and the caller says "dir/". Then DO_MATCH_DIRECTORY logic would say "dir/" does match "dir".

So taken together with the remainder of the loop, perhaps

        for (i = 0; i < active_nr; i++) {
                ce = active_cache[i];

                if (!S_ISGITLINK(ce->ce_mode) ||
                    !match_pathspec(..., is_dir=1))
                        continue;

                ALLOC_GROW(ce_entries, ce_nr + 1, ce_alloc);
                ce_entries[ce_nr++] = ce;
                while (...)
                        skip the entries with the same name;
        }

would be what we want. Pathspec matching is much more expensive than ce_mode check, and after passing that check, you know the last parameter to the match_pathspec() at that point, so the above structure would also make sense from performance point of view, I think. And of course, the structure makes it clear that we only care about GITLINK entries and nothing else in this loop, so it is good from readability point of view, too.