On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote: > 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. > 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
