Re: [RFC PATCH] credential: minor documentation fixes

From: Jeff King <peff@peff.net>
Date: May 4, 2020, 14:44 UTC
Message-ID: <20200504144436.GA9893@coredump.intra.peff.net>
In-Reply-To: <20200504074520.GB86805@Carlos-MBP>

On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:

Show 14 quoted lines

> On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:
> > On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:
> > 
> > > the order of parameters used in credential_match was inconsistent
> > > between credential.c and credential.h as well, so update both to
> > > match the current implementation.
> > 
> > Yes, looks like this has been wrong since the beginning in 118250728e
> > (credential: apply helper config, 2011-12-10). I checked the callers to
> > make sure none of them had gotten it backwards, but they all look right
> > (and just the declaration is wrong).
> 
> thanks for checking, will include this (and your typo fix) in the
> submission; should I add your "Reviewed-by" then?

Sure, feel free to.

Show 6 quoted lines

> some things that I think might need clarification (or maybe even code changes
> after agreed on) are :
> 
> * the meaning of "exactly" for matching protocol and hostname in the URL
>   since 06 are both case insensitive per RFC3986 and we have been
>   ambiguous on that, leading to some helpers assuming case or encoding.

Yeah, IIRC we discussed case-sensitivity at the time and went with the stricter behavior in the name of safety over convenience. And I don't think anybody has complained since then. So I'm not really _opposed_ to loosening it to match the URL, but perhaps a maintenance release is not the best time to do so.

> * the rules for how helper matching are expected to be ordered, specially
>   considering the recent adding of wildcard matching and the revival of
>   partial matching, and the fact that the order is relevant for both
>   discovery of credentials and which helpers are used.

Yes, this could be better documented. I think the current rules are probably not ideal, especially when you mix credential.*.helper and credential.helper. So some fixes are possible there, but I think they might be best added as new feature (e.g., a new config like credential.*.helperOrder that says whether and how to use non-url-specific helpers; or something like that).

> * the use of hostname as a key, since the addition of cert:// that has none
>   and uses path instead (had emailed the author privately for clarification,
>   but hadn't heard yet) and the effect that has on which fields are expected
>   and which values are valid.

Yeah, there could be more discussion in gitcredentials(7) there.

> * the role of overrides (ex: the documented example of passing URL and later
>   updating it seems useful, eventhough I am not aware if being used)

I'd be OK to see this feature removed. I have used it for various debugging or experimenting scenarios, but Git in general would never pass anything except a broken-down set of fields, and only one of each field.

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

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

-Peff