| Message ID | 20250207-245-partially-atomic-ref-updates-v1-2-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: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. > 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. > - 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. > 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. > 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 > @@ -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. > 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? > @@ -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. > @@ -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? > 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. > 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. > @@ -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
Patrick Steinhardt <ps@pks.im> writes: > 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. > Yeah, I saw that you left a TODO in the reftable backend too. This change was not really needed for partial transactions. But it does make things a bit nicer and easier. >> 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. It is a bit intricate and requires a bit of unwinding to move it to the generic layer. But it is possible, I tried to timebox it for this patch series, but unfortunately it needs a lot more time. So perhaps, something for later. > >> - 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. > >> 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. > >> 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. > True. I was more thinking along the lines of the keeping the memory concerns separate. Also, I sure if there are any scenario's that a `ref_transaction` could outlive a `ref_update`. > 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 > Agreed. >> @@ -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. > >> 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? > We still do, right above this code, there is a call to `ref_transaction_add_update()`. So any ref_update added to the transaction via `ref_transaction_add_update()` will also add the refname to `transaction->refnames`. >> @@ -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. > Same as above. In summary, the need for adding new refnames to the list is now centralized and a part of adding a ref update to the transaction. It's a good question, so I'll also add a hint in the commit message. > >> @@ -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? > This one is handled in the next commit, I mostly separated them out because I was not sure why this needs to be here and to draw attention if I'm missing something when removing this. >> 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. > Yes indeed. >> 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. > I hope my answer earlier helps, especially since the diff here shows the call to `ref_transaction_add_update()`. >> @@ -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. > When a `ref_update` is added to the `transaction` via `ref_transaction_add_update()`, the refname is automatically added to `transaction->refnames`. As a result, checking for the refname in this list will always return true. I'll clarify this in the commit message. > Patrick
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); return tr; } @@ -1205,6 +1206,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } + string_list_clear(&transaction->refnames, 0); free(transaction->updates); free(transaction); } @@ -1218,6 +1220,7 @@ struct ref_update *ref_transaction_add_update( const char *committer_info, const char *msg) { + struct string_list_item *item; struct ref_update *update; if (transaction->state != REF_TRANSACTION_OPEN) @@ -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; 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 @@ -2383,9 +2383,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st */ static int split_head_update(struct ref_update *update, struct ref_transaction *transaction, - const char *head_ref, - struct string_list *affected_refnames, - struct strbuf *err) + const char *head_ref, struct strbuf *err) { struct ref_update *new_update; @@ -2403,7 +2401,7 @@ static int split_head_update(struct ref_update *update, * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, "multiple updates for 'HEAD' (including one " @@ -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; } @@ -2441,7 +2438,6 @@ static int split_head_update(struct ref_update *update, static int split_symref_update(struct ref_update *update, const char *referent, struct ref_transaction *transaction, - struct string_list *affected_refnames, struct strbuf *err) { struct ref_update *new_update; @@ -2453,7 +2449,7 @@ static int split_symref_update(struct ref_update *update, * size, but it happens at most once per symref in a * transaction. */ - if (string_list_has_string(affected_refnames, referent)) { + if (string_list_has_string(&transaction->refnames, referent)) { /* An entry already exists */ strbuf_addf(err, "multiple updates for '%s' (including one " @@ -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; } @@ -2561,9 +2548,7 @@ struct files_transaction_backend_data { static int lock_ref_for_update(struct files_ref_store *refs, struct ref_update *update, struct ref_transaction *transaction, - const char *head_ref, - struct string_list *affected_refnames, - struct strbuf *err) + const char *head_ref, struct strbuf *err) { struct strbuf referent = STRBUF_INIT; int mustexist = ref_update_expects_existing_old_ref(update); @@ -2579,8 +2564,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, update->flags |= REF_DELETING; if (head_ref) { - ret = split_head_update(update, transaction, head_ref, - affected_refnames, err); + ret = split_head_update(update, transaction, head_ref, err); if (ret) goto out; } @@ -2590,7 +2574,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, lock->count++; } else { ret = lock_raw_ref(refs, update->refname, mustexist, - affected_refnames, + &transaction->refnames, &lock, &referent, &update->type, err); if (ret) { @@ -2646,9 +2630,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, * of processing the split-off update, so we * don't have to do it here. */ - ret = split_symref_update(update, - referent.buf, transaction, - affected_refnames, err); + ret = split_symref_update(update, referent.buf, + transaction, err); if (ret) goto out; } @@ -2803,7 +2786,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, "ref_transaction_prepare"); size_t i; int ret = 0; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; char *head_ref = NULL; int head_type; struct files_transaction_backend_data *backend_data; @@ -2821,12 +2803,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, transaction->backend_data = backend_data; /* - * Fail if a refname appears more than once in the - * transaction. (If we end up splitting up any updates using - * split_symref_update() or split_head_update(), those - * functions will check that the new updates don't have the - * same refname as any existing ones.) Also fail if any of the - * updates use REF_IS_PRUNING without REF_NO_DEREF. + * Fail if any of the updates use REF_IS_PRUNING without REF_NO_DEREF. */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2834,16 +2811,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) BUG("REF_IS_PRUNING set without REF_NO_DEREF"); - - if (update->flags & REF_LOG_ONLY) - continue; - - string_list_append(&affected_refnames, update->refname); - } - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; } /* @@ -2884,7 +2851,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, struct ref_update *update = transaction->updates[i]; ret = lock_ref_for_update(refs, update, transaction, - head_ref, &affected_refnames, err); + head_ref, err); if (ret) goto cleanup; @@ -2957,7 +2924,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, cleanup: free(head_ref); - string_list_clear(&affected_refnames, 0); if (ret) files_transaction_cleanup(refs, transaction); @@ -3021,7 +2987,6 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, { size_t i; int ret = 0; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct ref_transaction *packed_transaction = NULL; struct ref_transaction *loose_transaction = NULL; @@ -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; } @@ -3054,7 +3014,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, * that we are creating already exists. */ if (refs_for_each_rawref(&refs->base, ref_present, - &affected_refnames)) + &transaction->refnames)) BUG("initial ref transaction called with existing refs"); packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, @@ -3072,7 +3032,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, BUG("initial ref transaction with old_sha1 set"); if (refs_verify_refname_available(&refs->base, update->refname, - &affected_refnames, NULL, 1, err)) { + &transaction->refnames, NULL, 1, err)) { ret = TRANSACTION_NAME_CONFLICT; goto cleanup; } @@ -3132,7 +3092,6 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (packed_transaction) ref_transaction_free(packed_transaction); transaction->state = REF_TRANSACTION_CLOSED; - string_list_clear(&affected_refnames, 0); return ret; } 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 @@ -1604,8 +1604,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store, struct packed_transaction_backend_data { /* True iff the transaction owns the packed-refs lock. */ int own_lock; - - struct string_list updates; }; static void packed_transaction_cleanup(struct packed_ref_store *refs, @@ -1614,8 +1612,6 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs, struct packed_transaction_backend_data *data = transaction->backend_data; if (data) { - string_list_clear(&data->updates, 0); - if (is_tempfile_active(refs->tempfile)) delete_tempfile(&refs->tempfile); @@ -1640,7 +1636,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_prepare"); struct packed_transaction_backend_data *data; - size_t i; int ret = TRANSACTION_GENERIC_ERROR; /* @@ -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; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index aaab711bb96844755dfa600d37efdb25b30a0765..f43766e1f00443eb689685cf4df0fa0b80018a03 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -3,6 +3,7 @@ #include "refs.h" #include "iterator.h" +#include "string-list.h" struct fsck_options; struct ref_transaction; @@ -198,6 +199,7 @@ enum ref_transaction_state { struct ref_transaction { struct ref_store *ref_store; struct ref_update **updates; + struct string_list refnames; size_t alloc; size_t nr; enum ref_transaction_state state; 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 @@ -1068,7 +1068,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare"); struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct reftable_transaction_data *tx_data = NULL; struct reftable_backend *be; struct object_id head_oid; @@ -1092,10 +1091,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->updates[i], err); if (ret) goto done; - - if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); } /* @@ -1107,17 +1102,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected; } - /* - * Fail if a refname appears more than once in the transaction. - * This code is taken from the files backend and is a good candidate to - * be moved into the generic layer. - */ - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto done; - } - /* * TODO: it's dubious whether we should reload the stack that "HEAD" * belongs to or not. In theory, it may happen that we only modify @@ -1185,14 +1169,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, !(u->flags & REF_LOG_ONLY) && !(u->flags & REF_UPDATE_VIA_HEAD) && !strcmp(rewritten_ref, head_referent.buf)) { - struct ref_update *new_update; - /* * First make sure that HEAD is not already in the * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(&affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, _("multiple updates for 'HEAD' (including one " @@ -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, @@ -1225,7 +1206,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * at a later point. */ ret = refs_verify_refname_available(ref_store, u->refname, - &affected_refnames, NULL, + &transaction->refnames, NULL, transaction->flags & REF_TRANSACTION_FLAG_INITIAL, err); if (ret < 0) @@ -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. @@ -1301,16 +1291,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, */ u->flags |= REF_LOG_ONLY | REF_NO_DEREF; u->flags &= ~REF_HAVE_OLD; - - if (string_list_has_string(&affected_refnames, new_update->refname)) { - 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; - } - string_list_insert(&affected_refnames, new_update->refname); } } @@ -1391,7 +1371,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, strbuf_addf(err, _("reftable: transaction prepare: %s"), reftable_error_str(ret)); } - string_list_clear(&affected_refnames, 0); strbuf_release(&referent); strbuf_release(&head_referent);
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. 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'. - 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. 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(-)