diff mbox series

[v3,1/8] refs/files: remove redundant check in split_symref_update()

Message ID 20250305-245-partially-atomic-ref-updates-v3-1-0c64e3052354@gmail.com (mailing list archive)
State New
Headers show
Series refs: introduce support for partial reference transactions | expand

Commit Message

Karthik Nayak March 5, 2025, 5:38 p.m. UTC 
In `split_symref_update()`, there were two checks for duplicate
refnames:

  - At the start, `string_list_has_string()` ensures the refname is not
    already in `affected_refnames`, preventing duplicates from being
    added.

  - After adding the refname, another check verifies whether the newly
    inserted item has a `util` value.

The second check is unnecessary because the first one guarantees that
`string_list_insert()` will never encounter a preexisting entry.

Since `item->util` is only used in this context, remove the assignment and
simplify the surrounding code.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

Comments

Junio C Hamano March 5, 2025, 9:20 p.m. UTC | #1 
Karthik Nayak <karthik.188@gmail.com> writes:

> In `split_symref_update()`, there were two checks for duplicate
> refnames:
>
>   - At the start, `string_list_has_string()` ensures the refname is not
>     already in `affected_refnames`, preventing duplicates from being
>     added.
>
>   - After adding the refname, another check verifies whether the newly
>     inserted item has a `util` value.
>
> The second check is unnecessary because the first one guarantees that
> `string_list_insert()` will never encounter a preexisting entry.
>
> Since `item->util` is only used in this context, remove the assignment and
> simplify the surrounding code.

It was a bit unclear what "this context" refers to.  We lost all
assignments to the .util member and that is a safe thing to do
because ...

> @@ -2843,13 +2835,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  		if (update->flags & REF_LOG_ONLY)
>  			continue;
>  
> -		item = string_list_append(&affected_refnames, update->refname);
> -		/*
> -		 * We store a pointer to update in item->util, but at
> -		 * the moment we never use the value of this field
> -		 * except to check whether it is non-NULL.
> -		 */
> -		item->util = update;

... of this comment, and the "except to check whether" used to
happen in this code ...

>  	 * be valid as long as affected_refnames is in use, and NOT
>  	 * referent, which might soon be freed by our caller.
>  	 */
> -	item = string_list_insert(affected_refnames, new_update->refname);
> -	if (item->util)
> -		BUG("%s unexpectedly found in affected_refnames",
> -		    new_update->refname);
> -	item->util = new_update;

... which the patch removed.

OK.  Makes perfect sense.

Thanks.
diff mbox series

Patch

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4e1c50fead..6c7df30738 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2382,7 +2382,6 @@  static int split_head_update(struct ref_update *update,
 			     struct string_list *affected_refnames,
 			     struct strbuf *err)
 {
-	struct string_list_item *item;
 	struct ref_update *new_update;
 
 	if ((update->flags & REF_LOG_ONLY) ||
@@ -2421,8 +2420,7 @@  static int split_head_update(struct ref_update *update,
 	 */
 	if (strcmp(new_update->refname, "HEAD"))
 		BUG("%s unexpectedly not 'HEAD'", new_update->refname);
-	item = string_list_insert(affected_refnames, new_update->refname);
-	item->util = new_update;
+	string_list_insert(affected_refnames, new_update->refname);
 
 	return 0;
 }
@@ -2441,7 +2439,6 @@  static int split_symref_update(struct ref_update *update,
 			       struct string_list *affected_refnames,
 			       struct strbuf *err)
 {
-	struct string_list_item *item;
 	struct ref_update *new_update;
 	unsigned int new_flags;
 
@@ -2496,11 +2493,7 @@  static int split_symref_update(struct ref_update *update,
 	 * be valid as long as affected_refnames is in use, and NOT
 	 * referent, which might soon be freed by our caller.
 	 */
-	item = string_list_insert(affected_refnames, new_update->refname);
-	if (item->util)
-		BUG("%s unexpectedly found in affected_refnames",
-		    new_update->refname);
-	item->util = new_update;
+	string_list_insert(affected_refnames, new_update->refname);
 
 	return 0;
 }
@@ -2834,7 +2827,6 @@  static int files_transaction_prepare(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
-		struct string_list_item *item;
 
 		if ((update->flags & REF_IS_PRUNING) &&
 		    !(update->flags & REF_NO_DEREF))
@@ -2843,13 +2835,7 @@  static int files_transaction_prepare(struct ref_store *ref_store,
 		if (update->flags & REF_LOG_ONLY)
 			continue;
 
-		item = string_list_append(&affected_refnames, update->refname);
-		/*
-		 * We store a pointer to update in item->util, but at
-		 * the moment we never use the value of this field
-		 * except to check whether it is non-NULL.
-		 */
-		item->util = update;
+		string_list_append(&affected_refnames, update->refname);
 	}
 	string_list_sort(&affected_refnames);
 	if (ref_update_reject_duplicates(&affected_refnames, err)) {