Re: [PATCH 1/2] submodule: fix confusing variable name

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Aug 8, 2013, 17:44 UTC
Message-ID: <5203D8F6.2020102@ramsay1.demon.co.uk>
In-Reply-To: <20130804212938.GL19369@paksenarrion.iveqy.com>

Fredrik Gustafsson wrote:

Show 9 quoted lines

> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:
>> But we'll have to use sm_path here (like everywhere else in the
>> submodule script), because we'll run into problems under Windows
>> otherwise (see 64394e3ae9 for details). Apart from that the patch
>> is fine.
> 
> We're still using path= in the foreach-script. Or rather, we're setting
> it. From what I can see and from the commit message 64394e3ae9 it could
> possible be a problem.

Please do not use a $path variable in any script intended to be run on windows; those poor souls who would otherwise have to fix the bugs will thank you! :-D

Actually, it's not so much the use of a $path variable, rather the act of _exporting_ such a variable that causes the problem. (Which is why using $path with eval_gettext[ln] is such a problem, of course.)

As noted in the above commit, $path is unfortunately a documented part of the public API for the foreach subcommand. However, the foreach subcommand is (mostly) fine; given the fact that the user script is eval-ed in a context in which $path is not exported. The reason for the 'mostly' is simply that the user could shoot himself in the foot by export-ing $path in their script, so that something like:

    $ git submodule foreach 'export path; echo $path `git rev-parse HEAD`'

will indeed fail (ie git rev-parse will not execute).

> Not sure how to solve it though... Just a simple correction would break
> all script depending on that.

$path is part of the public API, so we can't just remove it. It would require a deprecation period, etc,. (Adding/documenting $sm_path as an alternative *may* be worth doing. dunno.)

HTH

ATB, Ramsay Jones