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

From: Stefan Beller <sbeller@google.com>
Date: May 15, 2017, 17:22 UTC
Message-ID: <CAGZ79kbw9fagp7AprAhUGTD1eUd8YkeoXoAKEx73Uy7TPuVxVw@mail.gmail.com>
In-Reply-To: <20170512114404.10008-2-pc44800@gmail.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)

Show 49 quoted lines

> 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