Re: [PATCH 02/11] revision: put object filter into struct rev_info

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2022, 16:46 UTC
Message-ID: <xmqqbkyhvhfo.fsf@gitster.g>
In-Reply-To: <d00617ba-01e8-1a3a-3cf1-49324e604f01@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 9 quoted lines

>> member so we need an on-demand allocation.  CALLOC_ARRAY() instead
>> of xcalloc(), when we know we are creating one element and not an
>> array of elements whose size happens to be one, is not wrong but it
>> does look strange.  Was there a reason why we avoid xcalloc()?
>
> I think I've been using CALLOC_ARRAY(..., 1) over "... = xcalloc()"
> for simplicity's sake for a while. I see quite a few across the
> codebase, too, but I can swap the usage here if you feel that is
> important.

Not at all. I think it was just me who was confused; CALLOC_ARRAY() vs xcalloc() is not confusing. Both are capable of and meant for allocating an array of these elements, and use of it for a single element non-array is the same either way. Nothing to complain about.

Show 9 quoted lines

>> they may or may not be prepared to take.  This "we get rid of a
>> global struct and replace it with an on-demand allocated structure,
>> pointer to which is stored in the revs structure" rewrite somehow
>> makes me nervous.
>
> I think the main idea is that the filter being NULL indicates "no
> filter is used. Do a full object walk." If we use a static struct,
> then we need to instead use revs->filter.filter_spec.nr, but that
> is already being used as a BUG() statement:

Thanks. My observation was primarily that it looked deviating from the original code but that is not an objection unless the original was without room for improvement. And in fact in this case, I think the global variable that was a static struct should have been a global variable that is a pointer to a struct which is NULL unless the filtering capability is being used. So in that sense, the conversion done in this series is an improvement and going in the right direction.

> While we _could_ make that switch to using a static struct and
> change our checks to allow empty specs, that would be a more
> involved change. Maybe we can leave it for a follow up?

No, there is no need to. A pointer that is NULL when unused is the right thing to do.

Thanks.