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

[Derrick Stolee <derrickstolee@github.com>]

On 3/8/2022 4:25 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 07 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> 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 did miss this LOFC_DISABLED use, which must be the correct way to
interpret an "empty" filter set (re: earlier concerns that a .nr == 0
was used as a BUG() statement in some places).

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

Thanks for pointing this out.

Thanks,
-Stolee