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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 30/05/17 22:53, Stefan Beller wrote:
> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>> 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:

>> 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'.

Well, since I introduced it, I can confidently proclaim that it is
indeed the 'submodule path'. :-D

As I said above, I can't remember how git-ls-files worked back then,
but it seems that I thought of it as the path to the submodule from
the root of the working tree. Again, by definition, $sm_path == $path
(as documented). Of course, that may have changed since then.

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

Really? So, if it differs from $path, then something changed between
commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
commit/test, so take what I say with a pinch of salt ...

> Thanks for keeping discussing this.
> 
> So maybe we want to
> * keep path=sm_path

As I said in commit 64394e3ae9, $path was part of the API, so I could
not just rename it, without a deprecation period, etc ... So, I was
'crossing my fingers' that nobody would export $path in their user
scripts (not very likely, after all).

> * fix the documentation via s/$path/$sm_path/g in that section quoted above.

So, "$path is the name of the submodule directory relative to the
superproject", as currently documented in the man page, yes?

So, $sm_path == $path, at least for some period?

> * Introduce a new variable sm_display_path that contains what this patch
>   proposes sm_path to be.

So, this would be the path from the cwd to the submodule, yes?

> * fix the test in t7407 by checking both sm_path (fixed) as well
>   as sm_display_path (what is currently recorded in sm_path)

Hmm, ...

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

Well, as you saw in a separate thread, I can no longer get
cygwin to fail, so something (probably in the cygwin runtime)
has changed since 2012 to make this work now, despite the
case insensitive win32 environment block. (This may also be
true of MSYS2, but I haven't tested it).

I have not tested this on MYSY2/MinGW/Git-for-windows, but
Johannes Sixt was concerned about this, so I guess it may
still be a problem there.

I don't know how windows folks will feel about simply
removing $path, ...


ATB,
Ramsay Jones