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

[Brandon Williams <bmwill@google.com>]

On 05/22, Stefan Beller wrote:
> 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).

If we ever move to a new version of C, these initializers would be much
more readable as we could assign values to the fields themselves.  But
that is unrelated to this change.

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

-- 
Brandon Williams