Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()

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

Stefan Beller <sbeller@google.com> writes:

> On Wed, Aug 23, 2017 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Prathamesh Chavan <pc44800@gmail.com> writes:
>>
>>> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
>>> +                                   void *cb_data);
>>> +
>>>  static char *get_default_remote(void)
>>>  {
>>>       char *dest = NULL, *ret;
>>> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)
>>>       return 0;
>>>  }
>>>
>>> -static void init_submodule(const char *path, const char *prefix, int quiet)
>>> +static void for_each_submodule(const struct module_list *list,
>>> +                            submodule_list_func_t fn, void *cb_data)
>>
>> In the output from
>>
>>         $ git grep for_each \*.h
>>
>> we find that the convention is that an interator over a group of X
>> is for_each_X,
>
> ... which this is...
>
>> the callback function that is given to for_each_X is
>> of type each_X_fn.
>
> So you suggest s/submodule_list_func_t/each_submodule_fn/

It's not _I_ suggest---the remainder of the codebase screams that
the above name is wrong ;-).  Didn't it for the mentors while they
were reading this code?

>> An interator over a subset of group of X that
>> has trait Y, for_each_Y_X() iterates and calls back a function of
>> type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).
>
> This reads as a suggestion for for_each_listed_submodule
> as the name.

If you need to have two ways to iterate over them, i.e. (1) over all
submodules and (2) over only the listed ones (whatever that means),
then yes, for_each_listed_submodule() would be a good name for the
latter which would be a complement to for_each_submodule() that is
the former.

>
>> I do not offhand think of a reason why the above code need to
>> deviate from that pattern.