Re: [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path()

From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Duy Nguyen <pclouds@gmail.com>, Johannes Sixt <j6t@kdbg.org>,
	Jeff King <peff@peff.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path()
Date: Mon, 26 Jul 2021 10:56:44 -0700	[thread overview]
Message-ID: <xmqqk0ldj6w3.fsf@gitster.g> (raw)
In-Reply-To: <pull.66.v3.git.1627164413.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Sat, 24 Jul 2021 22:06:48 +0000")

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  * Since our convention of %(...) interpolation does not involve uppercase
>    keywords, I now use a lowercase one.

Makes sense.

>  * Since this keyword is interpolated to the compiled-in prefix if built
>    without runtime prefix support, I dropped the runtime part of the
>    keyword.

I have this nagging feeling that %(prefix) may be (mistakenly)
expected to interporate to $(git rev-parse --show-prefix).  Of
course, nobody would expect that in paths in the configuration
files, but because we are borrowing %(token) convention from
elsewhere, it is not outragous to imagine that either "for-each-ref"
family or "log" family of placeholders may want to use %(prefix)"
for such purpose (for that matter, they may also be helped to have
the runtime-prefix information available).

Perhaps %(installPrefix) or something may have less chance of making
us regret later.  I am just raising this as a possible problem; I
personally would not be confused if we settled on the %(prefix).

>  * Renamed the expand_user_path() to interpolate_path(), to remove the
>    distraction as to the implementation detail which things get to be
>    interpolated (because we extend it to interpolate more than just a home
>    directory, which might well be unclear from the former name, anyway).

Great.

>  * Adjusted the code comment above the interpolate_path() to remove a stale
>    part, clarify another part, and to extend it to talk about the prefix
>    expansion, too.

These looked good, too.

Thanks.