On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan <pc44...@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
