[1/1] mingw: handle absolute paths in expand_user_path()
diff mbox series

Message ID 2287dd96cf0b9e9e250fdf92a32dcf666510e67d.1541515994.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • mingw: handle absolute paths in expand_user_path()
Related show

Commit Message

Marco Trevisan via GitGitGadget Nov. 6, 2018, 2:53 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, an absolute POSIX path needs to be turned into a Windows
one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 path.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ramsay Jones Nov. 6, 2018, 3:54 p.m. UTC | #1
On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  path.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>  #include "path.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "exec-cmd.h"
>  
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>  
>  	if (path == NULL)
>  		goto return_null;
> +#ifdef __MINGW32__
> +	if (path[0] == '/')
> +		return system_path(path + 1);
> +#endif

Hmm, this doesn't quite fit with the intended use of this
function! ;-) (even on windows!)

I haven't looked very deeply, but doesn't this affect all
absolute paths in the config read by git_config_pathname(),
along with all 'included config' files?

I am pretty sure that I would not want the absolute paths
in my config file(s) magically 'moved' depending on whether
git has been compiled with 'runtime prefix' support or not!

ATB,
Ramsay Jones

>  	if (path[0] == '~') {
>  		const char *first_slash = strchrnul(path, '/');
>  		const char *username = path + 1;
>
Ramsay Jones Nov. 6, 2018, 4:10 p.m. UTC | #2
On 06/11/2018 15:54, Ramsay Jones wrote:
> 
> 
> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  path.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>>  #include "path.h"
>>  #include "packfile.h"
>>  #include "object-store.h"
>> +#include "exec-cmd.h"
>>  
>>  static int get_st_mode_bits(const char *path, int *mode)
>>  {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>  
>>  	if (path == NULL)
>>  		goto return_null;
>> +#ifdef __MINGW32__
>> +	if (path[0] == '/')
>> +		return system_path(path + 1);
>> +#endif
> 
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
> 
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?
> 
> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!

So, I hit 'send' before finishing my thought ...

I can't think of a non-backwards compatible way of doing
what you want. If backward compatibility wasn't an issue,
then we could (maybe) have used some kind of pathname prefix
like 'system:/path/relative/to/git/executable', or somesuch.

ATB,
Ramsay Jones
Duy Nguyen Nov. 6, 2018, 6:24 p.m. UTC | #3
On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  path.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>  #include "path.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "exec-cmd.h"
>
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>
>         if (path == NULL)
>                 goto return_null;
> +#ifdef __MINGW32__
> +       if (path[0] == '/')
> +               return system_path(path + 1);
> +#endif

Should this behavior be documented somewhere, maybe in config.txt?

>         if (path[0] == '~') {
>                 const char *first_slash = strchrnul(path, '/');
>                 const char *username = path + 1;
> --
> gitgitgadget
Duy Nguyen Nov. 6, 2018, 6:27 p.m. UTC | #4
On Tue, Nov 6, 2018 at 7:15 PM Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >>
> >>      if (path == NULL)
> >>              goto return_null;
> >> +#ifdef __MINGW32__
> >> +    if (path[0] == '/')
> >> +            return system_path(path + 1);
> >> +#endif
> >
> > Hmm, this doesn't quite fit with the intended use of this
> > function! ;-) (even on windows!)
> >
> > I haven't looked very deeply, but doesn't this affect all
> > absolute paths in the config read by git_config_pathname(),
> > along with all 'included config' files?
> >
> > I am pretty sure that I would not want the absolute paths
> > in my config file(s) magically 'moved' depending on whether
> > git has been compiled with 'runtime prefix' support or not!
>
> So, I hit 'send' before finishing my thought ...
>
> I can't think of a non-backwards compatible way of doing
> what you want. If backward compatibility wasn't an issue,
> then we could (maybe) have used some kind of pathname prefix
> like 'system:/path/relative/to/git/executable', or somesuch.

A pseudo variable might work, like $ROOT/path/relative/to/somewhere
Johannes Sixt Nov. 6, 2018, 9:32 p.m. UTC | #5
Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.

If I were picky, I would say that in a pure Windows application there 
cannot be POSIX paths to begin with.

Even if a path looks like a POSIX paths, i.e. it starts with a directory 
separator, but not with drive-letter-colon, it still has a particular 
meaning, namely (as far as I know) that the path is anchored at the root 
of the drive of the current working directory.

If a user specifies such a path on Windows, and it must undergo 
expand_user_path(), then that is a user error, or the user knows what 
they are doing.

I think it is wrong to interpret such a path as relative to some random 
other directory, like this patch seems to do.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   path.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>   #include "path.h"
>   #include "packfile.h"
>   #include "object-store.h"
> +#include "exec-cmd.h"
>   
>   static int get_st_mode_bits(const char *path, int *mode)
>   {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>   
>   	if (path == NULL)
>   		goto return_null;
> +#ifdef __MINGW32__
> +	if (path[0] == '/')
> +		return system_path(path + 1);
> +#endif
>   	if (path[0] == '~') {
>   		const char *first_slash = strchrnul(path, '/');
>   		const char *username = path + 1;
>
Junio C Hamano Nov. 7, 2018, 1:21 a.m. UTC | #6
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> 
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  path.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>>  #include "path.h"
>>  #include "packfile.h"
>>  #include "object-store.h"
>> +#include "exec-cmd.h"
>>  
>>  static int get_st_mode_bits(const char *path, int *mode)
>>  {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>  
>>  	if (path == NULL)
>>  		goto return_null;
>> +#ifdef __MINGW32__
>> +	if (path[0] == '/')
>> +		return system_path(path + 1);
>> +#endif
>
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
>
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?

I think so.  I have not thought things through to say if replacing a
"full path in the current drive" with system_path() is a sensible
thing to do in the first place, but I am getting the impression from
review comments that it probably is not.

> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!

In any case, the helper is about expanding ~/foo and ~who/foo to
absolute paths, without touching other paths, so it is a wrong
function to implement it in, even if the motivation were sensible.
Johannes Schindelin Nov. 7, 2018, 11:19 a.m. UTC | #7
Hi,

On Wed, 7 Nov 2018, Junio C Hamano wrote:

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> > On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
> >> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> 
> >> On Windows, an absolute POSIX path needs to be turned into a Windows
> >> one.
> >> 
> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> ---
> >>  path.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/path.c b/path.c
> >> index 34f0f98349..a72abf0e1f 100644
> >> --- a/path.c
> >> +++ b/path.c
> >> @@ -11,6 +11,7 @@
> >>  #include "path.h"
> >>  #include "packfile.h"
> >>  #include "object-store.h"
> >> +#include "exec-cmd.h"
> >>  
> >>  static int get_st_mode_bits(const char *path, int *mode)
> >>  {
> >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >>  
> >>  	if (path == NULL)
> >>  		goto return_null;
> >> +#ifdef __MINGW32__
> >> +	if (path[0] == '/')
> >> +		return system_path(path + 1);
> >> +#endif
> >
> > Hmm, this doesn't quite fit with the intended use of this
> > function! ;-) (even on windows!)
> >
> > I haven't looked very deeply, but doesn't this affect all
> > absolute paths in the config read by git_config_pathname(),
> > along with all 'included config' files?
> 
> I think so.  I have not thought things through to say if replacing a
> "full path in the current drive" with system_path() is a sensible
> thing to do in the first place, but I am getting the impression from
> review comments that it probably is not.
> 
> > I am pretty sure that I would not want the absolute paths
> > in my config file(s) magically 'moved' depending on whether
> > git has been compiled with 'runtime prefix' support or not!

The cute thing is: your absolute paths would not be moved because we are
talking about Windows. Therefore your absolute paths would not start with
a forward slash.

> In any case, the helper is about expanding ~/foo and ~who/foo to
> absolute paths, without touching other paths, so it is a wrong
> function to implement it in, even if the motivation were sensible.

It could be renamed. In any case, for this feature we would need to expand
a path that is not the final path, and this here location is the most
logical place to do so.

Ciao,
Dscho
Johannes Schindelin Nov. 7, 2018, 11:19 a.m. UTC | #8
Hi,

On Tue, 6 Nov 2018, Duy Nguyen wrote:

> On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On Windows, an absolute POSIX path needs to be turned into a Windows
> > one.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  path.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/path.c b/path.c
> > index 34f0f98349..a72abf0e1f 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -11,6 +11,7 @@
> >  #include "path.h"
> >  #include "packfile.h"
> >  #include "object-store.h"
> > +#include "exec-cmd.h"
> >
> >  static int get_st_mode_bits(const char *path, int *mode)
> >  {
> > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >
> >         if (path == NULL)
> >                 goto return_null;
> > +#ifdef __MINGW32__
> > +       if (path[0] == '/')
> > +               return system_path(path + 1);
> > +#endif
> 
> Should this behavior be documented somewhere, maybe in config.txt?

First we need to find a consensus how this should work.

Ciao,
Dscho

> 
> >         if (path[0] == '~') {
> >                 const char *first_slash = strchrnul(path, '/');
> >                 const char *username = path + 1;
> > --
> > gitgitgadget
> -- 
> Duy
>
Johannes Schindelin Nov. 7, 2018, 11:23 a.m. UTC | #9
Hi Hannes,

On Tue, 6 Nov 2018, Johannes Sixt wrote:

> Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > On Windows, an absolute POSIX path needs to be turned into a Windows
> > one.
> 
> If I were picky, I would say that in a pure Windows application there cannot
> be POSIX paths to begin with.
> 
> Even if a path looks like a POSIX paths, i.e. it starts with a directory
> separator, but not with drive-letter-colon, it still has a particular meaning,
> namely (as far as I know) that the path is anchored at the root of the drive
> of the current working directory.
> 
> If a user specifies such a path on Windows, and it must undergo
> expand_user_path(), then that is a user error, or the user knows what they are
> doing.
> 
> I think it is wrong to interpret such a path as relative to some random other
> directory, like this patch seems to do.

Okay, now we know everything you find wrong with the current patch. Do you
have any suggestion how to make it right? I.e. what would you suggest as a
way to specify in a gitconfig in a portable Git where the certificate
bundle is?

Thanks,
Dscho

> 
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   path.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/path.c b/path.c
> > index 34f0f98349..a72abf0e1f 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -11,6 +11,7 @@
> >   #include "path.h"
> >   #include "packfile.h"
> >   #include "object-store.h"
> > +#include "exec-cmd.h"
> >   
> >   static int get_st_mode_bits(const char *path, int *mode)
> >   {
> > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >   
> >    if (path == NULL)
> >   		goto return_null;
> > +#ifdef __MINGW32__
> > +	if (path[0] == '/')
> > +		return system_path(path + 1);
> > +#endif
> >    if (path[0] == '~') {
> >     const char *first_slash = strchrnul(path, '/');
> >     const char *username = path + 1;
> > 
> 
> 
>
Ramsay Jones Nov. 7, 2018, 5:42 p.m. UTC | #10
On 07/11/2018 11:19, Johannes Schindelin wrote:
[snip]
>>> Hmm, this doesn't quite fit with the intended use of this
>>> function! ;-) (even on windows!)
>>>
>>> I haven't looked very deeply, but doesn't this affect all
>>> absolute paths in the config read by git_config_pathname(),
>>> along with all 'included config' files?
>>
>> I think so.  I have not thought things through to say if replacing a
>> "full path in the current drive" with system_path() is a sensible
>> thing to do in the first place, but I am getting the impression from
>> review comments that it probably is not.
>>
>>> I am pretty sure that I would not want the absolute paths
>>> in my config file(s) magically 'moved' depending on whether
>>> git has been compiled with 'runtime prefix' support or not!
> 
> The cute thing is: your absolute paths would not be moved because we are
> talking about Windows. Therefore your absolute paths would not start with
> a forward slash.

Ah, sorry, I must have misunderstood a comment in your cover letter:

    The reason is this: something like this (make paths specified e.g. via 
    http.sslCAInfo relative to the runtime prefix) is potentially useful
    also in the non-Windows context, as long as Git was built with the
    runtime prefix feature.

... so I thought you meant to add this code for POSIX systems as well.

My mistake. :(

ATB,
Ramsay Jones
Johannes Sixt Nov. 7, 2018, 6:52 p.m. UTC | #11
Am 07.11.18 um 12:23 schrieb Johannes Schindelin:
> On Tue, 6 Nov 2018, Johannes Sixt wrote:
> 
>> Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:
>> Even if a path looks like a POSIX paths, i.e. it starts with a directory
>> separator, but not with drive-letter-colon, it still has a particular meaning,
>> namely (as far as I know) that the path is anchored at the root of the drive
>> of the current working directory.
>>
>> If a user specifies such a path on Windows, and it must undergo
>> expand_user_path(), then that is a user error, or the user knows what they are
>> doing.
>>
>> I think it is wrong to interpret such a path as relative to some random other
>> directory, like this patch seems to do.
> 
> Okay, now we know everything you find wrong with the current patch. Do you
> have any suggestion how to make it right? I.e. what would you suggest as a
> way to specify in a gitconfig in a portable Git where the certificate
> bundle is?

Ah, so your actual problem is quite a different one!

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.

-- Hannes
Jeff King Nov. 7, 2018, 8:41 p.m. UTC | #12
On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote:

> > Okay, now we know everything you find wrong with the current patch. Do you
> > have any suggestion how to make it right? I.e. what would you suggest as a
> > way to specify in a gitconfig in a portable Git where the certificate
> > bundle is?
> 
> Ah, so your actual problem is quite a different one!
> 
> 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.

-Peff
Johannes Sixt Nov. 7, 2018, 9:36 p.m. UTC | #13
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.

-- Hannes
Jeff King Nov. 7, 2018, 10:03 p.m. UTC | #14
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
Junio C Hamano Nov. 8, 2018, 12:16 a.m. UTC | #15
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> The cute thing is: your absolute paths would not be moved because we are
>> talking about Windows. Therefore your absolute paths would not start with
>> a forward slash.
>
> Ah, sorry, I must have misunderstood a comment in your cover letter:
>
>     The reason is this: something like this (make paths specified e.g. via 
>     http.sslCAInfo relative to the runtime prefix) is potentially useful
>     also in the non-Windows context, as long as Git was built with the
>     runtime prefix feature.
>
> ... so I thought you meant to add this code for POSIX systems as well.
>
> My mistake. :(

Well, I do not think you should feel so bad about it, as you are not
alone.

It wasn't clear to me either that it was to introduce a new syntax
that users would not have otherwise used before (i.e. avoids
negatively affecting existing settings---because the users would
have used a path that begins with a backslash if they meant "full
path down from the top of the current drive") to mean a new thing
(i.e. "relative to the runtime prefix") that the patch wanted to do.

If that is the motivation, then it does make sense to extend this
function, and possibly rename it, like this patch does, as we would
want this new syntax available in anywhere we take a pathname to an
untracked thing. And for such a pathname, we should be consistently
using expand_user_path() *anyway* even without this new "now we know
how to spell 'relative to the runtime prefix'" feature.

So I agree with the patch that the issue it wants to address is
worth addressing, and the function to enhance is this one.

I am still unsure about the solution, though.

I suspect that any build that uses runtime prefix would want access
to this feature, regardless of the platform.  The new syntax that is
"a pathname that begins with a slash is not an absolute path but is
relative to the runtime prefix" cannot avoid ambiguity with users'
existing settings on platforms other than Windows, which means this
feature cannot be made platform neutral in its posted form.  The
documentation must say "On windows, the way to spell runtime prefix
relative paths is this; on macs, you must spell it differently like
this." etc.  Which is rather unfortunate.

Perhaps we need to make an effort to find a syntax that is
universally unambiguous and use it consistently across platforms?

I am tempted to say "//<token>/<the remainder>" might also be such a
way, even in the POSIX world, but am not brave enough to do so, as I
suspect that may have a fallout in the Windows world X-<.

An earlier suggestion by Duy in [*1*] to pretend as if we take
$VARIABLE and define special variables might be a better direction.

Are there security implications if we started allowing references to
environment varibables in strings we pass expand_user_path()?  If we
cannot convince ourselves that it is safe to allow access to any and
all environment variables, then we probably can start by specific
"pseudo variables" under our control, like GIT_EXEC_PATH and
GIT_INSTALL_ROOT, that do not even have to be tied to environment
variables, perhaps with further restriction to allow it only at the
beginning of the string, or something like that, if necessary.

[References]

*1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com>
Junio C Hamano Nov. 8, 2018, 12:30 a.m. UTC | #16
Jeff King <peff@peff.net> writes:

> On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote:
>
> 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.

All of us are on the same page after seeing the clarification by
Dscho, it seems.  I came to pretty much the same conclusion this
morning before reading this subthread.  Outside config values, the
callers of expand_user_path() only feed "~/.git$constant", and they
are all about "personal customization" that do not want to be shared
with other users of the same installation, so "relative to runtime
prefix" feature would not be wanted.  But we do not know about new
caller's needs.  For now I am OK to have it in expand_user_path(),
possibly renaming the function if people feel it is needed (I don't).

Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have
a strong preference either way, but I am getting an impression that
the latter is more generally favoured in the discussion?
Jeff King Nov. 8, 2018, 1:18 a.m. UTC | #17
On Thu, Nov 08, 2018 at 09:30:15AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote:
> >
> > 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.
> 
> All of us are on the same page after seeing the clarification by
> Dscho, it seems.  I came to pretty much the same conclusion this
> morning before reading this subthread.  Outside config values, the
> callers of expand_user_path() only feed "~/.git$constant", and they
> are all about "personal customization" that do not want to be shared
> with other users of the same installation, so "relative to runtime
> prefix" feature would not be wanted.  But we do not know about new
> caller's needs.  For now I am OK to have it in expand_user_path(),
> possibly renaming the function if people feel it is needed (I don't).

I think we would want to carefully think about the call in enter_repo().
We do not want git-daemon to accidentally expose repositories in
$RUNTIME_PREFIX.

Looking over the code, I think this is OK. The expansion happens in
enter_repo(), and then we take the path that was found and do our
ok_paths checks on it (which makes sense -- even now you'd ask to export
"/home/" and it would need to look at "~peff/repo.git" and expand that
to "/home/peff/repo.git" before doing a simple string check.

> Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have
> a strong preference either way, but I am getting an impression that
> the latter is more generally favoured in the discussion?

I certainly prefer the latter, but I thought I was the only one to have
expressed support so far. ;)

-Peff
Junio C Hamano Nov. 8, 2018, 3:26 a.m. UTC | #18
Jeff King <peff@peff.net> writes:

> I think we would want to carefully think about the call in enter_repo().
> We do not want git-daemon to accidentally expose repositories in
> $RUNTIME_PREFIX.
>
> Looking over the code, I think this is OK. The expansion happens in
> enter_repo(), and then we take the path that was found and do our
> ok_paths checks on it (which makes sense -- even now you'd ask to export
> "/home/" and it would need to look at "~peff/repo.git" and expand that
> to "/home/peff/repo.git" before doing a simple string check.

Yup, that is another reason why I think this new "expansion feature"
belongs to the function, not to a wrapper that is aware of this new
feature in addition to ~tilde expansion.

>> Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have
>> a strong preference either way, but I am getting an impression that
>> the latter is more generally favoured in the discussion?
>
> I certainly prefer the latter, but I thought I was the only one to have
> expressed support so far. ;)

The first mention of pseudo-variable I saw was in Duy's message,
wondering if $ROOT is more appropriate than "/", and I counted it as
supporting the $VARIABLE syntax.
Johannes Schindelin Nov. 8, 2018, 1:04 p.m. UTC | #19
Hi,

On Thu, 8 Nov 2018, Junio C Hamano wrote:

> I am tempted to say "//<token>/<the remainder>" might also be such a
> way, even in the POSIX world, but am not brave enough to do so, as I
> suspect that may have a fallout in the Windows world X-<.

It does. //server/share is the way we refer to UNC paths (AKA network
drives).

> An earlier suggestion by Duy in [*1*] to pretend as if we take
> $VARIABLE and define special variables might be a better direction.

My only qualm with this is that `$VARIABLE` is a perfectly allowed part of
a path. That is, you *could* create a directory called `$VARIABLE` and
reference that, and then the expand_user_path() function (or whatever name
we will give it) would always expand this, with no way to switch it off.

Granted, this is a highly unlikely scenario, but I would feel a bit more
comfortable with something like

	<RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt

Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name,
but I would argue that it is even less likely to exist than
`$RUNTIME_PREFIX` because the user would have to escape *two* characters
rather than one.

> Are there security implications if we started allowing references to
> environment varibables in strings we pass expand_user_path()?

Probably. But then, the runtime prefix is not even available as
environment variable...

Ciao,
Dscho

> If we cannot convince ourselves that it is safe to allow access to any
> and all environment variables, then we probably can start by specific
> "pseudo variables" under our control, like GIT_EXEC_PATH and
> GIT_INSTALL_ROOT, that do not even have to be tied to environment
> variables, perhaps with further restriction to allow it only at the
> beginning of the string, or something like that, if necessary.
> 
> [References]
> 
> *1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com>
>
Johannes Schindelin Nov. 8, 2018, 1:11 p.m. UTC | #20
Hi Peff,

On Wed, 7 Nov 2018, Jeff King wrote:

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

Good point. I agree that `git_config_pathname()` is a better home for this
feature than `expand_user_path()`.

But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
The `~` prefix is *already* a reserved character, and while it would
probably not be super intuitive to have `~~` be expanded to the runtime
prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which
*is* a valid directory name).

Of course, `~~` is also a valid directory name, but then, so is `~` and we
do not have any way to specify *that* because `expand_user_path()` will
always expand it to the home directory. Or am I wrong? Do we have a way to
disable the expansion?

Ciao,
Dscho
Duy Nguyen Nov. 8, 2018, 2:25 p.m. UTC | #21
On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Peff,
>
> On Wed, 7 Nov 2018, Jeff King wrote:
>
> > All that said, if we're just interested in allowing this for config,
> > then we already have such a wrapper function: git_config_pathname().
>
> Good point. I agree that `git_config_pathname()` is a better home for this
> feature than `expand_user_path()`.
>
> But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
> The `~` prefix is *already* a reserved character, and while it would
> probably not be super intuitive to have `~~` be expanded to the runtime
> prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which
> *is* a valid directory name).

One thing I had in mind when proposing $VARIABLE is that it opens up a
namespace for us to expand more things (*) for example $GIT_DIR (from
~/.gitconfig).

(*) but in a controlled way, it may look like an environment variable,
but we only accept a selected few. And it's only expanded at the
beginning of a path.

> Of course, `~~` is also a valid directory name, but then, so is `~` and we
> do not have any way to specify *that* because `expand_user_path()` will
> always expand it to the home directory. Or am I wrong? Do we have a way to
> disable the expansion?
>
> Ciao,
> Dscho
Junio C Hamano Nov. 8, 2018, 2:43 p.m. UTC | #22
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Thu, 8 Nov 2018, Junio C Hamano wrote:
>
>> I am tempted to say "//<token>/<the remainder>" might also be such a
>> way, even in the POSIX world, but am not brave enough to do so, as I
>> suspect that may have a fallout in the Windows world X-<.
>
> It does. //server/share is the way we refer to UNC paths (AKA network
> drives).

Shucks.  That would mean the patch that started this thread would
not be a good idea, as an end-user could already be writing
"//server/share/some/path" and the code with the patch would see '/'
that begins it, and start treating it differently than the code
before the patch X-<.

> Granted, this is a highly unlikely scenario, but I would feel a bit more
> comfortable with something like
>
> 	<RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt
>
> Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name,
> but I would argue that it is even less likely to exist than
> `$RUNTIME_PREFIX` because the user would have to escape *two* characters
> rather than one.

Yes, and it is naturally extensible by allowing <OTHER_THINGS>
inside the special bra-ket pair (just like $OTHER_THINGS can be a
way to extend the system if we used a special variable syntax).

>> Are there security implications if we started allowing references to
>> environment varibables in strings we pass expand_user_path()?
>
> Probably. But then, the runtime prefix is not even available as
> environment variable...

Ah, sorry. I thought it was clear that I would next be suggesting to
add an environmet variable for it, _if_ the approach to allow env
references turns out to be viable.
Junio C Hamano Nov. 8, 2018, 2:47 p.m. UTC | #23
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
> The `~` prefix is *already* a reserved character,...

We would need to prepare for a future where we need yet another
special thing to be expanded, and it will quickly become cryptic if
you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix,
and ~~~/ is this new thing...".  ~runtime_prefix~/ (i.e. carve out
the namespace for USERNAME by reserving any names that ends with a
tilde) may be a viable way to do this, though.   It is just as good
as your other idea, <runtime_prefix>, in an earlier message.
Johannes Schindelin Nov. 8, 2018, 3:39 p.m. UTC | #24
Hi Junio,

On Thu, 8 Nov 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 8 Nov 2018, Junio C Hamano wrote:
> >
> >> I am tempted to say "//<token>/<the remainder>" might also be such a
> >> way, even in the POSIX world, but am not brave enough to do so, as I
> >> suspect that may have a fallout in the Windows world X-<.
> >
> > It does. //server/share is the way we refer to UNC paths (AKA network
> > drives).
> 
> Shucks.  That would mean the patch that started this thread would
> not be a good idea, as an end-user could already be writing
> "//server/share/some/path" and the code with the patch would see '/'
> that begins it, and start treating it differently than the code
> before the patch X-<.

Ouch. You're right!

> > Granted, this is a highly unlikely scenario, but I would feel a bit more
> > comfortable with something like
> >
> > 	<RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt
> >
> > Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name,
> > but I would argue that it is even less likely to exist than
> > `$RUNTIME_PREFIX` because the user would have to escape *two* characters
> > rather than one.
> 
> Yes, and it is naturally extensible by allowing <OTHER_THINGS>
> inside the special bra-ket pair (just like $OTHER_THINGS can be a
> way to extend the system if we used a special variable syntax).

True.

> >> Are there security implications if we started allowing references to
> >> environment varibables in strings we pass expand_user_path()?
> >
> > Probably. But then, the runtime prefix is not even available as
> > environment variable...
> 
> Ah, sorry. I thought it was clear that I would next be suggesting to
> add an environmet variable for it, _if_ the approach to allow env
> references turns out to be viable.

Of course, I should have assumed that. Sorry!

Ciao,
Dscho
Johannes Schindelin Nov. 8, 2018, 3:45 p.m. UTC | #25
Hi Duy,

On Thu, 8 Nov 2018, Duy Nguyen wrote:

> On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 7 Nov 2018, Jeff King wrote:
> >
> > > All that said, if we're just interested in allowing this for config,
> > > then we already have such a wrapper function: git_config_pathname().
> >
> > Good point. I agree that `git_config_pathname()` is a better home for this
> > feature than `expand_user_path()`.
> >
> > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
> > The `~` prefix is *already* a reserved character, and while it would
> > probably not be super intuitive to have `~~` be expanded to the runtime
> > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which
> > *is* a valid directory name).
> 
> One thing I had in mind when proposing $VARIABLE is that it opens up a
> namespace for us to expand more things (*) for example $GIT_DIR (from
> ~/.gitconfig).
> 
> (*) but in a controlled way, it may look like an environment variable,
> but we only accept a selected few. And it's only expanded at the
> beginning of a path.

I understand that desire, and I even agree. But the fact that it looks
like an environment variable, but is not, is actually a point in favor of
*not* doing this. It would violate the principle of least astonishment.

The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything
that it is not, I would think. The only thing against this form (at least
that I can think of) is that some people use this way to talk about paths
that vary between different setups, with the implicit assumption that the
reader will "interpolate" this *before* applying. So for example, when I
tell a user to please edit their <GIT_DIR>/config, I implicitly assume
that they know to not type out, literally, <GIT_DIR> but instead fill in
the correct path.

Ciao,
Dscho

> > Of course, `~~` is also a valid directory name, but then, so is `~` and we
> > do not have any way to specify *that* because `expand_user_path()` will
> > always expand it to the home directory. Or am I wrong? Do we have a way to
> > disable the expansion?
> >
> > Ciao,
> > Dscho
> 
> 
> 
> -- 
> Duy
>
Johannes Schindelin Nov. 8, 2018, 3:46 p.m. UTC | #26
Hi Junio,

On Thu, 8 Nov 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
> > The `~` prefix is *already* a reserved character,...
> 
> We would need to prepare for a future where we need yet another
> special thing to be expanded, and it will quickly become cryptic if
> you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix,
> and ~~~/ is this new thing...".  ~runtime_prefix~/ (i.e. carve out
> the namespace for USERNAME by reserving any names that ends with a
> tilde) may be a viable way to do this, though.   It is just as good
> as your other idea, <runtime_prefix>, in an earlier message.

Indeed. Your "cryptic" matches my "unintuitive".

Ciao,
Dscho
Eric Sunshine Nov. 8, 2018, 5:40 p.m. UTC | #27
On Thu, Nov 8, 2018 at 10:45 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 8 Nov 2018, Duy Nguyen wrote:
> > One thing I had in mind when proposing $VARIABLE is that it opens up a
> > namespace for us to expand more things (*) for example $GIT_DIR (from
> > ~/.gitconfig).
> >
> > (*) but in a controlled way, it may look like an environment variable,
> > but we only accept a selected few. And it's only expanded at the
> > beginning of a path.
>
> I understand that desire, and I even agree. But the fact that it looks
> like an environment variable, but is not, is actually a point in favor of
> *not* doing this. It would violate the principle of least astonishment.

Perhaps something like $:VARIABLE, which gives the convenience of
$VARIABLE, but looks sufficiently different that it shouldn't trip up
readers into thinking it is one. (Well-written code checking for a
DOS/Windows drive letter at the beginning of a path shouldn't be
confused by it.)
Joseph Moisan Nov. 9, 2018, 2:05 a.m. UTC | #28
Can someone please tell me how to unsubscribe from this email.  I am no longer interested in receiving these emails, and cannot find how to unsubscribe.
Thank you in advance.

Joseph Moisan | Software Engineer
Building Technologies and Solutions
Tel: +1-978-731-8950
Joseph.Moisan@JCI.com

-----Original Message-----
From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Junio C Hamano
Sent: Wednesday, November 7, 2018 7:17 PM
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>; git@vger.kernel.org
Subject: Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> The cute thing is: your absolute paths would not be moved because we 
>> are talking about Windows. Therefore your absolute paths would not 
>> start with a forward slash.
>
> Ah, sorry, I must have misunderstood a comment in your cover letter:
>
>     The reason is this: something like this (make paths specified e.g. via 
>     http.sslCAInfo relative to the runtime prefix) is potentially useful
>     also in the non-Windows context, as long as Git was built with the
>     runtime prefix feature.
>
> ... so I thought you meant to add this code for POSIX systems as well.
>
> My mistake. :(

Well, I do not think you should feel so bad about it, as you are not alone.

It wasn't clear to me either that it was to introduce a new syntax that users would not have otherwise used before (i.e. avoids negatively affecting existing settings---because the users would have used a path that begins with a backslash if they meant "full path down from the top of the current drive") to mean a new thing (i.e. "relative to the runtime prefix") that the patch wanted to do.

If that is the motivation, then it does make sense to extend this function, and possibly rename it, like this patch does, as we would want this new syntax available in anywhere we take a pathname to an untracked thing. And for such a pathname, we should be consistently using expand_user_path() *anyway* even without this new "now we know how to spell 'relative to the runtime prefix'" feature.

So I agree with the patch that the issue it wants to address is worth addressing, and the function to enhance is this one.

I am still unsure about the solution, though.

I suspect that any build that uses runtime prefix would want access to this feature, regardless of the platform.  The new syntax that is "a pathname that begins with a slash is not an absolute path but is relative to the runtime prefix" cannot avoid ambiguity with users'
existing settings on platforms other than Windows, which means this feature cannot be made platform neutral in its posted form.  The documentation must say "On windows, the way to spell runtime prefix relative paths is this; on macs, you must spell it differently like this." etc.  Which is rather unfortunate.

Perhaps we need to make an effort to find a syntax that is universally unambiguous and use it consistently across platforms?

I am tempted to say "//<token>/<the remainder>" might also be such a way, even in the POSIX world, but am not brave enough to do so, as I suspect that may have a fallout in the Windows world X-<.

An earlier suggestion by Duy in [*1*] to pretend as if we take $VARIABLE and define special variables might be a better direction.

Are there security implications if we started allowing references to environment varibables in strings we pass expand_user_path()?  If we cannot convince ourselves that it is safe to allow access to any and all environment variables, then we probably can start by specific "pseudo variables" under our control, like GIT_EXEC_PATH and GIT_INSTALL_ROOT, that do not even have to be tied to environment variables, perhaps with further restriction to allow it only at the beginning of the string, or something like that, if necessary.

[References]

*1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com>
Jeff King Nov. 9, 2018, 10:19 a.m. UTC | #29
On Thu, Nov 08, 2018 at 04:45:16PM +0100, Johannes Schindelin wrote:

> > One thing I had in mind when proposing $VARIABLE is that it opens up a
> > namespace for us to expand more things (*) for example $GIT_DIR (from
> > ~/.gitconfig).
> > 
> > (*) but in a controlled way, it may look like an environment variable,
> > but we only accept a selected few. And it's only expanded at the
> > beginning of a path.
> 
> I understand that desire, and I even agree. But the fact that it looks
> like an environment variable, but is not, is actually a point in favor of
> *not* doing this. It would violate the principle of least astonishment.

I agree that it is potentially surprising. OTOH, it is at least pretty
obvious when you see in the wild something like:

  [core]
  foo = $RUNTIME_PREFIX/bar

what it is _trying_ to do. My big concern with "~"-based things is that
they're kind of subtle. If I saw:

  [core]
  foo = ~~/bar

I'm not sure what I would think it does. ;)

> The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything
> that it is not, I would think. The only thing against this form (at least
> that I can think of) is that some people use this way to talk about paths
> that vary between different setups, with the implicit assumption that the
> reader will "interpolate" this *before* applying. So for example, when I
> tell a user to please edit their <GIT_DIR>/config, I implicitly assume
> that they know to not type out, literally, <GIT_DIR> but instead fill in
> the correct path.

So yeah, some alternative syntax that is verbose but distinct makes
sense to me. We use %-substitution elsewhere. Could we do something with
that? "%RUNTIME_PREFIX%" gives me too many flashbacks, but something
like "%(RUNTIME_PREFIX)" matches our formatting language.

I dunno. I actually do not think "$FOO" is so bad, as long as we clearly
document that:

  - we do not allow arbitrary $FOO from the environment (though I am
    actually not all that opposed to doing so)

  - we add some special $FOOs that are not actually environment
    variables

-Peff
Jeff King Nov. 9, 2018, 10:21 a.m. UTC | #30
On Fri, Nov 09, 2018 at 02:05:48AM +0000, Joseph Moisan wrote:

> Can someone please tell me how to unsubscribe from this email.  I am
> no longer interested in receiving these emails, and cannot find how to
> unsubscribe.

Details are at http://vger.kernel.org/vger-lists.html#git.

-Peff
Duy Nguyen Nov. 9, 2018, 4:16 p.m. UTC | #31
On Fri, Nov 9, 2018 at 11:19 AM Jeff King <peff@peff.net> wrote:
> > The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything
> > that it is not, I would think. The only thing against this form (at least
> > that I can think of) is that some people use this way to talk about paths
> > that vary between different setups, with the implicit assumption that the
> > reader will "interpolate" this *before* applying. So for example, when I
> > tell a user to please edit their <GIT_DIR>/config, I implicitly assume
> > that they know to not type out, literally, <GIT_DIR> but instead fill in
> > the correct path.
>
> So yeah, some alternative syntax that is verbose but distinct makes
> sense to me. We use %-substitution elsewhere. Could we do something with
> that? "%RUNTIME_PREFIX%" gives me too many flashbacks, but something
> like "%(RUNTIME_PREFIX)" matches our formatting language.

FWIW I don't have any preference, as long as the variable can still
have a name (that is not a symbol).

A side question regardless of syntax. What do we do with
%(unrecognized name)/foo? I see three options

 - expand to empty, so "/foo"
 - keep it and try the literal path "%(unrecognized name)/foo"
 - somehow tell the caller that the path is invalid and treat it like
non-existing path, even if there is some real thing at "%(unrecognized
name)/foo"

The last option is more predictable. But we need to be more careful
about the syntax because if "%(some path like this)" actually exists
somewhere, then it will be broken. And I think it's also more work.
Junio C Hamano Nov. 12, 2018, 3:03 a.m. UTC | #32
Duy Nguyen <pclouds@gmail.com> writes:

> FWIW I don't have any preference, as long as the variable can still
> have a name (that is not a symbol).

Same here.

> A side question regardless of syntax. What do we do with
> %(unrecognized name)/foo? I see three options
>
>  - expand to empty, so "/foo"
>  - keep it and try the literal path "%(unrecognized name)/foo"

Neither of these is good for future proofing purposes.

>  - somehow tell the caller that the path is invalid and treat it like
> non-existing path, even if there is some real thing at "%(unrecognized
> name)/foo"

Another thing to worry about is how to spell the real thing that has
such a funny name, perhaps by escaping like "%%(unrecognized
name)/foo".

And from that point of view, "~runtime-prefix~/foo" (i.e. what J6t
started) may make the most sense, as I do not think we need to
support a username that ends with a tilde by introducing a quoting
convention.

Patch
diff mbox series

diff --git a/path.c b/path.c
index 34f0f98349..a72abf0e1f 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@ 
 #include "path.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "exec-cmd.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -709,6 +710,10 @@  char *expand_user_path(const char *path, int real_home)
 
 	if (path == NULL)
 		goto return_null;
+#ifdef __MINGW32__
+	if (path[0] == '/')
+		return system_path(path + 1);
+#endif
 	if (path[0] == '~') {
 		const char *first_slash = strchrnul(path, '/');
 		const char *username = path + 1;