diff mbox series

[1/2] fetch: allow passing a transaction to `s_update_ref()`

Message ID e627e729e5cd14c94dcf819f4f87b1412b9a9e9b.1610027375.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series fetch: implement support for atomic reference updates | expand

Commit Message

Patrick Steinhardt Jan. 7, 2021, 1:51 p.m. UTC
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(-)

Comments

Junio C Hamano Jan. 7, 2021, 10:59 p.m. UTC | #1
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?
Christian Couder Jan. 8, 2021, 12:45 a.m. UTC | #2
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/
Junio C Hamano Jan. 8, 2021, 7:18 a.m. UTC | #3
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 mbox series

Patch

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, &current->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, &current->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(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, what, rm, &note,
-						       summary_width);
+				rc |= update_local_ref(ref, NULL, what,
+						       rm, &note, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*