threads / patch / 42174

patch, 2 partsDocumentation: config: improve word ordering for http.cookieFile

Subject: [PATCH 1/2] Documentation: config: improve word ordering for http.cookieFile

## tl;dr

12 messages between Apr 29, 2016 and Apr 29, 2016. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Brian Norris· Apr 29, 2016, 06:23 UTC · lore
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/config.txt +1 −1
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50477b2..a775ad885a76 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1660,7 +1660,7 @@ http.cookieFile::
 	in the Git http session, if they match the server. The file format
 	of the file to read cookies from should be plain HTTP headers or
 	the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
-	NOTE that the file specified with http.cookieFile is only used as
+	NOTE that the file specified with http.cookieFile is used only as
 	input unless http.saveCookies is set.
 
 http.saveCookies::

-- 
2.8.1.340.g018a5d0.dirty
Brian Norris· Apr 29, 2016, 06:23 UTC · re: Brian Norris · lore

[PATCH 2/2] http: expand http.cookieFile as a path

This should handle .gitconfig files that specify things like:
[http]
	cookieFile = "~/.gitcookies"
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 Documentation/config.txt | 3 +++
 http.c                   | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)
Show changes to 2 files +4 −1

Documentation/config.txt, http.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a775ad885a76..d3ef2d3b5d13 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1660,6 +1660,9 @@ http.cookieFile::
 	in the Git http session, if they match the server. The file format
 	of the file to read cookies from should be plain HTTP headers or
 	the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
+	The value of `http.cookieFile` is subject to tilde expansion: `~/` is
+	expanded to the value of `$HOME`, and `~user/` to the specified user's
+	home directory.
 	NOTE that the file specified with http.cookieFile is used only as
 	input unless http.saveCookies is set.
 
diff --git a/http.c b/http.c
index 4304b80ad3ac..1044f9ba0e28 100644
--- a/http.c
+++ b/http.c
@@ -293,7 +293,7 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&http_proxy_authmethod, var, value);
 
 	if (!strcmp("http.cookiefile", var))
-		return git_config_string(&curl_cookie_file, var, value);
+		return git_config_pathname(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
 		curl_save_cookies = git_config_bool(var, value);
 		return 0;

-- 
2.8.1.340.g018a5d0.dirty
Jeff King· Apr 29, 2016, 14:12 UTC · re: Brian Norris · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

On Fri, Apr 29, 2016 at 12:23:57AM -0600, Brian Norris wrote:
> This should handle .gitconfig files that specify things like:
> 
> [http]
> 	cookieFile = "~/.gitcookies"
Seems like a good idea, and the implementation looks obviously correct.
For the documentation:
Show 13 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index a775ad885a76..d3ef2d3b5d13 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1660,6 +1660,9 @@ http.cookieFile::
>  	in the Git http session, if they match the server. The file format
>  	of the file to read cookies from should be plain HTTP headers or
>  	the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
> +	The value of `http.cookieFile` is subject to tilde expansion: `~/` is
> +	expanded to the value of `$HOME`, and `~user/` to the specified user's
> +	home directory.
>  	NOTE that the file specified with http.cookieFile is used only as
>  	input unless http.saveCookies is set.

I'm not sure if it's a good idea to go into so much detail about expand_user_path() here. There are a lot of options that use the same rules, and we probably don't want to go into a complete explanation inside each option's description. Is there a canonical definition of how we do expansion in config.txt that we can just reference (and if not, can we add one)?

-Peff
Brian Norris· Apr 29, 2016, 15:55 UTC · re: Jeff King · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

On Fri, Apr 29, 2016 at 10:12:12AM -0400, Jeff King wrote:
Show 31 quoted lines
> On Fri, Apr 29, 2016 at 12:23:57AM -0600, Brian Norris wrote:
> 
> > This should handle .gitconfig files that specify things like:
> > 
> > [http]
> > 	cookieFile = "~/.gitcookies"
> 
> Seems like a good idea, and the implementation looks obviously correct.
> 
> For the documentation:
> 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index a775ad885a76..d3ef2d3b5d13 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1660,6 +1660,9 @@ http.cookieFile::
> >  	in the Git http session, if they match the server. The file format
> >  	of the file to read cookies from should be plain HTTP headers or
> >  	the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
> > +	The value of `http.cookieFile` is subject to tilde expansion: `~/` is
> > +	expanded to the value of `$HOME`, and `~user/` to the specified user's
> > +	home directory.
> >  	NOTE that the file specified with http.cookieFile is used only as
> >  	input unless http.saveCookies is set.
> 
> I'm not sure if it's a good idea to go into so much detail about
> expand_user_path() here. There are a lot of options that use the same
> rules, and we probably don't want to go into a complete explanation
> inside each option's description. Is there a canonical definition of how
> we do expansion in config.txt that we can just reference (and if not,
> can we add one)?

I mostly just copied from boilerplate on another option. IIRC, there were at least two other options that were documented similarly.

I think it's very important to note this somehow in the documentation. For months, I've just had to keep a delta among the (otherwise identical, shared) .gitconfig on my various machines just to account for the different home directories. I thought that the "no-expansion" thing was an intentional policy, since there are various blogs/forums that mention the lack of this kind of expansion when you search for related problems. But apparently this was a bug/oversight. So having clear documentation to state the reality is imperative, IMO.

The best kind of documentation might mention that all paths can be expanded in this way, and then just include "path" language on the relevant options. But then we'd have to do a quick audit to make sure that every path-based option does indeed use this path-expansion helper. (As this patch proves, we haven't been very consistent so far.)

If you have a good overall recommendation for this, I can try to send a new patch sometime next week.

Brian
Junio C Hamano· Apr 29, 2016, 17:48 UTC · re: Brian Norris · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

Brian Norris <computersforpeace@gmail.com> writes:
> I mostly just copied from boilerplate on another option. IIRC, there
> were at least two other options that were documented similarly.

My quick grep didn't find 'another option' other than include.path, but how about this as a preparatory step?

-- >8 --
Subject: [PATCH] config: describe 'pathname' value type

We have a dedicated section for various value-types used in the configuration variables already, because we needed to describe how booleans and scaled integers can be spelled, and the pathname type would fit there.

Adjust the description of `include.path` variable slightly to clarify that the variable is of this type.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Show changes to Documentation/config.txt +13 −3
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..64a57fa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -81,13 +81,16 @@ Includes
 
 You can include one config file from another by setting the special
 `include.path` variable to the name of the file to be included. The
+variable takes a pathname as its value, and is subject to tilde
+expansion.
+
+The
 included file is expanded immediately, as if its contents had been
 found at the location of the include directive. If the value of the
 `include.path` variable is a relative path, the path is considered to be
 relative to the configuration file in which the include directive was
-found. The value of `include.path` is subject to tilde expansion: `~/`
-is expanded to the value of `$HOME`, and `~user/` to the specified
-user's home directory. See below for examples.
+found.  See below for examples.
+
 
 Example
 ~~~~~~~
@@ -169,6 +172,13 @@ thing on the same output line (e.g. opening parenthesis before the
 list of branch names in `log --decorate` output) is set to be
 painted with `bold` or some other attribute.
 
+pathname::
+	A variable that takes a pathname value can be given a
+	string that begins with "~/" or "~user/", and the usual
+	tilde expansion happens to such a string: `~/`
+	is expanded to the value of `$HOME`, and `~user/` to the
+	specified user's home directory.
+
 
 Variables
 ~~~~~~~~~

-- 
2.8.1-521-g705491b
Jeff King· Apr 29, 2016, 17:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

On Fri, Apr 29, 2016 at 10:48:16AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Brian Norris <computersforpeace@gmail.com> writes:
> 
> > I mostly just copied from boilerplate on another option. IIRC, there
> > were at least two other options that were documented similarly.
> 
> My quick grep didn't find 'another option' other than include.path,
> but how about this as a preparatory step?

I found core.excludesFile and commit.template which could use the same treatment.

-Peff
Junio C Hamano· Apr 29, 2016, 17:55 UTC · re: Jeff King · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Fri, Apr 29, 2016 at 10:48:16AM -0700, Junio C Hamano wrote:
>
>> Brian Norris <computersforpeace@gmail.com> writes:
>> 
>> > I mostly just copied from boilerplate on another option. IIRC, there
>> > were at least two other options that were documented similarly.
>> 
>> My quick grep didn't find 'another option' other than include.path,
>> but how about this as a preparatory step?
>
> I found core.excludesFile and commit.template which could use the same
> treatment.
>
> -Peff

Thanks. Perhaps squash this to the patch in the message you are responding to.

Show changes to Documentation/config.txt +6 −8
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ff7eaaf..786e0fa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -603,11 +603,10 @@ be delta compressed, but larger binary media files won't be.
 Common unit suffixes of 'k', 'm', or 'g' are supported.
 
 core.excludesFile::
-	In addition to '.gitignore' (per-directory) and
-	'.git/info/exclude', Git looks into this file for patterns
-	of files which are not meant to be tracked.  "`~/`" is expanded
-	to the value of `$HOME` and "`~user/`" to the specified user's
-	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
+	Specifies the pathname to the file that contains patterns to
+	describe paths that are not meant to be tracked, in addition
+	to '.gitignore' (per-directory) and '.git/info/exclude'.
+	Defaults to $XDG_CONFIG_HOME/git/ignore.
 	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/ignore
 	is used instead. See linkgit:gitignore[5].
 
@@ -1116,9 +1115,8 @@ commit.status::
 	message.  Defaults to true.
 
 commit.template::
-	Specify a file to use as the template for new commit messages.
-	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
-	specified user's home directory.
+	Specify the pathname of a file to use as the template for
+	new commit messages.
 
 credential.helper::
 	Specify an external helper to be called when a username or
Jeff King· Apr 29, 2016, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

On Fri, Apr 29, 2016 at 10:55:00AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Thanks.  Perhaps squash this to the patch in the message you are
> responding to.
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ff7eaaf..786e0fa 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> [...]

Yeah, this (and the rest of the plan you outlined) all look good to me. Thanks.

-Peff
Junio C Hamano· Apr 29, 2016, 17:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> Brian Norris <computersforpeace@gmail.com> writes:
>
>> I mostly just copied from boilerplate on another option. IIRC, there
>> were at least two other options that were documented similarly.
>
> My quick grep didn't find 'another option' other than include.path,
> but how about this as a preparatory step?
>
> -- >8 --
> Subject: [PATCH] config: describe 'pathname' value type

And then, after applying this either before or after your 1/2, we can squash this into your 2/2.

Show changes to Documentation/config.txt +2 −4
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 39867f5..b7d3e69 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1665,13 +1665,11 @@ http.emptyAuth::
 	authentication.
 
 http.cookieFile::
-	File containing previously stored cookie lines which should be used
+	The pathname of a file containing previously stored cookie lines,
+	which should be used
 	in the Git http session, if they match the server. The file format
 	of the file to read cookies from should be plain HTTP headers or
 	the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
-	The value of `http.cookieFile` is subject to tilde expansion: `~/` is
-	expanded to the value of `$HOME`, and `~user/` to the specified user's
-	home directory.
 	NOTE that the file specified with http.cookieFile is used only as
 	input unless http.saveCookies is set.
Junio C Hamano· Apr 29, 2016, 17:11 UTC · re: Jeff King · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> I'm not sure if it's a good idea to go into so much detail about
> expand_user_path() here. There are a lot of options that use the same
> rules, and we probably don't want to go into a complete explanation
> inside each option's description. Is there a canonical definition of how
> we do expansion in config.txt that we can just reference (and if not,
> can we add one)?

We have a dedicated section for various value-types used in the configuration variables already, because we needed to describe how booleans and scaled integers can be spelled, and the pathname type would fit there.

 Documentation/config.txt | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to Documentation/config.txt +5 −0
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 59d7046..1bf42a6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -169,6 +169,11 @@ thing on the same output line (e.g. opening parenthesis before the
 list of branch names in `log --decorate` output) is set to be
 painted with `bold` or some other attribute.
 
+pathname::
+	A variable that takes a pathname value can be given a
+	string that begins with "~/" or "~user/", and the usual
+	tilde expansion happens to such a string.
+
 
 Variables
 ~~~~~~~~~
Jeff King· Apr 29, 2016, 17:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

On Fri, Apr 29, 2016 at 10:11:48AM -0700, Junio C Hamano wrote:
Show 33 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > I'm not sure if it's a good idea to go into so much detail about
> > expand_user_path() here. There are a lot of options that use the same
> > rules, and we probably don't want to go into a complete explanation
> > inside each option's description. Is there a canonical definition of how
> > we do expansion in config.txt that we can just reference (and if not,
> > can we add one)?
> 
> We have a dedicated section for various value-types used in the
> configuration variables already, because we needed to describe how
> booleans and scaled integers can be spelled, and the pathname type
> would fit there.
> 
>  Documentation/config.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 59d7046..1bf42a6 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -169,6 +169,11 @@ thing on the same output line (e.g. opening parenthesis before the
>  list of branch names in `log --decorate` output) is set to be
>  painted with `bold` or some other attribute.
>  
> +pathname::
> +	A variable that takes a pathname value can be given a
> +	string that begins with "~/" or "~user/", and the usual
> +	tilde expansion happens to such a string.
> +
>  
>  Variables
>  ~~~~~~~~~

Yeah, this is what I had in mind. My only reservation would be that we need to make sure it is clear that this applies only to keys marked as taking a "pathname" type in the documentation. I'm suspect there are ones that are logically paths but do not currently do the expansion, but the wording above makes it sound like any pathname-like thing does.

Alternatively, it might be worth going through the list to make sure all paths use git_config_pathname() internally. Brian asked earlier if the "no expansion" was an intentional policy, but it's not. It's just that pathname expansion came much later, and config keys were ported over to it one by one as people found it useful to do so.

-Peff
Junio C Hamano· Apr 29, 2016, 17:27 UTC · re: Jeff King · lore

Re: [PATCH 2/2] http: expand http.cookieFile as a path

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> Yeah, this is what I had in mind. My only reservation would be that we
> need to make sure it is clear that this applies only to keys marked as
> taking a "pathname" type in the documentation. I'm suspect there are
> ones that are logically paths but do not currently do the expansion, but
> the wording above makes it sound like any pathname-like thing does.

Yeah, my initial draft phrased it more like how we describe boolean, but somehow the language used there felt awkward to me.

With "A variable that take a pathname value", the users who read it would find "ones that are logically paths but do not do the expansion" and file a bug. We'd resolve each of them by seeing if the documentation says the variable does take a pathname, and adjust either the documentation (if the value for the variable should not be expanded but the documentation hints it might be a pathname-like thing, clarify that it is not pathname-like at all) or the code (if the value for the variable should be expanded but we forgot, we call the user_path() function).

> Alternatively, it might be worth going through the list to make sure all
> paths use git_config_pathname() internally.

I was hoping that with the patch we can farm out the bug-hunting process to the end users.

> Brian asked earlier if the "no expansion" was an intentional
> policy, but it's not. It's just that pathname expansion came much
> later, and config keys were ported over to it one by one as people
> found it useful to do so.
Yes, that matches the actual order of events.

← back to recent threads