Re: [RFC PATCH] credential: minor documentation fixes

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: May 4, 2020, 15:58 UTC
Message-ID: <20200504155837.GD86805@Carlos-MBP>
In-Reply-To: <20200504144436.GA9893@coredump.intra.peff.net>

On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:

Show 24 quoted lines

> On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:
> > * clarification on which fields can be updated by the helper; currently I
> >   don't think we allow overrides for protocol and host and assume all others
> >   but the documentation doesn't clarify, and that might be a problem for
> >   cert:// where file is more relevant.
> 
> I think we do allow a helper to transform a credential in any way:
> 
>   echo url=https://example.com/ |
>   git \
>     -c credential.helper='!sed >&2 s/^/one:/; echo host=other.example.com;:' \
>     -c credential.helper='!sed >&2 s/^/two:/;:' \
>     credential fill
> 
> produces:
> 
>   one:protocol=https
>   one:host=example.com
>   two:protocol=https
>   two:host=other.example.com
>   Username for 'https://other.example.com': 
> 
> So after the first helper runs, subsequent helpers (and the internal
> terminal prompt) will consider the modified hostname.

correct, and that is because the specification (and the current code) does say that is a valid operation.

what I meant is that we don't REALLY allow those modification to take place since the credential helper suggestions can't really affect what git assumes is the right host or protocol (or FWIW path), unlike username/password and so they don't seem worth having IMHO.

> Now whether that's a sane feature or not, I'm not sure.

would restricting the output of the credential helpers to username and password make more sense in this context then?

obviously this change will required both the protocol (definition and implementation) to change an changes on ALL helpers but I thought that was the objective of that documentation suggestion in Jonathan's original email.

I agree though the discussion of if that is something we should do in a maintenance release or not, is worth having; but I suspect it could be implemented with reasonable enough backward compatibility to be considered.

Carlo

CC adding the author of cert:// too easy on that part of the discussion