Re: [PATCH v5 01/14] bulk-checkin: rename 'state' variable and separate 'plugged' boolean

From: Neeraj Singh <nksingh85@gmail.com>
Date: Mar 31, 2022, 04:17 UTC
Message-ID: <CANQDOdcDY-8TZzCHx+tWZJoD0rsULnfaWRhAOox3drSgxW_+ow@mail.gmail.com>
In-Reply-To: <xmqqr16j6ve3.fsf@gitster.g>

On Wed, Mar 30, 2022 at 1:24 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

>
> Neeraj Singh <nksingh85@gmail.com> writes:
>
> >> We should add a new function, flush_bulk_checking_packfile(), to
> >> flush only the packfile part of the bulk_checkin_state without
> >> affecting other things---the "plugged" bit is the only one in the
> >> current code before this series, but it does not have to stay to be
> >> so
> >
> > I'm happy to rename the packfile related stuff to end with _packfile
> > to make it clear that all of that state and functionality is related
> > to batching of packfile additions.
>
> I do not care about names, though.  If you took that I hinted any
> such change, sorry about that.  _state is fine as-is.
>
> I do care about not ejecting plugged out of the structure and
> instead keeping them together, with proper way to flush the part
> that deflate_to_pack() wants to flush, instead of abusing the
> "finish".
>
> Thanks.

Just to understand your feedback better, is it a problem to separate the state of each separate "thing" under ODB transactions into separate file-scope global(s)? In this series I declared the fsync state as completely separate from the packfile state. That's why I was thinking of it as more of a naming problem, since the remaining state aside from the plugged boolean is entirely packfile related.

My argument in favor of having separate file-scoped variables for each 'pluggable thing' would be that future implementations can evolve separately without authors first having to disentangle a single struct.

Thanks, Neeraj