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

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

[Johannes Schindelin via GitGitGadget]

While this patch has been "in production" in Git for Windows for a good
while, this patch series is half meant as a request for comments.

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.

The main problem with non-Windows platforms is that paths starting with a
single slash are unambiguously absolute, whereas we can say with certainty
that they are not absolute Windows paths.

So maybe someone on this list has a clever idea how we could specify paths
(unambiguously, even on non-Windows platforms) that Git should interpret as
relative to the runtime prefix?

Johannes Schindelin (1):
  mingw: handle absolute paths in expand_user_path()

 path.c | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: cd69ec8cde54af1817630331fc441f493866f0d4
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/66
-- 
gitgitgadget

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

[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.

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;
-- 
gitgitgadget

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

[Ramsay Jones]

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;
> 

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

[Ramsay Jones]

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

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

[Duy Nguyen]

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

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

[Duy Nguyen]

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
-- 
Duy

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

[Johannes Sixt]

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;
> 

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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
> 

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

[Johannes Schindelin]

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;
> > 
> 
> 
> 

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

[Ramsay Jones]

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

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

[Johannes Sixt]

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

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

[Jeff King]

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

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

[Johannes Sixt]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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>

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

[Junio C Hamano]

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?

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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>
> 

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

[Johannes Schindelin]

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

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

[Duy Nguyen]

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



-- 
Duy

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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
> 

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

[Johannes Schindelin]

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

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

[Eric Sunshine]

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.)

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

[Joseph Moisan]

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>

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

[Jeff King]

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

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

[Jeff King]

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

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

[Duy Nguyen]

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.
-- 
Duy

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

[Junio C Hamano]

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 v2 0/2] mingw: handle absolute paths in expand_user_path()

[Johannes Schindelin via GitGitGadget]

In Git for Windows, we ran with a patch "in production" for quite a while
where paths starting with a slash (i.e. looking like Unix paths, not like
Windows paths) were interpreted as being relative to the runtime prefix,
when expanded via expand_user_path().

This was sent to the Git mailing list as a discussion starter, and it was
pointed out that this is neither portable nor unambiguous.

After the dust settled, I thought about the presented ideas for a while
(quite a while...), and ended up with the following: any path starting with
<RUNTIME-PREFIX>/ is expanded. This is ambiguous because it could be a valid
path. But then, it is unlikely, and if someone really wants to specify such
a path, it is easy to slap a ./ in front and they're done.

Changes since v1:

 * Included a test for the RUNTIME_PREFIX that I had meant to send for ages
   already, and based on which...
 * A test case was added to verify that this actually works as intended
 * It is no longer Windows-specific
 * I added some documentation

Johannes Schindelin (2):
  tests: exercise the RUNTIME_PREFIX feature
  expand_user_path(): support specifying paths relative to the runtime
    prefix

 Documentation/config.txt | 10 ++++++++++
 Makefile                 |  5 +++++
 path.c                   |  5 +++++
 t/t0060-path-utils.sh    | 26 ++++++++++++++++++++++++++
 4 files changed, 46 insertions(+)


base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/66

Range-diff vs v1:

 -:  ----------- > 1:  cc8f09baba9 tests: exercise the RUNTIME_PREFIX feature
 1:  2287dd96cf0 ! 2:  66df56f5db0 mingw: handle absolute paths in expand_user_path()
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    mingw: handle absolute paths in expand_user_path()
     +    expand_user_path(): support specifying paths relative to the runtime prefix
      
     -    On Windows, an absolute POSIX path needs to be turned into a Windows
     -    one.
     +    Ever since Git learned to detect its install location at runtime, there
     +    was the slightly awkward problem that it was impossible to specify paths
     +    relative to said location.
     +
     +    For example, if a version of Git was shipped with custom SSL
     +    certificates to use, there was no portable way to specify
     +    `http.sslCAInfo`.
     +
     +    In Git for Windows, the problem was "solved" for years by interpreting
     +    paths starting with a slash as relative to the runtime prefix.
     +
     +    However, this is not correct: such paths _are_ legal on Windows, and
     +    they are interpreted as absolute paths in the same drive as the current
     +    directory.
     +
     +    After a lengthy discussion, and a way lengthier time to mull over the
     +    problem and its best solution, we decided to introduce support for the
     +    magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the
     +    remainder is interpreted as relative to the detected runtime prefix.
     +
     +    This solves the problem, but what new problems does it stir up? Here are
     +    the two most obvious ones:
     +
     +    - What if Git was not compiled with support for a runtime prefix?
     +
     +      In that case, we will simply use the compiled-in hard-coded prefix.
     +
     +    - What if a user _wants_ to specify a path starting with the magic
     +      sequence?
     +
     +      In that case, the user will simply need to prefix the magic sequence
     +      with `./` and voilà, the path won't be expanded.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     + ## Documentation/config.txt ##
     +@@ Documentation/config.txt: pathname::
     + 	tilde expansion happens to such a string: `~/`
     + 	is expanded to the value of `$HOME`, and `~user/` to the
     + 	specified user's home directory.
     +++
     ++If a path starts with `<RUNTIME-PREFIX>/`, the remainder is
     ++interpreted as a path relative to Git's "runtime prefix", i.e. relative
     ++to the location where Git itself was installed. For example,
     ++`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git
     ++executable itself lives. If Git was compiled without runtime prefix
     ++support, the compiled-in prefix will be subsituted instead. In the
     ++unlikely event that a literal path needs to be specified that should
     ++_not_ be expanded, it needs to be prefixed by `./`, like so:
     ++`./<RUNTIME-PREFIX>/bin`.
     + 
     + 
     + Variables
     +
       ## path.c ##
      @@
     - #include "path.h"
       #include "packfile.h"
       #include "object-store.h"
     + #include "lockfile.h"
      +#include "exec-cmd.h"
       
       static int get_st_mode_bits(const char *path, int *mode)
     @@ path.c: 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 (skip_prefix(path, "<RUNTIME-PREFIX>/", &path))
     ++		return system_path(path);
     ++
       	if (path[0] == '~') {
       		const char *first_slash = strchrnul(path, '/');
       		const char *username = path + 1;
     +
     + ## t/t0060-path-utils.sh ##
     +@@ t/t0060-path-utils.sh: test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '
     + 	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
     + 	GIT_EXEC_PATH= ./pretend/bin/git here >actual &&
     + 	echo HERE >expect &&
     ++	test_cmp expect actual'
     ++
     ++test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' '
     ++	mkdir -p pretend/bin &&
     ++	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
     ++	git config yes.path "<RUNTIME-PREFIX>/yes" &&
     ++	GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual &&
     ++	echo "$(pwd)/pretend/yes" >expect &&
     + 	test_cmp expect actual
     + '
     + 

-- 
gitgitgadget

[PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, we refrained from adding a regression test in 7b6c6496374
(system_path(): Add prefix computation at runtime if RUNTIME_PREFIX set,
2008-08-10), and in 226c0ddd0d6 (exec_cmd: RUNTIME_PREFIX on some POSIX
systems, 2018-04-10).

The reason was that it was deemed too tricky to test.

Turns out that it is not tricky to test at all: we simply create a
pseudo-root, copy the `git` executable into the `git/` subdirectory of
that pseudo-root, then copy a script into the `libexec/git-core/`
directory and expect that to be picked up.

As long as the trash directory is in a location where binaries can be
executed, this works.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile              |  5 +++++
 t/t0060-path-utils.sh | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Makefile b/Makefile
index c3565fc0f8f..4c3e4ccabcd 100644
--- a/Makefile
+++ b/Makefile
@@ -2826,6 +2826,11 @@ ifdef GIT_TEST_INDEX_VERSION
 endif
 ifdef GIT_TEST_PERL_FATAL_WARNINGS
 	@echo GIT_TEST_PERL_FATAL_WARNINGS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_PERL_FATAL_WARNINGS)))'\' >>$@+
+endif
+ifdef RUNTIME_PREFIX
+	@echo RUNTIME_PREFIX=\'true\' >>$@+
+else
+	@echo RUNTIME_PREFIX=\'false\' >>$@+
 endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
 
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index de4960783f0..a76728c27bf 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -525,4 +525,22 @@ test_expect_success MINGW 'is_valid_path() on Windows' '
 		"PRN./abc"
 '
 
+test_lazy_prereq RUNTIME_PREFIX '
+	test true = "$RUNTIME_PREFIX"
+'
+
+test_lazy_prereq CAN_EXEC_IN_PWD '
+	cp "$GIT_EXEC_PATH"/git$X ./ &&
+	./git rev-parse
+'
+
+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '
+	mkdir -p pretend/bin pretend/libexec/git-core &&
+	echo "echo HERE" | write_script pretend/libexec/git-core/git-here &&
+	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
+	GIT_EXEC_PATH= ./pretend/bin/git here >actual &&
+	echo HERE >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 2/2] expand_user_path(): support specifying paths relative to the runtime prefix

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Ever since Git learned to detect its install location at runtime, there
was the slightly awkward problem that it was impossible to specify paths
relative to said location.

For example, if a version of Git was shipped with custom SSL
certificates to use, there was no portable way to specify
`http.sslCAInfo`.

In Git for Windows, the problem was "solved" for years by interpreting
paths starting with a slash as relative to the runtime prefix.

However, this is not correct: such paths _are_ legal on Windows, and
they are interpreted as absolute paths in the same drive as the current
directory.

After a lengthy discussion, and a way lengthier time to mull over the
problem and its best solution, we decided to introduce support for the
magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the
remainder is interpreted as relative to the detected runtime prefix.

This solves the problem, but what new problems does it stir up? Here are
the two most obvious ones:

- What if Git was not compiled with support for a runtime prefix?

  In that case, we will simply use the compiled-in hard-coded prefix.

- What if a user _wants_ to specify a path starting with the magic
  sequence?

  In that case, the user will simply need to prefix the magic sequence
  with `./` and voilà, the path won't be expanded.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt | 10 ++++++++++
 path.c                   |  5 +++++
 t/t0060-path-utils.sh    |  8 ++++++++
 3 files changed, 23 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf82766a6a2..fd56e2c1220 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -298,6 +298,16 @@ pathname::
 	tilde expansion happens to such a string: `~/`
 	is expanded to the value of `$HOME`, and `~user/` to the
 	specified user's home directory.
++
+If a path starts with `<RUNTIME-PREFIX>/`, the remainder is
+interpreted as a path relative to Git's "runtime prefix", i.e. relative
+to the location where Git itself was installed. For example,
+`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git
+executable itself lives. If Git was compiled without runtime prefix
+support, the compiled-in prefix will be subsituted instead. In the
+unlikely event that a literal path needs to be specified that should
+_not_ be expanded, it needs to be prefixed by `./`, like so:
+`./<RUNTIME-PREFIX>/bin`.
 
 
 Variables
diff --git a/path.c b/path.c
index 7bccd830e95..d8542a7b27b 100644
--- a/path.c
+++ b/path.c
@@ -12,6 +12,7 @@
 #include "packfile.h"
 #include "object-store.h"
 #include "lockfile.h"
+#include "exec-cmd.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -732,6 +733,10 @@ char *expand_user_path(const char *path, int real_home)
 
 	if (path == NULL)
 		goto return_null;
+
+	if (skip_prefix(path, "<RUNTIME-PREFIX>/", &path))
+		return system_path(path);
+
 	if (path[0] == '~') {
 		const char *first_slash = strchrnul(path, '/');
 		const char *username = path + 1;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index a76728c27bf..cb7fbfb9af2 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -540,6 +540,14 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '
 	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
 	GIT_EXEC_PATH= ./pretend/bin/git here >actual &&
 	echo HERE >expect &&
+	test_cmp expect actual'
+
+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' '
+	mkdir -p pretend/bin &&
+	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
+	git config yes.path "<RUNTIME-PREFIX>/yes" &&
+	GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual &&
+	echo "$(pwd)/pretend/yes" >expect &&
 	test_cmp expect actual
 '
 
-- 
gitgitgadget

Re: [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path()

[Junio C Hamano]

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

> In Git for Windows, we ran with a patch "in production" for quite a while
> where paths starting with a slash (i.e. looking like Unix paths, not like
> Windows paths) were interpreted as being relative to the runtime prefix,
> when expanded via expand_user_path().
>
> This was sent to the Git mailing list as a discussion starter, and it was
> pointed out that this is neither portable nor unambiguous.
>
> After the dust settled, I thought about the presented ideas for a while
> (quite a while...), and ended up with the following: any path starting with
> <RUNTIME-PREFIX>/ is expanded. This is ambiguous because it could be a valid
> path. But then, it is unlikely, and if someone really wants to specify such
> a path, it is easy to slap a ./ in front and they're done.

I just went back to briefly scan the discussion in late 2018.

I think the rough consensus back then was that 

 * It indeed is a problem that there is no syntax for users of
   "relocatable Git" to use to point at things that come as part of
   the "relocatable Git" package.

 * A change to expand_user_path() would be too broad, it makes sense
   for this feature to be in git_config_pathname();

 * We need to get the syntax right.

As to the last item, there were a handful of choices raised:

 - Use "~~"--the downside is that this is not extensible.  Use
   "~runtime-prefix/" would be a better choice (the likelyhood of
   <RUNTIME-PREFIX>, $RUNTIME_PREFIX, and any other random choice
   happens to be used as a valid directory name is just as slim as
   the likelyhood of "runtime-prefix" used as a valid username).

 - "$RUNTIME_PREFIX" to make it read like a variable---the downside
   was that it looked TOO MUCH like a variable and risked user
   confusion (e.g. it may be upsetting that "$HOME/.gitconfig" is
   not expanded like "~/.gitconfig" is).

 - %(RUNTIME-PREFIX) to make it similar to how Git replaces various
   tokens that are understood only in the context of Git.

 - <RUNTIME-PREFIX>---the downside is that this is an unnecessary
   new syntax we do not have to introduce.  If <RUNTIME-PREFIX> is
   unlikely to be used as a valid directory name, %(RUNTIME-PREFIX)
   is just as unlikely.

There might have been other ideas floated back then.  I have to say
that the one you chose in this round is the least favourite of mine,
and if you consulted me privately before redoing this round, I would
probably have said %(RUNTIME_PREFIX) would make the most sense among
the candidates.

Thanks.

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

[Johannes Schindelin via GitGitGadget]

In Git for Windows, we ran with a patch "in production" for quite a while
where paths starting with a slash (i.e. looking like Unix paths, not like
Windows paths) were interpreted as being relative to the runtime prefix,
when expanded via expand_user_path().

This was sent to the Git mailing list as a discussion starter, and it was
pointed out that this is neither portable nor unambiguous.

After the dust settled, I thought about the presented ideas for a while
(quite a while...), ended up with a solution, then adapted it to Junio's
preference: any path starting with %(prefix)/ is expanded. This is ambiguous
because it could be a valid path. But then, it is unlikely, and if someone
really wants to specify such a path, it is easy to slap a ./ in front and
they're done.

Changes since v2:

 * Adjusted to Junio's preference %(...) over <...>. Of course, the
   preferred preference has the disadvantage of actually being allowed in
   Win32 filenames, but then, the workaround is as easy as on non-Windows
   platforms.
 * Since our convention of %(...) interpolation does not involve uppercase
   keywords, I now use a lowercase one.
 * Since this keyword is interpolated to the compiled-in prefix if built
   without runtime prefix support, I dropped the runtime part of the
   keyword.
 * 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).
 * 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.

Changes since v1:

 * Included a test for the RUNTIME_PREFIX that I had meant to send for ages
   already, and based on which...
 * A test case was added to verify that this actually works as intended
 * It is no longer Windows-specific
 * I added some documentation

Johannes Schindelin (5):
  tests: exercise the RUNTIME_PREFIX feature
  expand_user_path(): remove stale part of the comment
  expand_user_path(): clarify the role of the `real_home` parameter
  Use a better name for the function interpolating paths
  interpolate_path(): allow specifying paths relative to the runtime
    prefix

 Documentation/config.txt   |  9 +++++++++
 Makefile                   |  5 +++++
 builtin/credential-cache.c |  2 +-
 builtin/credential-store.c |  2 +-
 builtin/gc.c               |  2 +-
 cache.h                    |  2 +-
 config.c                   |  8 ++++----
 path.c                     | 19 +++++++++++++------
 sequencer.c                |  2 +-
 t/t0060-path-utils.sh      | 26 ++++++++++++++++++++++++++
 10 files changed, 62 insertions(+), 15 deletions(-)


base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/66

Range-diff vs v2:

 1:  cc8f09baba9 = 1:  cc8f09baba9 tests: exercise the RUNTIME_PREFIX feature
 -:  ----------- > 2:  a4ff3a461bc expand_user_path(): remove stale part of the comment
 -:  ----------- > 3:  7b79ba66dd0 expand_user_path(): clarify the role of the `real_home` parameter
 -:  ----------- > 4:  19fd9c3c803 Use a better name for the function interpolating paths
 2:  66df56f5db0 ! 5:  d286583082e expand_user_path(): support specifying paths relative to the runtime prefix
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    expand_user_path(): support specifying paths relative to the runtime prefix
     +    interpolate_path(): allow specifying paths relative to the runtime prefix
      
          Ever since Git learned to detect its install location at runtime, there
          was the slightly awkward problem that it was impossible to specify paths
     @@ Commit message
          they are interpreted as absolute paths in the same drive as the current
          directory.
      
     -    After a lengthy discussion, and a way lengthier time to mull over the
     -    problem and its best solution, we decided to introduce support for the
     -    magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the
     -    remainder is interpreted as relative to the detected runtime prefix.
     +    After a lengthy discussion, and an even lengthier time to mull over the
     +    problem and its best solution, and then more discussions, we eventually
     +    decided to introduce support for the magic sequence `%(prefix)/`. If a
     +    path starts with this, the remainder is interpreted as relative to the
     +    detected (runtime) prefix. If built without runtime prefix support, Git
     +    will simply interpolate the compiled-in prefix.
      
     -    This solves the problem, but what new problems does it stir up? Here are
     -    the two most obvious ones:
     -
     -    - What if Git was not compiled with support for a runtime prefix?
     -
     -      In that case, we will simply use the compiled-in hard-coded prefix.
     -
     -    - What if a user _wants_ to specify a path starting with the magic
     -      sequence?
     -
     -      In that case, the user will simply need to prefix the magic sequence
     -      with `./` and voilà, the path won't be expanded.
     +    If a user _wants_ to specify a path starting with the magic sequence,
     +    they can prefix the magic sequence with `./` and voilà, the path won't
     +    be expanded.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ Documentation/config.txt: pathname::
       	is expanded to the value of `$HOME`, and `~user/` to the
       	specified user's home directory.
      ++
     -+If a path starts with `<RUNTIME-PREFIX>/`, the remainder is
     -+interpreted as a path relative to Git's "runtime prefix", i.e. relative
     -+to the location where Git itself was installed. For example,
     -+`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git
     -+executable itself lives. If Git was compiled without runtime prefix
     -+support, the compiled-in prefix will be subsituted instead. In the
     -+unlikely event that a literal path needs to be specified that should
     -+_not_ be expanded, it needs to be prefixed by `./`, like so:
     -+`./<RUNTIME-PREFIX>/bin`.
     ++If a path starts with `%(prefix)/`, the remainder is interpreted as a
     ++path relative to Git's "runtime prefix", i.e. relative to the location
     ++where Git itself was installed. For example, `%(prefix)/bin/` refers to
     ++the directory in which the Git executable itself lives. If Git was
     ++compiled without runtime prefix support, the compiled-in prefix will be
     ++subsituted instead. In the unlikely event that a literal path needs to
     ++be specified that should _not_ be expanded, it needs to be prefixed by
     ++`./`, like so: `./%(prefix)/bin`.
       
       
       Variables
     @@ path.c
       
       static int get_st_mode_bits(const char *path, int *mode)
       {
     -@@ path.c: char *expand_user_path(const char *path, int real_home)
     +@@ path.c: static struct passwd *getpw_str(const char *username, size_t len)
     +  * failure or if path is NULL.
     +  *
     +  * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion.
     ++ *
     ++ * If the path starts with `%(prefix)/`, the remainder is interpreted as
     ++ * relative to where Git is installed, and expanded to the absolute path.
     +  */
     + char *interpolate_path(const char *path, int real_home)
     + {
     +@@ path.c: char *interpolate_path(const char *path, int real_home)
       
       	if (path == NULL)
       		goto return_null;
      +
     -+	if (skip_prefix(path, "<RUNTIME-PREFIX>/", &path))
     ++	if (skip_prefix(path, "%(prefix)/", &path))
      +		return system_path(path);
      +
       	if (path[0] == '~') {
     @@ t/t0060-path-utils.sh: test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTI
       	echo HERE >expect &&
      +	test_cmp expect actual'
      +
     -+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' '
     ++test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '
      +	mkdir -p pretend/bin &&
      +	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
     -+	git config yes.path "<RUNTIME-PREFIX>/yes" &&
     ++	git config yes.path "%(prefix)/yes" &&
      +	GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual &&
      +	echo "$(pwd)/pretend/yes" >expect &&
       	test_cmp expect actual

-- 
gitgitgadget

[PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, we refrained from adding a regression test in 7b6c6496374
(system_path(): Add prefix computation at runtime if RUNTIME_PREFIX set,
2008-08-10), and in 226c0ddd0d6 (exec_cmd: RUNTIME_PREFIX on some POSIX
systems, 2018-04-10).

The reason was that it was deemed too tricky to test.

Turns out that it is not tricky to test at all: we simply create a
pseudo-root, copy the `git` executable into the `git/` subdirectory of
that pseudo-root, then copy a script into the `libexec/git-core/`
directory and expect that to be picked up.

As long as the trash directory is in a location where binaries can be
executed, this works.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile              |  5 +++++
 t/t0060-path-utils.sh | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Makefile b/Makefile
index c3565fc0f8f..4c3e4ccabcd 100644
--- a/Makefile
+++ b/Makefile
@@ -2826,6 +2826,11 @@ ifdef GIT_TEST_INDEX_VERSION
 endif
 ifdef GIT_TEST_PERL_FATAL_WARNINGS
 	@echo GIT_TEST_PERL_FATAL_WARNINGS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_PERL_FATAL_WARNINGS)))'\' >>$@+
+endif
+ifdef RUNTIME_PREFIX
+	@echo RUNTIME_PREFIX=\'true\' >>$@+
+else
+	@echo RUNTIME_PREFIX=\'false\' >>$@+
 endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
 
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index de4960783f0..a76728c27bf 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -525,4 +525,22 @@ test_expect_success MINGW 'is_valid_path() on Windows' '
 		"PRN./abc"
 '
 
+test_lazy_prereq RUNTIME_PREFIX '
+	test true = "$RUNTIME_PREFIX"
+'
+
+test_lazy_prereq CAN_EXEC_IN_PWD '
+	cp "$GIT_EXEC_PATH"/git$X ./ &&
+	./git rev-parse
+'
+
+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '
+	mkdir -p pretend/bin pretend/libexec/git-core &&
+	echo "echo HERE" | write_script pretend/libexec/git-core/git-here &&
+	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
+	GIT_EXEC_PATH= ./pretend/bin/git here >actual &&
+	echo HERE >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 2/5] expand_user_path(): remove stale part of the comment

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 395de250d9d (Expand ~ and ~user in core.excludesfile,
commit.template, 2009-11-17), the `user_path()` function was refactored
into the `expand_user_path()`. During that refactoring, the `buf`
parameter was lost, but the code comment above said function still talks
about it. Let's remove that stale part of the comment.

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

diff --git a/path.c b/path.c
index 7bccd830e95..3318ad24336 100644
--- a/path.c
+++ b/path.c
@@ -719,9 +719,8 @@ static struct passwd *getpw_str(const char *username, size_t len)
 }
 
 /*
- * Return a string with ~ and ~user expanded via getpw*.  If buf != NULL,
- * then it is a newly allocated string. Returns NULL on getpw failure or
- * if path is NULL.
+ * Return a string with ~ and ~user expanded via getpw*. Returns NULL on getpw
+ * failure or if path is NULL.
  *
  * If real_home is true, strbuf_realpath($HOME) is used in the expansion.
  */
-- 
gitgitgadget

[PATCH v3 3/5] expand_user_path(): clarify the role of the `real_home` parameter

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `real_home` parameter only has an effect when expanding paths
starting with `~/`, not when expanding paths starting with `~<user>/`.
Let's make that clear.

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

diff --git a/path.c b/path.c
index 3318ad24336..bf329e535cf 100644
--- a/path.c
+++ b/path.c
@@ -722,7 +722,7 @@ static struct passwd *getpw_str(const char *username, size_t len)
  * Return a string with ~ and ~user expanded via getpw*. Returns NULL on getpw
  * failure or if path is NULL.
  *
- * If real_home is true, strbuf_realpath($HOME) is used in the expansion.
+ * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion.
  */
 char *expand_user_path(const char *path, int real_home)
 {
-- 
gitgitgadget

[PATCH v3 4/5] Use a better name for the function interpolating paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It is not immediately clear what `expand_user_path()` means, so let's
rename it to `interpolate_path()`. This also opens the path for
interpolating more than just a home directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/credential-cache.c | 2 +-
 builtin/credential-store.c | 2 +-
 builtin/gc.c               | 2 +-
 cache.h                    | 2 +-
 config.c                   | 8 ++++----
 path.c                     | 4 ++--
 sequencer.c                | 2 +-
 7 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 76a6ba37223..e8a74157471 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -90,7 +90,7 @@ static char *get_socket_path(void)
 {
 	struct stat sb;
 	char *old_dir, *socket;
-	old_dir = expand_user_path("~/.git-credential-cache", 0);
+	old_dir = interpolate_path("~/.git-credential-cache", 0);
 	if (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))
 		socket = xstrfmt("%s/socket", old_dir);
 	else
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index ae3c1ba75fe..62a4f3c2653 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -173,7 +173,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix)
 	if (file) {
 		string_list_append(&fns, file);
 	} else {
-		if ((file = expand_user_path("~/.git-credentials", 0)))
+		if ((file = interpolate_path("~/.git-credentials", 0)))
 			string_list_append_nodup(&fns, file);
 		file = xdg_config_home("credentials");
 		if (file)
diff --git a/builtin/gc.c b/builtin/gc.c
index f05d2f0a1ac..6ce5ca45126 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1542,7 +1542,7 @@ static char *launchctl_service_filename(const char *name)
 	struct strbuf filename = STRBUF_INIT;
 	strbuf_addf(&filename, "~/Library/LaunchAgents/%s.plist", name);
 
-	expanded = expand_user_path(filename.buf, 1);
+	expanded = interpolate_path(filename.buf, 1);
 	if (!expanded)
 		die(_("failed to expand path '%s'"), filename.buf);
 
diff --git a/cache.h b/cache.h
index ba04ff8bd36..87e4cbe9c5f 100644
--- a/cache.h
+++ b/cache.h
@@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb);
 int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
 
 int mkdir_in_gitdir(const char *path);
-char *expand_user_path(const char *path, int real_home);
+char *interpolate_path(const char *path, int real_home);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
diff --git a/config.c b/config.c
index f9c400ad306..c17b9797292 100644
--- a/config.c
+++ b/config.c
@@ -137,7 +137,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	if (!path)
 		return config_error_nonbool("include.path");
 
-	expanded = expand_user_path(path, 0);
+	expanded = interpolate_path(path, 0);
 	if (!expanded)
 		return error(_("could not expand include path '%s'"), path);
 	path = expanded;
@@ -185,7 +185,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
 	char *expanded;
 	int prefix = 0;
 
-	expanded = expand_user_path(pat->buf, 1);
+	expanded = interpolate_path(pat->buf, 1);
 	if (expanded) {
 		strbuf_reset(pat);
 		strbuf_addstr(pat, expanded);
@@ -1270,7 +1270,7 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 {
 	if (!value)
 		return config_error_nonbool(var);
-	*dest = expand_user_path(value, 0);
+	*dest = interpolate_path(value, 0);
 	if (!*dest)
 		die(_("failed to expand user dir in: '%s'"), value);
 	return 0;
@@ -1844,7 +1844,7 @@ void git_global_config(char **user_out, char **xdg_out)
 	char *xdg_config = NULL;
 
 	if (!user_config) {
-		user_config = expand_user_path("~/.gitconfig", 0);
+		user_config = interpolate_path("~/.gitconfig", 0);
 		xdg_config = xdg_config_home("config");
 	}
 
diff --git a/path.c b/path.c
index bf329e535cf..8dc5ad2cb55 100644
--- a/path.c
+++ b/path.c
@@ -724,7 +724,7 @@ static struct passwd *getpw_str(const char *username, size_t len)
  *
  * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion.
  */
-char *expand_user_path(const char *path, int real_home)
+char *interpolate_path(const char *path, int real_home)
 {
 	struct strbuf user_path = STRBUF_INIT;
 	const char *to_copy = path;
@@ -811,7 +811,7 @@ const char *enter_repo(const char *path, int strict)
 		strbuf_add(&validated_path, path, len);
 
 		if (used_path.buf[0] == '~') {
-			char *newpath = expand_user_path(used_path.buf, 0);
+			char *newpath = interpolate_path(used_path.buf, 0);
 			if (!newpath)
 				return NULL;
 			strbuf_attach(&used_path, newpath, strlen(newpath),
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..007a851804d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1241,7 +1241,7 @@ N_("Your name and email address were configured automatically based\n"
 
 static const char *implicit_ident_advice(void)
 {
-	char *user_config = expand_user_path("~/.gitconfig", 0);
+	char *user_config = interpolate_path("~/.gitconfig", 0);
 	char *xdg_config = xdg_config_home("config");
 	int config_exists = file_exists(user_config) || file_exists(xdg_config);
 
-- 
gitgitgadget

[PATCH v3 5/5] interpolate_path(): allow specifying paths relative to the runtime prefix

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Ever since Git learned to detect its install location at runtime, there
was the slightly awkward problem that it was impossible to specify paths
relative to said location.

For example, if a version of Git was shipped with custom SSL
certificates to use, there was no portable way to specify
`http.sslCAInfo`.

In Git for Windows, the problem was "solved" for years by interpreting
paths starting with a slash as relative to the runtime prefix.

However, this is not correct: such paths _are_ legal on Windows, and
they are interpreted as absolute paths in the same drive as the current
directory.

After a lengthy discussion, and an even lengthier time to mull over the
problem and its best solution, and then more discussions, we eventually
decided to introduce support for the magic sequence `%(prefix)/`. If a
path starts with this, the remainder is interpreted as relative to the
detected (runtime) prefix. If built without runtime prefix support, Git
will simply interpolate the compiled-in prefix.

If a user _wants_ to specify a path starting with the magic sequence,
they can prefix the magic sequence with `./` and voilà, the path won't
be expanded.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt | 9 +++++++++
 path.c                   | 8 ++++++++
 t/t0060-path-utils.sh    | 8 ++++++++
 3 files changed, 25 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf82766a6a2..0c0e6b859f1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -298,6 +298,15 @@ pathname::
 	tilde expansion happens to such a string: `~/`
 	is expanded to the value of `$HOME`, and `~user/` to the
 	specified user's home directory.
++
+If a path starts with `%(prefix)/`, the remainder is interpreted as a
+path relative to Git's "runtime prefix", i.e. relative to the location
+where Git itself was installed. For example, `%(prefix)/bin/` refers to
+the directory in which the Git executable itself lives. If Git was
+compiled without runtime prefix support, the compiled-in prefix will be
+subsituted instead. In the unlikely event that a literal path needs to
+be specified that should _not_ be expanded, it needs to be prefixed by
+`./`, like so: `./%(prefix)/bin`.
 
 
 Variables
diff --git a/path.c b/path.c
index 8dc5ad2cb55..0bc788ea40f 100644
--- a/path.c
+++ b/path.c
@@ -12,6 +12,7 @@
 #include "packfile.h"
 #include "object-store.h"
 #include "lockfile.h"
+#include "exec-cmd.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -723,6 +724,9 @@ static struct passwd *getpw_str(const char *username, size_t len)
  * failure or if path is NULL.
  *
  * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion.
+ *
+ * If the path starts with `%(prefix)/`, the remainder is interpreted as
+ * relative to where Git is installed, and expanded to the absolute path.
  */
 char *interpolate_path(const char *path, int real_home)
 {
@@ -731,6 +735,10 @@ char *interpolate_path(const char *path, int real_home)
 
 	if (path == NULL)
 		goto return_null;
+
+	if (skip_prefix(path, "%(prefix)/", &path))
+		return system_path(path);
+
 	if (path[0] == '~') {
 		const char *first_slash = strchrnul(path, '/');
 		const char *username = path + 1;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index a76728c27bf..34d1061f321 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -540,6 +540,14 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' '
 	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
 	GIT_EXEC_PATH= ./pretend/bin/git here >actual &&
 	echo HERE >expect &&
+	test_cmp expect actual'
+
+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' '
+	mkdir -p pretend/bin &&
+	cp "$GIT_EXEC_PATH"/git$X pretend/bin/ &&
+	git config yes.path "%(prefix)/yes" &&
+	GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual &&
+	echo "$(pwd)/pretend/yes" >expect &&
 	test_cmp expect actual
 '
 
-- 
gitgitgadget

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

[Junio C Hamano]

"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.

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is not immediately clear what `expand_user_path()` means, so let's
> rename it to `interpolate_path()`. This also opens the path for
> interpolating more than just a home directory.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> ...
> diff --git a/cache.h b/cache.h
> index ba04ff8bd36..87e4cbe9c5f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb);
>  int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
>  
>  int mkdir_in_gitdir(const char *path);
> -char *expand_user_path(const char *path, int real_home);
> +char *interpolate_path(const char *path, int real_home);

This of course breaks any topic in flight that adds more places to
use expand_user_path().

I think Fabian's "ssh signing" is not as ready as this topic, and it
can afford to wait by rebasing on top of this topic.  By the time
"ssh signing" gets into testable shape (right now, it does not pass
tests when merged to 'seen'), hopefully the "expand install-prefix"
topic may already be in 'next' if not in 'master'.

In the meantime, I am adding this band-aid at the tip of this topic
to help these two topics play together better.

Thanks.


diff --git a/cache.h b/cache.h
index 87e4cbe9c5..679a27e17c 100644
--- a/cache.h
+++ b/cache.h
@@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
 
 int mkdir_in_gitdir(const char *path);
 char *interpolate_path(const char *path, int real_home);
+/* NEEDSWORK: remove this synonym once in-flight topics have migrated */
+#define expand_user_path interpolate_path
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Fabian Stelzer]

On 27.07.21 00:05, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> It is not immediately clear what `expand_user_path()` means, so let's
>> rename it to `interpolate_path()`. This also opens the path for
>> interpolating more than just a home directory.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> ...
>> diff --git a/cache.h b/cache.h
>> index ba04ff8bd36..87e4cbe9c5f 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb);
>>   int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
>>   
>>   int mkdir_in_gitdir(const char *path);
>> -char *expand_user_path(const char *path, int real_home);
>> +char *interpolate_path(const char *path, int real_home);
> This of course breaks any topic in flight that adds more places to
> use expand_user_path().
>
> I think Fabian's "ssh signing" is not as ready as this topic, and it
> can afford to wait by rebasing on top of this topic.  By the time
> "ssh signing" gets into testable shape (right now, it does not pass
> tests when merged to 'seen'), hopefully the "expand install-prefix"
> topic may already be in 'next' if not in 'master'.
I think the test problem is not due to my patch.
Like Ævar wrote it's "hn/reftable probably interacting with my 
ab/refs-files-cleanup" [1]
The failed tests i can see in [2] are either t0031-reftable.sh or a 
compile failure referencing reftable as well.
If i merge the ssh code into the current seen branch everything works 
fine for me. Let me know if you have any other results / CI runs that 
might help.

[1] https://lore.kernel.org/git/87a6mevkrx.fsf@evledraar.gmail.com/
[2] https://github.com/git/git/actions/runs/1053603028
>
> In the meantime, I am adding this band-aid at the tip of this topic
> to help these two topics play together better.
>
> Thanks.
>
>
> diff --git a/cache.h b/cache.h
> index 87e4cbe9c5..679a27e17c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
>   
>   int mkdir_in_gitdir(const char *path);
>   char *interpolate_path(const char *path, int real_home);
> +/* NEEDSWORK: remove this synonym once in-flight topics have migrated */
> +#define expand_user_path interpolate_path
>   const char *enter_repo(const char *path, int strict);
>   static inline int is_absolute_path(const char *path)
>   {

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

>> I think Fabian's "ssh signing" is not as ready as this topic, and it
>> can afford to wait by rebasing on top of this topic.  By the time
>> "ssh signing" gets into testable shape (right now, it does not pass
>> tests when merged to 'seen'), hopefully the "expand install-prefix"
>> topic may already be in 'next' if not in 'master'.
> I think the test problem is not due to my patch.

I've been seeing these test failers locally, every time
fs/ssh-signing topic is merged to 'seen' (without the reftable
thing).

Test Summary Report
-------------------
t5534-push-signed.sh                             (Wstat: 256 Tests: 13 Failed: 2)
  Failed tests:  8, 12
  Non-zero exit status: 1
t7528-signed-commit-ssh.sh                       (Wstat: 256 Tests: 23 Failed: 2)
  Failed tests:  13, 17
  Non-zero exit status: 1

When reftable thing is merged, either compilation fails or t0031
fails, and I suspect that these are not due to the ssh signing
topic.

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Fabian Stelzer <fs@gigacodes.de> writes:
>
>>> I think Fabian's "ssh signing" is not as ready as this topic, and it
>>> can afford to wait by rebasing on top of this topic.  By the time
>>> "ssh signing" gets into testable shape (right now, it does not pass
>>> tests when merged to 'seen'), hopefully the "expand install-prefix"
>>> topic may already be in 'next' if not in 'master'.
>> I think the test problem is not due to my patch.
>
> I've been seeing these test failers locally, every time
> fs/ssh-signing topic is merged to 'seen' (without the reftable
> thing).
>
> Test Summary Report
> -------------------
> t5534-push-signed.sh                             (Wstat: 256 Tests: 13 Failed: 2)
>   Failed tests:  8, 12
>   Non-zero exit status: 1
> t7528-signed-commit-ssh.sh                       (Wstat: 256 Tests: 23 Failed: 2)
>   Failed tests:  13, 17
>   Non-zero exit status: 1
>
> When reftable thing is merged, either compilation fails or t0031
> fails, and I suspect that these are not due to the ssh signing
> topic.

Interesting.  It seems that the failure has some correlation with
the use of --root=<trash directory> option.

    $ sh t5534-push-signed.sh -i
    ok 1 - setup
    ok 2 - unsigned push does not send push certificate
    ok 3 - talking with a receiver without push certificate support
    ok 4 - push --signed fails with a receiver without push certificate
    support
    ok 5 - push --signed=1 is accepted
    ok 6 - no certificate for a signed push with no update
    ok 7 - signed push sends push certificate
    ok 8 - ssh signed push sends push certificate
    ok 9 - inconsistent push options in signed push not allowed
    ok 10 - fail without key and heed user.signingkey
    ok 11 - fail without key and heed user.signingkey x509
    ok 12 - fail without key and heed user.signingkey ssh
    ok 13 - failed atomic push does not execute GPG
    # passed all 13 test(s)
    1..13

passes just fine, but

    $ TESTPEN=/dev/shm/testpen.$$
    $ rm -fr "$TESTPEN" && mkdir "$TESTPEN"
    $ sh t5534-push-signed.sh --root=$TESTPEN -i -v

dies like this:

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/.git/
expecting success of 5534.1 'setup': 
	# main, ff and noff branches pointing at the same commit
	test_tick &&
	git commit --allow-empty -m initial &&

	git checkout -b noop &&
	git checkout -b ff &&
	git checkout -b noff &&

	# noop stays the same, ff advances, noff rewrites
	test_tick &&
	git commit --allow-empty --amend -m rewritten &&
	git checkout ff &&

	test_tick &&
	git commit --allow-empty -m second

[main (root-commit) 66fe8b3] initial
 Author: A U Thor <author@example.com>
Switched to a new branch 'noop'
Switched to a new branch 'ff'
Switched to a new branch 'noff'
[noff 6391b7f] rewritten
 Author: A U Thor <author@example.com>
 Date: Thu Apr 7 15:13:13 2005 -0700
Switched to branch 'ff'
[ff 566fbd3] second
 Author: A U Thor <author@example.com>
ok 1 - setup

expecting success of 5534.2 'unsigned push does not send push certificate': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	write_script dst/.git/hooks/post-receive <<-\EOF &&
	# discard the update list
	cat >/dev/null
	# record the push certificate
	if test -n "${GIT_PUSH_CERT-}"
	then
		git cat-file blob $GIT_PUSH_CERT >../push-cert
	fi
	EOF

	git push dst noop ff +noff &&
	! test -f dst/push-cert

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
To dst
   66fe8b3..566fbd3  ff -> ff
 + 66fe8b3...6391b7f noff -> noff (forced update)
ok 2 - unsigned push does not send push certificate

expecting success of 5534.3 'talking with a receiver without push certificate support': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	write_script dst/.git/hooks/post-receive <<-\EOF &&
	# discard the update list
	cat >/dev/null
	# record the push certificate
	if test -n "${GIT_PUSH_CERT-}"
	then
		git cat-file blob $GIT_PUSH_CERT >../push-cert
	fi
	EOF

	git push dst noop ff +noff &&
	! test -f dst/push-cert

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
To dst
   66fe8b3..566fbd3  ff -> ff
 + 66fe8b3...6391b7f noff -> noff (forced update)
ok 3 - talking with a receiver without push certificate support

expecting success of 5534.4 'push --signed fails with a receiver without push certificate support': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	test_must_fail git push --signed dst noop ff +noff 2>err &&
	test_i18ngrep "the receiving end does not support" err

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
fatal: the receiving end does not support --signed push
ok 4 - push --signed fails with a receiver without push certificate support

expecting success of 5534.5 'push --signed=1 is accepted': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	test_must_fail git push --signed=1 dst noop ff +noff 2>err &&
	test_i18ngrep "the receiving end does not support" err

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
fatal: the receiving end does not support --signed push
ok 5 - push --signed=1 is accepted

checking prerequisite: GPG

mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPG" &&
(
	cd "$TRASH_DIRECTORY/prereq-test-dir-GPG" &&
	gpg_version=$(gpg --version 2>&1)
	test $? != 127 || exit 1

	# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19
	# the gpg version 1.0.6 did not parse trust packets correctly, so for
	# that version, creation of signed tags using the generated key fails.
	case "$gpg_version" in
	"gpg (GnuPG) 1.0.6"*)
		say "Your version of gpg (1.0.6) is too buggy for testing"
		exit 1
		;;
	*)
		# Available key info:
		# * Type DSA and Elgamal, size 2048 bits, no expiration date,
		#   name and email: C O Mitter <committer@example.com>
		# * Type RSA, size 2048 bits, no expiration date,
		#   name and email: Eris Discordia <discord@example.net>
		# No password given, to enable non-interactive operation.
		# To generate new key:
		#	gpg --homedir /tmp/gpghome --gen-key
		# To write armored exported key to keyring:
		#	gpg --homedir /tmp/gpghome --export-secret-keys \
		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
		#	gpg --homedir /tmp/gpghome --export \
		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
		# To export ownertrust:
		#	gpg --homedir /tmp/gpghome --export-ownertrust \
		#		> lib-gpg/ownertrust
		mkdir "$GNUPGHOME" &&
		chmod 0700 "$GNUPGHOME" &&
		(gpgconf --kill gpg-agent || : ) &&
		gpg --homedir "${GNUPGHOME}" --import \
			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
		gpg --homedir "${GNUPGHOME}" --import-ownertrust \
			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null \
			--sign -u committer@example.com
		;;
	esac

)
gpg: keybox '/dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/pubring.kbx' created
gpg: /dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/trustdb.gpg: trustdb created
gpg: key 13B6F51ECDDE430D: public key "C O Mitter <committer@example.com>" imported
gpg: key 13B6F51ECDDE430D: secret key imported
gpg: key 61092E85B7227189: public key "Eris Discordia <discord@example.net>" imported
gpg: key 61092E85B7227189: secret key imported
gpg: key 13B6F51ECDDE430D: "C O Mitter <committer@example.com>" not changed
gpg: key 61092E85B7227189: "Eris Discordia <discord@example.net>" not changed
gpg: Total number processed: 4
gpg:               imported: 2
gpg:              unchanged: 2
gpg:       secret keys read: 2
gpg:   secret keys imported: 2
gpg: inserting ownertrust of 6
gpg: inserting ownertrust of 3
prerequisite GPG ok
expecting success of 5534.6 'no certificate for a signed push with no update': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	write_script dst/.git/hooks/post-receive <<-\EOF &&
	if test -n "${GIT_PUSH_CERT-}"
	then
		git cat-file blob $GIT_PUSH_CERT >../push-cert
	fi
	EOF
	git push dst noop &&
	! test -f dst/push-cert

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
Everything up-to-date
ok 6 - no certificate for a signed push with no update

expecting success of 5534.7 'signed push sends push certificate': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	git -C dst config receive.certnonceseed sekrit &&
	write_script dst/.git/hooks/post-receive <<-\EOF &&
	# discard the update list
	cat >/dev/null
	# record the push certificate
	if test -n "${GIT_PUSH_CERT-}"
	then
		git cat-file blob $GIT_PUSH_CERT >../push-cert
	fi &&

	cat >../push-cert-status <<E_O_F
	SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
	KEY=${GIT_PUSH_CERT_KEY-nokey}
	STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
	NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
	NONCE=${GIT_PUSH_CERT_NONCE-nononce}
	E_O_F

	EOF

	git push --signed dst noop ff +noff &&

	(
		cat <<-\EOF &&
		SIGNER=C O Mitter <committer@example.com>
		KEY=13B6F51ECDDE430D
		STATUS=G
		NONCE_STATUS=OK
		EOF
		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
	) >expect &&

	noop=$(git rev-parse noop) &&
	ff=$(git rev-parse ff) &&
	noff=$(git rev-parse noff) &&
	grep "$noop $ff refs/heads/ff" dst/push-cert &&
	grep "$noop $noff refs/heads/noff" dst/push-cert &&
	test_cmp expect dst/push-cert-status

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
To dst
   66fe8b3..566fbd3  ff -> ff
 + 66fe8b3...6391b7f noff -> noff (forced update)
66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff
66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff
ok 7 - signed push sends push certificate

checking prerequisite: GPGSSH

mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" &&
(
	cd "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" &&
	ssh_version=$(ssh-keygen -Y find-principals -n "git" 2>&1)
	test $? != 127 || exit 1
	echo $ssh_version | grep -q "find-principals:missing signature file"
	test $? = 0 || exit 1;
	mkdir -p "${GNUPGHOME}" &&
	chmod 0700 "${GNUPGHOME}" &&
	ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/ed25519_ssh_signing_key" >/dev/null &&
	ssh-keygen -t rsa -b 2048 -N "" -f "${GNUPGHOME}/rsa_2048_ssh_signing_key" >/dev/null &&
	ssh-keygen -t ed25519 -N "super_secret" -f "${GNUPGHOME}/protected_ssh_signing_key" >/dev/null &&
	find "${GNUPGHOME}" -name *ssh_signing_key.pub -exec cat {} \; | awk "{print \"\\\"principal with number \" NR \"\\\" \" \$0}" > "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" &&
	cat "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" &&
	ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/untrusted_ssh_signing_key" >/dev/null

)
"principal with number 1" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIK+hDFZT7sYN3M+1ciMGLTM6pu/xmJrNDTK/vN+QIVnq jch@gitster.c.googlers.com
"principal with number 2" ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDWLnqVpDYNstR7jCPKr1FaWzxt7NNw/kEV61GgKThW9S54/p/0WN+SCtUI0j7dCv/NkNhy87WNhohC9rCvZowPf/HRflHZ28vVk5d0DERCmlLBHnTq65Buyzj2R5xMtyVOBBMd+Io6tXk6GfJ6Dvq9l1wIJLv3OodOLBym3idV+8C+GOw9teTOlQWwkZD2hAt0n1Dy5WtaeGm3O2aUwRyg7KuxxmsgadBJ13a9thMYpwkS1McnwB+7oS81VXeeF0WcKAa2tGvxlg8NdBZuvvLJkl5vcISnpjt0NIuhqHzMlBIprpKujkZ5ze18YHXA52w6Y+9hG40n819EKjQfBVtD jch@gitster.c.googlers.com
"principal with number 3" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAgtFx51cu1d0gzZOjIdw4M9oBYgV+tX6Bsm2L+riv/Z jch@gitster.c.googlers.com
prerequisite GPGSSH ok
expecting success of 5534.8 'ssh signed push sends push certificate': 
	prepare_dst &&
	mkdir -p dst/.git/hooks &&
	git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" &&
	git -C dst config receive.certnonceseed sekrit &&
	write_script dst/.git/hooks/post-receive <<-\EOF &&
	# discard the update list
	cat >/dev/null
	# record the push certificate
	if test -n "${GIT_PUSH_CERT-}"
	then
		git cat-file blob $GIT_PUSH_CERT >../push-cert
	fi &&

	cat >../push-cert-status <<E_O_F
	SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
	KEY=${GIT_PUSH_CERT_KEY-nokey}
	STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
	NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
	NONCE=${GIT_PUSH_CERT_NONCE-nononce}
	E_O_F

	EOF

	test_config gpg.format ssh &&
	test_config user.signingkey "${SIGNING_KEY_PRIMARY}" &&
	FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") &&
	git push --signed dst noop ff +noff &&

	(
		cat <<-\EOF &&
		SIGNER=principal with number 1
		KEY=FINGERPRINT
		STATUS=G
		NONCE_STATUS=OK
		EOF
		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
	) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect &&

	noop=$(git rev-parse noop) &&
	ff=$(git rev-parse ff) &&
	noff=$(git rev-parse noff) &&
	grep "$noop $ff refs/heads/ff" dst/push-cert &&
	grep "$noop $noff refs/heads/noff" dst/push-cert &&
	test_cmp expect dst/push-cert-status

Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/
To dst
 * [new branch]      main -> noop
 * [new branch]      main -> ff
 * [new branch]      main -> noff
To dst
   66fe8b3..566fbd3  ff -> ff
 + 66fe8b3...6391b7f noff -> noff (forced update)
66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff
66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff
--- expect	2021-07-28 00:11:20.863019887 +0000
+++ dst/push-cert-status	2021-07-28 00:11:20.855019156 +0000
@@ -1,4 +1,4 @@
-SIGNER=principal with number 1
+SIGNER=principal with number 3
 KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE
 STATUS=G
 NONCE_STATUS=OK
not ok 8 - ssh signed push sends push certificate
#	
#		prepare_dst &&
#		mkdir -p dst/.git/hooks &&
#		git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" &&
#		git -C dst config receive.certnonceseed sekrit &&
#		write_script dst/.git/hooks/post-receive <<-\EOF &&
#		# discard the update list
#		cat >/dev/null
#		# record the push certificate
#		if test -n "${GIT_PUSH_CERT-}"
#		then
#			git cat-file blob $GIT_PUSH_CERT >../push-cert
#		fi &&
#	
#		cat >../push-cert-status <<E_O_F
#		SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
#		KEY=${GIT_PUSH_CERT_KEY-nokey}
#		STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
#		NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
#		NONCE=${GIT_PUSH_CERT_NONCE-nononce}
#		E_O_F
#	
#		EOF
#	
#		test_config gpg.format ssh &&
#		test_config user.signingkey "${SIGNING_KEY_PRIMARY}" &&
#		FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") &&
#		git push --signed dst noop ff +noff &&
#	
#		(
#			cat <<-\EOF &&
#			SIGNER=principal with number 1
#			KEY=FINGERPRINT
#			STATUS=G
#			NONCE_STATUS=OK
#			EOF
#			sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
#		) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect &&
#	
#		noop=$(git rev-parse noop) &&
#		ff=$(git rev-parse ff) &&
#		noff=$(git rev-parse noff) &&
#		grep "$noop $ff refs/heads/ff" dst/push-cert &&
#		grep "$noop $noff refs/heads/noff" dst/push-cert &&
#		test_cmp expect dst/push-cert-status
#	

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Fabian Stelzer <fs@gigacodes.de> writes:
>>
>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it
>>>> can afford to wait by rebasing on top of this topic.  By the time
>>>> "ssh signing" gets into testable shape (right now, it does not pass
>>>> tests when merged to 'seen'), hopefully the "expand install-prefix"
>>>> topic may already be in 'next' if not in 'master'.
>>> I think the test problem is not due to my patch.
>>
>> I've been seeing these test failers locally, every time
>> fs/ssh-signing topic is merged to 'seen' (without the reftable
>> thing).
>>
>> Test Summary Report
>> -------------------
>> t5534-push-signed.sh                             (Wstat: 256 Tests: 13 Failed: 2)
>>   Failed tests:  8, 12
>>   Non-zero exit status: 1
>> t7528-signed-commit-ssh.sh                       (Wstat: 256 Tests: 23 Failed: 2)
>>   Failed tests:  13, 17
>>   Non-zero exit status: 1
>>
>> When reftable thing is merged, either compilation fails or t0031
>> fails, and I suspect that these are not due to the ssh signing
>> topic.
>
> Interesting.  It seems that the failure has some correlation with
> the use of --root=<trash directory> option.
>
>     $ sh t5534-push-signed.sh -i

And 7528 fails with --root set to a /dev/shm/ trash directory.

So,... this "principal with number #" differences come from the
differences in location of the trash directory?  Why?

> --- expect	2021-07-28 00:11:20.863019887 +0000
> +++ dst/push-cert-status	2021-07-28 00:11:20.855019156 +0000
> @@ -1,4 +1,4 @@
> -SIGNER=principal with number 1
> +SIGNER=principal with number 3
>  KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE
>  STATUS=G
>  NONCE_STATUS=OK

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Fabian Stelzer <fs@gigacodes.de> writes:
>>>
>>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it
>>>>> can afford to wait by rebasing on top of this topic.  By the time
>>>>> "ssh signing" gets into testable shape (right now, it does not pass
>>>>> tests when merged to 'seen'), hopefully the "expand install-prefix"
>>>>> topic may already be in 'next' if not in 'master'.
>>>> I think the test problem is not due to my patch.
>>>
>>> I've been seeing these test failers locally, every time
>>> fs/ssh-signing topic is merged to 'seen' (without the reftable
>>> thing).
>>> ...
>> Interesting.  It seems that the failure has some correlation with
>> the use of --root=<trash directory> option.
>>
>>     $ sh t5534-push-signed.sh -i
>
> And 7528 fails with --root set to a /dev/shm/ trash directory.

An update.  The same failure can be seen _without_ merging the topic
to 'seen'.  The topic by itself will fail t5534 and t7528 when run
with --root= set to somewhere:

    $ make
    $ testpen=/dev/shm/testpen.$$
    $ rm -fr "$testpen" && mkdir "$testpen"
    $ cd t
    $ sh t5534-*.sh --root=$testpen -i
    $ sh t7528-*.sh --root=$testpen -i

on the branch itself, without getting interference by any other
topic, should hopefully be an easy enough way to reproduce the
problem.

Thanks.

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Fabian Stelzer]

On 28.07.21 07:43, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Fabian Stelzer <fs@gigacodes.de> writes:
>>>>
>>>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it
>>>>>> can afford to wait by rebasing on top of this topic.  By the time
>>>>>> "ssh signing" gets into testable shape (right now, it does not pass
>>>>>> tests when merged to 'seen'), hopefully the "expand install-prefix"
>>>>>> topic may already be in 'next' if not in 'master'.
>>>>> I think the test problem is not due to my patch.
>>>> I've been seeing these test failers locally, every time
>>>> fs/ssh-signing topic is merged to 'seen' (without the reftable
>>>> thing).
>>>> ...
>>> Interesting.  It seems that the failure has some correlation with
>>> the use of --root=<trash directory> option.
>>>
>>>      $ sh t5534-push-signed.sh -i
>> And 7528 fails with --root set to a /dev/shm/ trash directory.
> An update.  The same failure can be seen _without_ merging the topic
> to 'seen'.  The topic by itself will fail t5534 and t7528 when run
> with --root= set to somewhere:
>
>      $ make
>      $ testpen=/dev/shm/testpen.$$
>      $ rm -fr "$testpen" && mkdir "$testpen"
>      $ cd t
>      $ sh t5534-*.sh --root=$testpen -i
>      $ sh t7528-*.sh --root=$testpen -i
>
> on the branch itself, without getting interference by any other
> topic, should hopefully be an easy enough way to reproduce the
> problem.
>
> Thanks.


ok, funny issue. in the ssh test setup i generated a few ssh keys for 
testing and (wanting to be clever) concatenated them with a prefixed 
principal into an allowedSigners file using find & awk.

Turns out the directory entries in /dev/shm are the other way around.

touch ./t1 ./t2 /dev/shm/t1 /dev/shm/t2

find ./ -name 't[0-9]' results in:
./t1
./t2

a find /dev/shm -name 't[0-9]' returns:
/dev/shm/t2
/dev/shm/t1

I'll change the test setup code to do this statically for each key. Not 
such a good idea to rely on the file order in the dir anyway.

Thanks

Re: [PATCH v3 4/5] Use a better name for the function interpolating paths

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> ok, funny issue. in the ssh test setup i generated a few ssh keys for
> testing and (wanting to be clever) concatenated them with a prefixed 
> principal into an allowedSigners file using find & awk.
>
> Turns out the directory entries in /dev/shm are the other way around.

Good finding.  Yes, relying on the order "find" discovers filesystem
entities is asking for trouble.

> I'll change the test setup code to do this statically for each
> key. Not such a good idea to rely on the file order in the dir anyway.

Thanks.