Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 5, 2017, 22:20 UTC
Message-ID: <20170605222031.GB21733@aiede.mtv.corp.google.com>
In-Reply-To: <20170603003710.5558-1-sbeller@google.com>

Hi,

This patch seems to aim to do multiple things.  Initial thoughts:

Stefan Beller wrote:

[...]

Show 8 quoted lines

> To ameliorate the situation, perform these changes
> * Document 'sm_path' instead of 'path'.
>   As using a variable '$path' may be harmful to users due to
>   capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
>   use $path variable in eval_gettext string, 2012-04-17). Adjust
>   the documentation to advocate for using $sm_path,  which contains
>   the same value. We still make the 'path' variable available,
>   though not documented.

Making sm_path part of the public API as described here sounds like a good idea (as a separate patch), to avoid conflicting with $PATH on Windows. It's convenient that scripts have access to the private variable 'sm_path'. The 'path' variable would still need to be documented as a deprecated synonym so people working with existing scripts can know how to update them.

> * Clarify the 'toplevel' variable documentation.
>   It does not contain the topmost superproject as the author assumed,
>   but the direct superproject, such that $toplevel/$sm_path is the
>   actual absolute path of the submodule.

This is very confusing. I suspect it's a bug. Can we make 'toplevel' point to the topmost superproject (as a separate path)?

> * The variable '$displaypath' was accessible but undocumented.
>   Rename it '$displaypath' to '$dpath'. Document what it contains.
>   Users that are broken by the behavior change of 'sm_path' introduced
>   in this commit, can switch from '$path' to '$dpath'.

What does dpath stand for? Renaming the variable to $dpath means that scripts trying to adapt to this change would not work with previous versions of git. Would it make sense to use $displaypath for this for compatibility?

What is the intent behind the sm_path behavior change in this patch? Stepping back, what kind of scripts is this interface meant to support (e.g., what is an example script that used this interface that would be affected), and is there a straightforward way to support those use cases without breaking existing scripts except where necessary?

To summarize, the patch leaves me a bit confused. I think it would be best to have multiple patches that solve one problem at a time, which would hopefully make the story clearer.

Thanks and hope that helps, Jonathan