Re: [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`
From: Junio C Hamano <hidden>
Date: 2021-01-08 07:19:19
Christian Couder [off-list ref] writes:
Yeah, I think we also don't need the df_conflict variable, and I don't
like the duplication of the following clean up code:
ref_transaction_free(transaction_to_free);
strbuf_release(&err);
free(msg);
Patrick's patch didn't introduce them, but we might still want to get
rid of them (see below).quoted hunk
diff --git a/builtin/fetch.c b/builtin/fetch.c index ecf8537605..8017fc19da 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -581,6 +581,22 @@ static struct ref *get_ref_map(struct remote *remote,#define STORE_REF_ERROR_OTHER 1 #define STORE_REF_ERROR_DF_CONFLICT 2 +static int do_s_update_ref(struct ref_transaction *transaction, ... } static int refcol_width = 10; ------------------------------------------------------------------------- After the above patch, Patrick's patch would become:
OK, I think the above as a preliminary clean-up would not hurt, but the "if the caller gave us NULL as the transaction, we are responsible for handling that bogosity" bit feels a bit too magical to my taste. I do understand that your intention is that it releaves the caller ...
quoted hunk
@@ -613,9 +615,16 @@ static int s_update_ref(const char *action, rla = default_rla.buf; msg = xstrfmt("%s: %s", rla, action); - transaction = ref_transaction_begin(&err); + /* + * 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 = our_transaction = ref_transaction_begin(&err);
... here from having to deal with NULL from ref_transaction_begin(), but I somehow do not see it as a code structure with a good taste. I dunno. Without that "if (!transaction ||" bit, I think your do_s_update_ref() is a good clean-up, though. Thanks.
quoted hunk
- ret = do_s_update_ref(transaction, ref, check_old, &err, msg); + ret = do_s_update_ref(transaction, ref, check_old, !!our_transaction, + &err, msg); if (ret) { error("%s", err.buf); ret = (ret == TRANSACTION_NAME_CONFLICT)@@ -623,7 +632,7 @@ static int s_update_ref(const char *action, : STORE_REF_ERROR_OTHER; } - ref_transaction_free(transaction); + ref_transaction_free(our_transaction); strbuf_release(&err); free(msg); return ret;... ------------------------------------------------------------------------- You can find these patches, with Patrick as the author, there: https://gitlab.com/gitlab-org/gitlab-git/-/commits/cc-improve-s-update-ref/