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

From: Stefan Beller <sbeller@google.com>
Date: May 30, 2017, 21:53 UTC
Message-ID: <CAGZ79kYc9wx23N9quxhuYAf2H1Rm3tGNg_0Ydz0KO4qPH-Kz5w@mail.gmail.com>
In-Reply-To: <24ebb17b-4324-c6ef-7e3a-5576cda3b595@ramsayjones.plus.com>

On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 47 quoted lines

>
>
> On 26/05/17 18:07, Stefan Beller wrote:
>> 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 ...

Don't take that stance. Sometimes I shoot quickly from the hip without considering consequences (Figuratively).

In a foreach command I can see value both in the 'displaypath' (what sm_path would become here) and the 'submodule path' from the superproject. The naming of 'sm_path' hints at that it ought to be the 'submodule path'.

Show 10 quoted lines

>>
>>     $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).

Well the corner case is described in the patchs notes. So that patch would fix it to be consistent with the new world view (that I have in mind) as I do not know about the 2012 ideas how submodules ought to behave correctly.

Show 12 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?

After rereading that test, I would think so?

Thanks for keeping discussing this.

So maybe we want to
* keep path=sm_path
* fix the documentation via s/$path/$sm_path/g in that section quoted above.
* Introduce a new variable sm_display_path that contains what this patch
  proposes sm_path to be.
* fix the test in t7407 by checking both sm_path (fixed) as well
  as sm_display_path (what is currently recorded in sm_path)
---
In the next patch:
* Additionally in the rewrite in C, we would do an

    #ifndef WINDOWS /* need to lookup the exact macro */
        argv_array_push(env_vars, "path=%s", sm_path);
    #endif

such that Windows users are forced to migrate to sm_path as path/Path is case sensitive there. sm_path being documented value, so it should work fine?

Thanks, Stefan