| Message ID | 20240503124115.252413-5-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add support for transactional symref updates | expand |
Hi Karthik I've left a few comments below - the most important one is about the error messages in the reftable backend, non of the others are worth re-rolling for on their own. On 03/05/2024 13:41, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > The reference backends currently support transactional reference > updates. While this is exposed to users via 'git-update-ref' and its > '--stdin' mode, it is also used internally within various commands. > > However, we never supported transactional updates of symrefs. Let's adds s/we never supported/we do not support/ s/Let's/This commit/ > support for symrefs in both the 'files' and the 'reftable' backend. s/backend/backends/ > Here, we add and use `ref_update_has_null_new_value()`, a helper > function which is used to check if there is a new_value in a reference > update. The new value could either be a symref target `new_target` or a > OID `new_oid`. > > With this, now transactional updates (verify, create, delete, update) s/With this, // > can be used for: > - regular refs > - symbolic refs > - conversion of regular to symbolic refs and vice versa Excellent > This also allows us to expose this to users via new commands in > 'git-update-ref' in the future. I'm slightly concerned that splitting out the update-ref changes means we don't have any test coverage of the new code beyond the part that is used by refs_create_symref() > Note that a dangling symref update does not record a new reflog entry, > which is unchanged before and after this commit. > > +/* > + * Check whether the old_target values stored in update are consistent > + * with current_target, which is the symbolic reference's current value. > + * If everything is OK, return 0; otherwise, write an error message to > + * err and return -1. > + */ > +static int check_old_target(struct ref_update *update, > + const char *current_target, > + struct strbuf *err) > +{ > + if (!update->old_target) > + BUG("called without old_target set"); > + > + if (!strcmp(update->old_target, current_target)) > + return 0; > + > + if (!strcmp(current_target, "")) > + strbuf_addf(err, "cannot lock ref '%s': " > + "reference is missing but expected %s", > + original_update_refname(update), > + update->old_target); > + else > + strbuf_addf(err, "cannot lock ref '%s': " > + "is at %s but expected %s", > + original_update_refname(update), > + current_target, update->old_target); > + > + return -1; > +} > @@ -2576,9 +2623,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, > } > } > > - if ((update->flags & REF_HAVE_NEW) && > - !(update->flags & REF_DELETING) && > - !(update->flags & REF_LOG_ONLY)) { > + if (update->new_target && !(update->flags & REF_LOG_ONLY)) { > + if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) { This line looks quite long > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, > * individual refs. But the error messages match what the files > * backend returns, which keeps our tests happy. > */ > - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { > + if (u->old_target) { > + if (strcmp(referent.buf, u->old_target)) { > + if (!strcmp(referent.buf, "")) > + strbuf_addf(err, "verifying symref target: '%s': " > + "reference is missing but expected %s", > + original_update_refname(u), > + u->old_target); > + else > + strbuf_addf(err, "verifying symref target: '%s': " > + "is at %s but expected %s", > + original_update_refname(u), > + referent.buf, u->old_target); The messages in this function differ from the equivalent messages in check_old_target() from the files backend above. This is potentially confusing to users, creates more work for translators and makes it hard to write tests that are independent of the backend. Can we export check_old_target() so it can be reused here. If not we should reword these messages to match the other messages all of which talk about not being able to lock the ref. > + ret = -1; > + goto done; > + } > + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { > if (is_null_oid(&u->old_oid)) > strbuf_addf(err, _("cannot lock ref '%s': " > "reference already exists"), > @@ -1043,7 +1060,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data > * - `core.logAllRefUpdates` tells us to create the reflog for > * the given ref. > */ > - if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) { > + if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) { The old line was already quite long and the new one is even longer - perhaps we could break it after the second "&&" > + if (create_reflog) { > + ALLOC_GROW(logs, logs_nr + 1, logs_alloc); > + log = &logs[logs_nr++]; > + memset(log, 0, sizeof(*log)); > + > + fill_reftable_log_record(log); > + log->update_index = ts; > + log->refname = xstrdup(u->refname); > + memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); > + memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); Both these lines would benefit from being folded Best Wishes Phillip
Hello Phillip, Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Karthik > > I've left a few comments below - the most important one is about the > error messages in the reftable backend, non of the others are worth > re-rolling for on their own. > > On 03/05/2024 13:41, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> The reference backends currently support transactional reference >> updates. While this is exposed to users via 'git-update-ref' and its >> '--stdin' mode, it is also used internally within various commands. >> >> However, we never supported transactional updates of symrefs. Let's adds > > s/we never supported/we do not support/ > > s/Let's/This commit/ > >> support for symrefs in both the 'files' and the 'reftable' backend. > > s/backend/backends/ > >> Here, we add and use `ref_update_has_null_new_value()`, a helper >> function which is used to check if there is a new_value in a reference >> update. The new value could either be a symref target `new_target` or a >> OID `new_oid`. >> >> With this, now transactional updates (verify, create, delete, update) > > s/With this, // > Thanks, will make the above changes. >> can be used for: >> - regular refs >> - symbolic refs >> - conversion of regular to symbolic refs and vice versa > > Excellent > >> This also allows us to expose this to users via new commands in >> 'git-update-ref' in the future. > > I'm slightly concerned that splitting out the update-ref changes means > we don't have any test coverage of the new code beyond the part that is > used by refs_create_symref() > This is definitely true. But I also caught a bunch of edge cases this way because the tests which indirectly use 'refs_create_symref()' are quite intensive. >> Note that a dangling symref update does not record a new reflog entry, >> which is unchanged before and after this commit. >> >> +/* >> + * Check whether the old_target values stored in update are consistent >> + * with current_target, which is the symbolic reference's current value. >> + * If everything is OK, return 0; otherwise, write an error message to >> + * err and return -1. >> + */ >> +static int check_old_target(struct ref_update *update, >> + const char *current_target, >> + struct strbuf *err) >> +{ >> + if (!update->old_target) >> + BUG("called without old_target set"); >> + >> + if (!strcmp(update->old_target, current_target)) >> + return 0; >> + >> + if (!strcmp(current_target, "")) >> + strbuf_addf(err, "cannot lock ref '%s': " >> + "reference is missing but expected %s", >> + original_update_refname(update), >> + update->old_target); >> + else >> + strbuf_addf(err, "cannot lock ref '%s': " >> + "is at %s but expected %s", >> + original_update_refname(update), >> + current_target, update->old_target); >> + >> + return -1; >> +} >> @@ -2576,9 +2623,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> } >> } >> >> - if ((update->flags & REF_HAVE_NEW) && >> - !(update->flags & REF_DELETING) && >> - !(update->flags & REF_LOG_ONLY)) { >> + if (update->new_target && !(update->flags & REF_LOG_ONLY)) { >> + if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) { > > This line looks quite long > Somehow I find it much easier to read these longer lines, unless there is a logical operator, but I'll split the line mid way with the arguments. >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, >> * individual refs. But the error messages match what the files >> * backend returns, which keeps our tests happy. >> */ >> - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { >> + if (u->old_target) { >> + if (strcmp(referent.buf, u->old_target)) { >> + if (!strcmp(referent.buf, "")) >> + strbuf_addf(err, "verifying symref target: '%s': " >> + "reference is missing but expected %s", >> + original_update_refname(u), >> + u->old_target); >> + else >> + strbuf_addf(err, "verifying symref target: '%s': " >> + "is at %s but expected %s", >> + original_update_refname(u), >> + referent.buf, u->old_target); > > The messages in this function differ from the equivalent messages in > check_old_target() from the files backend above. This is potentially > confusing to users, creates more work for translators and makes it hard > to write tests that are independent of the backend. Can we export > check_old_target() so it can be reused here. If not we should reword > these messages to match the other messages all of which talk about not > being able to lock the ref. > This is very intentional, the way the backends work at this point are quite different and while in the files backend, we talk about locking a particular ref. In the reftable backend we do not lock single refs. We lock tables. So keeping it consistent doesn't make sense here. However, we could make the files backend similar to this one, I would be okay doing that. >> + ret = -1; >> + goto done; >> + } >> + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { >> if (is_null_oid(&u->old_oid)) >> strbuf_addf(err, _("cannot lock ref '%s': " >> "reference already exists"), >> @@ -1043,7 +1060,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data >> * - `core.logAllRefUpdates` tells us to create the reflog for >> * the given ref. >> */ >> - if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) { >> + if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) { > > The old line was already quite long and the new one is even longer - > perhaps we could break it after the second "&&" > I will break it both the && I think that is easier on the eyes. >> + if (create_reflog) { >> + ALLOC_GROW(logs, logs_nr + 1, logs_alloc); >> + log = &logs[logs_nr++]; >> + memset(log, 0, sizeof(*log)); >> + >> + fill_reftable_log_record(log); >> + log->update_index = ts; >> + log->refname = xstrdup(u->refname); >> + memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); >> + memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); > > Both these lines would benefit from being folded > > Best Wishes > > Phillip Thanks for the review. Karthik
Hi Karthik On 05/05/2024 17:09, Karthik Nayak wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: >> On 03/05/2024 13:41, Karthik Nayak wrote: >>> --- a/refs/reftable-backend.c >>> +++ b/refs/reftable-backend.c >>> @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, >>> * individual refs. But the error messages match what the files >>> * backend returns, which keeps our tests happy. >>> */ >>> - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { >>> + if (u->old_target) { >>> + if (strcmp(referent.buf, u->old_target)) { >>> + if (!strcmp(referent.buf, "")) >>> + strbuf_addf(err, "verifying symref target: '%s': " >>> + "reference is missing but expected %s", >>> + original_update_refname(u), >>> + u->old_target); >>> + else >>> + strbuf_addf(err, "verifying symref target: '%s': " >>> + "is at %s but expected %s", >>> + original_update_refname(u), >>> + referent.buf, u->old_target); >> >> The messages in this function differ from the equivalent messages in >> check_old_target() from the files backend above. This is potentially >> confusing to users, creates more work for translators and makes it hard >> to write tests that are independent of the backend. Can we export >> check_old_target() so it can be reused here. If not we should reword >> these messages to match the other messages all of which talk about not >> being able to lock the ref. >> > > This is very intentional, the way the backends work at this point are > quite different and while in the files backend, we talk about locking a > particular ref. I agree that the existing messages could be improved - these messages are returned when checking the old value of the ref so talking about being unable to lock the ref is not helpful as the important information is that the old value does not match the expected value. However that is not dependent on the backend or on whether the expected value is a symref or an oid so it feels a bit random to make these two messages different. > In the reftable backend we do not lock single refs. We > lock tables. So keeping it consistent doesn't make sense here. Where an update is prevented by another process holding a lock I think that the granularity of the lock that prevents the ref from being updated is not particularly relevant as far as the user is concerned. As far as I can see the existing error messages in the reftable backend try to be consistent with the messages in the files backend. > However, we could make the files backend similar to this one, I would be > okay doing that. I would be very happy to see the messages improved for both backends when the old value does not match the expected (oid or symref) value. I do think we should have consistent error messages in this case that are essentially independent of the backend and type of the expected value. Best Wishes Phillip
Hi Karthik On 05/05/2024 17:09, Karthik Nayak wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: >> I'm slightly concerned that splitting out the update-ref changes means >> we don't have any test coverage of the new code beyond the part that is >> used by refs_create_symref() >> > > This is definitely true. But I also caught a bunch of edge cases this > way because the tests which indirectly use 'refs_create_symref()' are > quite intensive. I forgot to say in my last mail that this is good to know. So it sounds like the only new code that isn't being exercised by the tests is the check for the old value? Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Karthik > > On 05/05/2024 17:09, Karthik Nayak wrote: >> Phillip Wood <phillip.wood123@gmail.com> writes: >>> On 03/05/2024 13:41, Karthik Nayak wrote: >>>> --- a/refs/reftable-backend.c >>>> +++ b/refs/reftable-backend.c >>>> @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, >>>> * individual refs. But the error messages match what the files >>>> * backend returns, which keeps our tests happy. >>>> */ >>>> - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { >>>> + if (u->old_target) { >>>> + if (strcmp(referent.buf, u->old_target)) { >>>> + if (!strcmp(referent.buf, "")) >>>> + strbuf_addf(err, "verifying symref target: '%s': " >>>> + "reference is missing but expected %s", >>>> + original_update_refname(u), >>>> + u->old_target); >>>> + else >>>> + strbuf_addf(err, "verifying symref target: '%s': " >>>> + "is at %s but expected %s", >>>> + original_update_refname(u), >>>> + referent.buf, u->old_target); >>> >>> The messages in this function differ from the equivalent messages in >>> check_old_target() from the files backend above. This is potentially >>> confusing to users, creates more work for translators and makes it hard >>> to write tests that are independent of the backend. Can we export >>> check_old_target() so it can be reused here. If not we should reword >>> these messages to match the other messages all of which talk about not >>> being able to lock the ref. >>> >> >> This is very intentional, the way the backends work at this point are >> quite different and while in the files backend, we talk about locking a >> particular ref. > > I agree that the existing messages could be improved - these messages > are returned when checking the old value of the ref so talking about > being unable to lock the ref is not helpful as the important information > is that the old value does not match the expected value. However that is > not dependent on the backend or on whether the expected value is a > symref or an oid so it feels a bit random to make these two messages > different. > >> In the reftable backend we do not lock single refs. We >> lock tables. So keeping it consistent doesn't make sense here. > > Where an update is prevented by another process holding a lock I think > that the granularity of the lock that prevents the ref from being > updated is not particularly relevant as far as the user is concerned. As > far as I can see the existing error messages in the reftable backend try > to be consistent with the messages in the files backend. > Well, I agree. But we do have to note, that the files backend always had `check_old_oid` which has messages along the lines of 'cannot lock ref ...' and now the new `check_old_target` will not be consistent with those messages. Which is OK, since, like you mentioned, these messages could be improved. >> However, we could make the files backend similar to this one, I would be >> okay doing that. > > I would be very happy to see the messages improved for both backends > when the old value does not match the expected (oid or symref) value. I > do think we should have consistent error messages in this case that are > essentially independent of the backend and type of the expected value. > > Best Wishes > > Phillip Overall, I think we're reaching the same consensus, i.e. to export this functionality to a generic function and shared between the both backends. I will make this change. I think the message used in the reftable backend along the lines of 'verifying symref target ...' is verbose yet generic enough, so I'll keep those messages. Thanks Karthik
Hello, Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Karthik > > On 05/05/2024 17:09, Karthik Nayak wrote: >> Phillip Wood <phillip.wood123@gmail.com> writes: >>> I'm slightly concerned that splitting out the update-ref changes means >>> we don't have any test coverage of the new code beyond the part that is >>> used by refs_create_symref() >>> >> >> This is definitely true. But I also caught a bunch of edge cases this >> way because the tests which indirectly use 'refs_create_symref()' are >> quite intensive. > > I forgot to say in my last mail that this is good to know. So it sounds > like the only new code that isn't being exercised by the tests is the > check for the old value? > > Best Wishes > > Phillip That's correct. I think testing that _currently_ would require us to probably expose and test via the unit testing library. I plan to follow this patch series soon with the symref-* ones. While that's not the best argument for not having full test coverage, I hope it is an OK state to be since that path has no users as of this point. Karthik
Hi Karthik On 06/05/2024 12:22, Karthik Nayak wrote: > > That's correct. I think testing that _currently_ would require us to > probably expose and test via the unit testing library. I plan to follow > this patch series soon with the symref-* ones. While that's not the best > argument for not having full test coverage, I hope it is an OK state to > be since that path has no users as of this point. That seems reasonable - we have good test coverage for the code we're currently using and you're clearly committed to adding tests for the rest when you extend "git update-ref" to accommodate symrefs. Thanks Phillip
Hi Karthik On 06/05/2024 12:19, Karthik Nayak wrote: > > Overall, I think we're reaching the same consensus, i.e. to export this > functionality to a generic function and shared between the both > backends. I will make this change. I think the message used in the > reftable backend along the lines of 'verifying symref target ...' is > verbose yet generic enough, so I'll keep those messages. That sounds good to me Thanks Phillip
diff --git a/refs.c b/refs.c index 203a101988..acb153b6f8 100644 --- a/refs.c +++ b/refs.c @@ -1217,6 +1217,8 @@ void ref_transaction_free(struct ref_transaction *transaction) for (i = 0; i < transaction->nr; i++) { free(transaction->updates[i]->msg); + free((char *)transaction->updates[i]->new_target); + free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } free(transaction->updates); @@ -1247,10 +1249,13 @@ struct ref_update *ref_transaction_add_update( update->flags = flags; - if (flags & REF_HAVE_NEW) + update->new_target = xstrdup_or_null(new_target); + update->old_target = xstrdup_or_null(old_target); + if ((flags & REF_HAVE_NEW) && new_oid) oidcpy(&update->new_oid, new_oid); - if (flags & REF_HAVE_OLD) + if ((flags & REF_HAVE_OLD) && old_oid) oidcpy(&update->old_oid, old_oid); + update->msg = normalize_reflog_message(msg); return update; } @@ -1286,6 +1291,7 @@ int ref_transaction_update(struct ref_transaction *transaction, flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS; flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); + flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, new_oid, old_oid, new_target, @@ -2814,3 +2820,8 @@ int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg { return refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg); } + +int ref_update_has_null_new_value(struct ref_update *update) +{ + return !update->new_target && is_null_oid(&update->new_oid); +} diff --git a/refs/files-backend.c b/refs/files-backend.c index 74a713090c..e1f0ca74c0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2434,6 +2434,36 @@ static const char *original_update_refname(struct ref_update *update) return update->refname; } +/* + * Check whether the old_target values stored in update are consistent + * with current_target, which is the symbolic reference's current value. + * If everything is OK, return 0; otherwise, write an error message to + * err and return -1. + */ +static int check_old_target(struct ref_update *update, + const char *current_target, + struct strbuf *err) +{ + if (!update->old_target) + BUG("called without old_target set"); + + if (!strcmp(update->old_target, current_target)) + return 0; + + if (!strcmp(current_target, "")) + strbuf_addf(err, "cannot lock ref '%s': " + "reference is missing but expected %s", + original_update_refname(update), + update->old_target); + else + strbuf_addf(err, "cannot lock ref '%s': " + "is at %s but expected %s", + original_update_refname(update), + current_target, update->old_target); + + return -1; +} + /* * Check whether the REF_HAVE_OLD and old_oid values stored in update * are consistent with oid, which is the reference's current value. If @@ -2494,7 +2524,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, files_assert_main_repository(refs, "lock_ref_for_update"); - if ((update->flags & REF_HAVE_NEW) && is_null_oid(&update->new_oid)) + if ((update->flags & REF_HAVE_NEW) && ref_update_has_null_new_value(update)) update->flags |= REF_DELETING; if (head_ref) { @@ -2537,7 +2567,14 @@ static int lock_ref_for_update(struct files_ref_store *refs, ret = TRANSACTION_GENERIC_ERROR; goto out; } - } else if (check_old_oid(update, &lock->old_oid, err)) { + } + + if (update->old_target) { + if (check_old_target(update, referent.buf, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + } else if (check_old_oid(update, &lock->old_oid, err)) { ret = TRANSACTION_GENERIC_ERROR; goto out; } @@ -2558,7 +2595,17 @@ static int lock_ref_for_update(struct files_ref_store *refs, } else { struct ref_update *parent_update; - if (check_old_oid(update, &lock->old_oid, err)) { + /* + * Even if the ref is a regular ref, if `old_target` is set, we + * check the referent value. Ideally `old_target` should only + * be set for symrefs, but we're strict about its usage. + */ + if (update->old_target) { + if (check_old_target(update, referent.buf, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + } else if (check_old_oid(update, &lock->old_oid, err)) { ret = TRANSACTION_GENERIC_ERROR; goto out; } @@ -2576,9 +2623,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, } } - if ((update->flags & REF_HAVE_NEW) && - !(update->flags & REF_DELETING) && - !(update->flags & REF_LOG_ONLY)) { + if (update->new_target && !(update->flags & REF_LOG_ONLY)) { + if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + + if (close_ref_gently(lock)) { + strbuf_addf(err, "couldn't close '%s.lock'", + update->refname); + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + + /* + * Once we have created the symref lock, the commit + * phase of the transaction only needs to commit the lock. + */ + update->flags |= REF_NEEDS_COMMIT; + } else if ((update->flags & REF_HAVE_NEW) && + !(update->flags & REF_DELETING) && + !(update->flags & REF_LOG_ONLY)) { if (!(update->type & REF_ISSYMREF) && oideq(&lock->old_oid, &update->new_oid)) { /* @@ -2841,6 +2906,43 @@ static int files_transaction_prepare(struct ref_store *ref_store, return ret; } +static int parse_and_write_reflog(struct files_ref_store *refs, + struct ref_update *update, + struct ref_lock *lock, + struct strbuf *err) +{ + if (update->new_target) { + /* + * We want to get the resolved OID for the target, to ensure + * that the correct value is added to the reflog. + */ + if (!refs_resolve_ref_unsafe(&refs->base, update->new_target, + RESOLVE_REF_READING, + &update->new_oid, NULL)) { + /* + * TODO: currently we skip creating reflogs for dangling + * symref updates. It would be nice to capture this as + * zero oid updates however. + */ + return 0; + } + } + + if (files_log_ref_write(refs, lock->ref_name, &lock->old_oid, + &update->new_oid, update->msg, update->flags, err)) { + char *old_msg = strbuf_detach(err, NULL); + + strbuf_addf(err, "cannot update the ref '%s': %s", + lock->ref_name, old_msg); + free(old_msg); + unlock_ref(lock); + update->backend_data = NULL; + return -1; + } + + return 0; +} + static int files_transaction_finish(struct ref_store *ref_store, struct ref_transaction *transaction, struct strbuf *err) @@ -2871,23 +2973,20 @@ static int files_transaction_finish(struct ref_store *ref_store, if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { - if (files_log_ref_write(refs, - lock->ref_name, - &lock->old_oid, - &update->new_oid, - update->msg, update->flags, - err)) { - char *old_msg = strbuf_detach(err, NULL); - - strbuf_addf(err, "cannot update the ref '%s': %s", - lock->ref_name, old_msg); - free(old_msg); - unlock_ref(lock); - update->backend_data = NULL; + if (parse_and_write_reflog(refs, update, lock, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } } + + /* + * We try creating a symlink, if that succeeds we continue to the + * next update. If not, we try and create a regular symref. + */ + if (update->new_target && prefer_symlink_refs) + if (!create_ref_symlink(lock, update->new_target)) + continue; + if (update->flags & REF_NEEDS_COMMIT) { clear_loose_ref_cache(refs); if (commit_ref(lock)) { diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 108f4ec419..03eda70ea4 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -749,4 +749,11 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo, */ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store); +/* + * Helper function to check if the new value is null, this + * takes into consideration that the update could be a regular + * ref or a symbolic ref. + */ +int ref_update_has_null_new_value(struct ref_update *update); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 6104471199..54c4d8b771 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -856,7 +856,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * There is no need to write the reference deletion * when the reference in question doesn't exist. */ - if (u->flags & REF_HAVE_NEW && !is_null_oid(&u->new_oid)) { + if ((u->flags & REF_HAVE_NEW) && !ref_update_has_null_new_value(u)) { ret = queue_transaction_update(refs, tx_data, u, ¤t_oid, err); if (ret) @@ -907,8 +907,10 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * intertwined with the locking in files-backend.c. */ new_update = ref_transaction_add_update( - transaction, referent.buf, new_flags, - &u->new_oid, &u->old_oid, NULL, NULL, u->msg); + transaction, referent.buf, new_flags, + &u->new_oid, &u->old_oid, u->new_target, + u->old_target, u->msg); + new_update->parent_update = u; /* @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * individual refs. But the error messages match what the files * backend returns, which keeps our tests happy. */ - if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) { + if (u->old_target) { + if (strcmp(referent.buf, u->old_target)) { + if (!strcmp(referent.buf, "")) + strbuf_addf(err, "verifying symref target: '%s': " + "reference is missing but expected %s", + original_update_refname(u), + u->old_target); + else + strbuf_addf(err, "verifying symref target: '%s': " + "is at %s but expected %s", + original_update_refname(u), + referent.buf, u->old_target); + ret = -1; + goto done; + } + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { if (is_null_oid(&u->old_oid)) strbuf_addf(err, _("cannot lock ref '%s': " "reference already exists"), @@ -1043,7 +1060,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data * - `core.logAllRefUpdates` tells us to create the reflog for * the given ref. */ - if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) { + if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) { struct reftable_log_record log = {0}; struct reftable_iterator it = {0}; @@ -1084,24 +1101,50 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data (u->flags & REF_FORCE_CREATE_REFLOG || should_write_log(&arg->refs->base, u->refname))) { struct reftable_log_record *log; + int create_reflog = 1; + + if (u->new_target) { + if (!refs_resolve_ref_unsafe(&arg->refs->base, u->new_target, + RESOLVE_REF_READING, &u->new_oid, NULL)) { + /* + * TODO: currently we skip creating reflogs for dangling + * symref updates. It would be nice to capture this as + * zero oid updates however. + */ + create_reflog = 0; + } + } - ALLOC_GROW(logs, logs_nr + 1, logs_alloc); - log = &logs[logs_nr++]; - memset(log, 0, sizeof(*log)); - - fill_reftable_log_record(log); - log->update_index = ts; - log->refname = xstrdup(u->refname); - memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); - memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); - log->value.update.message = - xstrndup(u->msg, arg->refs->write_options.block_size / 2); + if (create_reflog) { + ALLOC_GROW(logs, logs_nr + 1, logs_alloc); + log = &logs[logs_nr++]; + memset(log, 0, sizeof(*log)); + + fill_reftable_log_record(log); + log->update_index = ts; + log->refname = xstrdup(u->refname); + memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ); + memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ); + log->value.update.message = + xstrndup(u->msg, arg->refs->write_options.block_size / 2); + } } if (u->flags & REF_LOG_ONLY) continue; - if (u->flags & REF_HAVE_NEW && is_null_oid(&u->new_oid)) { + if (u->new_target) { + struct reftable_ref_record ref = { + .refname = (char *)u->refname, + .value_type = REFTABLE_REF_SYMREF, + .value.symref = (char *)u->new_target, + .update_index = ts, + }; + + ret = reftable_writer_add_ref(writer, &ref); + if (ret < 0) + goto done; + } else if ((u->flags & REF_HAVE_NEW) && ref_update_has_null_new_value(u)) { struct reftable_ref_record ref = { .refname = (char *)u->refname, .update_index = ts,