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

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <derrickstolee@github.com> writes:

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

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