git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/6] refs: move duplicate refname update check to generic layer

From
Patrick Steinhardt <ps@pks.im>
Date
Feb 7, 2025, 16:12 UTC
Message-ID
<Z6Yw_f6PGJ2X_Iue@pks.im>
In-Reply-To
<20250207-245-partially-atomic-ref-updates-v1-2-e6a3690ff23a@gmail.com>
On Fri, Feb 07, 2025 at 08:34:37AM +0100, Karthik Nayak wrote:
> Move the tracking of refnames in `affected_refnames` from individual
> backends into the generic layer in 'refs.c'. This centralizes the
> duplicate refname detection that was previously handled separately by
> each backend.
Exciting, this has been on my TODO list for quite a while already.
Show 9 quoted lines
> Make some changes to accommodate this move:
> 
>   - Add a `string_list` field `refnames` to `ref_transaction` to contain
>     all the references in a transaction. This field is updated whenever
>     a new update is added.
> 
>   - Modify the backends to use this field internally as needed. The
>     backends need to check if an update for refname already exists when
>     splitting symrefs or adding an update for 'HEAD'.

Okay. Is this actually necessary to be handled by the backends? I would've expected that it is possible to split up symref updates so that we insert both symref and target into the list. I wouldn't be surprised if this wasn't easily possible though -- the logic here is surprisingly intricate.

Show 7 quoted lines
>   - In the reftable backend, in `reftable_be_transaction_prepare()`,
>     move the instance of `string_list_has_string()` above
>     `ref_transaction_add_update()` to check before the reference is
>     added.
> 
> This helps reduce duplication of functionality between the backends and
> makes it easier to make changes in a more centralized manner.
Show 8 quoted lines
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c                  | 17 ++++++++++++
>  refs/files-backend.c    | 69 ++++++++++---------------------------------------
>  refs/packed-backend.c   | 25 +-----------------
>  refs/refs-internal.h    |  2 ++
>  refs/reftable-backend.c | 53 ++++++++++++-------------------------
>  5 files changed, 50 insertions(+), 116 deletions(-)
Nice.
Show 9 quoted lines
> diff --git a/refs.c b/refs.c
> index f4094a326a9f88f979654b668cc9c3d27d83cb5d..4c9b706461977995be1d55e7667f7fb708fbbb76 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
>  	CALLOC_ARRAY(tr, 1);
>  	tr->ref_store = refs;
>  	tr->flags = flags;
> +	string_list_init_dup(&tr->refnames);

Do we actually have to duplicate strings? I would've expected that we keep strings alive via the `ref_update`s anyway during the transaction's lifetime.

It might also be interesting to check whether using a strset for this is more efficient. But that is certainly outside the scope of your patch series and can be done at a later point. #leftoverbit

Show 27 quoted lines
> @@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update(
>  		update->msg = normalize_reflog_message(msg);
>  	}
>  
> +	/*
> +	 * This list is generally used by the backends to avoid duplicates.
> +	 * But we do support multiple log updates for a given refname within
> +	 * a single transaction.
> +	 */
> +	if (!(update->flags & REF_LOG_ONLY)) {
> +		item = string_list_append(&transaction->refnames, refname);
> +		item->util = update;
> +	}
> +
>  	return update;
>  }
> @@ -2397,6 +2410,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
>  		return -1;
>  	}
>  
> +	string_list_sort(&transaction->refnames);
> +	if (ref_update_reject_duplicates(&transaction->refnames, err))
> +		return TRANSACTION_GENERIC_ERROR;
> +
>  	ret = refs->be->transaction_prepare(refs, transaction, err);
>  	if (ret)
>  		return ret;

Okay, we keep the list unserted initially, but sort it later before passing it to the backends so that `string_list_has_string()` works correctly. Good.

Show 12 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index c6a3f6d6261a894e1c294bb1329fdf8079a39eb4..18da30c3f37dc5c09f7d81a9083d6b41d0463bd5 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2425,7 +2423,6 @@ static int split_head_update(struct ref_update *update,
>  	 */
>  	if (strcmp(new_update->refname, "HEAD"))
>  		BUG("%s unexpectedly not 'HEAD'", new_update->refname);
> -	string_list_insert(affected_refnames, new_update->refname);
>  
>  	return 0;
>  }

Previously we would've inserted "HEAD" into the list of affected refnames even if it wasn't directly updated. Why don't we have to do that now anymore?

Show 16 quoted lines
> @@ -2441,7 +2438,6 @@ static int split_head_update(struct ref_update *update,
> @@ -2491,15 +2487,6 @@ static int split_symref_update(struct ref_update *update,
>  	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
>  	update->flags &= ~REF_HAVE_OLD;
>  
> -	/*
> -	 * Add the referent. This insertion is O(N) in the transaction
> -	 * size, but it happens at most once per symref in a
> -	 * transaction. Make sure to add new_update->refname, which will
> -	 * be valid as long as affected_refnames is in use, and NOT
> -	 * referent, which might soon be freed by our caller.
> -	 */
> -	string_list_insert(affected_refnames, new_update->refname);
> -
>  	return 0;
>  }
Same question here, but for symref updates.
Show 16 quoted lines
> @@ -3030,13 +2995,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  	if (transaction->state != REF_TRANSACTION_PREPARED)
>  		BUG("commit called for transaction that is not prepared");
>  
> -	/* Fail if a refname appears more than once in the transaction: */
> -	for (i = 0; i < transaction->nr; i++)
> -		if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
> -			string_list_append(&affected_refnames,
> -					   transaction->updates[i]->refname);
> -	string_list_sort(&affected_refnames);
> -	if (ref_update_reject_duplicates(&affected_refnames, err)) {
> +	string_list_sort(&transaction->refnames);
> +	if (ref_update_reject_duplicates(&transaction->refnames, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;
>  		goto cleanup;
>  	}
Can't we also make this check generic for initial transactions?
Show 40 quoted lines
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index a7b6f74b6e35f897f619c540cbc600bbd888bc67..6e7acb077e81435715a1ca3cc928550147c8c56a 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1653,34 +1648,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
>  	 */
>  
>  	CALLOC_ARRAY(data, 1);
> -	string_list_init_nodup(&data->updates);
>  
>  	transaction->backend_data = data;
>  
> -	/*
> -	 * Stick the updates in a string list by refname so that we
> -	 * can sort them:
> -	 */
> -	for (i = 0; i < transaction->nr; i++) {
> -		struct ref_update *update = transaction->updates[i];
> -		struct string_list_item *item =
> -			string_list_append(&data->updates, update->refname);
> -
> -		/* Store a pointer to update in item->util: */
> -		item->util = update;
> -	}
> -	string_list_sort(&data->updates);
> -
> -	if (ref_update_reject_duplicates(&data->updates, err))
> -		goto failure;
> -
>  	if (!is_lock_file_locked(&refs->lock)) {
>  		if (packed_refs_lock(ref_store, 0, err))
>  			goto failure;
>  		data->own_lock = 1;
>  	}
>  
> -	if (write_with_updates(refs, &data->updates, err))
> +	if (write_with_updates(refs, &transaction->refnames, err))
>  		goto failure;
>  
>  	transaction->state = REF_TRANSACTION_PREPARED;

This change is a lot more straight-forward because the packed backend does not support symrefs at all. Nice.

Show 22 quoted lines
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index d39a14c5a469d7d219362e9eae4f578784d65a5b..dd2099d94948a4f23fd9f7ddc06bf3d741229eba 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1202,12 +1184,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  				goto done;
>  			}
>  
> -			new_update = ref_transaction_add_update(
> -					transaction, "HEAD",
> -					u->flags | REF_LOG_ONLY | REF_NO_DEREF,
> -					&u->new_oid, &u->old_oid, NULL, NULL, NULL,
> -					u->msg);
> -			string_list_insert(&affected_refnames, new_update->refname);
> +			ref_transaction_add_update(
> +				transaction, "HEAD",
> +				u->flags | REF_LOG_ONLY | REF_NO_DEREF,
> +				&u->new_oid, &u->old_oid, NULL, NULL, NULL,
> +				u->msg);
>  		}
>  
>  		ret = reftable_backend_read_ref(be, rewritten_ref,
Equivalent question as for the files backend.
Show 16 quoted lines
> @@ -1277,6 +1258,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  				if (!strcmp(rewritten_ref, "HEAD"))
>  					new_flags |= REF_UPDATE_VIA_HEAD;
>  
> +				if (string_list_has_string(&transaction->refnames, referent.buf)) {
> +					strbuf_addf(err,
> +						    _("multiple updates for '%s' (including one "
> +						    "via symref '%s') are not allowed"),
> +						    referent.buf, u->refname);
> +					ret = TRANSACTION_NAME_CONFLICT;
> +					goto done;
> +				}
> +
>  				/*
>  				 * If we are updating a symref (eg. HEAD), we should also
>  				 * update the branch that the symref points to.

This change surprised me a bit. You mention it in the commit message, but don't state a reason why you do it.

Patrick
Previous: Karthik NayakNext: Karthik Nayak
Message 6 of 49 in “refs: introduce support for partial reference transactions”
  1. 0/6 refs: introduce support for partial reference transactionsKarthik Nayak, Feb 7, 2025
  2. 1/6 refs/files: remove duplicate check in `split_symref_update()`Karthik Nayak, Feb 7, 2025
  3. Patrick SteinhardtFeb 7, 2025
  4. Karthik NayakFeb 11, 2025
  5. 2/6 refs: move duplicate refname update check to generic layerKarthik Nayak, Feb 7, 2025
  6. Patrick SteinhardtFeb 7, 2025
  7. Karthik NayakFeb 11, 2025
  8. 3/6 refs/files: remove duplicate duplicates checkKarthik Nayak, Feb 7, 2025
  9. Patrick SteinhardtFeb 7, 2025
  10. 4/6 refs/reftable: extract code from the transaction preparationKarthik Nayak, Feb 7, 2025
  11. 5/6 refs: implement partial reference transaction supportKarthik Nayak, Feb 7, 2025
  12. Patrick SteinhardtFeb 7, 2025
  13. Karthik NayakFeb 21, 2025
  14. 6/6 update-ref: add --allow-partial flag for stdin modeKarthik Nayak, Feb 7, 2025
  15. Patrick SteinhardtFeb 7, 2025
  16. Karthik NayakFeb 21, 2025
  17. Phillip WoodFeb 11, 2025
  18. Phillip WoodFeb 11, 2025
  19. Karthik NayakFeb 12, 2025
  20. Karthik NayakFeb 12, 2025
  21. Phillip WoodFeb 19, 2025
  22. Patrick SteinhardtFeb 19, 2025
  23. Karthik NayakFeb 21, 2025
  24. 0/8 refs: introduce support for batched reference updatesKarthik Nayak, Mar 27, 2025
  25. 1/8 refs/files: remove redundant check in split_symref_update()Karthik Nayak, Mar 27, 2025
  26. 2/8 refs: move duplicate refname update check to generic layerKarthik Nayak, Mar 27, 2025
  27. 3/8 refs/files: remove duplicate duplicates checkKarthik Nayak, Mar 27, 2025
  28. 4/8 refs/reftable: extract code from the transaction preparationKarthik Nayak, Mar 27, 2025
  29. 5/8 refs: introduce enum-based transaction error typesKarthik Nayak, Mar 27, 2025
  30. 6/8 refs: implement batch reference update supportKarthik Nayak, Mar 27, 2025
  31. 7/8 refs: support rejection in batch updates during F/D checksKarthik Nayak, Mar 27, 2025
  32. 8/8 update-ref: add --batch-updates flag for stdin modeKarthik Nayak, Mar 27, 2025
  33. Jean-Noël AVILAMar 28, 2025
  34. Junio C HamanoMar 29, 2025
  35. Karthik NayakMar 29, 2025
  36. Patrick SteinhardtMar 28, 2025
  37. 0/8 refs: introduce support for batched reference updatesKarthik Nayak, Apr 8, 2025
  38. 1/8 refs/files: remove redundant check in split_symref_update()Karthik Nayak, Apr 8, 2025
  39. 2/8 refs: move duplicate refname update check to generic layerKarthik Nayak, Apr 8, 2025
  40. 3/8 refs/files: remove duplicate duplicates checkKarthik Nayak, Apr 8, 2025
  41. 4/8 refs/reftable: extract code from the transaction preparationKarthik Nayak, Apr 8, 2025
  42. 5/8 refs: introduce enum-based transaction error typesKarthik Nayak, Apr 8, 2025
  43. 6/8 refs: implement batch reference update supportKarthik Nayak, Apr 8, 2025
  44. 7/8 refs: support rejection in batch updates during F/D checksKarthik Nayak, Apr 8, 2025
  45. 8/8 update-ref: add --batch-updates flag for stdin modeKarthik Nayak, Apr 8, 2025
  46. Junio C HamanoApr 8, 2025
  47. Karthik NayakApr 8, 2025
  48. Junio C HamanoApr 8, 2025
  49. Karthik NayakApr 10, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.