Re: [PATCH v3 05/11] update-index: use the bulk-checkin infrastructure

[Neeraj Singh <nksingh85@gmail.com>]

On Thu, Mar 24, 2022 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +static void end_odb_transaction_if_active(void)
> > +{
> > +     if (!odb_transaction_active)
> > +             return;
> > +
> > +     end_odb_transaction();
> > +     odb_transaction_active = 0;
> > +}
>
> >  __attribute__((format (printf, 1, 2)))
> >  static void report(const char *fmt, ...)
> >  {
> > @@ -57,6 +68,16 @@ static void report(const char *fmt, ...)
> >       if (!verbose)
> >               return;
> >
> > +     /*
> > +      * It is possible, though unlikely, that a caller
> > +      * could use the verbose output to synchronize with
> > +      * addition of objects to the object database, so
> > +      * unplug bulk checkin to make sure that future objects
> > +      * are immediately visible.
> > +      */
> > +
> > +     end_odb_transaction_if_active();
> > +
> >       va_start(vp, fmt);
> >       vprintf(fmt, vp);
> >       putchar('\n');
> > @@ -1116,6 +1137,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >        */
> >       parse_options_start(&ctx, argc, argv, prefix,
> >                           options, PARSE_OPT_STOP_AT_NON_OPTION);
> > +
> > +     /*
> > +      * Allow the object layer to optimize adding multiple objects in
> > +      * a batch.
> > +      */
> > +     begin_odb_transaction();
> > +     odb_transaction_active = 1;
>
> This looks strange.  Shouldn't begin/end pair be responsible for
> knowing if there is a transaction active already?  For that matter,
> didn't the original unplug in plug/unplug pair automatically turned
> into no-op when it is already unplugged?
>
> IOW, I am not sure end_if_active() should exist in the first place.
> Shouldn't end_transaction() do that instead?
>

Today there's an "assert(bulk_checkin_plugged)" in
end_odb_transaction. In principle we could just drop the assert and
allow a transaction to be ended multiple times.  But maybe in the long
run for composability we'd like to have nested callers to begin/end
transaction (e.g. we could have a nested transaction around writing
the cache tree to the ODB to minimize fsyncs there).  In that world,
having a subsystem not maintain a balanced pairing could be a problem.
An alternative API here could be to have an "flush_odb_transaction"
call to make the objects visible at this point.  Lastly, I could take
your original suggested approach of adding a new flag to update-index.
I preferred the unplug-on-verbose approach since it would
automatically optimize most callers to update-index that might exist
in the wild, without users having to change anything.

Thanks,
Neeraj