Re: [RFC PATCH] credential: minor documentation fixes

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: May 4, 2020, 07:45 UTC
Message-ID: <20200504074520.GB86805@Carlos-MBP>
In-Reply-To: <20200503065826.GB1829906@coredump.intra.peff.net>

On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:

Show 10 quoted lines

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

was also curious about the other documentation updates mentioned[1] by Jonathan, and that I was hoping this patch will be included with.

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

Carlo

[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/