Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

From: Jeff King <peff@peff.net>
To: Johannes Sixt <j6t@kdbg.org>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Date: Wed, 7 Nov 2018 17:03:20 -0500	[thread overview]
Message-ID: <20181107220320.GA8970@sigill.intra.peff.net> (raw)
In-Reply-To: <e7ff6f22-fe5a-3cca-9305-2c8a6fb55d45@kdbg.org>

On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote:

> Am 07.11.18 um 21:41 schrieb Jeff King:
> > On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote:
> > > Do I understand correctly, that you use a leading slash as an indicator to
> > > construct a path relative to system_path(). How about a "reserved" user
> > > name? For example,
> > > 
> > >    [http] sslcert = ~system_path/what/ever
> > > 
> > > although a more unique name, perhaps with some punctuation, may be
> > > desirable.
> > 
> > It's syntactically a bit further afield, but something like:
> > 
> >    [http]
> >    sslcert = $RUNTIME_PREFIX/what/ever
> > 
> > would make sense to me, and is a bit less subtle than the fake user. I
> > don't know if that would confuse people into thinking that we
> > interpolate arbitrary environment variables, though.
> 
> The expansion of a fake user name would have to go in expand_user_path(), a
> fake variable name would have to be expanded elsewhere. Now, Dscho mentions
> in the cover letter that his patch has already seen some field testing ("has
> been 'in production' for a good while"). So, we have gained some confidence
> that the point where the substitution happens, in expand_user_path(), is
> suitable. Therefore, I have slight preference for a fake user.

I don't think that necessarily needs to limit us. expand_user_path() is
named that because right now it just expands "~". But there's no reason
it couldn't be used for general expansion. The problem in the original
patch is that it expands _even when the user has not asked us to do it_.
Looking at the callers, most of them would be fine with the new
expansion (the only exception is the one in daemon.c, though it manually
checks for "~" already).

Now I agree that the new function would probably need a better name, at
which point you could easily have a function expand_path() which just
wraps expand_user_path().

All that said, if we're just interested in allowing this for config,
then we already have such a wrapper function: git_config_pathname().

So I don't think it's a big deal to implement it in any of these ways.
It's much more important to get the syntax right, because that's
user-facing and will be with us forever.

-Peff