Re: [PATCH 08/11] bundle: parse filter capability

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 7, 2022, 15:38 UTC
Message-ID: <220307.86bkyhzs3r.gmgdl@evledraar.gmail.com>
In-Reply-To: <e7dbb46e6acb5c22a0b456135f3af42b974e0268.1645638911.git.gitgitgadget@gmail.com>

On Wed, Feb 23 2022, Derrick Stolee via GitGitGadget wrote:

Show 88 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> The v3 bundle format has capabilities, allowing newer versions of Git to
> create bundles with newer features. Older versions that do not
> understand these new capabilities will fail with a helpful warning.
>
> Create a new capability allowing Git to understand that the contained
> pack-file is filtered according to some object filter. Typically, this
> filter will be "blob:none" for a blobless partial clone.
>
> This change teaches Git to parse this capability, place its value in the
> bundle header, and demonstrate this understanding by adding a message to
> 'git bundle verify'.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  bundle.c                      | 17 ++++++++++++++++-
>  bundle.h                      |  3 +++
>  list-objects-filter-options.c |  2 +-
>  list-objects-filter-options.h |  5 +++++
>  4 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index dc56db9a50a..2afced4d991 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -11,7 +11,7 @@
>  #include "run-command.h"
>  #include "refs.h"
>  #include "strvec.h"
> -
> +#include "list-objects-filter-options.h"
>  
>  static const char v2_bundle_signature[] = "# v2 git bundle\n";
>  static const char v3_bundle_signature[] = "# v3 git bundle\n";
> @@ -33,6 +33,8 @@ void bundle_header_release(struct bundle_header *header)
>  {
>  	string_list_clear(&header->prerequisites, 1);
>  	string_list_clear(&header->references, 1);
> +	list_objects_filter_release(header->filter);
> +	free(header->filter);
>  }
>  
>  static int parse_capability(struct bundle_header *header, const char *capability)
> @@ -45,6 +47,11 @@ static int parse_capability(struct bundle_header *header, const char *capability
>  		header->hash_algo = &hash_algos[algo];
>  		return 0;
>  	}
> +	if (skip_prefix(capability, "filter=", &arg)) {
> +		CALLOC_ARRAY(header->filter, 1);
> +		parse_list_objects_filter(header->filter, arg);
> +		return 0;
> +	}
>  	return error(_("unknown capability '%s'"), capability);
>  }
>  
> @@ -220,6 +227,8 @@ int verify_bundle(struct repository *r,
>  	req_nr = revs.pending.nr;
>  	setup_revisions(2, argv, &revs, NULL);
>  
> +	revs.filter = header->filter;
> +
>  	if (prepare_revision_walk(&revs))
>  		die(_("revision walk setup failed"));
>  
> @@ -259,6 +268,12 @@ int verify_bundle(struct repository *r,
>  			     r->nr),
>  			  r->nr);
>  		list_refs(r, 0, NULL);
> +
> +		if (header->filter) {
> +			printf_ln("The bundle uses this filter: %s",
> +				  list_objects_filter_spec(header->filter));
> +		}
> +
>  		r = &header->prerequisites;
>  		if (!r->nr) {
>  			printf_ln(_("The bundle records a complete history."));
> diff --git a/bundle.h b/bundle.h
> index 06009fe6b1f..eb026153d56 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -5,11 +5,14 @@
>  #include "cache.h"
>  #include "string-list.h"
>  
> +struct list_objects_filter_options;
> +

For the other ones we include the relevant header, do the same here (or if there's a need to not do it, do we need it for the rest too?)

Show 7 quoted lines

>  struct bundle_header {
>  	unsigned version;
>  	struct string_list prerequisites;
>  	struct string_list references;
>  	const struct git_hash_algo *hash_algo;
> +	struct list_objects_filter_options *filter;
>  };

I haven't tried, but any reason this needs to be a *filter v.s. embedding it in the struct?

Then we'd just need list_objects_filter_release() and not the free() as well.

Is it because you're piggy-backing on "if (header->filter)" as "do we have it" state, better to check .nr?

> @@ -55,7 +55,7 @@ const char *list_object_filter_config_name(enum list_objects_filter_choice c)
>   * expand_list_objects_filter_spec() first).  We also "intern" the arg for the
>   * convenience of the current command.
>   */

These API docs....

Show 12 quoted lines

> -static int gently_parse_list_objects_filter(
> +int gently_parse_list_objects_filter(
>  	struct list_objects_filter_options *filter_options,
>  	const char *arg,
>  	struct strbuf *errbuf)
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index da5b6737e27..347a99c28cf 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -72,6 +72,11 @@ struct list_objects_filter_options {
>  /* Normalized command line arguments */
>  #define CL_ARG__FILTER "filter"

...should be moved here, presumably.

Show 5 quoted lines

> +int gently_parse_list_objects_filter(
> +	struct list_objects_filter_options *filter_options,
> +	const char *arg,
> +	struct strbuf *errbuf);
> +