Re: [PATCH v2 08/12] bundle: parse filter capability

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 8, 2022, 09:25 UTC
Message-ID: <220308.86lexkyelv.gmgdl@evledraar.gmail.com>
In-Reply-To: <898a7d945131042c48f8e99acccf26378a4c8586.1646689840.git.gitgitgadget@gmail.com>

On Mon, Mar 07 2022, Derrick Stolee via GitGitGadget wrote:

Show 50 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'.
>
> Since we will use gently_parse_list_objects_filter() outside of
> list-objects-filter-options.c, make it an external method and move its
> API documentation to before its declaration.
> [...]
> --- 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);
>  }
>

Re the comment I had on the v1 about embedding this data in the struct instead: https://lore.kernel.org/git/220307.86y21lycne.gmgdl@evledraar.gmail.com/

The below diff passes all your tests, i.e. re using NULL as a marker I think you may have missed that the API already has a LOFC_DISABLED for this (and grepping reveals similar API use of it).

I'm not 100% sure it's correct, but if it isn't that's also going to suggest missing test coverage in this series.

In any case you want the BUNDLE_HEADER_INIT change, your version is buggy in making that header use NODUP strings by hardcoding { 0 }.

diff --git a/builtin/clone.c b/builtin/clone.c
index 52e50f17baf..000379eea7f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1172,9 +1172,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	transport->cloning = 1;
 
 	if (is_bundle) {
-		struct bundle_header header = { 0 };
+		struct bundle_header header = BUNDLE_HEADER_INIT;
 		int fd = read_bundle_header(path, &header);
-		int has_filter = !!header.filter;
+		int has_filter = header.filter.choice != LOFC_DISABLED;
 
 		if (fd > 0)
 			close(fd);
diff --git a/bundle.c b/bundle.c
index 9ca6a7eb1c2..3846108f7a6 100644
--- a/bundle.c
+++ b/bundle.c
@@ -33,8 +33,7 @@ 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);
+	list_objects_filter_release(&header->filter);
 }
 
 static int parse_capability(struct bundle_header *header, const char *capability)
@@ -48,8 +47,7 @@ static int parse_capability(struct bundle_header *header, const char *capability
 		return 0;
 	}
 	if (skip_prefix(capability, "filter=", &arg)) {
-		CALLOC_ARRAY(header->filter, 1);
-		parse_list_objects_filter(header->filter, arg);
+		parse_list_objects_filter(&header->filter, arg);
 		return 0;
 	}
 	return error(_("unknown capability '%s'"), capability);
@@ -227,7 +225,7 @@ int verify_bundle(struct repository *r,
 	req_nr = revs.pending.nr;
 	setup_revisions(2, argv, &revs, NULL);
 
-	revs.filter = header->filter;
+	revs.filter = &header->filter;
 
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
@@ -269,9 +267,9 @@ int verify_bundle(struct repository *r,
 			  r->nr);
 		list_refs(r, 0, NULL);
 
-		if (header->filter) {
+		if (header->filter.choice != LOFC_DISABLED) {
 			printf_ln("The bundle uses this filter: %s",
-				  list_objects_filter_spec(header->filter));
+				  list_objects_filter_spec(&header->filter));
 		}
 
 		r = &header->prerequisites;
@@ -631,7 +629,7 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
 	/* If there is a filter, then we need to create the promisor pack. */
-	if (header->filter)
+	if (header->filter.choice != LOFC_DISABLED)
 		strvec_push(&ip.args, "--promisor=from-bundle");
 
 	if (extra_index_pack_args) {
diff --git a/bundle.h b/bundle.h
index eb026153d56..7fef2108f43 100644
--- a/bundle.h
+++ b/bundle.h
@@ -4,15 +4,14 @@
 #include "strvec.h"
 #include "cache.h"
 #include "string-list.h"
-
-struct list_objects_filter_options;
+#include "list-objects-filter-options.h"
 
 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;
+	struct list_objects_filter_options filter;
 };
 
 #define BUNDLE_HEADER_INIT \