On 05/22, Stefan Beller wrote: > On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan <pc44800@xxxxxxxxx> 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