| Message ID | 20250207-245-partially-atomic-ref-updates-v1-1-e6a3690ff23a@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | refs: introduce support for partial reference transactions | expand |
On Fri, Feb 07, 2025 at 08:34:36AM +0100, Karthik Nayak wrote: > In split_symref_update(), there were two redundant checks: > - At the start: checking if refname exists in `affected_refnames`. > - After adding refname: checking if the item added to > `affected_refnames` contains the util field. Okay, it took me a bit longer to understand what's going on here. What you're saying is that we already use `string_list_has_string()` at the start of `split_symref_update()`, and if that returns true then we would bail out. Consequently, it is impossible for `string_list_insert()` to find a preexisting values. Makes sense, but I think that could be explained a bit better. > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 29f08dced40418eb815072c6335e0c3d1a45c7d8..c6a3f6d6261a894e1c294bb1329fdf8079a39eb4 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2846,13 +2838,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); Nice to see this and other code removed. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Feb 07, 2025 at 08:34:36AM +0100, Karthik Nayak wrote: >> In split_symref_update(), there were two redundant checks: >> - At the start: checking if refname exists in `affected_refnames`. >> - After adding refname: checking if the item added to >> `affected_refnames` contains the util field. > > Okay, it took me a bit longer to understand what's going on here. What > you're saying is that we already use `string_list_has_string()` at the > start of `split_symref_update()`, and if that returns true then we would > bail out. Consequently, it is impossible for `string_list_insert()` to > find a preexisting values. > > Makes sense, but I think that could be explained a bit better. > That's correct. I'll rewrite it to make it clearer. Thanks. >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 29f08dced40418eb815072c6335e0c3d1a45c7d8..c6a3f6d6261a894e1c294bb1329fdf8079a39eb4 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -2846,13 +2838,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); > > > Nice to see this and other code removed. > > Patrick
diff --git a/refs/files-backend.c b/refs/files-backend.c index 29f08dced40418eb815072c6335e0c3d1a45c7d8..c6a3f6d6261a894e1c294bb1329fdf8079a39eb4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2387,7 +2387,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) || @@ -2426,8 +2425,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; } @@ -2446,7 +2444,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; @@ -2501,11 +2498,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; } @@ -2837,7 +2830,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)) @@ -2846,13 +2838,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 redundant checks: - At the start: checking if refname exists in `affected_refnames`. - After adding refname: checking if the item added to `affected_refnames` contains the util field. Remove the second check since the first one already prevents duplicate refnames from being added to the transaction updates. Since this is the only place that utilizes the `item->util` value, avoid setting the value in the first place and cleanup code around it. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- refs/files-backend.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)