Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C

[Stefan Beller <sbeller@google.com>]

On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:

> I have also made some changes in git-submodule.sh for correcting
> the $path variable. And hence made the corresponding changes in
> the new test introduced in t7407-submodule-foreach as well.
> I have push this work at:
> https://github.com/pratham-pc/git/commits/foreach-bug-fixed

This one seems to pass the test suite by having the bug fixed.
(The patches posted here seems to be
https://github.com/pratham-pc/git/commits/foreach
which does not pass tests? These two series seem to only differ in
the bug fix commit, which I think is a good idea to include, as then we
have a bug fixed and the tests pass.)

> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
..
> +       return;

no need for an explicit return in a void function.

> +struct cb_foreach {
> +       int argc;
> +       const char **argv;
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }

This static initializer doesn't quite match the struct,
(I would expect two NULLs as we have two const char pointers).

> +
> +       info.argc = argc;
> +       info.argv = argv;
> +       info.prefix = prefix;
> +       info.quiet = !!quiet;
> +       info.recursive = !!recursive;

as you assign all fields of the struct yourself, you could also omit the
static initialization via _INIT above.


Apart from these two minor nits the code looks good to me.
However we'd really want to have the bug fix patch as well.
(At the time of submission of a patch we should not be aware
of any tests failing, which we are without said bug fix patch)

Thanks,
Stefan