| Message ID | 20240501202229.2695774-5-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add support for transactional symref updates | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > 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 add > support for symrefs in both the 'files' and the 'reftable' backend. > > Here, we add and use `ref_update_is_null_new_value()`, a helper function > which is used to check if there is a new_value in a reference update. I know you want to express a condition where you answer yes to "Is the new value specified in this ref update NULL?", but "is" at that position in the name somehow sounds awkward. Any of ref_update_has_null_new_value ref_update_with_no_new_value ref_update_without_new_value might be nicer to ears. > We do not add reflog for dangling symref updates, because currently > 'git-symbolic-ref' doesn't add reflog for dangling symref updates and it > would be best to keep this behavior consistent as we would move it to > start using transaction based updates in the following commit. If we are not changing the behaviour, does it deserve a four-line paragraph? It is not like we describe every no changes (i.e. "we could break the behaviour by introducing this and that bugs, but we did not" is not something we usually say in proposed log messages). At most, if you want to highlight that behaviour, I would expect a brief mention like: Note that a dangling symref update does not record a new reflog entry, which is unchanged before and after this commit. As a reflog entry records name of the object that is pointed by the ref (either directly or indirectly) before and after an operation, an operation that involve a dangling reflog that does not point at any object cannot be expressed in a reflog, no? It is way too late to change this, but it would have been interesting if the design of reflog had a room to log the change of symbolic ref target as well as object names. It would have allowed us to say "HEAD at time T pointed at refs/heads/main (which did not exist)", "HEAD at time T+1 directly pointed at commit X (detached)", "HEAD at time T+2 pointed at refs/heads/next", etc. and allowed us to much more cleanly support "previous branch". > @@ -1247,10 +1249,15 @@ struct ref_update *ref_transaction_add_update( > > update->flags = flags; > > - if (flags & REF_HAVE_NEW) > + if (new_target) > + update->new_target = xstrdup(new_target); > + if (old_target) > + update->old_target = xstrdup(old_target); "Is the assumption that *update is 0-initialized?" was the first question that came to my mind. Doing an unconditional update->new_target = xstrdup_or_null(new_target); update->old_target = xstrdup_or_null(old_target); would convey the intention much more clearly without having readers guess the answer to the above question. > + if (new_oid && flags & REF_HAVE_NEW) Even though syntactically not required, if (new_oid && (flags & REF_HAVE_NEW)) or better yet if ((flags & REF_HAVE_NEW) && new_oid) would be easier to see. > oidcpy(&update->new_oid, new_oid); Again is the expectation that update->new_oid is initialized to all-0? I am wondering if we want an else clause, i.e. if (!(flags & REF_HAVE_NEW)) oidcpy(&update->new_oid, null_oid()); else oidcpy(&update->new_oid, new_oid ? new_oid : null_oid()); to clarify the intention of the code, since the way you wrote the consumer of thes two members and REF_HAVE_NEW bit in the previous step implied that the new_oid member gets used even when REF_HAVE_* bit is off, only for its null_oid() value. I'll stop here for now. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> 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 add >> support for symrefs in both the 'files' and the 'reftable' backend. >> >> Here, we add and use `ref_update_is_null_new_value()`, a helper function >> which is used to check if there is a new_value in a reference update. > > I know you want to express a condition where you answer yes to "Is > the new value specified in this ref update NULL?", but "is" at that > position in the name somehow sounds awkward. Any of > > ref_update_has_null_new_value > ref_update_with_no_new_value > ref_update_without_new_value > > might be nicer to ears. > Yes, thanks with this, I do agree that `ref_update_has_null_new_value` sounds better. >> We do not add reflog for dangling symref updates, because currently >> 'git-symbolic-ref' doesn't add reflog for dangling symref updates and it >> would be best to keep this behavior consistent as we would move it to >> start using transaction based updates in the following commit. > > If we are not changing the behaviour, does it deserve a four-line > paragraph? It is not like we describe every no changes (i.e. "we > could break the behaviour by introducing this and that bugs, but we > did not" is not something we usually say in proposed log messages). > > At most, if you want to highlight that behaviour, I would expect a > brief mention like: > > Note that a dangling symref update does not record a new reflog > entry, which is unchanged before and after this commit. > > As a reflog entry records name of the object that is pointed by the > ref (either directly or indirectly) before and after an operation, > an operation that involve a dangling reflog that does not point at > any object cannot be expressed in a reflog, no? It is way too late > to change this, but it would have been interesting if the design of > reflog had a room to log the change of symbolic ref target as well > as object names. It would have allowed us to say "HEAD at time T > pointed at refs/heads/main (which did not exist)", "HEAD at time T+1 > directly pointed at commit X (detached)", "HEAD at time T+2 pointed > at refs/heads/next", etc. and allowed us to much more cleanly > support "previous branch". > While I agree that four lines may seem excessive, I think it is indeed an important point to note. Mostly because this shows that when doing dangling symref updates, there is no record of this update. The best situation would be like you mentioned, to record the symref target changes. But even with the current design, it would have been nice to at least acknowledge that there was some update done to the symref. By having zero-oid for the new and old value in the reflog. But seems like we can't do that either. >> @@ -1247,10 +1249,15 @@ struct ref_update *ref_transaction_add_update( >> >> update->flags = flags; >> >> - if (flags & REF_HAVE_NEW) >> + if (new_target) >> + update->new_target = xstrdup(new_target); >> + if (old_target) >> + update->old_target = xstrdup(old_target); > > "Is the assumption that *update is 0-initialized?" was the first > question that came to my mind. > > Doing an unconditional > > update->new_target = xstrdup_or_null(new_target); > update->old_target = xstrdup_or_null(old_target); > > would convey the intention much more clearly without having readers > guess the answer to the above question. > Right, I didn't catch the nuance last time, thanks for the explanation. >> + if (new_oid && flags & REF_HAVE_NEW) > > Even though syntactically not required, > > if (new_oid && (flags & REF_HAVE_NEW)) > > or better yet > > if ((flags & REF_HAVE_NEW) && new_oid) > > would be easier to see. > >> oidcpy(&update->new_oid, new_oid); > > Again is the expectation that update->new_oid is initialized to > all-0? I am wondering if we want an else clause, i.e. > > if (!(flags & REF_HAVE_NEW)) > oidcpy(&update->new_oid, null_oid()); > else > oidcpy(&update->new_oid, new_oid ? new_oid : null_oid()); > > to clarify the intention of the code, since the way you wrote the > consumer of thes two members and REF_HAVE_NEW bit in the previous > step implied that the new_oid member gets used even when REF_HAVE_* > bit is off, only for its null_oid() value. > Yes I understand what you're saying, but since we're doing a `FLEX_ALLOC_MEM` right above this code, I thought the fact that the 'update' struct is 0 initialized is known. With this, and the fact that here `update->new_oid` is a static variable, while `new_oid` is a pointer. I think being too verbose is not required. > I'll stop here for now. > > Thanks. > Thanks Junio for taking the time to review.
On Thu, May 02, 2024 at 05:50:47AM +0000, Karthik Nayak wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Karthik Nayak <karthik.188@gmail.com> writes: > >> From: Karthik Nayak <karthik.188@gmail.com> > >> We do not add reflog for dangling symref updates, because currently > >> 'git-symbolic-ref' doesn't add reflog for dangling symref updates and it > >> would be best to keep this behavior consistent as we would move it to > >> start using transaction based updates in the following commit. > > > > If we are not changing the behaviour, does it deserve a four-line > > paragraph? It is not like we describe every no changes (i.e. "we > > could break the behaviour by introducing this and that bugs, but we > > did not" is not something we usually say in proposed log messages). > > > > At most, if you want to highlight that behaviour, I would expect a > > brief mention like: > > > > Note that a dangling symref update does not record a new reflog > > entry, which is unchanged before and after this commit. > > > > As a reflog entry records name of the object that is pointed by the > > ref (either directly or indirectly) before and after an operation, > > an operation that involve a dangling reflog that does not point at > > any object cannot be expressed in a reflog, no? It is way too late > > to change this, but it would have been interesting if the design of > > reflog had a room to log the change of symbolic ref target as well > > as object names. It would have allowed us to say "HEAD at time T > > pointed at refs/heads/main (which did not exist)", "HEAD at time T+1 > > directly pointed at commit X (detached)", "HEAD at time T+2 pointed > > at refs/heads/next", etc. and allowed us to much more cleanly > > support "previous branch". > > > > While I agree that four lines may seem excessive, I think it is indeed > an important point to note. Mostly because this shows that when doing > dangling symref updates, there is no record of this update. The best > situation would be like you mentioned, to record the symref target > changes. But even with the current design, it would have been nice to at > least acknowledge that there was some update done to the symref. By > having zero-oid for the new and old value in the reflog. But seems like > we can't do that either. I wouldn't say we can't do that. We already do log when symrefs become dangling when updating references via HEAD by logging a zero OID as new OID. That is, if we have "HEAD -> refs/heads/foo" and you delete the latter, then we create a new reflog message for "HEAD" with zero OID as new OID. I would claim that the current behaviour where we don't create a reflog entry when updating a ref to become dangling is a mere bug. I think it's fair to declare this a #leftoverbit and handle it in a follow-up patch series. But it would be nice to say so in an in-code comment. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Thu, May 02, 2024 at 05:50:47AM +0000, Karthik Nayak wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> > Karthik Nayak <karthik.188@gmail.com> writes: >> >> From: Karthik Nayak <karthik.188@gmail.com> >> >> We do not add reflog for dangling symref updates, because currently >> >> 'git-symbolic-ref' doesn't add reflog for dangling symref updates and it >> >> would be best to keep this behavior consistent as we would move it to >> >> start using transaction based updates in the following commit. >> > >> > If we are not changing the behaviour, does it deserve a four-line >> > paragraph? It is not like we describe every no changes (i.e. "we >> > could break the behaviour by introducing this and that bugs, but we >> > did not" is not something we usually say in proposed log messages). >> > >> > At most, if you want to highlight that behaviour, I would expect a >> > brief mention like: >> > >> > Note that a dangling symref update does not record a new reflog >> > entry, which is unchanged before and after this commit. >> > >> > As a reflog entry records name of the object that is pointed by the >> > ref (either directly or indirectly) before and after an operation, >> > an operation that involve a dangling reflog that does not point at >> > any object cannot be expressed in a reflog, no? It is way too late >> > to change this, but it would have been interesting if the design of >> > reflog had a room to log the change of symbolic ref target as well >> > as object names. It would have allowed us to say "HEAD at time T >> > pointed at refs/heads/main (which did not exist)", "HEAD at time T+1 >> > directly pointed at commit X (detached)", "HEAD at time T+2 pointed >> > at refs/heads/next", etc. and allowed us to much more cleanly >> > support "previous branch". >> > >> >> While I agree that four lines may seem excessive, I think it is indeed >> an important point to note. Mostly because this shows that when doing >> dangling symref updates, there is no record of this update. The best >> situation would be like you mentioned, to record the symref target >> changes. But even with the current design, it would have been nice to at >> least acknowledge that there was some update done to the symref. By >> having zero-oid for the new and old value in the reflog. But seems like >> we can't do that either. > > I wouldn't say we can't do that. We already do log when symrefs become > dangling when updating references via HEAD by logging a zero OID as new > OID. That is, if we have "HEAD -> refs/heads/foo" and you delete the > latter, then we create a new reflog message for "HEAD" with zero OID as > new OID. > > I would claim that the current behaviour where we don't create a reflog > entry when updating a ref to become dangling is a mere bug. I think it's > fair to declare this a #leftoverbit and handle it in a follow-up patch > series. But it would be nice to say so in an in-code comment. > I think _can't_ wasn't the best terminology. My previous series actually added a reflog, but I noticed a bunch of tests were failing and I think it made sense to keep the existing behaviour. But addressing it as a bug would definitely be a good way to go and fix this, I'll add a comment in the code for now.
Karthik Nayak <karthik.188@gmail.com> writes: > ... By > having zero-oid for the new and old value in the reflog. But seems like > we can't do that either. Even if you could, the transition from refs/heads/main to refs/heads/next would not be captured. In any case, that is not something this step changes. My assumption is that for most of developers it would be unexpected for the theme to "allow symbolic updates to be transactional" to change it, hence "we do not explain what we did not change". Thanks.
Patrick Steinhardt <ps@pks.im> writes: > I wouldn't say we can't do that. We already do log when symrefs become > dangling when updating references via HEAD by logging a zero OID as new > OID. That is, if we have "HEAD -> refs/heads/foo" and you delete the > latter, then we create a new reflog message for "HEAD" with zero OID as > new OID. > > I would claim that the current behaviour where we don't create a reflog > entry when updating a ref to become dangling is a mere bug. I think it's > fair to declare this a #leftoverbit and handle it in a follow-up patch > series. But it would be nice to say so in an in-code comment. I like that. Thanks.
Karthik Nayak <karthik.188@gmail.com> writes: > @@ -2863,12 +2928,26 @@ 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)) { > + int create_reflog = 1; > + > + 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)) { > + /* for dangling symrefs we skip creating a reflog entry. */ > + create_reflog = 0; > + } > + } > + > + if (create_reflog && 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", This hunk is overly wide. You could of course fix it mechanically (e.g., by rewrapping overly wide comment block, wrapping an expression after &&-), but a code path that is too deeply indented may be a sign that the function may want to be split into calls to a smaller helper function for readability.
diff --git a/refs.c b/refs.c index 5dfe93060a..a4dca08244 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,15 @@ struct ref_update *ref_transaction_add_update( update->flags = flags; - if (flags & REF_HAVE_NEW) + if (new_target) + update->new_target = xstrdup(new_target); + if (old_target) + update->old_target = xstrdup(old_target); + if (new_oid && flags & REF_HAVE_NEW) oidcpy(&update->new_oid, new_oid); - if (flags & REF_HAVE_OLD) + if (old_oid && flags & REF_HAVE_OLD) oidcpy(&update->old_oid, old_oid); + update->msg = normalize_reflog_message(msg); return update; } @@ -1286,6 +1293,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, @@ -2810,3 +2818,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_is_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 878601ced0..85c4af7e89 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2426,6 +2426,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 @@ -2486,7 +2516,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_is_null_new_value(update)) update->flags |= REF_DELETING; if (head_ref) { @@ -2529,7 +2559,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; } @@ -2550,7 +2587,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; } @@ -2568,9 +2615,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)) { + 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)) { /* @@ -2863,12 +2928,26 @@ 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)) { + int create_reflog = 1; + + 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)) { + /* for dangling symrefs we skip creating a reflog entry. */ + create_reflog = 0; + } + } + + if (create_reflog && 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", @@ -2880,6 +2959,15 @@ static int files_transaction_finish(struct ref_store *ref_store, 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..9578665243 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_is_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..5e8a696d40 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_is_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_is_null_new_value(u)) { struct reftable_log_record log = {0}; struct reftable_iterator it = {0}; @@ -1084,24 +1101,44 @@ 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; - 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->new_target) + if (!refs_resolve_ref_unsafe(&arg->refs->base, u->new_target, + RESOLVE_REF_READING, &u->new_oid, NULL)) + /* for dangling symrefs we skip creating reflog */ + create_reflog = 0; + + 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->flags & REF_HAVE_NEW && 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_is_null_new_value(u)) { struct reftable_ref_record ref = { .refname = (char *)u->refname, .update_index = ts,