threads / discuss / 46424

Handling of paths

Subject: Handling of paths

## tl;dr

8 messages between Jul 19, 2017 and Jul 24, 2017.

replies: 7people: 4as markdown or json

Victor Toni· Jul 19, 2017, 16:48 UTC · lore
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

Junio C Hamano· Jul 20, 2017, 19:42 UTC · re: Victor Toni · lore

Re: Handling of paths

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

Charles Bailey· Jul 20, 2017, 20:05 UTC · re: Junio C Hamano · lore

Re: Handling of paths

On Thu, Jul 20, 2017 at 12:42:40PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 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.
Junio C Hamano· Jul 20, 2017, 20:30 UTC · re: Charles Bailey · lore

Re: Handling of paths

Charles Bailey <charles@hashpling.org> writes:
Show 24 quoted lines
> 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))
Charles Bailey· Jul 20, 2017, 20:52 UTC · re: Junio C Hamano · lore

Re: Handling of paths

On Thu, Jul 20, 2017 at 01:30:52PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 
> 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.
Victor Toni· Jul 20, 2017, 21:03 UTC · re: Junio C Hamano · lore

Re: Handling of paths

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

Junio C Hamano· Jul 21, 2017, 15:15 UTC · re: Victor Toni · lore

Re: Handling of paths

Victor Toni <victor.toni@gmail.com> writes:
Show 8 quoted lines
> 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.
Jeff King· Jul 24, 2017, 16:52 UTC · re: Junio C Hamano · lore

Re: Handling of paths

On Fri, Jul 21, 2017 at 08:15:17AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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.

Show 9 quoted lines
> 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

← back to recent threads