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

[Stefan Beller <sbeller@google.com>]

So we're looking for more reviewers for this patch,
one way to do it is e.g. via
$ git shortlog --since 12.month -sne -- ./git-submodule.sh
(so I cc'd Brandon)
Another way to find reviewers is
$ git blame ./git-submodule.sh
(so I cc'd Anders and Johan)

> It can been seen that the patch fails in test #9
> of t7407-submodule-foreach, which is the newly added
> test to that suite. The main reason of adding this test
> was to bring the behavior of $path for the submodule
> foreach --recursive case.
>
> The observation made was as follows:
>
> For a project - super containing dir (not a submodule)
> and a submodule sub which contains another submodule
> subsub. When we run a command from super/dir:
>
> git submodule foreach "echo \$path-\$sm_path"
>
> actual results:
> Entering '../sub'
> ../sub-../sub
> Entering '../sub/subsub'
> ../subsub-../subsub
>
> ported function's result:
> Entering '../sub'
> sub-../sub
> Entering '../sub/subsub'
> subsub-../sub/subsub
>
> This is occurring since in cmd_foreach of git-submodule.sh
> when we use to recurse, we call cmd_foreach
> and hence the process ran in the same shell.
> Because of this, the variable $wt_prefix is set only once
> which is at the beginning of the submodule foreach execution.
> wt_prefix=$(git rev-parse --show-prefix)
>
> And since sm_path and path are set using $wt_prefix as :
> sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
> path=$sm_path
> It differs with the value of displaypath as well.
>
> This make the value of $path confusing and I also feel it
> deviates from its documentation:
> $path is the name of the submodule directory relative
> to the superproject.
>
> But since in refactoring the code, we wish to maintain the
> code in same way, we need to pass wt_prefix on every
> recursive call, which may result in complex C code.
> Another option could be to first correct the $path value
> in git-submodule.sh and then port the updated cmd_foreach.
>

We discussed this patch off list before and I think the code is fine,
two minor nits below. But this observation is something we'd want
to talk about here on list. Sorry for dropping the ball for 3 days.

If we do not apply patch 1, the test suite passes, as far as I observe
the situation and patch 1 introduces a test that passes the shell
foreach, but breaks here, IIUC.

Specifically the $sm_path breaks the test in the sense that it is
actually correct now for nested submodules when the command
in the superproject is run from within a directory.

I think this is a rare bug, that we can just fix along the way, i.e.
mention in the commit message that we fix it and adapt the test
such that it passes with the rewrite in C, here.

> +       argv_array_pushf(&cp.env_array, "path=%s", list_item->name);

git-am claims there are trailing white spaces.
$ git am <patch>
Applying: submodule: port subcommand foreach from shell to C
.git/rebase-apply/patch:158: trailing whitespace.
argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
.git/rebase-apply/patch:172: trailing whitespace.
warning: 2 lines add whitespace errors.

Not sure which editor you use, but I could configure my editor to strip
trailing whitespaces on save, and had never any issues with them
since.

> +       struct cb_foreach info;
..
> +
> +       memset(&info, 0, sizeof(info));

Instead of this memset, you could also have a #define CB_FOREACH_INIT
similar to e.g. MODULE_LIST_INIT, STRBUF_INIT, STRING_LIST_INIT and so on.
This memset works, too.

Thanks,
Stefan