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

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

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Brian Norris]

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

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

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

[Jeff King]

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

-Peff

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

[Brian Norris]

On Fri, Apr 29, 2016 at 10:12:12AM -0400, Jeff King wrote:
> 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

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

[Junio C Hamano]

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

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

On Fri, Apr 29, 2016 at 10:55:00AM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

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

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.
 

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

[Junio C Hamano]

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

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

[Jeff King]

On Fri, Apr 29, 2016 at 10:11:48AM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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