Re: [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 7, 2021, 22:59 UTC
- Message-ID
- <xmqqtursjqva.fsf@gitster.c.googlers.com>
- In-Reply-To
- <e627e729e5cd14c94dcf819f4f87b1412b9a9e9b.1610027375.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 12 quoted lines
> static int s_update_ref(const char *action,
> struct ref *ref,
> + struct ref_transaction *transaction,
> int check_old)
> {
> char *msg;
> char *rla = getenv("GIT_REFLOG_ACTION");
> - struct ref_transaction *transaction;
> + struct ref_transaction *transaction_to_free = NULL;
> struct strbuf err = STRBUF_INIT;
> - int ret, df_conflict = 0;
> + int ret, df_conflict = 0, commit = 0;Show 18 quoted lines
> @@ -597,26 +598,38 @@ static int s_update_ref(const char *action,
> rla = default_rla.buf;
> msg = xstrfmt("%s: %s", rla, action);
>
> - transaction = ref_transaction_begin(&err);
> - if (!transaction ||
> - ref_transaction_update(transaction, ref->name,
> + /*
> + * If no transaction was passed to us, we manage the transaction
> + * ourselves. Otherwise, we trust the caller to handle the transaction
> + * lifecycle.
> + */
> + if (!transaction) {
> + transaction = transaction_to_free = ref_transaction_begin(&err);
> + if (!transaction)
> + goto fail;
> + commit = 1;
> + }The idea is that we still allow the caller to pass NULL in which case we begin and commit a transaction ourselves, but if the caller is to pass an already initialized transaction to us, we obviously do not have to "begin" but also we won't commit.
Which makes sense, but then do we still need the "commit" variable that reminds us "we are responsible for finishing the transaction we started"?
If we rename "transaction_to_free" to a name that makes it more clear that it is a transaction that we created and that we are responsible for seeing through to its end (after all, "to-free" captures only one half of that, i.e. before returning, we are responsible for calling ref_transation_free() on it), then we do not need such an extra variable that can go out of sync by mistake, no? The block that protects the call to ref_transaction_commit() can just check if the transaction_to_free variable (with a better name) is non-NULL, instead of looking at the commit variable.
Just my attempt to come up with an alternative name for transaction_to_free:
- "our_transaction" because it is ours?
- "auto_transaction" because it is automatically started and committed without bothering the caller?