From 1bc4cc3fc4276203e62de610a712c8ddea45b5cf Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:52 +0200 Subject: refs: accept symref values in `ref_transaction_update()` The function `ref_transaction_update()` obtains ref information and flags to create a `ref_update` and add them to the transaction at hand. To extend symref support in transactions, we need to also accept the old and new ref targets and process it. This commit adds the required parameters to the function and modifies all call sites. The two parameters added are `new_target` and `old_target`. The `new_target` is used to denote what the reference should point to when the transaction is applied. Some functions allow this parameter to be NULL, meaning that the reference is not changed. The `old_target` denotes the value the reference must have before the update. Some functions allow this parameter to be NULL, meaning that the old value of the reference is not checked. We also update the internal function `ref_transaction_add_update()` similarly to take the two new parameters. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- refs.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 55d2e0b2cb..e7b7c48d92 100644 --- a/refs.c +++ b/refs.c @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update( const char *refname, unsigned int flags, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, const char *old_target, const char *msg) { struct ref_update *update; @@ -1235,6 +1236,11 @@ struct ref_update *ref_transaction_add_update( if (transaction->state != REF_TRANSACTION_OPEN) BUG("update called for transaction that is not open"); + if (old_oid && old_target) + BUG("only one of old_oid and old_target should be non NULL"); + if (new_oid && new_target) + BUG("only one of new_oid and new_target should be non NULL"); + FLEX_ALLOC_STR(update, refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; @@ -1253,6 +1259,8 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, + const char *old_target, unsigned int flags, const char *msg, struct strbuf *err) { @@ -1280,7 +1288,8 @@ int ref_transaction_update(struct ref_transaction *transaction, flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, - new_oid, old_oid, msg); + new_oid, old_oid, new_target, + old_target, msg); return 0; } @@ -1295,7 +1304,8 @@ int ref_transaction_create(struct ref_transaction *transaction, return 1; } return ref_transaction_update(transaction, refname, new_oid, - null_oid(), flags, msg, err); + null_oid(), NULL, NULL, flags, + msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, @@ -1308,7 +1318,8 @@ int ref_transaction_delete(struct ref_transaction *transaction, BUG("delete called with old_oid set to zeros"); return ref_transaction_update(transaction, refname, null_oid(), old_oid, - flags, msg, err); + NULL, NULL, flags, + msg, err); } int ref_transaction_verify(struct ref_transaction *transaction, @@ -1321,6 +1332,7 @@ int ref_transaction_verify(struct ref_transaction *transaction, BUG("verify called with old_oid set to NULL"); return ref_transaction_update(transaction, refname, NULL, old_oid, + NULL, NULL, flags, NULL, err); } @@ -1335,8 +1347,8 @@ int refs_update_ref(struct ref_store *refs, const char *msg, t = ref_store_transaction_begin(refs, &err); if (!t || - ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, - &err) || + ref_transaction_update(t, refname, new_oid, old_oid, NULL, NULL, + flags, msg, &err) || ref_transaction_commit(t, &err)) { ret = 1; ref_transaction_free(t); -- cgit v1.2.3 From a8ae923f85da6434c3faf9c39719d6d5e5c77e65 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:54 +0200 Subject: refs: support symrefs in 'reference-transaction' hook The 'reference-transaction' hook runs whenever a reference update is made to the system. In a previous commit, we added the `old_target` and `new_target` fields to the `reference_transaction_update()`. In following commits we'll also add the code to handle symref's in the reference backends. Support symrefs also in the 'reference-transaction' hook, by modifying the current format: SP SP LF to be be: SP SP LF where for regular refs the output would not change and remain the same. But when either 'old-value' or 'new-value' is a symref, we print the ref as 'ref:'. This does break backward compatibility, but the 'reference-transaction' hook's documentation always stated that support for symbolic references may be added in the future. We do not add any tests in this commit since there is no git command which activates this flow, in an upcoming commit, we'll start using transaction based symref updates as the default, we'll add tests there for the hook too. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- Documentation/githooks.txt | 14 +++++++++----- refs.c | 20 ++++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) (limited to 'refs.c') diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index ee9b92c90d..06e997131b 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -486,7 +486,7 @@ reference-transaction This hook is invoked by any Git command that performs reference updates. It executes whenever a reference transaction is prepared, committed or aborted and may thus get called multiple times. The hook -does not cover symbolic references (but that may change in the future). +also supports symbolic reference updates. The hook takes exactly one argument, which is the current state the given reference transaction is in: @@ -503,16 +503,20 @@ given reference transaction is in: For each reference update that was added to the transaction, the hook receives on standard input a line of the format: - SP SP LF + SP SP LF -where `` is the old object name passed into the reference -transaction, `` is the new object name to be stored in the +where `` is the old object name passed into the reference +transaction, `` is the new object name to be stored in the ref and `` is the full name of the ref. When force updating the reference regardless of its current value or when the reference is -to be created anew, `` is the all-zeroes object name. To +to be created anew, `` is the all-zeroes object name. To distinguish these cases, you can inspect the current value of `` via `git rev-parse`. +For symbolic reference updates the `` and `` +fields could denote references instead of objects. A reference will be +denoted with a 'ref:' prefix, like `ref:`. + The exit status of the hook is ignored for any state except for the "prepared" state. In the "prepared" state, a non-zero exit status will cause the transaction to be aborted. The hook will not be called with diff --git a/refs.c b/refs.c index e7b7c48d92..9d722d798a 100644 --- a/refs.c +++ b/refs.c @@ -2350,10 +2350,22 @@ static int run_transaction_hook(struct ref_transaction *transaction, struct ref_update *update = transaction->updates[i]; strbuf_reset(&buf); - strbuf_addf(&buf, "%s %s %s\n", - oid_to_hex(&update->old_oid), - oid_to_hex(&update->new_oid), - update->refname); + + if (!(update->flags & REF_HAVE_OLD)) + strbuf_addf(&buf, "%s ", oid_to_hex(null_oid())); + else if (update->old_target) + strbuf_addf(&buf, "ref:%s ", update->old_target); + else + strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid)); + + if (!(update->flags & REF_HAVE_NEW)) + strbuf_addf(&buf, "%s ", oid_to_hex(null_oid())); + else if (update->new_target) + strbuf_addf(&buf, "ref:%s ", update->new_target); + else + strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid)); + + strbuf_addf(&buf, "%s\n", update->refname); if (write_in_full(proc.in, buf.buf, buf.len) < 0) { if (errno != EPIPE) { -- cgit v1.2.3 From e9965ba477de5df7546773920c581569bb54f315 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:55 +0200 Subject: refs: move `original_update_refname` to 'refs.c' The files backend and the reftable backend implement `original_update_refname` to obtain the original refname of the update. Move it out to 'refs.c' and only expose it internally to the refs library. This will be used in an upcoming commit to also introduce another common functionality for the two backends. We also rename the function to `ref_update_original_update_refname` to keep it consistent with the upcoming other 'ref_update_*' functions that'll be introduced. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- refs.c | 8 ++++++++ refs/files-backend.c | 21 +++++---------------- refs/refs-internal.h | 5 +++++ refs/reftable-backend.c | 24 +++++++----------------- 4 files changed, 25 insertions(+), 33 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 9d722d798a..990703ed16 100644 --- a/refs.c +++ b/refs.c @@ -2814,3 +2814,11 @@ 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); } + +const char *ref_update_original_update_refname(struct ref_update *update) +{ + while (update->parent_update) + update = update->parent_update; + + return update->refname; +} diff --git a/refs/files-backend.c b/refs/files-backend.c index 74a713090c..25e5d03496 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2423,17 +2423,6 @@ static int split_symref_update(struct ref_update *update, return 0; } -/* - * Return the refname under which update was originally requested. - */ -static const char *original_update_refname(struct ref_update *update) -{ - while (update->parent_update) - update = update->parent_update; - - return update->refname; -} - /* * 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 @@ -2450,16 +2439,16 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, if (is_null_oid(&update->old_oid)) strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", - original_update_refname(update)); + ref_update_original_update_refname(update)); else if (is_null_oid(oid)) strbuf_addf(err, "cannot lock ref '%s': " "reference is missing but expected %s", - original_update_refname(update), + ref_update_original_update_refname(update), oid_to_hex(&update->old_oid)); else strbuf_addf(err, "cannot lock ref '%s': " "is at %s but expected %s", - original_update_refname(update), + ref_update_original_update_refname(update), oid_to_hex(oid), oid_to_hex(&update->old_oid)); @@ -2513,7 +2502,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, reason = strbuf_detach(err, NULL); strbuf_addf(err, "cannot lock ref '%s': %s", - original_update_refname(update), reason); + ref_update_original_update_refname(update), reason); free(reason); goto out; } @@ -2533,7 +2522,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (update->flags & REF_HAVE_OLD) { strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", - original_update_refname(update)); + ref_update_original_update_refname(update)); ret = TRANSACTION_GENERIC_ERROR; goto out; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 108f4ec419..617b93a6c8 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -749,4 +749,9 @@ 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); +/* + * Return the refname under which update was originally requested. + */ +const char *ref_update_original_update_refname(struct ref_update *update); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e6122837c0..fe4a7681a0 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -578,16 +578,6 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, return ret; } -/* - * Return the refname under which update was originally requested. - */ -static const char *original_update_refname(struct ref_update *update) -{ - while (update->parent_update) - update = update->parent_update; - return update->refname; -} - struct reftable_transaction_update { struct ref_update *update; struct object_id current_oid; @@ -866,7 +856,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, /* The reference does not exist, but we expected it to. */ strbuf_addf(err, _("cannot lock ref '%s': " "unable to resolve reference '%s'"), - original_update_refname(u), u->refname); + ref_update_original_update_refname(u), u->refname); ret = -1; goto done; } @@ -938,17 +928,17 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, 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"), - original_update_refname(u)); + "reference already exists"), + ref_update_original_update_refname(u)); else if (is_null_oid(¤t_oid)) strbuf_addf(err, _("cannot lock ref '%s': " - "reference is missing but expected %s"), - original_update_refname(u), + "reference is missing but expected %s"), + ref_update_original_update_refname(u), oid_to_hex(&u->old_oid)); else strbuf_addf(err, _("cannot lock ref '%s': " - "is at %s but expected %s"), - original_update_refname(u), + "is at %s but expected %s"), + ref_update_original_update_refname(u), oid_to_hex(¤t_oid), oid_to_hex(&u->old_oid)); ret = -1; -- cgit v1.2.3 From 644daf7785d6f2fa74330fc246d2bd4d3f8bbab2 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:56 +0200 Subject: refs: add support for transactional symref updates 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 do not support transactional updates of symrefs. This commit adds support for symrefs in both the 'files' and the 'reftable' backend. 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`. We also add another common function `ref_update_check_old_target` which will be used to check if the update's old_target corresponds to a reference's current target. Now transactional updates (verify, create, delete, update) can be used for: - regular refs - symbolic refs - conversion of regular to symbolic refs and vice versa This also allows us to expose this to users via new commands in 'git-update-ref' in the future. Note that a dangling symref update does not record a new reflog entry, which is unchanged before and after this commit. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- refs.c | 37 +++++++++++++++- refs/files-backend.c | 113 +++++++++++++++++++++++++++++++++++++++--------- refs/refs-internal.h | 16 +++++++ refs/reftable-backend.c | 71 ++++++++++++++++++++++-------- 4 files changed, 197 insertions(+), 40 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 990703ed16..d0ea7573d8 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, @@ -2822,3 +2828,30 @@ const char *ref_update_original_update_refname(struct ref_update *update) return update->refname; } + +int ref_update_has_null_new_value(struct ref_update *update) +{ + return !update->new_target && is_null_oid(&update->new_oid); +} + +int ref_update_check_old_target(const char *referent, struct ref_update *update, + struct strbuf *err) +{ + if (!update->old_target) + BUG("called without old_target set"); + + if (!strcmp(referent, update->old_target)) + return 0; + + if (!strcmp(referent, "")) + strbuf_addf(err, "verifying symref target: '%s': " + "reference is missing but expected %s", + ref_update_original_update_refname(update), + update->old_target); + else + strbuf_addf(err, "verifying symref target: '%s': " + "is at %s but expected %s", + ref_update_original_update_refname(update), + referent, update->old_target); + return -1; +} diff --git a/refs/files-backend.c b/refs/files-backend.c index 25e5d03496..2d1525b240 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2394,8 +2394,9 @@ static int split_symref_update(struct ref_update *update, new_update = ref_transaction_add_update( transaction, referent, new_flags, - &update->new_oid, &update->old_oid, - NULL, NULL, update->msg); + update->new_target ? NULL : &update->new_oid, + update->old_target ? NULL : &update->old_oid, + update->new_target, update->old_target, update->msg); new_update->parent_update = update; @@ -2483,7 +2484,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) { @@ -2526,7 +2527,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 (ref_update_check_old_target(referent.buf, update, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + } else if (check_old_oid(update, &lock->old_oid, err)) { ret = TRANSACTION_GENERIC_ERROR; goto out; } @@ -2547,7 +2555,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 (ref_update_check_old_target(referent.buf, update, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } + } else if (check_old_oid(update, &lock->old_oid, err)) { ret = TRANSACTION_GENERIC_ERROR; goto out; } @@ -2565,9 +2583,28 @@ 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)) { /* @@ -2830,6 +2867,43 @@ cleanup: 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) @@ -2860,23 +2934,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 617b93a6c8..819157256e 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -754,4 +754,20 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor */ const char *ref_update_original_update_refname(struct ref_update *update); +/* + * 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); + +/* + * Check whether the old_target values stored in update are consistent + * with the referent, which is the symbolic reference's current value. + * If everything is OK, return 0; otherwise, write an error message to + * err and return -1. + */ +int ref_update_check_old_target(const char *referent, struct ref_update *update, + struct strbuf *err); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index fe4a7681a0..4ad0d81371 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -843,7 +843,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) @@ -894,8 +894,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; /* @@ -925,7 +927,12 @@ 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 (ref_update_check_old_target(referent.buf, u, err)) { + 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"), @@ -1030,7 +1037,9 @@ 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}; @@ -1071,24 +1080,52 @@ 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, -- cgit v1.2.3 From 300b38e46f951bab8c6c5cb779b59322be321081 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:57 +0200 Subject: refs: use transaction in `refs_create_symref()` The `refs_create_symref()` function updates a symref to a given new target. To do this, it uses a ref-backend specific function `create_symref()`. In the previous commits, we introduced symref support in transactions. This means we can now use transactions to perform symref updates and don't have to resort to `create_symref()`. Doing this allows us to remove and cleanup `create_symref()`, which we will do in the following commit. Modify the expected error message for a test in 't/t0610-reftable-basics.sh', since the error is now thrown from 'refs.c'. This is because in transactional updates, F/D conflicts are caught before we're in the reference backend. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- refs.c | 24 +++++++++++++++++------- t/t0610-reftable-basics.sh | 2 +- t/t1416-ref-transaction-hooks.sh | 23 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 8 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index d0ea7573d8..507f5f0525 100644 --- a/refs.c +++ b/refs.c @@ -2289,14 +2289,24 @@ int refs_create_symref(struct ref_store *refs, const char *refs_heads_master, const char *logmsg) { - char *msg; - int retval; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; + int ret = 0; - msg = normalize_reflog_message(logmsg); - retval = refs->be->create_symref(refs, ref_target, refs_heads_master, - msg); - free(msg); - return retval; + transaction = ref_store_transaction_begin(refs, &err); + if (!transaction || + ref_transaction_update(transaction, ref_target, NULL, NULL, + refs_heads_master, NULL, REF_NO_DEREF, + logmsg, &err) || + ref_transaction_commit(transaction, &err)) { + ret = error("%s", err.buf); + } + + strbuf_release(&err); + if (transaction) + ref_transaction_free(transaction); + + return ret; } int create_symref(const char *ref_target, const char *refs_heads_master, diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 931d888bbb..01758fc565 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -255,7 +255,7 @@ test_expect_success 'ref transaction: creating symbolic ref fails with F/D confl git init repo && test_commit -C repo A && cat >expect <<-EOF && - error: unable to write symref for refs/heads: file/directory conflict + error: ${SQ}refs/heads/main${SQ} exists; cannot create ${SQ}refs/heads${SQ} EOF test_must_fail git -C repo symbolic-ref refs/heads refs/heads/foo 2>err && test_cmp expect err diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 2092488090..067fd57290 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -134,4 +134,27 @@ test_expect_success 'interleaving hook calls succeed' ' test_cmp expect target-repo.git/actual ' +test_expect_success 'hook captures git-symbolic-ref updates' ' + test_when_finished "rm actual" && + + test_hook reference-transaction <<-\EOF && + echo "$*" >>actual + while read -r line + do + printf "%s\n" "$line" + done >>actual + EOF + + git symbolic-ref refs/heads/symref refs/heads/main && + + cat >expect <<-EOF && + prepared + $ZERO_OID ref:refs/heads/main refs/heads/symref + committed + $ZERO_OID ref:refs/heads/main refs/heads/symref + EOF + + test_cmp expect actual +' + test_done -- cgit v1.2.3 From f151dfe3c91de744a2ced6ae701d1d8c2215bfd6 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 7 May 2024 14:58:58 +0200 Subject: refs: rename `refs_create_symref()` to `refs_update_symref()` The `refs_create_symref()` function is used to update/create a symref. But it doesn't check the old target of the symref, if existing. It force updates the symref. In this regard, the name `refs_create_symref()` is a bit misleading. So let's rename it to `refs_update_symref()`. This is akin to how 'git-update-ref(1)' also allows us to create apart from update. While we're here, rename the arguments in the function to clarify what they actually signify and reduce confusion. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/worktree.c | 2 +- refs.c | 12 +++++------- refs.h | 2 +- t/helper/test-ref-store.c | 2 +- 5 files changed, 9 insertions(+), 11 deletions(-) (limited to 'refs.c') diff --git a/builtin/branch.c b/builtin/branch.c index dd3e3a7dc0..4491f7a20c 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -555,7 +555,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, continue; refs = get_worktree_ref_store(worktrees[i]); - if (refs_create_symref(refs, "HEAD", newref, logmsg)) + if (refs_update_symref(refs, "HEAD", newref, logmsg)) ret = error(_("HEAD of working tree %s is not updated"), worktrees[i]->path); } diff --git a/builtin/worktree.c b/builtin/worktree.c index 7c6c72536b..480202c517 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -517,7 +517,7 @@ static int add_worktree(const char *path, const char *refname, ret = refs_update_ref(wt_refs, NULL, "HEAD", &commit->object.oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR); else - ret = refs_create_symref(wt_refs, "HEAD", symref.buf, NULL); + ret = refs_update_symref(wt_refs, "HEAD", symref.buf, NULL); if (ret) goto done; diff --git a/refs.c b/refs.c index 507f5f0525..fa5471d219 100644 --- a/refs.c +++ b/refs.c @@ -2284,10 +2284,8 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled) return peel_object(base, peeled) ? -1 : 0; } -int refs_create_symref(struct ref_store *refs, - const char *ref_target, - const char *refs_heads_master, - const char *logmsg) +int refs_update_symref(struct ref_store *refs, const char *ref, + const char *target, const char *logmsg) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2295,8 +2293,8 @@ int refs_create_symref(struct ref_store *refs, transaction = ref_store_transaction_begin(refs, &err); if (!transaction || - ref_transaction_update(transaction, ref_target, NULL, NULL, - refs_heads_master, NULL, REF_NO_DEREF, + ref_transaction_update(transaction, ref, NULL, NULL, + target, NULL, REF_NO_DEREF, logmsg, &err) || ref_transaction_commit(transaction, &err)) { ret = error("%s", err.buf); @@ -2312,7 +2310,7 @@ int refs_create_symref(struct ref_store *refs, int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { - return refs_create_symref(get_main_ref_store(the_repository), ref_target, + return refs_update_symref(get_main_ref_store(the_repository), ref_target, refs_heads_master, logmsg); } diff --git a/refs.h b/refs.h index c7851bf587..71cc1c58e0 100644 --- a/refs.h +++ b/refs.h @@ -606,7 +606,7 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref, int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg); -int refs_create_symref(struct ref_store *refs, const char *refname, +int refs_update_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg); int create_symref(const char *refname, const char *target, const char *logmsg); diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 82bbf6e2e6..4651e4ced7 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -118,7 +118,7 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv) const char *target = notnull(*argv++, "target"); const char *logmsg = *argv++; - return refs_create_symref(refs, refname, target, logmsg); + return refs_update_symref(refs, refname, target, logmsg); } static struct flag_definition transaction_flags[] = { -- cgit v1.2.3