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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.