Re: [PATCH 4/4] credential: document protocol updates

From: Jeff King <peff@peff.net>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com
Subject: Re: [PATCH 4/4] credential: document protocol updates
Date: Wed, 6 May 2020 12:26:48 -0400	[thread overview]
Message-ID: <20200506162648.GA1275237@coredump.intra.peff.net> (raw)
In-Reply-To: <20200505013908.4596-5-carenas@gmail.com>

On Mon, May 04, 2020 at 06:39:08PM -0700, Carlo Marcelo Arenas Belón wrote:

> Document protocol changes after CVE-2020-11008, while at it do some
> minor improvements for clarity and consitency.

s/consitency/consistency/s

I think the overall direction is good. A few small comments below.

>  `host`::
>  
> -	The remote hostname for a network credential.
> +	The remote hostname for a network credential.  This includes
> +	the port number if one was specified.

It might be worth giving an example of the syntax, like:

  the port number if one was specified (e.g., "example.com:8088").

> @@ -146,8 +147,11 @@ Git understands the following attributes:
>  	value is parsed as a URL and treated as if its constituent parts
>  	were read (e.g., `url=https://example.com` would behave as if
>  	`protocol=https` and `host=example.com` had been provided). This
> -	can help callers avoid parsing URLs themselves.  Note that any
> -	components which are missing from the URL (e.g., there is no
> -	username in the example above) will be set to empty; if you want
> -	to provide a URL and override some attributes, provide the URL
> -	attribute first, followed by any overrides.
> +	can help callers avoid parsing URLs themselves.
> +
> +	Note that specifying a protocol is mandatory and if the URL
> +	type doesn't require a hostname (like for cert://) then an
> +	empty ("")  hostname will be generated.

So we are losing the bit about overriding. I think that is OK, as we'd
like to avoid suggesting that is a good idea, and we may even remove the
feature in the future.

The word "generated" confused me a bit. Maybe something like:

  ...and if the URL doesn't specify a hostname (e.g.,
  "cert:///path/to/file"), the credential will contain a hostname
  attribute whose value is an empty string.

> +	Components which are missing from the URL (e.g., there is no
> +	username in the example above) will be left unset.

Makes sense.

-Peff