Handling of paths

Handling of paths

[Victor Toni]

Hello,

I have a .gitconfig in which I try to separate work and private stuff
by using includes which works great.

When using [include] the path is treated either
- relative to the including file (if the path itself relative)
- relative to the home directory if it starts with ~
- absolute if the path is absolute

This is fine and expected.

What's unexpected is that paths used for sslKey or sslCert are treated
differently insofar as they are expected to be absolute.
Relative paths (whether with or without "~") don't work.

It would't be an issue to use absoulte paths if I wouldn't use the
same config for Linux and Windows and each OS has its own semantic
where it $HOME ishould be.

To avoid double configurations I tried to use the same directory
structure within my $HOME for both OS.

This approach fails since paths other than for [include] seem to have
to be absolute which seems like a bug to me.

Do you have any suggestions how I could make this work?

Thank you,
Victor

Re: Handling of paths

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

> What's unexpected is that paths used for sslKey or sslCert are treated
> differently insofar as they are expected to be absolute.
> Relative paths (whether with or without "~") don't work.

Looking at http.c::http_options(), I see that "sslcapath" and
"sslcainfo" do use git_config_pathname() when grabbing their values,
but "sslcert" and "sslkey" treat the value as a plain vanilla string
without expecting "~[username]/" at all.

The modern http.c codestructure was introduced at 29508e1e ("Isolate
shared HTTP request functionality", 2005-11-18) and was corrected
for interation between the multiple configuration files in 7059cd99
("http_init(): Fix config file parsing", 2009-03-09), but back in
these versions, all of them including "sslcapath" and "sslcainfo"
were all treated as plain vanilla strings.

It appears that only two of these among four were made aware of the
"~[username]/" prefix in bf9acba2 ("http: treat config options
sslCAPath and sslCAInfo as paths", 2015-11-23), but "sslkey" and
"sslcert" were still left as plain vanilla strings.  I do not know
if that was an elaborate omission, or a mere oversight, as it seems
that it happened while I was away, so...

Re: Handling of paths

[Charles Bailey]

On Thu, Jul 20, 2017 at 12:42:40PM -0700, Junio C Hamano wrote:
> Victor Toni <victor.toni@gmail.com> writes:
> 
> > What's unexpected is that paths used for sslKey or sslCert are treated
> > differently insofar as they are expected to be absolute.
> > Relative paths (whether with or without "~") don't work.
> 
> It appears that only two of these among four were made aware of the
> "~[username]/" prefix in bf9acba2 ("http: treat config options
> sslCAPath and sslCAInfo as paths", 2015-11-23), but "sslkey" and
> "sslcert" were still left as plain vanilla strings.  I do not know
> if that was an elaborate omission, or a mere oversight, as it seems
> that it happened while I was away, so...

It was more of an oversight than a deliberate omission, but more
accurately I didn't actively consider whether the other http.ssl*
variables were pathname-like or not.

At the time I was trying to make a config which needed to set
http.sslCAPath and/or http.sslCAInfo more portable between users and
these were "obviously" pathname-like to me. Now that I read
the help for http.sslCert and http.sslKey, I see no reason that they
shouldn't also use git_config_pathname. If I'd been more thorough I
would have proposed this at the time.

Charles.

Re: Handling of paths

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> On Thu, Jul 20, 2017 at 12:42:40PM -0700, Junio C Hamano wrote:
>> Victor Toni <victor.toni@gmail.com> writes:
>> 
>> > What's unexpected is that paths used for sslKey or sslCert are treated
>> > differently insofar as they are expected to be absolute.
>> > Relative paths (whether with or without "~") don't work.
>> 
>> It appears that only two of these among four were made aware of the
>> "~[username]/" prefix in bf9acba2 ("http: treat config options
>> sslCAPath and sslCAInfo as paths", 2015-11-23), but "sslkey" and
>> "sslcert" were still left as plain vanilla strings.  I do not know
>> if that was an elaborate omission, or a mere oversight, as it seems
>> that it happened while I was away, so...
>
> It was more of an oversight than a deliberate omission, but more
> accurately I didn't actively consider whether the other http.ssl*
> variables were pathname-like or not.
>
> At the time I was trying to make a config which needed to set
> http.sslCAPath and/or http.sslCAInfo more portable between users and
> these were "obviously" pathname-like to me. Now that I read
> the help for http.sslCert and http.sslKey, I see no reason that they
> shouldn't also use git_config_pathname. If I'd been more thorough I
> would have proposed this at the time.

Thanks.

I've read the function again and I think the attached patch covers
everything that ought to be a filename.

By the way, to credit you, do you prefer your bloomberg or hashpling
address?

-- >8 --
Subject: http.c: http.sslcert and http.sslkey are both pathnames

Back when the modern http_options() codepath was created to parse
various http.* options at 29508e1e ("Isolate shared HTTP request
functionality", 2005-11-18), and then later was corrected for
interation between the multiple configuration files in 7059cd99
("http_init(): Fix config file parsing", 2009-03-09), we parsed
configuration variables like http.sslkey, http.sslcert as plain
vanilla strings, because git_config_pathname() that understands
"~[username]/" prefix did not exist.  Later, we converted some of
them (namely, http.sslCAPath and http.sslCAInfo) to use the
function, and added variables like http.cookeyFile http.pinnedpubkey
to use the function from the beginning.  Because of that, these
variables all understand "~[username]/" prefix.

Make the remaining two variables, http.sslcert and http.sslkey, also
aware of the convention, as they are both clearly pathnames to
files.

Noticed-by: Victor Toni <victor.toni@gmail.com>
Helped-by: Charles Bailey <cbailey32@bloomberg.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index c6c010f881..76ff63c14d 100644
--- a/http.c
+++ b/http.c
@@ -272,10 +272,10 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.sslversion", var))
 		return git_config_string(&ssl_version, var, value);
 	if (!strcmp("http.sslcert", var))
-		return git_config_string(&ssl_cert, var, value);
+		return git_config_pathname(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
 	if (!strcmp("http.sslkey", var))
-		return git_config_string(&ssl_key, var, value);
+		return git_config_pathname(&ssl_key, var, value);
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var))

Re: Handling of paths

[Charles Bailey]

On Thu, Jul 20, 2017 at 01:30:52PM -0700, Junio C Hamano wrote:
> 
> I've read the function again and I think the attached patch covers
> everything that ought to be a filename.
> 
> By the way, to credit you, do you prefer your bloomberg or hashpling
> address?

The patch looks good to me.

It's not critical which address you credit.

I mark patches which result from my work at Bloomberg with my Bloomberg
email address and anything that I do entirely outside of work with my
hashpling address, although I will tend to use my hashpling email for
all communications because it co-operates with the mailing list
conventions a lot better.

In this case, this is a follow on from a cbailey32@bloomberg.net patch
so crediting that address seems the more appropriate option.

Charles.

Re: Handling of paths

[Victor Toni]

2017-07-20 22:30 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>
> I've read the function again and I think the attached patch covers
> everything that ought to be a filename.
>
Your swift reaction is very much appreciated.
With the background you gave I just started to to create a patch
myself just to see that you already finished the patch.

Thanks a lot!

Best regards,
Victor

Re: Handling of paths

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

> 2017-07-20 22:30 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>>
>> I've read the function again and I think the attached patch covers
>> everything that ought to be a filename.
>>
> Your swift reaction is very much appreciated.
> With the background you gave I just started to to create a patch
> myself just to see that you already finished the patch.

Heh, I guess I could have waited to save time ;-) 

I did the patch myself in order to avoid wasting your effort to find
and report the issue and time and distraction cost from Charles to
remember what happened 2 years ago and reply to me, because I will
certainly forget if I didn't have some patch readily usable in the
list archive.

In general, I (and other experienced reviewers here) prefer to give
chances to people who are new to the Git development community and
are inclined to do so to scratch their own itch, by giving analysis
of the problem and a suggested route to solve it, but without giving
the final solution in a patch form.  After all, many developers
(including me) started from small changes before getting involved
more deeply to the project and starting to play more important
roles.

Some reviewers are much better than myself in judging if a new
person wants satisfaction of solving himself or herself[*1*], and
they end their analysis and suggestion with a phrase like "Want to
try doing a patch yourself?"  I try to follow their example myself,
but I do not always succeed, and this is one of such cases.  I guess
you could have immediately responded "OK, let me try to see if I can
fix it myself" before starting to actually work on it ;-)

Having said all that, I suspect that your original problem
description might point at another thing we may want to look into.

The patch under discussion may have solved the "~[username]/" prefix
issue, but I offhand am not sure if a path-like variable that holds
a relative path behaves sensibly when they appear in configuration
files and in a file that has configuration snippets that is included
with the "[include] path=..." thing, and if there is a need to clarify
and/or update the rules.

In any case, thanks for reporting the bugs on two variables, and
welcome to the Git development community.


[Footnote]

*1* Some people just want to report an issue and move on, which is
    understandable.

Re: Handling of paths

[Jeff King]

On Fri, Jul 21, 2017 at 08:15:17AM -0700, Junio C Hamano wrote:

> In general, I (and other experienced reviewers here) prefer to give
> chances to people who are new to the Git development community and
> are inclined to do so to scratch their own itch, by giving analysis
> of the problem and a suggested route to solve it, but without giving
> the final solution in a patch form.  After all, many developers
> (including me) started from small changes before getting involved
> more deeply to the project and starting to play more important
> roles.

This is a good point, and I should remember to do it more, too.
It's often faster to do a small patch yourself than to help walk a
first-timer through it, but keeping the community healthy is an
important step.

At any rate, your patch to use config_pathname() looks like the right
thing to me.

> Having said all that, I suspect that your original problem
> description might point at another thing we may want to look into.
> 
> The patch under discussion may have solved the "~[username]/" prefix
> issue, but I offhand am not sure if a path-like variable that holds
> a relative path behaves sensibly when they appear in configuration
> files and in a file that has configuration snippets that is included
> with the "[include] path=..." thing, and if there is a need to clarify
> and/or update the rules.

The "[include]path" behavior is intentional and documented: it takes the
path relative to the including file. I think that would be a reasonable
behavior for path-like variables in general (and a path-like variable in
an included file would be relative to that included file; this should
Just Work because the include mechanism keeps a stack of files).

I could probably sketch out a patch, but per the above discussion I'll
leave it for now. Also, I'm lazy. ;)

-Peff