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

From: Brandon Williams <[email-masked]>
Date: Tue, 1 May 2018 15:36:19 -0700

On 05/01, Jonathan Tan wrote:
> 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 <[email-masked]>
> ---
>  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(-)
> 

> @@ -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.

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

-- 
Brandon Williams