On 3/7/2022 11:22 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Mar 07 2022, Derrick Stolee wrote:
>
>> On 3/7/2022 10:38 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Wed, Feb 23 2022, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>> ...
>>>> 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?)
>>
>> The others are .c files that require looking into the struct. This
>> declaration is all that's required for this header file.
>>
>>>> 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?
>>
>> Yes. I replied to Junio before that there is some assumption in
>> the filtering code that the .nr == 0 case is listed as a BUG()
>> so we would possibly be breaking expectations in a different
>> way doing the embedded version.
>
> Having an "unsigned int using_filter:1" or whatever IMO makes that much
> clearer than needing to carefully eyeball code that's already using
> embedded structs & see why the one exception that's malloc'd is because
> of that or some other reason...
I think your recommended "using_filter" is messy. Having this
struct be a pointer instead of embedded self-documents that it
is optional (and can be NULL) but that if it is non-NULL, then
it should be considered and valid.
Here, I'm focusing on not allowing a non-sensical state, such
as using_filter = 0 but filter is actually populated with a
valid filter. The possibility of this state means there is a
higher chance of introducing a bug over time by not keeping
these values coupled.
Thanks,
-Stolee
