Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: May 27, 2017, 01:10 UTC
Message-ID: <24ebb17b-4324-c6ef-7e3a-5576cda3b595@ramsayjones.plus.com>
In-Reply-To: <CAGZ79kYbi5QxWAsxdfPkuWEyMt9Qg753sm0vExsKaWyksDVw+Q@mail.gmail.com>

On 26/05/17 18:07, Stefan Beller wrote:

Show 21 quoted lines

> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>> Hmm, I'm not sure which documentation you are referring to,
> 
> Quite likely our fine manual pages. ;)
> 
>        foreach [--recursive] <command>
>            Evaluates an arbitrary shell command in each checked out submodule.
>            The command has access to the variables $name, $path, $sha1 and
>            $toplevel: $name is the name of the relevant submodule section in
>            .gitmodules, $path is the name of the submodule directory relative
>            to the superproject, $sha1 is the commit as recorded in the
>            superproject, and $toplevel is the absolute path to the top-level
>            of the superproject. Any submodules defined in the superproject but
>            not checked out are ignored by this command. Unless given --quiet,
>            foreach prints the name of each submodule before evaluating the
>            command. If --recursive is given, submodules are traversed
>            recursively (i.e. the given shell command is evaluated in nested
>            submodules as well). A non-zero return from the command in any
>            submodule causes the processing to terminate. This can be
>            overridden by adding || : to the end of the command.

I suspected as much, but I was wondering specifically if $sm_path had been documented anywhere. I didn't think so, but ...

> As $path is documented and $sm_path is not, we should care about
> $path first to be correct and either fix the documentation or the implementation
> such that we have a consistent world view. :)

Sure, but what is that world view? :-D

I suspect that commit 091a6eb0fe did not intend (should not have) used $sm_path in that test. If we were to 'fix' that test, would it still work?

Back in 2012, the submodule list was generated by filtering the output of 'git ls-files --error-unmatch --stage --'; but I don't recall if (at that time) git-ls-files required being at the top of the working tree, or if it would execute fine in a sub-directory. So, it's possible that the documentation of $path was wrong all along. ;-)

At that time, by definition, $path == $sm_path. However, you know this stuff much better than me (I don't use git-submodule), so ...

Show 12 quoted lines

>> but if
>> $path != $sm_path then something is wrong. (unless their definition
>> has changed, of course).
> 
> I would lean in doing so (changing their definition):
> 
>     $path (as documented) is the name of the submodule directory
>     relative to the direct superproject (so in nested submodules you
>     go up only one level).
> 
> $sm_path on the other hand is not documented at all and yields
> non-sense results in corner cases.

Hmm, at what point did '$sm_path yields non-sense results' start being the case? (perhaps the corner cases need to be fixed first).

Show 5 quoted lines

> With this patch it becomes less non-sensey and could be documented as:
> 
>     $sm_path is the relative path from the current working directory
>     to the submodule (ignoring relations to the superproject or nesting
>     of submodules).

OK.

>                      This documentation also fits into the narrative of
>     the test in t7407.

Hmm, does it?

ATB, Ramsay Jones