Re: [PATCH 2/2] {fetch,upload}-pack: support filter in protocol v2

From: Brandon Williams <bmwill@google.com>
Date: May 1, 2018, 22:36 UTC
Message-ID: <20180501223619.GA122314@google.com>
In-Reply-To: <0486dfdfe614f908b322552b02ebbb3f1f6cd6fc.1525213052.git.jonathantanmy@google.com>

On 05/01, Jonathan Tan wrote:

Show 18 quoted lines

> The fetch-pack/upload-pack protocol v2 was developed independently of
> the filter parameter (used in partial fetches), thus it did not include
> support for it. Add support for the filter parameter.
> 
> Like in the legacy protocol, the server advertises and supports "filter"
> only if uploadpack.allowfilter is configured.
> 
> Like in the legacy protocol, the client continues with a warning if
> "--filter" is specified, but the server does not advertise it.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/technical/protocol-v2.txt |  9 +++
>  fetch-pack.c                            | 23 +++++-
>  t/t5702-protocol-v2.sh                  | 97 +++++++++++++++++++++++++
>  upload-pack.c                           | 12 ++-
>  4 files changed, 135 insertions(+), 6 deletions(-)
>

Show 10 quoted lines

> @@ -1428,7 +1434,9 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
>  int upload_pack_advertise(struct repository *r,
>  			  struct strbuf *value)
>  {
> -	if (value)
> -		strbuf_addstr(value, "shallow");
> +	git_config(upload_pack_config, NULL);
> +	if (value) {
> +		strbuf_addf(value, "%sshallow", allow_filter ? "filter " : "");
> +	}

This is a bit difficult to read and there is no reason why we would need to read the entire upload_pack_config to determine if we need to filter things (we will need to read the config if cmd "fetch" is requested though). Instead it may be better to do the following:

  if (value) {
    strbuf_addstr(value, "shallow");
    if (repo_config_get(r, "uplaodpack.filter"))
      strbuf_addstr(value, " filter");
  }

This way its easier to read and you only are reading the required value from the config.

Show 5 quoted lines

>  	return 1;
>  }
> -- 
> 2.17.0.441.gb46fe60e1d-goog
>

-- 
Brandon Williams