Re: [PATCH v5 2/3] connect, transport: encapsulate arg in struct

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v5 2/3] connect, transport: encapsulate arg in struct
Date: Tue, 26 Jan 2021 13:54:52 -0800	[thread overview]
Message-ID: <xmqq4kj31hzn.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <4eec55166889f16e41b10ae65002fd2c821b6762.1611686656.git.jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 26 Jan 2021 10:55:56 -0800")

Jonathan Tan <jonathantanmy@google.com> writes:

> In a future patch we plan to return the name of an unborn current branch
> from deep in the callchain to a caller via a new pointer parameter that
> points at a variable in the caller when the caller calls
> get_remote_refs() and transport_get_remote_refs().
>
> In preparation for that, encapsulate the existing ref_prefixes
> parameter into a struct. The aforementioned unborn current branch will
> go into this new struct in the future patch.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/clone.c      | 18 +++++++++++-------
>  builtin/fetch-pack.c |  3 ++-
>  builtin/fetch.c      | 18 +++++++++++-------
>  builtin/ls-remote.c  |  9 +++++----
>  connect.c            |  4 +++-
>  remote.h             |  4 +++-
>  transport-helper.c   |  5 +++--
>  transport-internal.h |  9 +--------
>  transport.c          | 23 ++++++++++++-----------
>  transport.h          | 21 ++++++++++++++-------
>  10 files changed, 65 insertions(+), 49 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 58b7c1fbdc..c2d96f4c89 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -220,7 +220,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  	version = discover_version(&reader);
>  	switch (version) {
>  	case protocol_v2:
> -		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);
> +		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL,
> +				args.stateless_rpc);
>  		break;

This seems to be an unrelated line-wrapping, but there are overlong
lines that are longer than this line in the same function.

Everything else looks sensible to change the assumption that a strvec
is sufficient for the communication in the codepaths and make it more
easily extended by passing a ls-refs-options structure, which makes
quite a lot of sense.

> diff --git a/transport.h b/transport.h
> index 24558c027d..1f5b60e4d3 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -233,17 +233,24 @@ int transport_push(struct repository *repo,
>  		   struct refspec *rs, int flags,
>  		   unsigned int * reject_reasons);
>  
> +struct transport_ls_refs_options {
> +	/*
> +	 * Optionally, a list of ref prefixes can be provided which can be sent
> +	 * to the server (when communicating using protocol v2) to enable it to
> +	 * limit the ref advertisement.  Since ref filtering is done on the
> +	 * server's end (and only when using protocol v2),
> +	 * transport_get_remote_refs() could return refs which don't match the
> +	 * provided ref_prefixes.
> +	 */
> +	struct strvec ref_prefixes;
> +};
> +#define TRANSPORT_LS_REFS_OPTIONS_INIT { STRVEC_INIT }
> +

And of course, the first step only carries the strvec we have been
passing around, i.e. does not lose or gain features.

Looking good.  Thanks.