On Thu, Mar 24, 2022 at 11:18 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
