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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 08 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
> [...]
>  static const char v2_bundle_signature[] = "# v2 git bundle\n";
>  static const char v3_bundle_signature[] = "# v3 git bundle\n";
> @@ -33,6 +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);
>  }
>  
>  static int parse_capability(struct bundle_header *header, const char *capability)
> @@ -45,6 +46,10 @@ 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)) {
> +		parse_list_objects_filter(&header->filter, arg);
> +		return 0;
> +	}
>  	return error(_("unknown capability '%s'"), capability);
>  }

I haven't tested, but I did wonder if our purely "check reachability"
(or equivalent) "verify" would be slowed down by doing whatever filter
magic we're doing here, but then remembered/saw that we only parse the
header, so it can't be that bad :)

I.e. this is only checking the syntax of the filter, surely, and then
spits it back at us. That makes sense.

I think that this hunk from the subsequent 10/12 is in the wrong place
though, and should be here when we change "verify" (not "create" later):
	
	diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
	index 72ab8139052..831c4788a94 100644
	--- a/Documentation/git-bundle.txt
	+++ b/Documentation/git-bundle.txt
	@@ -75,8 +75,8 @@ verify <file>::
	 	cleanly to the current repository.  This includes checks on the
	 	bundle format itself as well as checking that the prerequisite
	 	commits exist and are fully linked in the current repository.
	-	'git bundle' prints a list of missing commits, if any, and exits
	-	with a non-zero status.
	+	'git bundle' prints the bundle's object filter and its list of
	+	missing commits, if any, and exits with a non-zero status.
	 
	 list-heads <file>::
	 	Lists the references defined in the bundle.  If followed by a

I think instead of starting to list every header we might add in the
future there it would make sense to just add after the pre-image
full-stop something like:

    We'll also print out information about any known capabilities, such
    as "object filter". See "Capabilities" in technical/...

Which future proofs it a bit...

> [...]

I think it would also be great to have tests for intentionally
corrupting the header and seeing what the "verify" output is, i.e. do we
die right away, proceed to validate the rest. So just (somewhat
pseudocode):

    git bundle create [...] my.bdl &&
    sed 's/@filter: .*/@filter: bad/' <my.bdl >bad.bdl &&
    test_must_fail git bundle verify bad.bdl 2>actual &&
    [...]
    test_cmp expect actual

Overall this series looks really good at this point, and I'm down to
minor shades of colors on the bikeshedding :)