| Message ID | e627e729e5cd14c94dcf819f4f87b1412b9a9e9b.1610027375.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: implement support for atomic reference updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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; > @@ -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?
On Fri, Jan 8, 2021 at 12:04 AM Junio C Hamano <gitster@pobox.com> wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > 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; > > > @@ -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"? 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). > 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? "our_transaction" looks fine. Here is a suggested refactoring patch that could come before Patrick's patch: ------------------------------------------------------------------------- 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, + struct ref *ref, + int check_old, + struct strbuf *err, + char *msg) +{ + if (!transaction || + ref_transaction_update(transaction, ref->name, + &ref->new_oid, + check_old ? &ref->old_oid : NULL, + 0, msg, err)) + return TRANSACTION_GENERIC_ERROR; + + return ref_transaction_commit(transaction, err); +} + static int s_update_ref(const char *action, struct ref *ref, int check_old) @@ -589,7 +605,7 @@ static int s_update_ref(const char *action, char *rla = getenv("GIT_REFLOG_ACTION"); struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - int ret, df_conflict = 0; + int ret = 0; if (dry_run) return 0; @@ -598,30 +614,19 @@ static int s_update_ref(const char *action, msg = xstrfmt("%s: %s", rla, action); transaction = ref_transaction_begin(&err); - if (!transaction || - ref_transaction_update(transaction, ref->name, - &ref->new_oid, - check_old ? &ref->old_oid : NULL, - 0, msg, &err)) - goto fail; - ret = ref_transaction_commit(transaction, &err); + ret = do_s_update_ref(transaction, ref, check_old, &err, msg); if (ret) { - df_conflict = (ret == TRANSACTION_NAME_CONFLICT); - goto fail; + error("%s", err.buf); + ret = (ret == TRANSACTION_NAME_CONFLICT) + ? STORE_REF_ERROR_DF_CONFLICT + : STORE_REF_ERROR_OTHER; } ref_transaction_free(transaction); strbuf_release(&err); free(msg); - return 0; -fail: - ref_transaction_free(transaction); - error("%s", err.buf); - strbuf_release(&err); - free(msg); - return df_conflict ? STORE_REF_ERROR_DF_CONFLICT - : STORE_REF_ERROR_OTHER; + return ret; } static int refcol_width = 10; ------------------------------------------------------------------------- After the above patch, Patrick's patch would become: ------------------------------------------------------------------------- diff --git a/builtin/fetch.c b/builtin/fetch.c index 8017fc19da..d58805c03d 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -584,6 +584,7 @@ static struct ref *get_ref_map(struct remote *remote, static int do_s_update_ref(struct ref_transaction *transaction, struct ref *ref, int check_old, + int commit, struct strbuf *err, char *msg) { @@ -594,16 +595,17 @@ static int do_s_update_ref(struct ref_transaction *transaction, 0, msg, err)) return TRANSACTION_GENERIC_ERROR; - return ref_transaction_commit(transaction, err); + return (commit) ? ref_transaction_commit(transaction, err) : 0; } static int s_update_ref(const char *action, + struct ref_transaction *transaction, struct ref *ref, int check_old) { char *msg; char *rla = getenv("GIT_REFLOG_ACTION"); - struct ref_transaction *transaction; + struct ref_transaction *our_transaction = NULL; struct strbuf err = STRBUF_INIT; int ret = 0; @@ -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); - 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/
Christian Couder <christian.couder@gmail.com> 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). > 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 ... > @@ -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. > - 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/
diff --git a/builtin/fetch.c b/builtin/fetch.c index ecf8537605..020a977bc7 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -583,13 +583,14 @@ static struct ref *get_ref_map(struct remote *remote, 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; if (dry_run) return 0; @@ -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; + } + + if (ref_transaction_update(transaction, ref->name, &ref->new_oid, check_old ? &ref->old_oid : NULL, 0, msg, &err)) goto fail; - ret = ref_transaction_commit(transaction, &err); - if (ret) { - df_conflict = (ret == TRANSACTION_NAME_CONFLICT); - goto fail; + if (commit) { + ret = ref_transaction_commit(transaction, &err); + if (ret) { + df_conflict = (ret == TRANSACTION_NAME_CONFLICT); + goto fail; + } } - ref_transaction_free(transaction); + ref_transaction_free(transaction_to_free); strbuf_release(&err); free(msg); return 0; fail: - ref_transaction_free(transaction); + ref_transaction_free(transaction_to_free); error("%s", err.buf); strbuf_release(&err); free(msg); @@ -759,6 +772,7 @@ static void format_display(struct strbuf *display, char code, } static int update_local_ref(struct ref *ref, + struct ref_transaction *transaction, const char *remote, const struct ref *remote_ref, struct strbuf *display, @@ -799,7 +813,7 @@ static int update_local_ref(struct ref *ref, starts_with(ref->name, "refs/tags/")) { if (force || ref->force) { int r; - r = s_update_ref("updating tag", ref, 0); + r = s_update_ref("updating tag", ref, transaction, 0); format_display(display, r ? '!' : 't', _("[tag update]"), r ? _("unable to update local ref") : NULL, remote, pretty_ref, summary_width); @@ -836,7 +850,7 @@ static int update_local_ref(struct ref *ref, what = _("[new ref]"); } - r = s_update_ref(msg, ref, 0); + r = s_update_ref(msg, ref, transaction, 0); format_display(display, r ? '!' : '*', what, r ? _("unable to update local ref") : NULL, remote, pretty_ref, summary_width); @@ -858,7 +872,7 @@ static int update_local_ref(struct ref *ref, strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV); strbuf_addstr(&quickref, ".."); strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV); - r = s_update_ref("fast-forward", ref, 1); + r = s_update_ref("fast-forward", ref, transaction, 1); format_display(display, r ? '!' : ' ', quickref.buf, r ? _("unable to update local ref") : NULL, remote, pretty_ref, summary_width); @@ -870,7 +884,7 @@ static int update_local_ref(struct ref *ref, strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV); strbuf_addstr(&quickref, "..."); strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV); - r = s_update_ref("forced-update", ref, 1); + r = s_update_ref("forced-update", ref, transaction, 1); format_display(display, r ? '!' : '+', quickref.buf, r ? _("unable to update local ref") : _("forced update"), remote, pretty_ref, summary_width); @@ -1034,8 +1048,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_reset(¬e); if (ref) { - rc |= update_local_ref(ref, what, rm, ¬e, - summary_width); + rc |= update_local_ref(ref, NULL, what, + rm, ¬e, summary_width); free(ref); } else if (write_fetch_head || dry_run) { /*
The handling of ref updates is completely handled by `s_update_ref()`, which will manage the complete lifecycle of the reference transaction. This is fine right now given that git-fetch(1) does not support atomic fetches, so each reference gets its own transaction. It is quite inflexible though, as `s_update_ref()` only knows about a single reference update at a time, so it doesn't allow us to alter the strategy. This commit prepares `s_update_ref()` and its only caller `update_local_ref()` to allow passing an external transaction. If none is given, then the existing behaviour is triggered which creates a new transaction and directly commits it. Otherwise, if the caller provides a transaction, then we only queue the update but don't commit it. This optionally allows the caller to manage when a transaction will be committed. Given that `update_local_ref()` is always called with a `NULL` transaction for now, no change in behaviour is expected from this change. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 48 +++++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 17 deletions(-)