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

From: Brian Norris <computersforpeace@gmail.com>
Date: Apr 29, 2016, 15:55 UTC
Message-ID: <20160429155532.GA57118@google.com>
In-Reply-To: <20160429141212.GB26643@sigill.intra.peff.net>

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