Re: [PATCH v3 05/11] update-index: use the bulk-checkin infrastructure
- From
- Neeraj Singh <nksingh85@gmail.com>
- Date
- Mar 24, 2022, 20:25 UTC
- Message-ID
- <CANQDOdcuBRvWx7iMYBvLYEEb6A_=SURLAGumk026ZyDODpfAsQ@mail.gmail.com>
- In-Reply-To
- <xmqqfsn7i594.fsf@gitster.g>
On Thu, Mar 24, 2022 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 52 quoted lines
>
> "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