| 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 |
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.
Junio C Hamano <gitster@pobox.com> writes: > 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 ... > Definitely could use some clarification. Will change to: The `item->util` field is assigned to validate that a rename doesn't already exist in the list. The validation is done after the first check. As this check is removed, clean up the validation and the assignment of this field. >> @@ -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. Thanks!
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)) {
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(-)