* [PATCH v4 3/7] update-ref: add support for 'symref-verify' command
2024-04-26 15:24 ` [PATCH v4 0/7] add symref-* commands to 'git-update-ref --stdin' Karthik Nayak
@ 2024-04-26 15:24 ` Karthik Nayak
2024-04-26 22:51 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Karthik Nayak @ 2024-04-26 15:24 UTC (permalink / raw)
To: karthik.188; +Cc: christian.couder, git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
In the previous commits, we added the required base for adding symref
commands to the '--stdin' mode provided by 'git-update-ref(1)'. Using
them, add a new 'symref-verify' command to verify symrefs.
The 'symref-verify' command allows users to verify if a provided <ref>
contains the provided <old-target> without changing the <ref>. If
<old-target> is not provided, the command will verify that the <ref>
doesn't exist. Since we're checking for symbolic refs, this command will
only work with the 'no-deref' mode. This is because any dereferenced
symbolic ref will point to an object and not a ref and the regular
'verify' command can be used in such situations.
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. The new
value could either be a symref target `new_target` or a OID `new_oid`.
We also add tests to test the command in both the regular stdin mode and
also with the '-z' flag.
We also disable the reference-transaction hook for symref-updates which
will be tackled in its own commit.
Add required tests for symref support in 'verify' while also adding
reflog checks for the pre-existing 'verify' tests.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 7 +++
builtin/update-ref.c | 80 +++++++++++++++++++++++----
refs.c | 30 +++++++++--
refs.h | 1 +
refs/files-backend.c | 43 +++++++++++++++
refs/refs-internal.h | 7 +++
refs/reftable-backend.c | 21 +++++++-
t/t1400-update-ref.sh | 93 +++++++++++++++++++++++++++++++-
8 files changed, 264 insertions(+), 18 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9fe78b3501 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
start LF
prepare LF
@@ -86,6 +87,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
start NUL
prepare NUL
@@ -117,6 +119,11 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-verify::
+ Verify symbolic <ref> against <old-target> but do not change it.
+ If <old-target> is missing, the ref must not exist. Can only be
+ used in `no-deref` mode.
+
option::
Modify the behavior of the next command naming a <ref>.
The only valid option is `no-deref` to avoid dereferencing
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 21fdbf6ac8..419b28169b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -76,6 +76,29 @@ static char *parse_refname(const char **next)
return strbuf_detach(&ref, NULL);
}
+/*
+ * Wrapper around parse_refname which skips the next delimiter.
+ */
+static char *parse_next_refname(const char **next)
+{
+ if (line_termination) {
+ /* Without -z, consume SP and use next argument */
+ if (!**next || **next == line_termination)
+ return NULL;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ } else {
+ /* With -z, read the next NUL-terminated line */
+ if (**next)
+ return NULL;
+ }
+ /* Skip the delimiter */
+ (*next)++;
+
+ return parse_refname(next);
+}
+
+
/*
* The value being parsed is <old-oid> (as opposed to <new-oid>; the
* difference affects which error messages are generated):
@@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
die("verify %s: extra input: %s", refname, next);
if (ref_transaction_verify(transaction, refname, &old_oid,
- update_flags, &err))
+ NULL, update_flags, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ strbuf_release(&err);
+}
+
+static void parse_cmd_symref_verify(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ struct object_id old_oid;
+ char *refname, *old_target;
+
+ if (!(update_flags & REF_NO_DEREF))
+ die("symref-verify: cannot operate with deref mode");
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-verify: missing <ref>");
+
+ /*
+ * old_ref is optional, but we want to differentiate between
+ * a NULL and zero value.
+ */
+ old_target = parse_next_refname(&next);
+ if (!old_target)
+ old_oid = *null_oid();
+
+ if (*next != line_termination)
+ die("symref-verify %s: extra input: %s", refname, next);
+
+ if (ref_transaction_verify(transaction, refname,
+ old_target ? NULL : &old_oid,
+ old_target, update_flags, &err))
die("%s", err.buf);
update_flags = default_flags;
free(refname);
+ free(old_target);
strbuf_release(&err);
}
@@ -380,15 +439,16 @@ static const struct parse_cmd {
unsigned args;
enum update_refs_state state;
} command[] = {
- { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
- { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
- { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
- { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
- { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
- { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
- { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
- { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
- { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
+ { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
+ { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
+ { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
+ { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
+ { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
+ { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
+ { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+ { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
+ { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
};
static void update_refs_stdin(void)
diff --git a/refs.c b/refs.c
index 060a31616d..0e1013b5ab 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((void *)transaction->updates[i]->old_target);
+ free((void *)transaction->updates[i]->new_target);
free(transaction->updates[i]);
}
free(transaction->updates);
@@ -1247,9 +1249,13 @@ 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 +1292,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,
@@ -1325,14 +1332,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err)
{
- if (!old_oid)
- BUG("verify called with old_oid set to NULL");
+ if (!old_target && !old_oid)
+ BUG("verify called with old_oid and old_target set to NULL");
+ if (old_target && !(flags & REF_NO_DEREF))
+ BUG("verify cannot operate on symrefs with deref mode");
return ref_transaction_update(transaction, refname,
NULL, old_oid,
- NULL, NULL,
+ NULL, old_target,
flags, NULL, err);
}
@@ -2349,6 +2359,12 @@ static int run_transaction_hook(struct ref_transaction *transaction,
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
+ /*
+ * Skip reference transaction for symbolic refs.
+ */
+ if (update->new_target || update->old_target)
+ continue;
+
strbuf_reset(&buf);
strbuf_addf(&buf, "%s %s %s\n",
oid_to_hex(&update->old_oid),
@@ -2802,3 +2818,7 @@ 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.h b/refs.h
index c792e13a64..27b9aeaf54 100644
--- a/refs.h
+++ b/refs.h
@@ -780,6 +780,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2420dac2aa..53197fa3af 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2425,6 +2425,37 @@ static const char *original_update_refname(struct ref_update *update)
return update->refname;
}
+/*
+ * Check whether the REF_HAVE_OLD and old_target values stored in
+ * update are consistent with ref, 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, char *ref,
+ struct strbuf *err)
+{
+ if (!(update->flags & REF_HAVE_OLD) ||
+ !strcmp(update->old_target, ref))
+ return 0;
+
+ if (!strcmp(update->old_target, ""))
+ strbuf_addf(err, "cannot lock ref '%s': "
+ "reference already exists",
+ original_update_refname(update));
+ else if (!strcmp(ref, ""))
+ 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),
+ ref, 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
@@ -2528,6 +2559,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
ret = TRANSACTION_GENERIC_ERROR;
goto out;
}
+ }
+
+ /*
+ * For symref verification, we need to check the reference value
+ * rather than the oid. If we're dealing with regular refs or we're
+ * verifying a dereferenced symref, we then check the oid.
+ */
+ 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;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3040d4797c..23e65f65e8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -748,4 +748,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..a2474245aa 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -938,7 +938,26 @@ 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->flags & REF_HAVE_OLD) && u->old_target) {
+ if (strcmp(referent.buf, u->old_target)) {
+ if (!strcmp(u->old_target, ""))
+ strbuf_addf(err, "verifying symref target: '%s': "
+ "provided target is empty",
+ original_update_refname(u));
+ else 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"),
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ec3443cc87..34b29eeac8 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -890,17 +890,23 @@ test_expect_success 'stdin update/create/verify combination works' '
'
test_expect_success 'stdin verify succeeds for correct value' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
git rev-parse $m >expect &&
echo "verify $m $m" >stdin &&
git update-ref --stdin <stdin &&
git rev-parse $m >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify succeeds for missing reference' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
echo "verify refs/heads/missing $Z" >stdin &&
git update-ref --stdin <stdin &&
- test_must_fail git rev-parse --verify -q refs/heads/missing
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify treats no value as missing' '
@@ -1641,4 +1647,87 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
+create_stdin_buf () {
+ if test "$1" = "-z"
+ then
+ shift
+ printf "$F" "$@" >stdin
+ else
+ echo "$@" >stdin
+ fi
+}
+
+for type in "" "-z"
+do
+
+ test_expect_success "stdin ${type} symref-verify fails without --no-deref" '
+ git symbolic-ref refs/heads/symref $a &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" &&
+ test_must_fail git update-ref --stdin ${type} <stdin 2>err &&
+ grep "fatal: symref-verify: cannot operate with deref mode" err
+ '
+
+ test_expect_success "stdin ${type} symref-verify fails with too many arguments" '
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" "$a" &&
+ test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-verify refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin ${type} symref-verify succeeds for correct value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "$a" &&
+ git update-ref --stdin ${type} --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin ${type} symref-verify no value is treated as zero value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "" &&
+ test_must_fail git update-ref --stdin ${type} --no-deref <stdin
+ '
+
+ test_expect_success "stdin ${type} symref-verify succeeds for dangling reference" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
+ test_must_fail git symbolic-ref refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref2" "refs/heads/nonexistent" &&
+ git update-ref --stdin ${type} --no-deref <stdin
+ '
+
+ test_expect_success "stdin ${type} symref-verify succeeds for missing reference" '
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ create_stdin_buf ${type} "symref-verify refs/heads/missing" "$Z" &&
+ git update-ref --stdin ${type} --no-deref <stdin &&
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin ${type} symref-verify fails for wrong value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "$b" &&
+ test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin ${type} symref-verify fails for mistaken null value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ create_stdin_buf ${type} "symref-verify refs/heads/symref" "$Z" &&
+ test_must_fail git update-ref --stdin ${type} --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+done
+
test_done
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/7] update-ref: add support for 'symref-verify' command
2024-04-26 15:24 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
@ 2024-04-26 22:51 ` Junio C Hamano
2024-04-28 22:28 ` Karthik Nayak
0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2024-04-26 22:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: christian.couder, git, ps
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> In the previous commits, we added the required base for adding symref
> commands to the '--stdin' mode provided by 'git-update-ref(1)'. Using
> them, add a new 'symref-verify' command to verify symrefs.
>
> The 'symref-verify' command allows users to verify if a provided <ref>
> contains the provided <old-target> without changing the <ref>. If
> <old-target> is not provided, the command will verify that the <ref>
> doesn't exist. Since we're checking for symbolic refs, this command will
> only work with the 'no-deref' mode. This is because any dereferenced
> symbolic ref will point to an object and not a ref and the regular
> 'verify' command can be used in such situations.
All makes sense, but a naïve reader may find it helpful if you
explained why having "verify" command is a good idea in the first
place ("I can just do 'git symoblic-ref' to read the current value,
and see if it is what I expect"). Presumably the value of "verify"
is that you can have it in a transaction and fail other operations
in the same transaction if the symref moved from what you expected
it to point at?
> 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. The new
> value could either be a symref target `new_target` or a OID `new_oid`.
> We also add tests to test the command in both the regular stdin mode and
> also with the '-z' flag.
This looks out of place, primarily because the helper function is
*NOT* used in this step. Without any actual user, and with the name
that says only what it checks without hinting why a caller may want
to check the condition it checks, it is hard to guess if it is a
good idea to have such a helper.
"If a ref_update object specifies no new-oid and no new-target, it
is not about updating but just validating" is how the callers are
expected to use it, then instead of is_null_new_value that says
what it checks, something like is_verify_only that says what the
caller may want to use it for would be a more friendly name for
readers and future developers.
> @@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
> die("verify %s: extra input: %s", refname, next);
>
> if (ref_transaction_verify(transaction, refname, &old_oid,
> - update_flags, &err))
> + NULL, update_flags, &err))
>
> update_flags = default_flags;
> free(refname);
> strbuf_release(&err);
> }
The only damage by this patch to parse_cmd_verify() is that
ref_transaction_verify() gained another parameter NULL, but with the
default "--diff-algorithm=myers" algorithm, it is very hard to see.
The "--patience" algorithm does a much beter job on this hunk.
And the following function is entirely new.
> +static void parse_cmd_symref_verify(struct ref_transaction *transaction,
> + const char *next, const char *end)
> +{
> + struct strbuf err = STRBUF_INIT;
> + struct object_id old_oid;
> + char *refname, *old_target;
> +
> + if (!(update_flags & REF_NO_DEREF))
> + die("symref-verify: cannot operate with deref mode");
> +
> + refname = parse_refname(&next);
> + if (!refname)
> + die("symref-verify: missing <ref>");
> +
> + /*
> + * old_ref is optional, but we want to differentiate between
> + * a NULL and zero value.
> + */
> + old_target = parse_next_refname(&next);
> + if (!old_target)
> + old_oid = *null_oid();
In many existing code paths, we do not do structure assignment like
this. Instead we do
oidcpy(&old_oid, null_oid());
We can see an existing example in a common context in a hunk for
refs.c in this patch.
> + if (*next != line_termination)
> + die("symref-verify %s: extra input: %s", refname, next);
> +
> + if (ref_transaction_verify(transaction, refname,
> + old_target ? NULL : &old_oid,
> + old_target, update_flags, &err))
> + die("%s", err.buf);
Are static analyzers smart enough to notice that we will not be
using old_oid uninitialized here? Just wondering.
Anyway. This ensures ref_transaction_verify() gets either
old_target or old_oid, but never both at the same time. The caller
to ref_transaction_verify() in the previous function passed NULL for
old_target but it always had a non-NULL old_oid so that is perfectly
fine.
> + update_flags = default_flags;
> + free(refname);
> + free(old_target);
> + strbuf_release(&err);
> +}
> diff --git a/refs.c b/refs.c
> index 060a31616d..0e1013b5ab 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((void *)transaction->updates[i]->old_target);
> + free((void *)transaction->updates[i]->new_target);
> free(transaction->updates[i]);
> }
> free(transaction->updates);
> @@ -1247,9 +1249,13 @@ 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);
Presumably "update" structure, when freshly initialized, has NULL in
both of these _target members? Otherwise ref_transaction_free()
would get in trouble, so double checking.
> + 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);
Since we can ask to work on a symbolic ref, new_oid / old_oid can be
NULL when REF_HAVE_NEW / REF_HAVE_OLD bit is on for _target members.
Makes me wonder if the code becomes easier to follow if the flag
bits are split into four (_NEW -> _NEW_OID + _NEW_TARGET), but let's
not worry about that for now.
> @@ -1286,6 +1292,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);
> @@ -1325,14 +1332,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
> int ref_transaction_verify(struct ref_transaction *transaction,
> const char *refname,
> const struct object_id *old_oid,
> + const char *old_target,
> unsigned int flags,
> struct strbuf *err)
> {
> - if (!old_oid)
> - BUG("verify called with old_oid set to NULL");
> + if (!old_target && !old_oid)
> + BUG("verify called with old_oid and old_target set to NULL");
Is it normal if you get _both_ set, or is it equally a BUG()?
The parse_*_verify() codepaths we saw earlier both made sure
only one of the two is non-NULL, and it is unclear what should
happen if both are non-NULL.
> + if (old_target && !(flags & REF_NO_DEREF))
> + BUG("verify cannot operate on symrefs with deref mode");
> return ref_transaction_update(transaction, refname,
> NULL, old_oid,
> - NULL, NULL,
> + NULL, old_target,
> flags, NULL, err);
> }
So this queues an ref_update object whose .new_oid and .new_target
are NULL, and .old_oid and .old_target are what the caller gave us
to check. The NULLs in .new* members hopefully do not mean "delete
this thing" ;-)
> @@ -2349,6 +2359,12 @@ static int run_transaction_hook(struct ref_transaction *transaction,
> for (i = 0; i < transaction->nr; i++) {
> struct ref_update *update = transaction->updates[i];
>
> + /*
> + * Skip reference transaction for symbolic refs.
> + */
> + if (update->new_target || update->old_target)
> + continue;
Is that a final design, or will the hooks have a chance to interfere?
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 2420dac2aa..53197fa3af 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2425,6 +2425,37 @@ static const char *original_update_refname(struct ref_update *update)
> return update->refname;
> }
>
> +/*
> + * Check whether the REF_HAVE_OLD and old_target values stored in
> + * update are consistent with ref, 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, char *ref,
> + struct strbuf *err)
> +{
> + if (!(update->flags & REF_HAVE_OLD) ||
> + !strcmp(update->old_target, ref))
> + return 0;
Earlier on the assignment side for "update" structure we saw above,
the guard was (old_target && flags & REF_HAVE_OLD), but here we
assume old_target is valid, which feels a bit asymmetric.
Yes, I can see that the caller does not call us when !old_target,
but still... Perhaps
if ((update->flags & REF_HAVE_OLD) && !update->old_target)
BUG(...);
or something? Or alternatively, perhaps !!update->old_target should
be the only thing we should check and ignore REF_HAVE_OLD bit? I am
not sure, but it smells like that the non-NULL-ness of old_target is
the only thing that matters (if it is not NULL, very early in the
control flow somebody would have set REF_HAVE_OLD bit to flags, no?).
It brings me back to my earlier question. Does REF_HAVE_OLD bit
serve a useful purpose in this code?
> + if (!strcmp(update->old_target, ""))
> + strbuf_addf(err, "cannot lock ref '%s': "
> + "reference already exists",
> + original_update_refname(update));
> + else if (!strcmp(ref, ""))
> + strbuf_addf(err, "cannot lock ref '%s': "
> + "reference is missing but expected %s",
> + original_update_refname(update),
> + update->old_target);
So... for old_target and ref, an empty string is a special value?
How? Shouldn't that be documented in the comment before the
function?
> + else
> + strbuf_addf(err, "cannot lock ref '%s': "
> + "is at %s but expected %s",
> + original_update_refname(update),
> + ref, 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
> @@ -2528,6 +2559,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> ret = TRANSACTION_GENERIC_ERROR;
> goto out;
> }
> + }
> +
> + /*
> + * For symref verification, we need to check the reference value
> + * rather than the oid. If we're dealing with regular refs or we're
> + * verifying a dereferenced symref, we then check the oid.
> + */
> + if (update->old_target) {
> + if (check_old_target(update, referent.buf, err)) {
> + ret = TRANSACTION_GENERIC_ERROR;
> + goto out;
> + }
We come here only when update->type has REF_ISSYMREF bit on (we
learned that value by calling lock_raw_ref()), and know referent.buf
has the current "target" value. That is consumed as "ref" parameter
to check_old_target() we just saw. OK.
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 6104471199..a2474245aa 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -938,7 +938,26 @@ 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->flags & REF_HAVE_OLD) && u->old_target) {
> + if (strcmp(referent.buf, u->old_target)) {
> + if (!strcmp(u->old_target, ""))
> + strbuf_addf(err, "verifying symref target: '%s': "
> + "provided target is empty",
> + original_update_refname(u));
> + else 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;
> + }
Again, the puzzling "empty string"s are handled here.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/7] update-ref: add support for 'symref-verify' command
2024-04-26 22:51 ` Junio C Hamano
@ 2024-04-28 22:28 ` Karthik Nayak
0 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-04-28 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git, ps
[-- Attachment #1: Type: text/plain, Size: 14893 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> In the previous commits, we added the required base for adding symref
>> commands to the '--stdin' mode provided by 'git-update-ref(1)'. Using
>> them, add a new 'symref-verify' command to verify symrefs.
>>
>> The 'symref-verify' command allows users to verify if a provided <ref>
>> contains the provided <old-target> without changing the <ref>. If
>> <old-target> is not provided, the command will verify that the <ref>
>> doesn't exist. Since we're checking for symbolic refs, this command will
>> only work with the 'no-deref' mode. This is because any dereferenced
>> symbolic ref will point to an object and not a ref and the regular
>> 'verify' command can be used in such situations.
>
> All makes sense, but a naïve reader may find it helpful if you
> explained why having "verify" command is a good idea in the first
> place ("I can just do 'git symoblic-ref' to read the current value,
> and see if it is what I expect"). Presumably the value of "verify"
> is that you can have it in a transaction and fail other operations
> in the same transaction if the symref moved from what you expected
> it to point at?
>
I would say none of the commits drive this point, and I would go ahead
and add something on these lines to each of them. I think it would add
good value to readers.
>> 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. The new
>> value could either be a symref target `new_target` or a OID `new_oid`.
>> We also add tests to test the command in both the regular stdin mode and
>> also with the '-z' flag.
>
> This looks out of place, primarily because the helper function is
> *NOT* used in this step. Without any actual user, and with the name
> that says only what it checks without hinting why a caller may want
> to check the condition it checks, it is hard to guess if it is a
> good idea to have such a helper.
>
I think over the revision, its usage from this commit was removed. It
makes sense to move it to a commit where its used, I'll do that.
> "If a ref_update object specifies no new-oid and no new-target, it
> is not about updating but just validating" is how the callers are
> expected to use it, then instead of is_null_new_value that says
> what it checks, something like is_verify_only that says what the
> caller may want to use it for would be a more friendly name for
> readers and future developers.
This is true for the old-oid and old-target. That is, when they are set
to null, we're validating.
With the new-oid and new-target, if they're null, it usually signifies
deletion. We could rename it to 'is_delete_only', but that would also
need checking the 'REF_HAVE_NEW' flag. So we could ideally change it to
```
int ref_update_is_delete_only(struct ref_update *update) {
return (update->flags & REF_HAVE_NEW) && !update->new_target &&
is_null_oid(&update->new_oid);
}
```
I'm okay with making this change.
>> @@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
>> die("verify %s: extra input: %s", refname, next);
>>
>> if (ref_transaction_verify(transaction, refname, &old_oid,
>> - update_flags, &err))
>> + NULL, update_flags, &err))
>>
>> update_flags = default_flags;
>> free(refname);
>> strbuf_release(&err);
>> }
>
> The only damage by this patch to parse_cmd_verify() is that
> ref_transaction_verify() gained another parameter NULL, but with the
> default "--diff-algorithm=myers" algorithm, it is very hard to see.
>
> The "--patience" algorithm does a much beter job on this hunk.
>
> And the following function is entirely new.
>
>> +static void parse_cmd_symref_verify(struct ref_transaction *transaction,
>> + const char *next, const char *end)
>> +{
>> + struct strbuf err = STRBUF_INIT;
>> + struct object_id old_oid;
>> + char *refname, *old_target;
>> +
>> + if (!(update_flags & REF_NO_DEREF))
>> + die("symref-verify: cannot operate with deref mode");
>> +
>> + refname = parse_refname(&next);
>> + if (!refname)
>> + die("symref-verify: missing <ref>");
>> +
>> + /*
>> + * old_ref is optional, but we want to differentiate between
>> + * a NULL and zero value.
>> + */
>> + old_target = parse_next_refname(&next);
>> + if (!old_target)
>> + old_oid = *null_oid();
>
> In many existing code paths, we do not do structure assignment like
> this. Instead we do
>
> oidcpy(&old_oid, null_oid());
>
> We can see an existing example in a common context in a hunk for
> refs.c in this patch.
>
Yeah, makes sense to switch this. Will do.
>> + if (*next != line_termination)
>> + die("symref-verify %s: extra input: %s", refname, next);
>> +
>> + if (ref_transaction_verify(transaction, refname,
>> + old_target ? NULL : &old_oid,
>> + old_target, update_flags, &err))
>> + die("%s", err.buf);
>
> Are static analyzers smart enough to notice that we will not be
> using old_oid uninitialized here? Just wondering.
Yup, at least the clang LSP server seems to detect and not bug me about
it.
> Anyway. This ensures ref_transaction_verify() gets either
> old_target or old_oid, but never both at the same time. The caller
> to ref_transaction_verify() in the previous function passed NULL for
> old_target but it always had a non-NULL old_oid so that is perfectly
> fine.
>
>> + update_flags = default_flags;
>> + free(refname);
>> + free(old_target);
>> + strbuf_release(&err);
>> +}
>
>> diff --git a/refs.c b/refs.c
>> index 060a31616d..0e1013b5ab 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((void *)transaction->updates[i]->old_target);
>> + free((void *)transaction->updates[i]->new_target);
>> free(transaction->updates[i]);
>> }
>> free(transaction->updates);
>> @@ -1247,9 +1249,13 @@ 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);
>
> Presumably "update" structure, when freshly initialized, has NULL in
> both of these _target members? Otherwise ref_transaction_free()
> would get in trouble, so double checking.
>
This is a good point. My understanding was that FLEX_ALLOC_MEM should
set everything to 0.
>> + 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);
>
> Since we can ask to work on a symbolic ref, new_oid / old_oid can be
> NULL when REF_HAVE_NEW / REF_HAVE_OLD bit is on for _target members.
>
> Makes me wonder if the code becomes easier to follow if the flag
> bits are split into four (_NEW -> _NEW_OID + _NEW_TARGET), but let's
> not worry about that for now.
>
The intersection of this is quite low currently, so I'm not really sure
if there's added benefit. I did start that way before, but perhaps with
the iterations in the last few version, maybe it makes the code simpler.
>> @@ -1286,6 +1292,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);
>
>> @@ -1325,14 +1332,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>> int ref_transaction_verify(struct ref_transaction *transaction,
>> const char *refname,
>> const struct object_id *old_oid,
>> + const char *old_target,
>> unsigned int flags,
>> struct strbuf *err)
>> {
>> - if (!old_oid)
>> - BUG("verify called with old_oid set to NULL");
>> + if (!old_target && !old_oid)
>> + BUG("verify called with old_oid and old_target set to NULL");
>
> Is it normal if you get _both_ set, or is it equally a BUG()?
> The parse_*_verify() codepaths we saw earlier both made sure
> only one of the two is non-NULL, and it is unclear what should
> happen if both are non-NULL.
>
It is a bug and this is caught in `ref_transaction_add_update`.
Introduced in the first patch of the series.
>> + if (old_target && !(flags & REF_NO_DEREF))
>> + BUG("verify cannot operate on symrefs with deref mode");
>> return ref_transaction_update(transaction, refname,
>> NULL, old_oid,
>> - NULL, NULL,
>> + NULL, old_target,
>> flags, NULL, err);
>> }
>
> So this queues an ref_update object whose .new_oid and .new_target
> are NULL, and .old_oid and .old_target are what the caller gave us
> to check. The NULLs in .new* members hopefully do not mean "delete
> this thing" ;-)
>
So the 'new_oid' being set to zero should be the delete this thing
queue.
>> @@ -2349,6 +2359,12 @@ static int run_transaction_hook(struct ref_transaction *transaction,
>> for (i = 0; i < transaction->nr; i++) {
>> struct ref_update *update = transaction->updates[i];
>>
>> + /*
>> + * Skip reference transaction for symbolic refs.
>> + */
>> + if (update->new_target || update->old_target)
>> + continue;
>
> Is that a final design, or will the hooks have a chance to interfere?
>
The last patch adds hook support.
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 2420dac2aa..53197fa3af 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2425,6 +2425,37 @@ static const char *original_update_refname(struct ref_update *update)
>> return update->refname;
>> }
>>
>> +/*
>> + * Check whether the REF_HAVE_OLD and old_target values stored in
>> + * update are consistent with ref, 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, char *ref,
>> + struct strbuf *err)
>> +{
>> + if (!(update->flags & REF_HAVE_OLD) ||
>> + !strcmp(update->old_target, ref))
>> + return 0;
>
> Earlier on the assignment side for "update" structure we saw above,
> the guard was (old_target && flags & REF_HAVE_OLD), but here we
> assume old_target is valid, which feels a bit asymmetric.
>
> Yes, I can see that the caller does not call us when !old_target,
> but still... Perhaps
>
> if ((update->flags & REF_HAVE_OLD) && !update->old_target)
> BUG(...);
>
I will add something like this.
> or something? Or alternatively, perhaps !!update->old_target should
> be the only thing we should check and ignore REF_HAVE_OLD bit? I am
> not sure, but it smells like that the non-NULL-ness of old_target is
> the only thing that matters (if it is not NULL, very early in the
> control flow somebody would have set REF_HAVE_OLD bit to flags, no?).
>
> It brings me back to my earlier question. Does REF_HAVE_OLD bit
> serve a useful purpose in this code?
>
I checked and it doesn't, it can be removed from usage in this code.
Will cleanup this part.
>> + if (!strcmp(update->old_target, ""))
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "reference already exists",
>> + original_update_refname(update));
>> + else if (!strcmp(ref, ""))
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "reference is missing but expected %s",
>> + original_update_refname(update),
>> + update->old_target);
>
> So... for old_target and ref, an empty string is a special value?
> How? Shouldn't that be documented in the comment before the
> function?
>
>> + else
>> + strbuf_addf(err, "cannot lock ref '%s': "
>> + "is at %s but expected %s",
>> + original_update_refname(update),
>> + ref, 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
>> @@ -2528,6 +2559,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>> ret = TRANSACTION_GENERIC_ERROR;
>> goto out;
>> }
>> + }
>> +
>> + /*
>> + * For symref verification, we need to check the reference value
>> + * rather than the oid. If we're dealing with regular refs or we're
>> + * verifying a dereferenced symref, we then check the oid.
>> + */
>> + if (update->old_target) {
>> + if (check_old_target(update, referent.buf, err)) {
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>> + }
>
> We come here only when update->type has REF_ISSYMREF bit on (we
> learned that value by calling lock_raw_ref()), and know referent.buf
> has the current "target" value. That is consumed as "ref" parameter
> to check_old_target() we just saw. OK.
>
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index 6104471199..a2474245aa 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -938,7 +938,26 @@ 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->flags & REF_HAVE_OLD) && u->old_target) {
>> + if (strcmp(referent.buf, u->old_target)) {
>> + if (!strcmp(u->old_target, ""))
>> + strbuf_addf(err, "verifying symref target: '%s': "
>> + "provided target is empty",
>> + original_update_refname(u));
>> + else 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;
>> + }
>
> Again, the puzzling "empty string"s are handled here.
For here and above, this too is dead code and no longer needed,
old_target being empty string is left over code from before we decided
to use zero_oid for deleting. I'll remove it. Thanks.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 0/7] update-ref: add symref support for --stdin
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-06 8:22 ` Karthik Nayak
` (2 more replies)
2024-06-05 10:29 ` [PATCH v4 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
` (6 subsequent siblings)
7 siblings, 3 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The 'update-ref' command is used to update refs using transactions. The
command allows users to also utilize a '--stdin' mode to provide a
batch of sub-commands which can be processed in a transaction.
Currently, the sub-commands involve {verify, delete, create, update}
and they allow users to work with regular refs in the repository. To
work with symrefs, users only have the option of using
'git-symbolic-ref', which doesn't provide transaction support to the
users eventhough it uses the same behind the hood.
Recently, we modified the reference backend to add symref support,
following which, 'git-symbolic-ref' also uses the transaction backend.
But, it doesn't expose this to the user. To allow users to work with
symrefs via transaction, this series adds support for new sub-commands
{symrer-verify, symref-delete, symref-create, symref-update} to the
'--stdin' mode of update-ref. These complement the existing
sub-commands.
The patches 1, 2, & 6 fix small issues in the reference backends. The other
patches 3, 4, 5, & 7 each add one of the new sub-commands.
The series is based off master, with 'kn/ref-transaction-symref' merged
in.
There was some discussion [1] also about adding `old_target` support to
the existing `update` command. I think its worthwhile to do this with
some tests cleanup, will follow that up as a separate series.
Changes since v3:
* Changed the position of `old_target` and `flags` in `ref_transaction_delete`
to make it a coherent.
* Added tests for deletion of regular refs using 'symref-delete', this lead to
adding a new commit to have specific errors for when a regular update contains
`old_target`.
[1]: https://lore.kernel.org/r/CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com
Karthik Nayak (7):
refs: create and use `ref_update_expects_existing_old_ref()`
refs: specify error for regular refs with `old_target`
update-ref: add support for 'symref-verify' command
update-ref: add support for 'symref-delete' command
update-ref: add support for 'symref-create' command
reftable: pick either 'oid' or 'target' for new updates
update-ref: add support for 'symref-update' command
Documentation/git-update-ref.txt | 25 ++
builtin/clone.c | 2 +-
builtin/fetch.c | 4 +-
builtin/receive-pack.c | 3 +-
builtin/update-ref.c | 237 ++++++++++++++++-
refs.c | 40 ++-
refs.h | 6 +-
refs/files-backend.c | 16 +-
refs/refs-internal.h | 6 +
refs/reftable-backend.c | 16 +-
t/t0600-reffiles-backend.sh | 32 +++
t/t1400-update-ref.sh | 430 ++++++++++++++++++++++++++++++-
t/t1416-ref-transaction-hooks.sh | 54 ++++
t/t5605-clone-local.sh | 2 +-
14 files changed, 832 insertions(+), 41 deletions(-)
Range-diff against v3:
-: ---------- > 1: cab5265c3c refs: create and use `ref_update_expects_existing_old_ref()`
-: ---------- > 2: 57b5ff46c0 refs: specify error for regular refs with `old_target`
1: ed54b0dfb9 = 3: 5710fa81bf update-ref: add support for 'symref-verify' command
2: b82b86ff40 ! 4: 5f8fc4eb6e update-ref: add support for 'symref-delete' command
@@ Commit message
within a transaction, which promises atomicity of the operation and can
be batched with other commands.
+ When no 'old_target' is provided it can also delete regular refs,
+ similar to how the 'delete' command can delete symrefs when no 'old_oid'
+ is provided.
+
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ Documentation/git-update-ref.txt: verify::
## builtin/fetch.c ##
@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
+ if (!dry_run) {
if (transaction) {
for (ref = stale_refs; ref; ref = ref->next) {
- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
- "fetch: prune", &err);
-+ NULL, "fetch: prune", &err);
++ result = ref_transaction_delete(transaction, ref->name, NULL,
++ NULL, 0, "fetch: prune", &err);
if (result)
goto cleanup;
}
@@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh
namespaced_name,
old_oid,
- 0, "push", &err)) {
-+ 0, NULL,
++ NULL, 0,
+ "push", &err)) {
rp_error("%s", err.buf);
ret = "failed to delete";
@@ builtin/update-ref.c: static void parse_cmd_delete(struct ref_transaction *trans
if (ref_transaction_delete(transaction, refname,
have_old ? &old_oid : NULL,
- update_flags, msg, &err))
-+ update_flags, NULL, msg, &err))
++ NULL, update_flags, msg, &err))
die("%s", err.buf);
update_flags = default_flags;
@@ builtin/update-ref.c: static void parse_cmd_delete(struct ref_transaction *trans
+ die("symref-delete %s: extra input: %s", refname, next);
+
+ if (ref_transaction_delete(transaction, refname, NULL,
-+ update_flags, old_target, msg, &err))
++ old_target, update_flags, msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
@@ refs.c: int refs_delete_ref(struct ref_store *refs, const char *msg,
if (!transaction ||
ref_transaction_delete(transaction, refname, old_oid,
- flags, msg, &err) ||
-+ flags, NULL, msg, &err) ||
++ NULL, flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
@@ refs.c: int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
-+ unsigned int flags,
+ const char *old_target,
++ unsigned int flags,
+ const char *msg,
struct strbuf *err)
{
@@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg,
for_each_string_list_item(item, refnames) {
ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, msg, &err);
-+ NULL, flags, NULL, msg, &err);
++ NULL, NULL, flags, msg, &err);
if (ret) {
warning(_("could not delete reference %s: %s"),
item->string, err.buf);
@@ refs.h: int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
-+ unsigned int flags,
+ const char *old_target,
++ unsigned int flags,
+ const char *msg,
struct strbuf *err);
@@ t/t1400-update-ref.sh: do
+ grep "fatal: symref-delete: missing <ref>" err
+ '
+
++ test_expect_success "stdin $type symref-delete fails deleting regular ref" '
++ test_when_finished "git update-ref -d refs/heads/regularref" &&
++ git update-ref refs/heads/regularref $a &&
++ format_command $type "symref-delete refs/heads/regularref" "$a" >stdin &&
++ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
++ grep "fatal: cannot update regular ref: ${SQ}refs/heads/regularref${SQ}: symref target ${SQ}$a${SQ} set" err
++ '
++
+ test_expect_success "stdin $type symref-delete fails with too many arguments" '
+ format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
@@ t/t1400-update-ref.sh: do
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git symbolic-ref -d refs/heads/symref2
+ '
++
++ test_expect_success "stdin $type symref-delete fails deleting regular ref without target" '
++ git update-ref refs/heads/regularref $a &&
++ format_command $type "symref-delete refs/heads/regularref" >stdin &&
++ git update-ref --stdin $type --no-deref <stdin
++ '
+
done
3: ae127f7d52 ! 5: 1542cfb806 update-ref: add support for 'symref-create' command
@@ t/t0600-reffiles-backend.sh: test_expect_success POSIXPERM 'git reflog expire ho
## t/t1400-update-ref.sh ##
@@ t/t1400-update-ref.sh: do
- test_must_fail git symbolic-ref -d refs/heads/symref2
+ git update-ref --stdin $type --no-deref <stdin
'
+ test_expect_success "stdin $type symref-create fails with too many arguments" '
4: 8889dcbf40 = 6: ec5380743d reftable: pick either 'oid' or 'target' for new updates
5: 19d85d56c4 ! 7: f8d91f7fc9 update-ref: add support for 'symref-update' command
@@ Commit message
we don't use a `(ref | oid)` prefix for the old_value, then there is
ambiguity around if the value provided should be treated as an oid or a
reference. This is more so the reason, because we allow anything
- committish to be provided as an oid.
+ committish to be provided as an oid. While 'symref-verify' and
+ 'symref-delete' also take in `<old-target>` we do not have this
+ divergence there as those commands only work with symrefs. Whereas
+ 'symref-update' also works with regular refs and allows users to convert
+ regular refs to symrefs.
The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
--
2.43.GIT
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 1/7] refs: create and use `ref_update_expects_existing_old_ref()`
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
` (5 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The files and reftable backend, need to check if a ref must exist, so
that the required validation can be done. A ref must exist only when the
`old_oid` value of the update has been explicitly set and it is not the
`null_oid` value.
Since we also support symrefs now, we need to ensure that even when
`old_target` is set a ref must exist. While this was missed when we
added symref support in transactions, there are no active users of this
path. As we introduce the 'symref-verify' command in the upcoming
commits, it is important to fix this.
So let's export this to a function called
`ref_update_expects_existing_old_ref()` and expose it internally via
'refs-internal.h'.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 6 ++++++
refs/files-backend.c | 3 +--
refs/refs-internal.h | 6 ++++++
refs/reftable-backend.c | 2 +-
4 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 8260c27cde..50d8d7d777 100644
--- a/refs.c
+++ b/refs.c
@@ -2679,3 +2679,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
referent, update->old_target);
return -1;
}
+
+int ref_update_expects_existing_old_ref(struct ref_update *update)
+{
+ return (update->flags & REF_HAVE_OLD) &&
+ (!is_null_oid(&update->old_oid) || update->old_target);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5f3089d947..194e74eb4d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2412,8 +2412,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
struct strbuf *err)
{
struct strbuf referent = STRBUF_INIT;
- int mustexist = (update->flags & REF_HAVE_OLD) &&
- !is_null_oid(&update->old_oid);
+ int mustexist = ref_update_expects_existing_old_ref(update);
int ret = 0;
struct ref_lock *lock;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 53a6c5d842..ee298ec0d5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
int ref_update_check_old_target(const char *referent, struct ref_update *update,
struct strbuf *err);
+/*
+ * Check if the ref must exist, this means that the old_oid or
+ * old_target is non NULL.
+ */
+int ref_update_expects_existing_old_ref(struct ref_update *update);
+
#endif /* REFS_REFS_INTERNAL_H */
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 1af86bbdec..b838cf8f00 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -824,7 +824,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
¤t_oid, &referent, &u->type);
if (ret < 0)
goto done;
- if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) {
+ if (ret > 0 && !ref_update_expects_existing_old_ref(u)) {
/*
* The reference does not exist, and we either have no
* old object ID or expect the reference to not exist.
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 2/7] refs: specify error for regular refs with `old_target`
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-06 11:02 ` Patrick Steinhardt
2024-06-05 10:29 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
When a regular reference update contains `old_target` set, we call the
`ref_update_check_old_target` function to check the referent value. But
for regular refs we know that the referent value is not set and this
simply raises a generic error which says nothing about this being a
regular ref. Instead let's raise a more specific error when a regular
ref update contains `old_target`.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/files-backend.c | 13 +++++++------
refs/reftable-backend.c | 9 +++++++++
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 194e74eb4d..f516d4d82f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,
/*
* 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.
+ * fail with an error.
*/
if (update->old_target) {
- if (ref_update_check_old_target(referent.buf, update, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto out;
- }
+ strbuf_addf(err, _("cannot update regular ref: '%s': "
+ "symref target '%s' set"),
+ ref_update_original_update_refname(update),
+ update->old_target);
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto out;
} else if (check_old_oid(update, &lock->old_oid, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto out;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b838cf8f00..bd89ce8d76 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -928,6 +928,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
* backend returns, which keeps our tests happy.
*/
if (u->old_target) {
+ if (!(u->type & REF_ISSYMREF)) {
+ strbuf_addf(err, _("cannot update regular ref: '%s': "
+ "symref target '%s' set"),
+ ref_update_original_update_refname(u),
+ u->old_target);
+ ret = -1;
+ goto done;
+ }
+
if (ref_update_check_old_target(referent.buf, u, err)) {
ret = -1;
goto done;
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 3/7] update-ref: add support for 'symref-verify' command
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
` (2 preceding siblings ...)
2024-06-05 10:29 ` [PATCH v4 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
` (3 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The 'symref-verify' command allows users to verify if a provided <ref>
contains the provided <old-target> without changing the <ref>. If
<old-target> is not provided, the command will verify that the <ref>
doesn't exist.
The command allows users to verify symbolic refs within a transaction,
and this means users can perform a set of changes in a transaction only
when the verification holds good.
Since we're checking for symbolic refs, this command will only work with
the 'no-deref' mode. This is because any dereferenced symbolic ref will
point to an object and not a ref and the regular 'verify' command can be
used in such situations.
Add required tests for symref support in 'verify'. Since we're here,
also add reflog checks for the pre-existing 'verify' tests, there is no
divergence from behavior, but we never tested to ensure that reflog
wasn't affected by the 'verify' command.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 7 +++
builtin/update-ref.c | 80 +++++++++++++++++++++++----
refs.c | 11 +++-
refs.h | 1 +
t/t1400-update-ref.sh | 94 +++++++++++++++++++++++++++++++-
t/t1416-ref-transaction-hooks.sh | 30 ++++++++++
6 files changed, 208 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9fe78b3501 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
start LF
prepare LF
@@ -86,6 +87,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
start NUL
prepare NUL
@@ -117,6 +119,11 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-verify::
+ Verify symbolic <ref> against <old-target> but do not change it.
+ If <old-target> is missing, the ref must not exist. Can only be
+ used in `no-deref` mode.
+
option::
Modify the behavior of the next command naming a <ref>.
The only valid option is `no-deref` to avoid dereferencing
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6cda1c08aa..50f5472160 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -76,6 +76,29 @@ static char *parse_refname(const char **next)
return strbuf_detach(&ref, NULL);
}
+/*
+ * Wrapper around parse_refname which skips the next delimiter.
+ */
+static char *parse_next_refname(const char **next)
+{
+ if (line_termination) {
+ /* Without -z, consume SP and use next argument */
+ if (!**next || **next == line_termination)
+ return NULL;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ } else {
+ /* With -z, read the next NUL-terminated line */
+ if (**next)
+ return NULL;
+ }
+ /* Skip the delimiter */
+ (*next)++;
+
+ return parse_refname(next);
+}
+
+
/*
* The value being parsed is <old-oid> (as opposed to <new-oid>; the
* difference affects which error messages are generated):
@@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
die("verify %s: extra input: %s", refname, next);
if (ref_transaction_verify(transaction, refname, &old_oid,
- update_flags, &err))
+ NULL, update_flags, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ strbuf_release(&err);
+}
+
+static void parse_cmd_symref_verify(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ struct object_id old_oid;
+ char *refname, *old_target;
+
+ if (!(update_flags & REF_NO_DEREF))
+ die("symref-verify: cannot operate with deref mode");
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-verify: missing <ref>");
+
+ /*
+ * old_ref is optional, if not provided, we need to ensure that the
+ * ref doesn't exist.
+ */
+ old_target = parse_next_refname(&next);
+ if (!old_target)
+ oidcpy(&old_oid, null_oid());
+
+ if (*next != line_termination)
+ die("symref-verify %s: extra input: %s", refname, next);
+
+ if (ref_transaction_verify(transaction, refname,
+ old_target ? NULL : &old_oid,
+ old_target, update_flags, &err))
die("%s", err.buf);
update_flags = default_flags;
free(refname);
+ free(old_target);
strbuf_release(&err);
}
@@ -380,15 +439,16 @@ static const struct parse_cmd {
unsigned args;
enum update_refs_state state;
} command[] = {
- { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
- { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
- { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
- { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
- { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
- { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
- { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
- { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
- { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
+ { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
+ { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
+ { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
+ { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
+ { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
+ { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
+ { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+ { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
+ { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
};
static void update_refs_stdin(void)
diff --git a/refs.c b/refs.c
index 50d8d7d777..cdc4d25557 100644
--- a/refs.c
+++ b/refs.c
@@ -1297,14 +1297,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err)
{
- if (!old_oid)
- BUG("verify called with old_oid set to NULL");
+ if (!old_target && !old_oid)
+ BUG("verify called with old_oid and old_target set to NULL");
+ if (old_oid && old_target)
+ BUG("verify called with both old_oid and old_target set");
+ if (old_target && !(flags & REF_NO_DEREF))
+ BUG("verify cannot operate on symrefs with deref mode");
return ref_transaction_update(transaction, refname,
NULL, old_oid,
- NULL, NULL,
+ NULL, old_target,
flags, NULL, err);
}
diff --git a/refs.h b/refs.h
index 34568ee1fb..906299351c 100644
--- a/refs.h
+++ b/refs.h
@@ -736,6 +736,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index bbee2783ab..52801be07d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -892,17 +892,23 @@ test_expect_success 'stdin update/create/verify combination works' '
'
test_expect_success 'stdin verify succeeds for correct value' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
git rev-parse $m >expect &&
echo "verify $m $m" >stdin &&
git update-ref --stdin <stdin &&
git rev-parse $m >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify succeeds for missing reference' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
echo "verify refs/heads/missing $Z" >stdin &&
git update-ref --stdin <stdin &&
- test_must_fail git rev-parse --verify -q refs/heads/missing
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify treats no value as missing' '
@@ -1643,4 +1649,88 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
+format_command () {
+ if test "$1" = "-z"
+ then
+ shift
+ printf "$F" "$@"
+ else
+ echo "$@"
+ fi
+}
+
+for type in "" "-z"
+do
+
+ test_expect_success "stdin $type symref-verify fails without --no-deref" '
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-verify: cannot operate with deref mode" err
+ '
+
+ test_expect_success "stdin $type symref-verify fails with too many arguments" '
+ format_command $type "symref-verify refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-verify refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-verify succeeds for correct value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin $type symref-verify fails with no value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-verify succeeds for dangling reference" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
+ test_must_fail git symbolic-ref refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+ format_command $type "symref-verify refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-verify fails for missing reference" '
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ format_command $type "symref-verify refs/heads/missing" "refs/heads/unknown" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}: unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin $type symref-verify fails for wrong value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "$b" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-verify fails for mistaken null value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "$Z" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+done
+
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 067fd57290..fd58b902f4 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
test_cmp expect actual
'
+test_expect_success 'hook gets all queued symref updates' '
+ test_when_finished "rm actual" &&
+
+ git update-ref refs/heads/branch $POST_OID &&
+ git symbolic-ref refs/heads/symref refs/heads/main &&
+
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ while read -r line
+ do
+ printf "%s\n" "$line"
+ done >>actual
+ EOF
+
+ cat >expect <<-EOF &&
+ prepared
+ ref:refs/heads/main $ZERO_OID refs/heads/symref
+ committed
+ ref:refs/heads/main $ZERO_OID refs/heads/symref
+ EOF
+
+ git update-ref --no-deref --stdin <<-EOF &&
+ start
+ symref-verify refs/heads/symref refs/heads/main
+ prepare
+ commit
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 4/7] update-ref: add support for 'symref-delete' command
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
` (3 preceding siblings ...)
2024-06-05 10:29 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
` (2 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add a new command 'symref-delete' to allow deletions of symbolic refs in
a transaction via the '--stdin' mode of the 'git-update-ref' command.
The 'symref-delete' command can, when given an <old-target>, delete the
provided <ref> only when it points to <old-target>.
This command is only compatible with the 'no-deref' mode because we
optionally want to check the 'old_target' of the ref being deleted.
De-referencing a symbolic ref would provide a regular ref and we already
have the 'delete' command for regular refs.
While users can also use 'git symbolic-ref -d' to delete symbolic refs,
the 'symref-delete' command in 'git-update-ref' allows users to do so
within a transaction, which promises atomicity of the operation and can
be batched with other commands.
When no 'old_target' is provided it can also delete regular refs,
similar to how the 'delete' command can delete symrefs when no 'old_oid'
is provided.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 5 +++
builtin/fetch.c | 4 +-
builtin/receive-pack.c | 3 +-
builtin/update-ref.c | 33 +++++++++++++++-
refs.c | 14 +++++--
refs.h | 4 +-
t/t1400-update-ref.sh | 68 ++++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 19 ++++++++-
8 files changed, 140 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9fe78b3501..16e02f6979 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
start LF
@@ -87,6 +88,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
start NUL
@@ -119,6 +121,9 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-delete::
+ Delete <ref> after verifying it exists with <old-target>, if given.
+
symref-verify::
Verify symbolic <ref> against <old-target> but do not change it.
If <old-target> is missing, the ref must not exist. Can only be
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 75255dc600..d63100e0d3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1386,8 +1386,8 @@ static int prune_refs(struct display_state *display_state,
if (!dry_run) {
if (transaction) {
for (ref = stale_refs; ref; ref = ref->next) {
- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
- "fetch: prune", &err);
+ result = ref_transaction_delete(transaction, ref->name, NULL,
+ NULL, 0, "fetch: prune", &err);
if (result)
goto cleanup;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 01c1f04ece..0a30fac239 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1576,7 +1576,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
if (ref_transaction_delete(transaction,
namespaced_name,
old_oid,
- 0, "push", &err)) {
+ NULL, 0,
+ "push", &err)) {
rp_error("%s", err.buf);
ret = "failed to delete";
} else {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 50f5472160..0cb7eef3c6 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -293,7 +293,7 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
if (ref_transaction_delete(transaction, refname,
have_old ? &old_oid : NULL,
- update_flags, msg, &err))
+ NULL, update_flags, msg, &err))
die("%s", err.buf);
update_flags = default_flags;
@@ -301,6 +301,36 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
+
+static void parse_cmd_symref_delete(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ char *refname, *old_target;
+
+ if (!(update_flags & REF_NO_DEREF))
+ die("symref-delete: cannot operate with deref mode");
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-delete: missing <ref>");
+
+ old_target = parse_next_refname(&next);
+
+ if (*next != line_termination)
+ die("symref-delete %s: extra input: %s", refname, next);
+
+ if (ref_transaction_delete(transaction, refname, NULL,
+ old_target, update_flags, msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(old_target);
+ strbuf_release(&err);
+}
+
+
static void parse_cmd_verify(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -443,6 +473,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
{ "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
{ "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
diff --git a/refs.c b/refs.c
index cdc4d25557..01f3188a09 100644
--- a/refs.c
+++ b/refs.c
@@ -950,7 +950,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
transaction = ref_store_transaction_begin(refs, &err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_oid,
- flags, msg, &err) ||
+ NULL, flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
@@ -1283,14 +1283,20 @@ int ref_transaction_create(struct ref_transaction *transaction,
int ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
+ const char *old_target,
+ unsigned int flags,
+ const char *msg,
struct strbuf *err)
{
if (old_oid && is_null_oid(old_oid))
BUG("delete called with old_oid set to zeros");
+ if (old_oid && old_target)
+ BUG("delete called with both old_oid and old_target set");
+ if (old_target && !(flags & REF_NO_DEREF))
+ BUG("delete cannot operate on symrefs with deref mode");
return ref_transaction_update(transaction, refname,
null_oid(), old_oid,
- NULL, NULL, flags,
+ NULL, old_target, flags,
msg, err);
}
@@ -2599,7 +2605,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
for_each_string_list_item(item, refnames) {
ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, msg, &err);
+ NULL, NULL, flags, msg, &err);
if (ret) {
warning(_("could not delete reference %s: %s"),
item->string, err.buf);
diff --git a/refs.h b/refs.h
index 906299351c..974cf4dd08 100644
--- a/refs.h
+++ b/refs.h
@@ -722,7 +722,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
int ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
+ const char *old_target,
+ unsigned int flags,
+ const char *msg,
struct strbuf *err);
/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 52801be07d..45111ce021 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1731,6 +1731,74 @@ do
test_cmp expect actual
'
+ test_expect_success "stdin $type symref-delete fails without --no-deref" '
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-delete: cannot operate with deref mode" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails with no ref" '
+ format_command $type "symref-delete " >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: symref-delete: missing <ref>" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails deleting regular ref" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: cannot update regular ref: ${SQ}refs/heads/regularref${SQ}: symref target ${SQ}$a${SQ} set" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails with too many arguments" '
+ format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-delete refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-delete fails with wrong old value" '
+ format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-delete works with right old value" '
+ format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git rev-parse --verify -q refs/heads/symref
+ '
+
+ test_expect_success "stdin $type symref-delete works with empty old value" '
+ git symbolic-ref refs/heads/symref $a >stdin &&
+ format_command $type "symref-delete refs/heads/symref" "" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git rev-parse --verify -q $b
+ '
+
+ test_expect_success "stdin $type symref-delete succeeds for dangling reference" '
+ test_must_fail git symbolic-ref refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+ format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git symbolic-ref -d refs/heads/symref2
+ '
+
+ test_expect_success "stdin $type symref-delete fails deleting regular ref without target" '
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index fd58b902f4..ccde1b944b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -162,6 +162,7 @@ test_expect_success 'hook gets all queued symref updates' '
git update-ref refs/heads/branch $POST_OID &&
git symbolic-ref refs/heads/symref refs/heads/main &&
+ git symbolic-ref refs/heads/symrefd refs/heads/main &&
test_hook reference-transaction <<-\EOF &&
echo "$*" >>actual
@@ -171,16 +172,32 @@ test_expect_success 'hook gets all queued symref updates' '
done >>actual
EOF
- cat >expect <<-EOF &&
+ # In the files backend, "delete" also triggers an additional transaction
+ # update on the packed-refs backend, which constitutes additional reflog
+ # entries.
+ if test_have_prereq REFFILES
+ then
+ cat >expect <<-EOF
+ aborted
+ $ZERO_OID $ZERO_OID refs/heads/symrefd
+ EOF
+ else
+ >expect
+ fi &&
+
+ cat >>expect <<-EOF &&
prepared
ref:refs/heads/main $ZERO_OID refs/heads/symref
+ ref:refs/heads/main $ZERO_OID refs/heads/symrefd
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
+ ref:refs/heads/main $ZERO_OID refs/heads/symrefd
EOF
git update-ref --no-deref --stdin <<-EOF &&
start
symref-verify refs/heads/symref refs/heads/main
+ symref-delete refs/heads/symrefd refs/heads/main
prepare
commit
EOF
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 5/7] update-ref: add support for 'symref-create' command
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
` (4 preceding siblings ...)
2024-06-05 10:29 ` [PATCH v4 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add 'symref-create' command to the '--stdin' mode 'git-update-ref' to
allow creation of symbolic refs in a transaction. The 'symref-create'
command takes in a <new-target>, which the created <ref> will point to.
Also, support the 'core.prefersymlinkrefs' config, wherein if the config
is set and the filesystem supports symlinks, we create the symbolic ref
as a symlink. We fallback to creating a regular symref if creating the
symlink is unsuccessful.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 6 +++
builtin/clone.c | 2 +-
builtin/update-ref.c | 32 +++++++++++++++-
refs.c | 9 +++--
refs.h | 1 +
t/t0600-reffiles-backend.sh | 32 ++++++++++++++++
t/t1400-update-ref.sh | 65 ++++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 3 ++
t/t5605-clone-local.sh | 2 +-
9 files changed, 146 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 16e02f6979..364ef78af1 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-create SP <ref> SP <new-target> LF
symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
@@ -88,6 +89,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-create SP <ref> NUL <new-target> NUL
symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
@@ -121,6 +123,10 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-create:
+ Create symbolic ref <ref> with <new-target> after verifying
+ it does not exist.
+
symref-delete::
Delete <ref> after verifying it exists with <old-target>, if given.
diff --git a/builtin/clone.c b/builtin/clone.c
index 23993b905b..6ddb3084e6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -576,7 +576,7 @@ static void write_remote_refs(const struct ref *local_refs)
if (!r->peer_ref)
continue;
if (ref_transaction_create(t, r->peer_ref->name, &r->old_oid,
- 0, NULL, &err))
+ NULL, 0, NULL, &err))
die("%s", err.buf);
}
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0cb7eef3c6..9c40e94626 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,7 +257,7 @@ static void parse_cmd_create(struct ref_transaction *transaction,
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
- if (ref_transaction_create(transaction, refname, &new_oid,
+ if (ref_transaction_create(transaction, refname, &new_oid, NULL,
update_flags | create_reflog_flag,
msg, &err))
die("%s", err.buf);
@@ -267,6 +267,35 @@ static void parse_cmd_create(struct ref_transaction *transaction,
strbuf_release(&err);
}
+
+static void parse_cmd_symref_create(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ char *refname, *new_target;
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-create: missing <ref>");
+
+ new_target = parse_next_refname(&next);
+ if (!new_target)
+ die("symref-create %s: missing <new-target>", refname);
+
+ if (*next != line_termination)
+ die("symref-create %s: extra input: %s", refname, next);
+
+ if (ref_transaction_create(transaction, refname, NULL, new_target,
+ update_flags | create_reflog_flag,
+ msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(new_target);
+ strbuf_release(&err);
+}
+
static void parse_cmd_delete(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -473,6 +502,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
{ "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
diff --git a/refs.c b/refs.c
index 01f3188a09..8807e87e3c 100644
--- a/refs.c
+++ b/refs.c
@@ -1268,15 +1268,18 @@ int ref_transaction_update(struct ref_transaction *transaction,
int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *new_oid,
+ const char *new_target,
unsigned int flags, const char *msg,
struct strbuf *err)
{
- if (!new_oid || is_null_oid(new_oid)) {
- strbuf_addf(err, "'%s' has a null OID", refname);
+ if (new_oid && new_target)
+ BUG("create called with both new_oid and new_target set");
+ if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
+ strbuf_addf(err, "'%s' has neither a valid OID nor a target", refname);
return 1;
}
return ref_transaction_update(transaction, refname, new_oid,
- null_oid(), NULL, NULL, flags,
+ null_oid(), new_target, NULL, flags,
msg, err);
}
diff --git a/refs.h b/refs.h
index 974cf4dd08..28e3bb8a42 100644
--- a/refs.h
+++ b/refs.h
@@ -708,6 +708,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *new_oid,
+ const char *new_target,
unsigned int flags, const char *msg,
struct strbuf *err);
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 92f570313d..b2a771ff2b 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
esac
'
+test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+ test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+ git update-ref refs/heads/new @ &&
+ test_config core.prefersymlinkrefs true &&
+ cat >stdin <<-EOF &&
+ start
+ symref-create TEST_SYMREF_HEAD refs/heads/new
+ prepare
+ commit
+ EOF
+ git update-ref --no-deref --stdin <stdin &&
+ test_path_is_symlink .git/TEST_SYMREF_HEAD &&
+ test "$(test_readlink .git/TEST_SYMREF_HEAD)" = refs/heads/new
+'
+
+test_expect_success 'symref transaction supports false symlink config' '
+ test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+ git update-ref refs/heads/new @ &&
+ test_config core.prefersymlinkrefs false &&
+ cat >stdin <<-EOF &&
+ start
+ symref-create TEST_SYMREF_HEAD refs/heads/new
+ prepare
+ commit
+ EOF
+ git update-ref --no-deref --stdin <stdin &&
+ test_path_is_file .git/TEST_SYMREF_HEAD &&
+ git symbolic-ref TEST_SYMREF_HEAD >actual &&
+ echo refs/heads/new >expect &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 45111ce021..27ac6ee7cb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1799,6 +1799,71 @@ do
git update-ref --stdin $type --no-deref <stdin
'
+ test_expect_success "stdin $type symref-create fails with too many arguments" '
+ format_command $type "symref-create refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-create refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-create fails with no target" '
+ format_command $type "symref-create refs/heads/symref" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-create fails with empty target" '
+ format_command $type "symref-create refs/heads/symref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-create works" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-create works with --no-deref" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" &&
+ git update-ref --stdin $type <stdin 2>err
+ '
+
+ test_expect_success "stdin $type create dangling symref ref works" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo refs/heads/unkown >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-create does not create reflogs by default" '
+ test_when_finished "git symbolic-ref -d refs/symref" &&
+ format_command $type "symref-create refs/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual &&
+ test_must_fail git reflog exists refs/symref
+ '
+
+ test_expect_success "stdin $type symref-create reflogs with --create-reflog" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+ git update-ref --create-reflog --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual &&
+ git reflog exists refs/heads/symref
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ccde1b944b..ff77dcca6b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -189,15 +189,18 @@ test_expect_success 'hook gets all queued symref updates' '
prepared
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+ $ZERO_OID ref:refs/heads/main refs/heads/symrefc
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+ $ZERO_OID ref:refs/heads/main refs/heads/symrefc
EOF
git update-ref --no-deref --stdin <<-EOF &&
start
symref-verify refs/heads/symref refs/heads/main
symref-delete refs/heads/symrefd refs/heads/main
+ symref-create refs/heads/symrefc refs/heads/main
prepare
commit
EOF
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index a3055869bc..339d8c786f 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -163,7 +163,7 @@ test_expect_success REFFILES 'local clone from repo with corrupt refs fails grac
echo a >corrupt/.git/refs/heads/topic &&
test_must_fail git clone corrupt working 2>err &&
- grep "has a null OID" err
+ grep "has neither a valid OID nor a target" err
'
test_done
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 6/7] reftable: pick either 'oid' or 'target' for new updates
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
` (5 preceding siblings ...)
2024-06-05 10:29 ` [PATCH v4 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
When creating a reference transaction update, we can provide the old/new
oid/target for the update. We have checks in place to ensure that for
each old/new, either oid or target is set and not both.
In the reftable backend, when dealing with updates without the
`REF_NO_DEREF` flag, we don't selectively propagate data as needed.
Since there are no active users of the path, this is not caught. As we
want to introduce the 'symref-update' command in the upcoming commit,
which would use this flow, correct it.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/reftable-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index bd89ce8d76..fee7a9904b 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -895,8 +895,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
*/
new_update = ref_transaction_add_update(
transaction, referent.buf, new_flags,
- &u->new_oid, &u->old_oid, u->new_target,
- u->old_target, u->msg);
+ u->new_target ? NULL : &u->new_oid,
+ u->old_target ? NULL : &u->old_oid,
+ u->new_target, u->old_target, u->msg);
new_update->parent_update = u;
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v4 7/7] update-ref: add support for 'symref-update' command
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
` (6 preceding siblings ...)
2024-06-05 10:29 ` [PATCH v4 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
@ 2024-06-05 10:29 ` Karthik Nayak
7 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-05 10:29 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
allow updates of symbolic refs. The 'symref-update' command takes in a
<new-target>, which the <ref> will be updated to. If the <ref> doesn't
exist it will be created.
It also optionally takes either an `ref <old-target>` or `oid
<old-oid>`. If the <old-target> is provided, it checks to see if the
<ref> targets the <old-target> before the update. If <old-oid> is provided
it checks <ref> to ensure that it is a regular ref and <old-oid> is the
OID before the update. This by extension also means that this when a
zero <old-oid> is provided, it ensures that the ref didn't exist before.
The divergence in syntax from the regular `update` command is because if
we don't use a `(ref | oid)` prefix for the old_value, then there is
ambiguity around if the value provided should be treated as an oid or a
reference. This is more so the reason, because we allow anything
committish to be provided as an oid. While 'symref-verify' and
'symref-delete' also take in `<old-target>` we do not have this
divergence there as those commands only work with symrefs. Whereas
'symref-update' also works with regular refs and allows users to convert
regular refs to symrefs.
The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
of operations together.
This command supports deref mode, to ensure that we can update
dereferenced regular refs to symrefs.
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 7 ++
builtin/update-ref.c | 92 ++++++++++++++
t/t1400-update-ref.sh | 203 +++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 4 +
4 files changed, 306 insertions(+)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 364ef78af1..afcf33cf60 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-update SP <ref> SP <new-target> [SP (ref SP <old-target> | oid SP <old-oid>)] LF
symref-create SP <ref> SP <new-target> LF
symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
@@ -89,6 +90,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-update SP <ref> NUL <new-target> [NUL (ref NUL <old-target> | oid NUL <old-oid>)] NUL
symref-create SP <ref> NUL <new-target> NUL
symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
@@ -119,6 +121,11 @@ delete::
Delete <ref> after verifying it exists with <old-oid>, if
given. If given, <old-oid> may not be zero.
+symref-update::
+ Set <ref> to <new-target> after verifying <old-target> or <old-oid>,
+ if given. Specify a zero <old-oid> to ensure that the ref does not
+ exist before the update.
+
verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9c40e94626..471fa5c8d1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -98,6 +98,42 @@ static char *parse_next_refname(const char **next)
return parse_refname(next);
}
+/*
+ * Wrapper around parse_arg which skips the next delimiter.
+ */
+static char *parse_next_arg(const char **next)
+{
+ struct strbuf arg = STRBUF_INIT;
+
+ if (line_termination) {
+ /* Without -z, consume SP and use next argument */
+ if (!**next || **next == line_termination)
+ return NULL;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ } else {
+ /* With -z, read the next NUL-terminated line */
+ if (**next)
+ return NULL;
+ }
+ /* Skip the delimiter */
+ (*next)++;
+
+ if (line_termination) {
+ /* Without -z, use the next argument */
+ *next = parse_arg(*next, &arg);
+ } else {
+ /* With -z, use everything up to the next NUL */
+ strbuf_addstr(&arg, *next);
+ *next += arg.len;
+ }
+
+ if (arg.len)
+ return strbuf_detach(&arg, NULL);
+
+ strbuf_release(&arg);
+ return NULL;
+}
/*
* The value being parsed is <old-oid> (as opposed to <new-oid>; the
@@ -237,6 +273,61 @@ static void parse_cmd_update(struct ref_transaction *transaction,
strbuf_release(&err);
}
+static void parse_cmd_symref_update(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ char *refname, *new_target, *old_arg;
+ char *old_target = NULL;
+ struct strbuf err = STRBUF_INIT;
+ struct object_id old_oid;
+ int have_old_oid = 0;
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-update: missing <ref>");
+
+ new_target = parse_next_refname(&next);
+ if (!new_target)
+ die("symref-update %s: missing <new-target>", refname);
+
+ old_arg = parse_next_arg(&next);
+ if (old_arg) {
+ old_target = parse_next_arg(&next);
+ if (!old_target)
+ die("symref-update %s: expected old value", refname);
+
+ if (!strcmp(old_arg, "oid")) {
+ if (repo_get_oid(the_repository, old_target, &old_oid))
+ die("symref-update %s: invalid oid: %s", refname, old_target);
+
+ have_old_oid = 1;
+ } else if (!strcmp(old_arg, "ref")) {
+ if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
+ die("symref-update %s: invalid ref: %s", refname, old_target);
+ } else {
+ die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
+ }
+ }
+
+ if (*next != line_termination)
+ die("symref-update %s: extra input: %s", refname, next);
+
+ if (ref_transaction_update(transaction, refname, NULL,
+ have_old_oid ? &old_oid : NULL,
+ new_target,
+ have_old_oid ? NULL : old_target,
+ update_flags | create_reflog_flag,
+ msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(old_arg);
+ free(old_target);
+ free(new_target);
+ strbuf_release(&err);
+}
+
static void parse_cmd_create(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -502,6 +593,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-update", parse_cmd_symref_update, 4, UPDATE_REFS_OPEN },
{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 27ac6ee7cb..9bda06d2dc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1362,6 +1362,7 @@ test_expect_success 'fails with duplicate HEAD update' '
'
test_expect_success 'fails with duplicate ref update via symref' '
+ test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
git branch target2 $A &&
git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
cat >stdin <<-EOF &&
@@ -1864,6 +1865,208 @@ do
git reflog exists refs/heads/symref
'
+ test_expect_success "stdin $type symref-update fails with too many arguments" '
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-update refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-update fails with wrong old value argument" '
+ format_command $type "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: symref-update refs/heads/symref: invalid arg ${SQ}foo${SQ} for old value" err
+ '
+
+ test_expect_success "stdin $type symref-update creates with zero old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates with no old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates dangling" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo refs/heads/nonexistent >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails with wrong old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
+ test_must_fail git rev-parse --verify -q $c
+ '
+
+ test_expect_success "stdin $type symref-update updates dangling ref" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update updates dangling ref with old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails update dangling ref with wrong old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ echo refs/heads/nonexistent >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update works with right old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $m >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update works with no old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $m >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails with empty old ref-target" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates (with deref)" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+ grep "$Z $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref with correct old-oid" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
+ git update-ref --stdin $type <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref fails with wrong old-oid" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: is at $(git rev-parse $a) but expected $(git rev-parse refs/heads/target2)" err &&
+ echo $(git rev-parse $a) >expect &&
+ git rev-parse refs/heads/regularref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref fails with invalid old-oid" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "not-a-ref-oid" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-update refs/heads/regularref: invalid oid: not-a-ref-oid" err &&
+ echo $(git rev-parse $a) >expect &&
+ git rev-parse refs/heads/regularref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update existing symref with zero old-oid" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref refs/heads/target2 &&
+ format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}: reference already exists" err &&
+ echo refs/heads/target2 >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref (with deref)" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
+ git update-ref refs/heads/symref2 $a &&
+ git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref $type --stdin <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
+ test_cmp expect actual &&
+ echo refs/heads/symref2 >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" >stdin &&
+ git update-ref $type --stdin <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ff77dcca6b..5a812ca3c0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -163,6 +163,7 @@ test_expect_success 'hook gets all queued symref updates' '
git update-ref refs/heads/branch $POST_OID &&
git symbolic-ref refs/heads/symref refs/heads/main &&
git symbolic-ref refs/heads/symrefd refs/heads/main &&
+ git symbolic-ref refs/heads/symrefu refs/heads/main &&
test_hook reference-transaction <<-\EOF &&
echo "$*" >>actual
@@ -190,10 +191,12 @@ test_expect_success 'hook gets all queued symref updates' '
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+ ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+ ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
EOF
git update-ref --no-deref --stdin <<-EOF &&
@@ -201,6 +204,7 @@ test_expect_success 'hook gets all queued symref updates' '
symref-verify refs/heads/symref refs/heads/main
symref-delete refs/heads/symrefd refs/heads/main
symref-create refs/heads/symrefc refs/heads/main
+ symref-update refs/heads/symrefu refs/heads/branch ref refs/heads/main
prepare
commit
EOF
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/7] update-ref: add symref support for --stdin
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
@ 2024-06-06 8:22 ` Karthik Nayak
2024-06-06 11:03 ` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
2 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-06 8:22 UTC (permalink / raw)
Cc: git, gitster, ps
[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> The 'update-ref' command is used to update refs using transactions. The
> command allows users to also utilize a '--stdin' mode to provide a
> batch of sub-commands which can be processed in a transaction.
>
> Currently, the sub-commands involve {verify, delete, create, update}
> and they allow users to work with regular refs in the repository. To
> work with symrefs, users only have the option of using
> 'git-symbolic-ref', which doesn't provide transaction support to the
> users eventhough it uses the same behind the hood.
>
> Recently, we modified the reference backend to add symref support,
> following which, 'git-symbolic-ref' also uses the transaction backend.
> But, it doesn't expose this to the user. To allow users to work with
> symrefs via transaction, this series adds support for new sub-commands
> {symrer-verify, symref-delete, symref-create, symref-update} to the
> '--stdin' mode of update-ref. These complement the existing
> sub-commands.
>
> The patches 1, 2, & 6 fix small issues in the reference backends. The other
> patches 3, 4, 5, & 7 each add one of the new sub-commands.
>
> The series is based off master, with 'kn/ref-transaction-symref' merged
> in.
>
> There was some discussion [1] also about adding `old_target` support to
> the existing `update` command. I think its worthwhile to do this with
> some tests cleanup, will follow that up as a separate series.
>
> Changes since v3:
> * Changed the position of `old_target` and `flags` in `ref_transaction_delete`
> to make it a coherent.
> * Added tests for deletion of regular refs using 'symref-delete', this lead to
> adding a new commit to have specific errors for when a regular update contains
> `old_target`.
>
> [1]: https://lore.kernel.org/r/CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com
>
Somehow I added the wrong link in the reply-to and this has come out as
a thread of its own. This is a follow up of v3:
https://lore.kernel.org/all/20240530120940.456817-1-knayak@gitlab.com/#t
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 2/7] refs: specify error for regular refs with `old_target`
2024-06-05 10:29 ` [PATCH v4 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
@ 2024-06-06 11:02 ` Patrick Steinhardt
2024-06-06 14:20 ` Karthik Nayak
0 siblings, 1 reply; 25+ messages in thread
From: Patrick Steinhardt @ 2024-06-06 11:02 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]
On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> When a regular reference update contains `old_target` set, we call the
s/contains/has its/
> `ref_update_check_old_target` function to check the referent value. But
> for regular refs we know that the referent value is not set and this
This is fairly technical and focussed on the implementation. Can we
maybe talk more about intent ("expected a symref, but is a direct ref")
rather than the exact implementation to make the commit message a bit
easier to understand for folks?
> simply raises a generic error which says nothing about this being a
> regular ref. Instead let's raise a more specific error when a regular
> ref update contains `old_target`.
It might be helpful to include before/after in this commit message to
show the change. Even better would be a test of course, but I think we
cannot add one at this point in time yet.
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> refs/files-backend.c | 13 +++++++------
> refs/reftable-backend.c | 9 +++++++++
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 194e74eb4d..f516d4d82f 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>
> /*
> * 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.
> + * fail with an error.
> */
> if (update->old_target) {
> - if (ref_update_check_old_target(referent.buf, update, err)) {
> - ret = TRANSACTION_GENERIC_ERROR;
> - goto out;
> - }
> + strbuf_addf(err, _("cannot update regular ref: '%s': "
> + "symref target '%s' set"),
> + ref_update_original_update_refname(update),
> + update->old_target);
> + ret = TRANSACTION_GENERIC_ERROR;
> + goto out;
I feel like these error messages are somewhat technical. If I were
reading it as a user, I don't think I'd understand what it is trying to
tell me. How about:
strbuf_addf(err, _("cannot lock ref '%s': %s",
"expected symref but is a direct ref"));
This also matches the other error messages we have in this function more
closely. The same is true for the equivalent in the reftable backend.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 0/7] update-ref: add symref support for --stdin
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
2024-06-06 8:22 ` Karthik Nayak
@ 2024-06-06 11:03 ` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
2 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-06 11:03 UTC (permalink / raw)
Cc: git, gitster, ps
[-- Attachment #1: Type: text/plain, Size: 17910 bytes --]
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> The 'update-ref' command is used to update refs using transactions. The
> command allows users to also utilize a '--stdin' mode to provide a
> batch of sub-commands which can be processed in a transaction.
>
> Currently, the sub-commands involve {verify, delete, create, update}
> and they allow users to work with regular refs in the repository. To
> work with symrefs, users only have the option of using
> 'git-symbolic-ref', which doesn't provide transaction support to the
> users eventhough it uses the same behind the hood.
>
> Recently, we modified the reference backend to add symref support,
> following which, 'git-symbolic-ref' also uses the transaction backend.
> But, it doesn't expose this to the user. To allow users to work with
> symrefs via transaction, this series adds support for new sub-commands
> {symrer-verify, symref-delete, symref-create, symref-update} to the
> '--stdin' mode of update-ref. These complement the existing
> sub-commands.
>
> The patches 1, 2, & 6 fix small issues in the reference backends. The other
> patches 3, 4, 5, & 7 each add one of the new sub-commands.
>
> The series is based off master, with 'kn/ref-transaction-symref' merged
> in.
>
> There was some discussion [1] also about adding `old_target` support to
> the existing `update` command. I think its worthwhile to do this with
> some tests cleanup, will follow that up as a separate series.
>
> Changes since v3:
> * Changed the position of `old_target` and `flags` in `ref_transaction_delete`
> to make it a coherent.
> * Added tests for deletion of regular refs using 'symref-delete', this lead to
> adding a new commit to have specific errors for when a regular update contains
> `old_target`.
>
> [1]: https://lore.kernel.org/r/CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com
>
> Karthik Nayak (7):
> refs: create and use `ref_update_expects_existing_old_ref()`
> refs: specify error for regular refs with `old_target`
> update-ref: add support for 'symref-verify' command
> update-ref: add support for 'symref-delete' command
> update-ref: add support for 'symref-create' command
> reftable: pick either 'oid' or 'target' for new updates
> update-ref: add support for 'symref-update' command
>
> Documentation/git-update-ref.txt | 25 ++
> builtin/clone.c | 2 +-
> builtin/fetch.c | 4 +-
> builtin/receive-pack.c | 3 +-
> builtin/update-ref.c | 237 ++++++++++++++++-
> refs.c | 40 ++-
> refs.h | 6 +-
> refs/files-backend.c | 16 +-
> refs/refs-internal.h | 6 +
> refs/reftable-backend.c | 16 +-
> t/t0600-reffiles-backend.sh | 32 +++
> t/t1400-update-ref.sh | 430 ++++++++++++++++++++++++++++++-
> t/t1416-ref-transaction-hooks.sh | 54 ++++
> t/t5605-clone-local.sh | 2 +-
> 14 files changed, 832 insertions(+), 41 deletions(-)
>
> Range-diff against v3:
> -: ---------- > 1: cab5265c3c refs: create and use `ref_update_expects_existing_old_ref()`
> -: ---------- > 2: 57b5ff46c0 refs: specify error for regular refs with `old_target`
> 1: ed54b0dfb9 = 3: 5710fa81bf update-ref: add support for 'symref-verify' command
> 2: b82b86ff40 ! 4: 5f8fc4eb6e update-ref: add support for 'symref-delete' command
> @@ Commit message
> within a transaction, which promises atomicity of the operation and can
> be batched with other commands.
>
> + When no 'old_target' is provided it can also delete regular refs,
> + similar to how the 'delete' command can delete symrefs when no 'old_oid'
> + is provided.
> +
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
> @@ Documentation/git-update-ref.txt: verify::
>
> ## builtin/fetch.c ##
> @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
> + if (!dry_run) {
> if (transaction) {
> for (ref = stale_refs; ref; ref = ref->next) {
> - result = ref_transaction_delete(transaction, ref->name, NULL, 0,
> +- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
> - "fetch: prune", &err);
> -+ NULL, "fetch: prune", &err);
> ++ result = ref_transaction_delete(transaction, ref->name, NULL,
> ++ NULL, 0, "fetch: prune", &err);
> if (result)
> goto cleanup;
> }
> @@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh
> namespaced_name,
> old_oid,
> - 0, "push", &err)) {
> -+ 0, NULL,
> ++ NULL, 0,
> + "push", &err)) {
> rp_error("%s", err.buf);
> ret = "failed to delete";
> @@ builtin/update-ref.c: static void parse_cmd_delete(struct ref_transaction *trans
> if (ref_transaction_delete(transaction, refname,
> have_old ? &old_oid : NULL,
> - update_flags, msg, &err))
> -+ update_flags, NULL, msg, &err))
> ++ NULL, update_flags, msg, &err))
> die("%s", err.buf);
>
> update_flags = default_flags;
> @@ builtin/update-ref.c: static void parse_cmd_delete(struct ref_transaction *trans
> + die("symref-delete %s: extra input: %s", refname, next);
> +
> + if (ref_transaction_delete(transaction, refname, NULL,
> -+ update_flags, old_target, msg, &err))
> ++ old_target, update_flags, msg, &err))
> + die("%s", err.buf);
> +
> + update_flags = default_flags;
> @@ refs.c: int refs_delete_ref(struct ref_store *refs, const char *msg,
> if (!transaction ||
> ref_transaction_delete(transaction, refname, old_oid,
> - flags, msg, &err) ||
> -+ flags, NULL, msg, &err) ||
> ++ NULL, flags, msg, &err) ||
> ref_transaction_commit(transaction, &err)) {
> error("%s", err.buf);
> ref_transaction_free(transaction);
> @@ refs.c: int ref_transaction_create(struct ref_transaction *transaction,
> const char *refname,
> const struct object_id *old_oid,
> - unsigned int flags, const char *msg,
> -+ unsigned int flags,
> + const char *old_target,
> ++ unsigned int flags,
> + const char *msg,
> struct strbuf *err)
> {
> @@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg,
> for_each_string_list_item(item, refnames) {
> ret = ref_transaction_delete(transaction, item->string,
> - NULL, flags, msg, &err);
> -+ NULL, flags, NULL, msg, &err);
> ++ NULL, NULL, flags, msg, &err);
> if (ret) {
> warning(_("could not delete reference %s: %s"),
> item->string, err.buf);
> @@ refs.h: int ref_transaction_create(struct ref_transaction *transaction,
> const char *refname,
> const struct object_id *old_oid,
> - unsigned int flags, const char *msg,
> -+ unsigned int flags,
> + const char *old_target,
> ++ unsigned int flags,
> + const char *msg,
> struct strbuf *err);
>
> @@ t/t1400-update-ref.sh: do
> + grep "fatal: symref-delete: missing <ref>" err
> + '
> +
> ++ test_expect_success "stdin $type symref-delete fails deleting regular ref" '
> ++ test_when_finished "git update-ref -d refs/heads/regularref" &&
> ++ git update-ref refs/heads/regularref $a &&
> ++ format_command $type "symref-delete refs/heads/regularref" "$a" >stdin &&
> ++ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
> ++ grep "fatal: cannot update regular ref: ${SQ}refs/heads/regularref${SQ}: symref target ${SQ}$a${SQ} set" err
> ++ '
> ++
> + test_expect_success "stdin $type symref-delete fails with too many arguments" '
> + format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
> + test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
> @@ t/t1400-update-ref.sh: do
> + git update-ref --stdin $type --no-deref <stdin &&
> + test_must_fail git symbolic-ref -d refs/heads/symref2
> + '
> ++
> ++ test_expect_success "stdin $type symref-delete fails deleting regular ref without target" '
> ++ git update-ref refs/heads/regularref $a &&
> ++ format_command $type "symref-delete refs/heads/regularref" >stdin &&
> ++ git update-ref --stdin $type --no-deref <stdin
> ++ '
> +
> done
>
> 3: ae127f7d52 ! 5: 1542cfb806 update-ref: add support for 'symref-create' command
> @@ t/t0600-reffiles-backend.sh: test_expect_success POSIXPERM 'git reflog expire ho
>
> ## t/t1400-update-ref.sh ##
> @@ t/t1400-update-ref.sh: do
> - test_must_fail git symbolic-ref -d refs/heads/symref2
> + git update-ref --stdin $type --no-deref <stdin
> '
>
> + test_expect_success "stdin $type symref-create fails with too many arguments" '
> 4: 8889dcbf40 = 6: ec5380743d reftable: pick either 'oid' or 'target' for new updates
> 5: 19d85d56c4 ! 7: f8d91f7fc9 update-ref: add support for 'symref-update' command
> @@ Commit message
> we don't use a `(ref | oid)` prefix for the old_value, then there is
> ambiguity around if the value provided should be treated as an oid or a
> reference. This is more so the reason, because we allow anything
> - committish to be provided as an oid.
> + committish to be provided as an oid. While 'symref-verify' and
> + 'symref-delete' also take in `<old-target>` we do not have this
> + divergence there as those commands only work with symrefs. Whereas
> + 'symref-update' also works with regular refs and allows users to convert
> + regular refs to symrefs.
>
> The command allows users to perform symbolic ref updates within a
> transaction. This provides atomicity and allows users to perform a set
> --
> 2.43.GIT
I also noticed that the range-diff seems a bit weird. I used
git format-patch -v 4 --cover-letter --range-diff=symref-cmd-v3 @~7..@
to generate the patch series, but seems like when doing this it
internally computes the merge-base, which happens to be the first commit
in this series, skewing the range-diff by one commit. I guess its always
better to provide the commit range to `--range-diff`. Adding the range
diff for the same, when run using
git format-patch -v 4 --cover-letter
--range-diff=6ab505f161..symref-cmd-v3 @~7..@
where `6ab505f161` is the common base for both v3 and v4, and is from
merging in `ps/leakfixes`.
Range-diff against v3:
1: cab5265c3c = 1: cab5265c3c refs: create and use
`ref_update_expects_existing_old_ref()`
-: ---------- > 2: 57b5ff46c0 refs: specify error for regular refs
with `old_target`
2: ed54b0dfb9 = 3: 5710fa81bf update-ref: add support for
'symref-verify' command
3: b82b86ff40 ! 4: 5f8fc4eb6e update-ref: add support for
'symref-delete' command
@@ Commit message
within a transaction, which promises atomicity of the operation and can
be batched with other commands.
+ When no 'old_target' is provided it can also delete regular refs,
+ similar to how the 'delete' command can delete symrefs when
no 'old_oid'
+ is provided.
+
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ Documentation/git-update-ref.txt: verify::
## builtin/fetch.c ##
@@ builtin/fetch.c: static int prune_refs(struct display_state
*display_state,
+ if (!dry_run) {
if (transaction) {
for (ref = stale_refs; ref; ref = ref->next) {
- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
- "fetch: prune", &err);
-+ NULL, "fetch: prune", &err);
++ result = ref_transaction_delete(transaction, ref->name, NULL,
++ NULL, 0, "fetch: prune", &err);
if (result)
goto cleanup;
}
@@ builtin/receive-pack.c: static const char *update(struct
command *cmd, struct sh
namespaced_name,
old_oid,
- 0, "push", &err)) {
-+ 0, NULL,
++ NULL, 0,
+ "push", &err)) {
rp_error("%s", err.buf);
ret = "failed to delete";
@@ builtin/update-ref.c: static void parse_cmd_delete(struct
ref_transaction *trans
if (ref_transaction_delete(transaction, refname,
have_old ? &old_oid : NULL,
- update_flags, msg, &err))
-+ update_flags, NULL, msg, &err))
++ NULL, update_flags, msg, &err))
die("%s", err.buf);
update_flags = default_flags;
@@ builtin/update-ref.c: static void parse_cmd_delete(struct
ref_transaction *trans
+ die("symref-delete %s: extra input: %s", refname, next);
+
+ if (ref_transaction_delete(transaction, refname, NULL,
-+ update_flags, old_target, msg, &err))
++ old_target, update_flags, msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
@@ refs.c: int refs_delete_ref(struct ref_store *refs, const char *msg,
if (!transaction ||
ref_transaction_delete(transaction, refname, old_oid,
- flags, msg, &err) ||
-+ flags, NULL, msg, &err) ||
++ NULL, flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
@@ refs.c: int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
-+ unsigned int flags,
+ const char *old_target,
++ unsigned int flags,
+ const char *msg,
struct strbuf *err)
{
@@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg,
for_each_string_list_item(item, refnames) {
ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, msg, &err);
-+ NULL, flags, NULL, msg, &err);
++ NULL, NULL, flags, msg, &err);
if (ret) {
warning(_("could not delete reference %s: %s"),
item->string, err.buf);
@@ refs.h: int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
-+ unsigned int flags,
+ const char *old_target,
++ unsigned int flags,
+ const char *msg,
struct strbuf *err);
@@ t/t1400-update-ref.sh: do
+ grep "fatal: symref-delete: missing <ref>" err
+ '
+
++ test_expect_success "stdin $type symref-delete fails deleting
regular ref" '
++ test_when_finished "git update-ref -d refs/heads/regularref" &&
++ git update-ref refs/heads/regularref $a &&
++ format_command $type "symref-delete refs/heads/regularref"
"$a" >stdin &&
++ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
++ grep "fatal: cannot update regular ref:
${SQ}refs/heads/regularref${SQ}: symref target ${SQ}$a${SQ} set" err
++ '
++
+ test_expect_success "stdin $type symref-delete fails with too
many arguments" '
+ format_command $type "symref-delete refs/heads/symref" "$a"
"$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
@@ t/t1400-update-ref.sh: do
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git symbolic-ref -d refs/heads/symref2
+ '
++
++ test_expect_success "stdin $type symref-delete fails deleting
regular ref without target" '
++ git update-ref refs/heads/regularref $a &&
++ format_command $type "symref-delete refs/heads/regularref" >stdin &&
++ git update-ref --stdin $type --no-deref <stdin
++ '
+
done
4: ae127f7d52 ! 5: 1542cfb806 update-ref: add support for
'symref-create' command
@@ t/t0600-reffiles-backend.sh: test_expect_success POSIXPERM 'git
reflog expire ho
## t/t1400-update-ref.sh ##
@@ t/t1400-update-ref.sh: do
- test_must_fail git symbolic-ref -d refs/heads/symref2
+ git update-ref --stdin $type --no-deref <stdin
'
+ test_expect_success "stdin $type symref-create fails with too
many arguments" '
5: 8889dcbf40 = 6: ec5380743d reftable: pick either 'oid' or
'target' for new updates
6: 19d85d56c4 ! 7: f8d91f7fc9 update-ref: add support for
'symref-update' command
@@ Commit message
we don't use a `(ref | oid)` prefix for the old_value, then there is
ambiguity around if the value provided should be treated as an oid or a
reference. This is more so the reason, because we allow anything
- committish to be provided as an oid.
+ committish to be provided as an oid. While 'symref-verify' and
+ 'symref-delete' also take in `<old-target>` we do not have this
+ divergence there as those commands only work with symrefs. Whereas
+ 'symref-update' also works with regular refs and allows users
to convert
+ regular refs to symrefs.
The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 2/7] refs: specify error for regular refs with `old_target`
2024-06-06 11:02 ` Patrick Steinhardt
@ 2024-06-06 14:20 ` Karthik Nayak
0 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-06 14:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> When a regular reference update contains `old_target` set, we call the
>
> s/contains/has its/
>
>> `ref_update_check_old_target` function to check the referent value. But
>> for regular refs we know that the referent value is not set and this
>
> This is fairly technical and focussed on the implementation. Can we
> maybe talk more about intent ("expected a symref, but is a direct ref")
> rather than the exact implementation to make the commit message a bit
> easier to understand for folks?
>
Fair enough, will change this.
>> simply raises a generic error which says nothing about this being a
>> regular ref. Instead let's raise a more specific error when a regular
>> ref update contains `old_target`.
>
> It might be helpful to include before/after in this commit message to
> show the change. Even better would be a test of course, but I think we
> cannot add one at this point in time yet.
>
Yeah will do.
Yeah, adding a test is only possible in the commit where we introduce
`symref-delete` and we do test it there.
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> refs/files-backend.c | 13 +++++++------
>> refs/reftable-backend.c | 9 +++++++++
>> 2 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 194e74eb4d..f516d4d82f 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>
>> /*
>> * 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.
>> + * fail with an error.
>> */
>> if (update->old_target) {
>> - if (ref_update_check_old_target(referent.buf, update, err)) {
>> - ret = TRANSACTION_GENERIC_ERROR;
>> - goto out;
>> - }
>> + strbuf_addf(err, _("cannot update regular ref: '%s': "
>> + "symref target '%s' set"),
>> + ref_update_original_update_refname(update),
>> + update->old_target);
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>
> I feel like these error messages are somewhat technical. If I were
> reading it as a user, I don't think I'd understand what it is trying to
> tell me. How about:
>
> strbuf_addf(err, _("cannot lock ref '%s': %s",
> "expected symref but is a direct ref"));
>
> This also matches the other error messages we have in this function more
> closely. The same is true for the equivalent in the reftable backend.
>
> Patrick
>
I do want to also note that there is a old_target set so perhaps
modified refs/files-backend.c
@@ -2494,8 +2494,9 @@ static int lock_ref_for_update(struct
files_ref_store *refs,
* fail with an error.
*/
if (update->old_target) {
- strbuf_addf(err, _("cannot update regular ref: '%s': "
- "symref target '%s' set"),
+ strbuf_addf(err, _("cannot lock ref '%s': "
+ "expected symref with target '%s': "
+ "but is a regular ref"),
ref_update_original_update_refname(update),
update->old_target);
ret = TRANSACTION_GENERIC_ERROR;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 0/7] update-ref: add symref support for --stdin
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
2024-06-06 8:22 ` Karthik Nayak
2024-06-06 11:03 ` Karthik Nayak
@ 2024-06-07 13:32 ` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
` (6 more replies)
2 siblings, 7 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:32 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The 'update-ref' command is used to update refs using transactions. The
command allows users to also utilize a '--stdin' mode to provide a
batch of sub-commands which can be processed in a transaction.
Currently, the sub-commands involve {verify, delete, create, update}
and they allow users to work with regular refs in the repository. To
work with symrefs, users only have the option of using
'git-symbolic-ref', which doesn't provide transaction support to the
users eventhough it uses the same behind the hood.
Recently, we modified the reference backend to add symref support,
following which, 'git-symbolic-ref' also uses the transaction backend.
But, it doesn't expose this to the user. To allow users to work with
symrefs via transaction, this series adds support for new sub-commands
{symrer-verify, symref-delete, symref-create, symref-update} to the
'--stdin' mode of update-ref. These complement the existing
sub-commands.
The patches 1, 2, & 6 fix small issues in the reference backends. The other
patches 3, 4, 5, & 7 each add one of the new sub-commands.
The series is based off master, with 'kn/ref-transaction-symref' merged
in.
There was some discussion [1] also about adding `old_target` support to
the existing `update` command. I think its worthwhile to do this with
some tests cleanup, will follow that up as a separate series.
Changes since v4:
* Modified the commit message for the second patch and also the error to be
less technical and more user friendly.
* Renamed an incorrectly named test.
Version 1-3 can be found here:
https://lore.kernel.org/all/20240530120940.456817-1-knayak@gitlab.com/
[1]: https://lore.kernel.org/r/CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com
Karthik Nayak (7):
refs: create and use `ref_update_expects_existing_old_ref()`
refs: specify error for regular refs with `old_target`
update-ref: add support for 'symref-verify' command
update-ref: add support for 'symref-delete' command
update-ref: add support for 'symref-create' command
reftable: pick either 'oid' or 'target' for new updates
update-ref: add support for 'symref-update' command
Documentation/git-update-ref.txt | 25 ++
builtin/clone.c | 2 +-
builtin/fetch.c | 4 +-
builtin/receive-pack.c | 3 +-
builtin/update-ref.c | 237 ++++++++++++++++-
refs.c | 40 ++-
refs.h | 6 +-
refs/files-backend.c | 17 +-
refs/refs-internal.h | 6 +
refs/reftable-backend.c | 17 +-
t/t0600-reffiles-backend.sh | 32 +++
t/t1400-update-ref.sh | 430 ++++++++++++++++++++++++++++++-
t/t1416-ref-transaction-hooks.sh | 54 ++++
t/t5605-clone-local.sh | 2 +-
14 files changed, 834 insertions(+), 41 deletions(-)
Range-diff against v4:
1: cab5265c3c = 1: cab5265c3c refs: create and use `ref_update_expects_existing_old_ref()`
2: 57b5ff46c0 ! 2: 94077e69cc refs: specify error for regular refs with `old_target`
@@ Metadata
## Commit message ##
refs: specify error for regular refs with `old_target`
- When a regular reference update contains `old_target` set, we call the
- `ref_update_check_old_target` function to check the referent value. But
- for regular refs we know that the referent value is not set and this
- simply raises a generic error which says nothing about this being a
- regular ref. Instead let's raise a more specific error when a regular
- ref update contains `old_target`.
+ When a reference update tries to update a symref, but the ref in
+ question is actually a regular ref, we raise an error. However the error
+ raised in this situation is:
+
+ verifying symref target: '<ref>': reference is missing but expected <old-target>
+
+ which is very generic and doesn't indicate the mismatch of types. Let's
+ make this error more specific:
+
+ cannot lock ref '<ref>': expected symref with target '<old-target>': but is a regular ref
+
+ so that users have a clearer understanding.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref
- ret = TRANSACTION_GENERIC_ERROR;
- goto out;
- }
-+ strbuf_addf(err, _("cannot update regular ref: '%s': "
-+ "symref target '%s' set"),
++ strbuf_addf(err, _("cannot lock ref '%s': "
++ "expected symref with target '%s': "
++ "but is a regular ref"),
+ ref_update_original_update_refname(update),
+ update->old_target);
+ ret = TRANSACTION_GENERIC_ERROR;
@@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s
*/
if (u->old_target) {
+ if (!(u->type & REF_ISSYMREF)) {
-+ strbuf_addf(err, _("cannot update regular ref: '%s': "
-+ "symref target '%s' set"),
++ strbuf_addf(err, _("cannot lock ref '%s': "
++ "expected symref with target '%s': "
++ "but is a regular ref"),
+ ref_update_original_update_refname(u),
+ u->old_target);
+ ret = -1;
3: 5710fa81bf = 3: 153d8cce75 update-ref: add support for 'symref-verify' command
4: 5f8fc4eb6e ! 4: 07382bfa09 update-ref: add support for 'symref-delete' command
@@ t/t1400-update-ref.sh: do
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
-+ grep "fatal: cannot update regular ref: ${SQ}refs/heads/regularref${SQ}: symref target ${SQ}$a${SQ} set" err
++ grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: expected symref with target ${SQ}$a${SQ}: but is a regular ref" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails with too many arguments" '
@@ t/t1400-update-ref.sh: do
+ test_must_fail git symbolic-ref -d refs/heads/symref2
+ '
+
-+ test_expect_success "stdin $type symref-delete fails deleting regular ref without target" '
++ test_expect_success "stdin $type symref-delete deletes regular ref without target" '
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin
5: 1542cfb806 = 5: d5f92f610d update-ref: add support for 'symref-create' command
6: ec5380743d = 6: 117b9e49cd reftable: pick either 'oid' or 'target' for new updates
7: f8d91f7fc9 = 7: cc140fedbd update-ref: add support for 'symref-update' command
--
2.43.GIT
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 1/7] refs: create and use `ref_update_expects_existing_old_ref()`
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
@ 2024-06-07 13:32 ` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
` (5 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:32 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The files and reftable backend, need to check if a ref must exist, so
that the required validation can be done. A ref must exist only when the
`old_oid` value of the update has been explicitly set and it is not the
`null_oid` value.
Since we also support symrefs now, we need to ensure that even when
`old_target` is set a ref must exist. While this was missed when we
added symref support in transactions, there are no active users of this
path. As we introduce the 'symref-verify' command in the upcoming
commits, it is important to fix this.
So let's export this to a function called
`ref_update_expects_existing_old_ref()` and expose it internally via
'refs-internal.h'.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 6 ++++++
refs/files-backend.c | 3 +--
refs/refs-internal.h | 6 ++++++
refs/reftable-backend.c | 2 +-
4 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 8260c27cde..50d8d7d777 100644
--- a/refs.c
+++ b/refs.c
@@ -2679,3 +2679,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
referent, update->old_target);
return -1;
}
+
+int ref_update_expects_existing_old_ref(struct ref_update *update)
+{
+ return (update->flags & REF_HAVE_OLD) &&
+ (!is_null_oid(&update->old_oid) || update->old_target);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5f3089d947..194e74eb4d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2412,8 +2412,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
struct strbuf *err)
{
struct strbuf referent = STRBUF_INIT;
- int mustexist = (update->flags & REF_HAVE_OLD) &&
- !is_null_oid(&update->old_oid);
+ int mustexist = ref_update_expects_existing_old_ref(update);
int ret = 0;
struct ref_lock *lock;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 53a6c5d842..ee298ec0d5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
int ref_update_check_old_target(const char *referent, struct ref_update *update,
struct strbuf *err);
+/*
+ * Check if the ref must exist, this means that the old_oid or
+ * old_target is non NULL.
+ */
+int ref_update_expects_existing_old_ref(struct ref_update *update);
+
#endif /* REFS_REFS_INTERNAL_H */
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 1af86bbdec..b838cf8f00 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -824,7 +824,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
¤t_oid, &referent, &u->type);
if (ret < 0)
goto done;
- if (ret > 0 && (!(u->flags & REF_HAVE_OLD) || is_null_oid(&u->old_oid))) {
+ if (ret > 0 && !ref_update_expects_existing_old_ref(u)) {
/*
* The reference does not exist, and we either have no
* old object ID or expect the reference to not exist.
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 2/7] refs: specify error for regular refs with `old_target`
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
@ 2024-06-07 13:32 ` Karthik Nayak
2024-06-10 6:54 ` Patrick Steinhardt
2024-06-07 13:33 ` [PATCH v5 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
` (4 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:32 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
When a reference update tries to update a symref, but the ref in
question is actually a regular ref, we raise an error. However the error
raised in this situation is:
verifying symref target: '<ref>': reference is missing but expected <old-target>
which is very generic and doesn't indicate the mismatch of types. Let's
make this error more specific:
cannot lock ref '<ref>': expected symref with target '<old-target>': but is a regular ref
so that users have a clearer understanding.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/files-backend.c | 14 ++++++++------
refs/reftable-backend.c | 10 ++++++++++
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 194e74eb4d..fc57c9d220 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
/*
* 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.
+ * fail with an error.
*/
if (update->old_target) {
- if (ref_update_check_old_target(referent.buf, update, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto out;
- }
+ strbuf_addf(err, _("cannot lock ref '%s': "
+ "expected symref with target '%s': "
+ "but is a regular ref"),
+ ref_update_original_update_refname(update),
+ update->old_target);
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto out;
} else if (check_old_oid(update, &lock->old_oid, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto out;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b838cf8f00..c66ab9ecd8 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -928,6 +928,16 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
* backend returns, which keeps our tests happy.
*/
if (u->old_target) {
+ if (!(u->type & REF_ISSYMREF)) {
+ strbuf_addf(err, _("cannot lock ref '%s': "
+ "expected symref with target '%s': "
+ "but is a regular ref"),
+ ref_update_original_update_refname(u),
+ u->old_target);
+ ret = -1;
+ goto done;
+ }
+
if (ref_update_check_old_target(referent.buf, u, err)) {
ret = -1;
goto done;
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 3/7] update-ref: add support for 'symref-verify' command
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
@ 2024-06-07 13:33 ` Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
` (3 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:33 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
The 'symref-verify' command allows users to verify if a provided <ref>
contains the provided <old-target> without changing the <ref>. If
<old-target> is not provided, the command will verify that the <ref>
doesn't exist.
The command allows users to verify symbolic refs within a transaction,
and this means users can perform a set of changes in a transaction only
when the verification holds good.
Since we're checking for symbolic refs, this command will only work with
the 'no-deref' mode. This is because any dereferenced symbolic ref will
point to an object and not a ref and the regular 'verify' command can be
used in such situations.
Add required tests for symref support in 'verify'. Since we're here,
also add reflog checks for the pre-existing 'verify' tests, there is no
divergence from behavior, but we never tested to ensure that reflog
wasn't affected by the 'verify' command.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 7 +++
builtin/update-ref.c | 80 +++++++++++++++++++++++----
refs.c | 11 +++-
refs.h | 1 +
t/t1400-update-ref.sh | 94 +++++++++++++++++++++++++++++++-
t/t1416-ref-transaction-hooks.sh | 30 ++++++++++
6 files changed, 208 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 374a2ebd2b..9fe78b3501 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
start LF
prepare LF
@@ -86,6 +87,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
start NUL
prepare NUL
@@ -117,6 +119,11 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-verify::
+ Verify symbolic <ref> against <old-target> but do not change it.
+ If <old-target> is missing, the ref must not exist. Can only be
+ used in `no-deref` mode.
+
option::
Modify the behavior of the next command naming a <ref>.
The only valid option is `no-deref` to avoid dereferencing
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6cda1c08aa..50f5472160 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -76,6 +76,29 @@ static char *parse_refname(const char **next)
return strbuf_detach(&ref, NULL);
}
+/*
+ * Wrapper around parse_refname which skips the next delimiter.
+ */
+static char *parse_next_refname(const char **next)
+{
+ if (line_termination) {
+ /* Without -z, consume SP and use next argument */
+ if (!**next || **next == line_termination)
+ return NULL;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ } else {
+ /* With -z, read the next NUL-terminated line */
+ if (**next)
+ return NULL;
+ }
+ /* Skip the delimiter */
+ (*next)++;
+
+ return parse_refname(next);
+}
+
+
/*
* The value being parsed is <old-oid> (as opposed to <new-oid>; the
* difference affects which error messages are generated):
@@ -297,11 +320,47 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
die("verify %s: extra input: %s", refname, next);
if (ref_transaction_verify(transaction, refname, &old_oid,
- update_flags, &err))
+ NULL, update_flags, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ strbuf_release(&err);
+}
+
+static void parse_cmd_symref_verify(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ struct object_id old_oid;
+ char *refname, *old_target;
+
+ if (!(update_flags & REF_NO_DEREF))
+ die("symref-verify: cannot operate with deref mode");
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-verify: missing <ref>");
+
+ /*
+ * old_ref is optional, if not provided, we need to ensure that the
+ * ref doesn't exist.
+ */
+ old_target = parse_next_refname(&next);
+ if (!old_target)
+ oidcpy(&old_oid, null_oid());
+
+ if (*next != line_termination)
+ die("symref-verify %s: extra input: %s", refname, next);
+
+ if (ref_transaction_verify(transaction, refname,
+ old_target ? NULL : &old_oid,
+ old_target, update_flags, &err))
die("%s", err.buf);
update_flags = default_flags;
free(refname);
+ free(old_target);
strbuf_release(&err);
}
@@ -380,15 +439,16 @@ static const struct parse_cmd {
unsigned args;
enum update_refs_state state;
} command[] = {
- { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
- { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
- { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
- { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
- { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
- { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
- { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
- { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
- { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
+ { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN },
+ { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
+ { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
+ { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
+ { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
+ { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
+ { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+ { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },
+ { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },
};
static void update_refs_stdin(void)
diff --git a/refs.c b/refs.c
index 50d8d7d777..cdc4d25557 100644
--- a/refs.c
+++ b/refs.c
@@ -1297,14 +1297,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err)
{
- if (!old_oid)
- BUG("verify called with old_oid set to NULL");
+ if (!old_target && !old_oid)
+ BUG("verify called with old_oid and old_target set to NULL");
+ if (old_oid && old_target)
+ BUG("verify called with both old_oid and old_target set");
+ if (old_target && !(flags & REF_NO_DEREF))
+ BUG("verify cannot operate on symrefs with deref mode");
return ref_transaction_update(transaction, refname,
NULL, old_oid,
- NULL, NULL,
+ NULL, old_target,
flags, NULL, err);
}
diff --git a/refs.h b/refs.h
index 34568ee1fb..906299351c 100644
--- a/refs.h
+++ b/refs.h
@@ -736,6 +736,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
int ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
+ const char *old_target,
unsigned int flags,
struct strbuf *err);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index bbee2783ab..52801be07d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -892,17 +892,23 @@ test_expect_success 'stdin update/create/verify combination works' '
'
test_expect_success 'stdin verify succeeds for correct value' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
git rev-parse $m >expect &&
echo "verify $m $m" >stdin &&
git update-ref --stdin <stdin &&
git rev-parse $m >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify succeeds for missing reference' '
+ test-tool ref-store main for-each-reflog-ent $m >before &&
echo "verify refs/heads/missing $Z" >stdin &&
git update-ref --stdin <stdin &&
- test_must_fail git rev-parse --verify -q refs/heads/missing
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent $m >after &&
+ test_cmp before after
'
test_expect_success 'stdin verify treats no value as missing' '
@@ -1643,4 +1649,88 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
+format_command () {
+ if test "$1" = "-z"
+ then
+ shift
+ printf "$F" "$@"
+ else
+ echo "$@"
+ fi
+}
+
+for type in "" "-z"
+do
+
+ test_expect_success "stdin $type symref-verify fails without --no-deref" '
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-verify: cannot operate with deref mode" err
+ '
+
+ test_expect_success "stdin $type symref-verify fails with too many arguments" '
+ format_command $type "symref-verify refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-verify refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-verify succeeds for correct value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ format_command $type "symref-verify refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin $type symref-verify fails with no value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-verify succeeds for dangling reference" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
+ test_must_fail git symbolic-ref refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+ format_command $type "symref-verify refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-verify fails for missing reference" '
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >before &&
+ format_command $type "symref-verify refs/heads/missing" "refs/heads/unknown" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/missing${SQ}: unable to resolve reference ${SQ}refs/heads/missing${SQ}" err &&
+ test_must_fail git rev-parse --verify -q refs/heads/missing &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >after &&
+ test_cmp before after
+ '
+
+ test_expect_success "stdin $type symref-verify fails for wrong value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "$b" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-verify fails for mistaken null value" '
+ git symbolic-ref refs/heads/symref >expect &&
+ format_command $type "symref-verify refs/heads/symref" "$Z" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+done
+
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 067fd57290..fd58b902f4 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -157,4 +157,34 @@ test_expect_success 'hook captures git-symbolic-ref updates' '
test_cmp expect actual
'
+test_expect_success 'hook gets all queued symref updates' '
+ test_when_finished "rm actual" &&
+
+ git update-ref refs/heads/branch $POST_OID &&
+ git symbolic-ref refs/heads/symref refs/heads/main &&
+
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ while read -r line
+ do
+ printf "%s\n" "$line"
+ done >>actual
+ EOF
+
+ cat >expect <<-EOF &&
+ prepared
+ ref:refs/heads/main $ZERO_OID refs/heads/symref
+ committed
+ ref:refs/heads/main $ZERO_OID refs/heads/symref
+ EOF
+
+ git update-ref --no-deref --stdin <<-EOF &&
+ start
+ symref-verify refs/heads/symref refs/heads/main
+ prepare
+ commit
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 4/7] update-ref: add support for 'symref-delete' command
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
` (2 preceding siblings ...)
2024-06-07 13:33 ` [PATCH v5 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
@ 2024-06-07 13:33 ` Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
` (2 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:33 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add a new command 'symref-delete' to allow deletions of symbolic refs in
a transaction via the '--stdin' mode of the 'git-update-ref' command.
The 'symref-delete' command can, when given an <old-target>, delete the
provided <ref> only when it points to <old-target>.
This command is only compatible with the 'no-deref' mode because we
optionally want to check the 'old_target' of the ref being deleted.
De-referencing a symbolic ref would provide a regular ref and we already
have the 'delete' command for regular refs.
While users can also use 'git symbolic-ref -d' to delete symbolic refs,
the 'symref-delete' command in 'git-update-ref' allows users to do so
within a transaction, which promises atomicity of the operation and can
be batched with other commands.
When no 'old_target' is provided it can also delete regular refs,
similar to how the 'delete' command can delete symrefs when no 'old_oid'
is provided.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 5 +++
builtin/fetch.c | 4 +-
builtin/receive-pack.c | 3 +-
builtin/update-ref.c | 33 +++++++++++++++-
refs.c | 14 +++++--
refs.h | 4 +-
t/t1400-update-ref.sh | 68 ++++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 19 ++++++++-
8 files changed, 140 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9fe78b3501..16e02f6979 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
start LF
@@ -87,6 +88,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
start NUL
@@ -119,6 +121,9 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-delete::
+ Delete <ref> after verifying it exists with <old-target>, if given.
+
symref-verify::
Verify symbolic <ref> against <old-target> but do not change it.
If <old-target> is missing, the ref must not exist. Can only be
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 75255dc600..d63100e0d3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1386,8 +1386,8 @@ static int prune_refs(struct display_state *display_state,
if (!dry_run) {
if (transaction) {
for (ref = stale_refs; ref; ref = ref->next) {
- result = ref_transaction_delete(transaction, ref->name, NULL, 0,
- "fetch: prune", &err);
+ result = ref_transaction_delete(transaction, ref->name, NULL,
+ NULL, 0, "fetch: prune", &err);
if (result)
goto cleanup;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 01c1f04ece..0a30fac239 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1576,7 +1576,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
if (ref_transaction_delete(transaction,
namespaced_name,
old_oid,
- 0, "push", &err)) {
+ NULL, 0,
+ "push", &err)) {
rp_error("%s", err.buf);
ret = "failed to delete";
} else {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 50f5472160..0cb7eef3c6 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -293,7 +293,7 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
if (ref_transaction_delete(transaction, refname,
have_old ? &old_oid : NULL,
- update_flags, msg, &err))
+ NULL, update_flags, msg, &err))
die("%s", err.buf);
update_flags = default_flags;
@@ -301,6 +301,36 @@ static void parse_cmd_delete(struct ref_transaction *transaction,
strbuf_release(&err);
}
+
+static void parse_cmd_symref_delete(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ char *refname, *old_target;
+
+ if (!(update_flags & REF_NO_DEREF))
+ die("symref-delete: cannot operate with deref mode");
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-delete: missing <ref>");
+
+ old_target = parse_next_refname(&next);
+
+ if (*next != line_termination)
+ die("symref-delete %s: extra input: %s", refname, next);
+
+ if (ref_transaction_delete(transaction, refname, NULL,
+ old_target, update_flags, msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(old_target);
+ strbuf_release(&err);
+}
+
+
static void parse_cmd_verify(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -443,6 +473,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
{ "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
{ "start", parse_cmd_start, 0, UPDATE_REFS_STARTED },
diff --git a/refs.c b/refs.c
index cdc4d25557..01f3188a09 100644
--- a/refs.c
+++ b/refs.c
@@ -950,7 +950,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
transaction = ref_store_transaction_begin(refs, &err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_oid,
- flags, msg, &err) ||
+ NULL, flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
@@ -1283,14 +1283,20 @@ int ref_transaction_create(struct ref_transaction *transaction,
int ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
+ const char *old_target,
+ unsigned int flags,
+ const char *msg,
struct strbuf *err)
{
if (old_oid && is_null_oid(old_oid))
BUG("delete called with old_oid set to zeros");
+ if (old_oid && old_target)
+ BUG("delete called with both old_oid and old_target set");
+ if (old_target && !(flags & REF_NO_DEREF))
+ BUG("delete cannot operate on symrefs with deref mode");
return ref_transaction_update(transaction, refname,
null_oid(), old_oid,
- NULL, NULL, flags,
+ NULL, old_target, flags,
msg, err);
}
@@ -2599,7 +2605,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
for_each_string_list_item(item, refnames) {
ret = ref_transaction_delete(transaction, item->string,
- NULL, flags, msg, &err);
+ NULL, NULL, flags, msg, &err);
if (ret) {
warning(_("could not delete reference %s: %s"),
item->string, err.buf);
diff --git a/refs.h b/refs.h
index 906299351c..974cf4dd08 100644
--- a/refs.h
+++ b/refs.h
@@ -722,7 +722,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
int ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
const struct object_id *old_oid,
- unsigned int flags, const char *msg,
+ const char *old_target,
+ unsigned int flags,
+ const char *msg,
struct strbuf *err);
/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 52801be07d..9dbe28bd13 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1731,6 +1731,74 @@ do
test_cmp expect actual
'
+ test_expect_success "stdin $type symref-delete fails without --no-deref" '
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-delete: cannot operate with deref mode" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails with no ref" '
+ format_command $type "symref-delete " >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: symref-delete: missing <ref>" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails deleting regular ref" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: expected symref with target ${SQ}$a${SQ}: but is a regular ref" err
+ '
+
+ test_expect_success "stdin $type symref-delete fails with too many arguments" '
+ format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-delete refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-delete fails with wrong old value" '
+ format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-delete works with right old value" '
+ format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git rev-parse --verify -q refs/heads/symref
+ '
+
+ test_expect_success "stdin $type symref-delete works with empty old value" '
+ git symbolic-ref refs/heads/symref $a >stdin &&
+ format_command $type "symref-delete refs/heads/symref" "" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git rev-parse --verify -q $b
+ '
+
+ test_expect_success "stdin $type symref-delete succeeds for dangling reference" '
+ test_must_fail git symbolic-ref refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
+ format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ test_must_fail git symbolic-ref -d refs/heads/symref2
+ '
+
+ test_expect_success "stdin $type symref-delete deletes regular ref without target" '
+ git update-ref refs/heads/regularref $a &&
+ format_command $type "symref-delete refs/heads/regularref" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index fd58b902f4..ccde1b944b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -162,6 +162,7 @@ test_expect_success 'hook gets all queued symref updates' '
git update-ref refs/heads/branch $POST_OID &&
git symbolic-ref refs/heads/symref refs/heads/main &&
+ git symbolic-ref refs/heads/symrefd refs/heads/main &&
test_hook reference-transaction <<-\EOF &&
echo "$*" >>actual
@@ -171,16 +172,32 @@ test_expect_success 'hook gets all queued symref updates' '
done >>actual
EOF
- cat >expect <<-EOF &&
+ # In the files backend, "delete" also triggers an additional transaction
+ # update on the packed-refs backend, which constitutes additional reflog
+ # entries.
+ if test_have_prereq REFFILES
+ then
+ cat >expect <<-EOF
+ aborted
+ $ZERO_OID $ZERO_OID refs/heads/symrefd
+ EOF
+ else
+ >expect
+ fi &&
+
+ cat >>expect <<-EOF &&
prepared
ref:refs/heads/main $ZERO_OID refs/heads/symref
+ ref:refs/heads/main $ZERO_OID refs/heads/symrefd
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
+ ref:refs/heads/main $ZERO_OID refs/heads/symrefd
EOF
git update-ref --no-deref --stdin <<-EOF &&
start
symref-verify refs/heads/symref refs/heads/main
+ symref-delete refs/heads/symrefd refs/heads/main
prepare
commit
EOF
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 5/7] update-ref: add support for 'symref-create' command
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
` (3 preceding siblings ...)
2024-06-07 13:33 ` [PATCH v5 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
@ 2024-06-07 13:33 ` Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:33 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add 'symref-create' command to the '--stdin' mode 'git-update-ref' to
allow creation of symbolic refs in a transaction. The 'symref-create'
command takes in a <new-target>, which the created <ref> will point to.
Also, support the 'core.prefersymlinkrefs' config, wherein if the config
is set and the filesystem supports symlinks, we create the symbolic ref
as a symlink. We fallback to creating a regular symref if creating the
symlink is unsuccessful.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 6 +++
builtin/clone.c | 2 +-
builtin/update-ref.c | 32 +++++++++++++++-
refs.c | 9 +++--
refs.h | 1 +
t/t0600-reffiles-backend.sh | 32 ++++++++++++++++
t/t1400-update-ref.sh | 65 ++++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 3 ++
t/t5605-clone-local.sh | 2 +-
9 files changed, 146 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 16e02f6979..364ef78af1 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-create SP <ref> SP <new-target> LF
symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
option SP <opt> LF
@@ -88,6 +89,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-create SP <ref> NUL <new-target> NUL
symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
option SP <opt> NUL
@@ -121,6 +123,10 @@ verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
+symref-create:
+ Create symbolic ref <ref> with <new-target> after verifying
+ it does not exist.
+
symref-delete::
Delete <ref> after verifying it exists with <old-target>, if given.
diff --git a/builtin/clone.c b/builtin/clone.c
index 23993b905b..6ddb3084e6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -576,7 +576,7 @@ static void write_remote_refs(const struct ref *local_refs)
if (!r->peer_ref)
continue;
if (ref_transaction_create(t, r->peer_ref->name, &r->old_oid,
- 0, NULL, &err))
+ NULL, 0, NULL, &err))
die("%s", err.buf);
}
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0cb7eef3c6..9c40e94626 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,7 +257,7 @@ static void parse_cmd_create(struct ref_transaction *transaction,
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
- if (ref_transaction_create(transaction, refname, &new_oid,
+ if (ref_transaction_create(transaction, refname, &new_oid, NULL,
update_flags | create_reflog_flag,
msg, &err))
die("%s", err.buf);
@@ -267,6 +267,35 @@ static void parse_cmd_create(struct ref_transaction *transaction,
strbuf_release(&err);
}
+
+static void parse_cmd_symref_create(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ struct strbuf err = STRBUF_INIT;
+ char *refname, *new_target;
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-create: missing <ref>");
+
+ new_target = parse_next_refname(&next);
+ if (!new_target)
+ die("symref-create %s: missing <new-target>", refname);
+
+ if (*next != line_termination)
+ die("symref-create %s: extra input: %s", refname, next);
+
+ if (ref_transaction_create(transaction, refname, NULL, new_target,
+ update_flags | create_reflog_flag,
+ msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(new_target);
+ strbuf_release(&err);
+}
+
static void parse_cmd_delete(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -473,6 +502,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
{ "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
diff --git a/refs.c b/refs.c
index 01f3188a09..8807e87e3c 100644
--- a/refs.c
+++ b/refs.c
@@ -1268,15 +1268,18 @@ int ref_transaction_update(struct ref_transaction *transaction,
int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *new_oid,
+ const char *new_target,
unsigned int flags, const char *msg,
struct strbuf *err)
{
- if (!new_oid || is_null_oid(new_oid)) {
- strbuf_addf(err, "'%s' has a null OID", refname);
+ if (new_oid && new_target)
+ BUG("create called with both new_oid and new_target set");
+ if ((!new_oid || is_null_oid(new_oid)) && !new_target) {
+ strbuf_addf(err, "'%s' has neither a valid OID nor a target", refname);
return 1;
}
return ref_transaction_update(transaction, refname, new_oid,
- null_oid(), NULL, NULL, flags,
+ null_oid(), new_target, NULL, flags,
msg, err);
}
diff --git a/refs.h b/refs.h
index 974cf4dd08..28e3bb8a42 100644
--- a/refs.h
+++ b/refs.h
@@ -708,6 +708,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
int ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
const struct object_id *new_oid,
+ const char *new_target,
unsigned int flags, const char *msg,
struct strbuf *err);
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 92f570313d..b2a771ff2b 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -468,4 +468,36 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
esac
'
+test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+ test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+ git update-ref refs/heads/new @ &&
+ test_config core.prefersymlinkrefs true &&
+ cat >stdin <<-EOF &&
+ start
+ symref-create TEST_SYMREF_HEAD refs/heads/new
+ prepare
+ commit
+ EOF
+ git update-ref --no-deref --stdin <stdin &&
+ test_path_is_symlink .git/TEST_SYMREF_HEAD &&
+ test "$(test_readlink .git/TEST_SYMREF_HEAD)" = refs/heads/new
+'
+
+test_expect_success 'symref transaction supports false symlink config' '
+ test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
+ git update-ref refs/heads/new @ &&
+ test_config core.prefersymlinkrefs false &&
+ cat >stdin <<-EOF &&
+ start
+ symref-create TEST_SYMREF_HEAD refs/heads/new
+ prepare
+ commit
+ EOF
+ git update-ref --no-deref --stdin <stdin &&
+ test_path_is_file .git/TEST_SYMREF_HEAD &&
+ git symbolic-ref TEST_SYMREF_HEAD >actual &&
+ echo refs/heads/new >expect &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9dbe28bd13..253263320d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1799,6 +1799,71 @@ do
git update-ref --stdin $type --no-deref <stdin
'
+ test_expect_success "stdin $type symref-create fails with too many arguments" '
+ format_command $type "symref-create refs/heads/symref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-create refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-create fails with no target" '
+ format_command $type "symref-create refs/heads/symref" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-create fails with empty target" '
+ format_command $type "symref-create refs/heads/symref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin
+ '
+
+ test_expect_success "stdin $type symref-create works" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-create works with --no-deref" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" &&
+ git update-ref --stdin $type <stdin 2>err
+ '
+
+ test_expect_success "stdin $type create dangling symref ref works" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "refs/heads/unkown" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo refs/heads/unkown >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-create does not create reflogs by default" '
+ test_when_finished "git symbolic-ref -d refs/symref" &&
+ format_command $type "symref-create refs/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual &&
+ test_must_fail git reflog exists refs/symref
+ '
+
+ test_expect_success "stdin $type symref-create reflogs with --create-reflog" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-create refs/heads/symref" "$a" >stdin &&
+ git update-ref --create-reflog --stdin $type --no-deref <stdin &&
+ git symbolic-ref refs/heads/symref >expect &&
+ echo $a >actual &&
+ test_cmp expect actual &&
+ git reflog exists refs/heads/symref
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ccde1b944b..ff77dcca6b 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -189,15 +189,18 @@ test_expect_success 'hook gets all queued symref updates' '
prepared
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+ $ZERO_OID ref:refs/heads/main refs/heads/symrefc
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
+ $ZERO_OID ref:refs/heads/main refs/heads/symrefc
EOF
git update-ref --no-deref --stdin <<-EOF &&
start
symref-verify refs/heads/symref refs/heads/main
symref-delete refs/heads/symrefd refs/heads/main
+ symref-create refs/heads/symrefc refs/heads/main
prepare
commit
EOF
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index a3055869bc..339d8c786f 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -163,7 +163,7 @@ test_expect_success REFFILES 'local clone from repo with corrupt refs fails grac
echo a >corrupt/.git/refs/heads/topic &&
test_must_fail git clone corrupt working 2>err &&
- grep "has a null OID" err
+ grep "has neither a valid OID nor a target" err
'
test_done
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 6/7] reftable: pick either 'oid' or 'target' for new updates
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
` (4 preceding siblings ...)
2024-06-07 13:33 ` [PATCH v5 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
@ 2024-06-07 13:33 ` Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:33 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
When creating a reference transaction update, we can provide the old/new
oid/target for the update. We have checks in place to ensure that for
each old/new, either oid or target is set and not both.
In the reftable backend, when dealing with updates without the
`REF_NO_DEREF` flag, we don't selectively propagate data as needed.
Since there are no active users of the path, this is not caught. As we
want to introduce the 'symref-update' command in the upcoming commit,
which would use this flow, correct it.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/reftable-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index c66ab9ecd8..1c46fc87f8 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -895,8 +895,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
*/
new_update = ref_transaction_add_update(
transaction, referent.buf, new_flags,
- &u->new_oid, &u->old_oid, u->new_target,
- u->old_target, u->msg);
+ u->new_target ? NULL : &u->new_oid,
+ u->old_target ? NULL : &u->old_oid,
+ u->new_target, u->old_target, u->msg);
new_update->parent_update = u;
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 7/7] update-ref: add support for 'symref-update' command
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
` (5 preceding siblings ...)
2024-06-07 13:33 ` [PATCH v5 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
@ 2024-06-07 13:33 ` Karthik Nayak
6 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-07 13:33 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
From: Karthik Nayak <karthik.188@gmail.com>
Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
allow updates of symbolic refs. The 'symref-update' command takes in a
<new-target>, which the <ref> will be updated to. If the <ref> doesn't
exist it will be created.
It also optionally takes either an `ref <old-target>` or `oid
<old-oid>`. If the <old-target> is provided, it checks to see if the
<ref> targets the <old-target> before the update. If <old-oid> is provided
it checks <ref> to ensure that it is a regular ref and <old-oid> is the
OID before the update. This by extension also means that this when a
zero <old-oid> is provided, it ensures that the ref didn't exist before.
The divergence in syntax from the regular `update` command is because if
we don't use a `(ref | oid)` prefix for the old_value, then there is
ambiguity around if the value provided should be treated as an oid or a
reference. This is more so the reason, because we allow anything
committish to be provided as an oid. While 'symref-verify' and
'symref-delete' also take in `<old-target>` we do not have this
divergence there as those commands only work with symrefs. Whereas
'symref-update' also works with regular refs and allows users to convert
regular refs to symrefs.
The command allows users to perform symbolic ref updates within a
transaction. This provides atomicity and allows users to perform a set
of operations together.
This command supports deref mode, to ensure that we can update
dereferenced regular refs to symrefs.
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-update-ref.txt | 7 ++
builtin/update-ref.c | 92 ++++++++++++++
t/t1400-update-ref.sh | 203 +++++++++++++++++++++++++++++++
t/t1416-ref-transaction-hooks.sh | 4 +
4 files changed, 306 insertions(+)
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 364ef78af1..afcf33cf60 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form:
create SP <ref> SP <new-oid> LF
delete SP <ref> [SP <old-oid>] LF
verify SP <ref> [SP <old-oid>] LF
+ symref-update SP <ref> SP <new-target> [SP (ref SP <old-target> | oid SP <old-oid>)] LF
symref-create SP <ref> SP <new-target> LF
symref-delete SP <ref> [SP <old-target>] LF
symref-verify SP <ref> [SP <old-target>] LF
@@ -89,6 +90,7 @@ quoting:
create SP <ref> NUL <new-oid> NUL
delete SP <ref> NUL [<old-oid>] NUL
verify SP <ref> NUL [<old-oid>] NUL
+ symref-update SP <ref> NUL <new-target> [NUL (ref NUL <old-target> | oid NUL <old-oid>)] NUL
symref-create SP <ref> NUL <new-target> NUL
symref-delete SP <ref> [NUL <old-target>] NUL
symref-verify SP <ref> [NUL <old-target>] NUL
@@ -119,6 +121,11 @@ delete::
Delete <ref> after verifying it exists with <old-oid>, if
given. If given, <old-oid> may not be zero.
+symref-update::
+ Set <ref> to <new-target> after verifying <old-target> or <old-oid>,
+ if given. Specify a zero <old-oid> to ensure that the ref does not
+ exist before the update.
+
verify::
Verify <ref> against <old-oid> but do not change it. If
<old-oid> is zero or missing, the ref must not exist.
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9c40e94626..471fa5c8d1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -98,6 +98,42 @@ static char *parse_next_refname(const char **next)
return parse_refname(next);
}
+/*
+ * Wrapper around parse_arg which skips the next delimiter.
+ */
+static char *parse_next_arg(const char **next)
+{
+ struct strbuf arg = STRBUF_INIT;
+
+ if (line_termination) {
+ /* Without -z, consume SP and use next argument */
+ if (!**next || **next == line_termination)
+ return NULL;
+ if (**next != ' ')
+ die("expected SP but got: %s", *next);
+ } else {
+ /* With -z, read the next NUL-terminated line */
+ if (**next)
+ return NULL;
+ }
+ /* Skip the delimiter */
+ (*next)++;
+
+ if (line_termination) {
+ /* Without -z, use the next argument */
+ *next = parse_arg(*next, &arg);
+ } else {
+ /* With -z, use everything up to the next NUL */
+ strbuf_addstr(&arg, *next);
+ *next += arg.len;
+ }
+
+ if (arg.len)
+ return strbuf_detach(&arg, NULL);
+
+ strbuf_release(&arg);
+ return NULL;
+}
/*
* The value being parsed is <old-oid> (as opposed to <new-oid>; the
@@ -237,6 +273,61 @@ static void parse_cmd_update(struct ref_transaction *transaction,
strbuf_release(&err);
}
+static void parse_cmd_symref_update(struct ref_transaction *transaction,
+ const char *next, const char *end)
+{
+ char *refname, *new_target, *old_arg;
+ char *old_target = NULL;
+ struct strbuf err = STRBUF_INIT;
+ struct object_id old_oid;
+ int have_old_oid = 0;
+
+ refname = parse_refname(&next);
+ if (!refname)
+ die("symref-update: missing <ref>");
+
+ new_target = parse_next_refname(&next);
+ if (!new_target)
+ die("symref-update %s: missing <new-target>", refname);
+
+ old_arg = parse_next_arg(&next);
+ if (old_arg) {
+ old_target = parse_next_arg(&next);
+ if (!old_target)
+ die("symref-update %s: expected old value", refname);
+
+ if (!strcmp(old_arg, "oid")) {
+ if (repo_get_oid(the_repository, old_target, &old_oid))
+ die("symref-update %s: invalid oid: %s", refname, old_target);
+
+ have_old_oid = 1;
+ } else if (!strcmp(old_arg, "ref")) {
+ if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
+ die("symref-update %s: invalid ref: %s", refname, old_target);
+ } else {
+ die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
+ }
+ }
+
+ if (*next != line_termination)
+ die("symref-update %s: extra input: %s", refname, next);
+
+ if (ref_transaction_update(transaction, refname, NULL,
+ have_old_oid ? &old_oid : NULL,
+ new_target,
+ have_old_oid ? NULL : old_target,
+ update_flags | create_reflog_flag,
+ msg, &err))
+ die("%s", err.buf);
+
+ update_flags = default_flags;
+ free(refname);
+ free(old_arg);
+ free(old_target);
+ free(new_target);
+ strbuf_release(&err);
+}
+
static void parse_cmd_create(struct ref_transaction *transaction,
const char *next, const char *end)
{
@@ -502,6 +593,7 @@ static const struct parse_cmd {
{ "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
{ "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
{ "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
+ { "symref-update", parse_cmd_symref_update, 4, UPDATE_REFS_OPEN },
{ "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
{ "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
{ "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 253263320d..eb1691860d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1362,6 +1362,7 @@ test_expect_success 'fails with duplicate HEAD update' '
'
test_expect_success 'fails with duplicate ref update via symref' '
+ test_when_finished "git symbolic-ref -d refs/heads/symref2" &&
git branch target2 $A &&
git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
cat >stdin <<-EOF &&
@@ -1864,6 +1865,208 @@ do
git reflog exists refs/heads/symref
'
+ test_expect_success "stdin $type symref-update fails with too many arguments" '
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ if test "$type" = "-z"
+ then
+ grep "fatal: unknown command: $a" err
+ else
+ grep "fatal: symref-update refs/heads/symref: extra input: $a" err
+ fi
+ '
+
+ test_expect_success "stdin $type symref-update fails with wrong old value argument" '
+ format_command $type "symref-update refs/heads/symref" "$a" "foo" "$a" "$a" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: symref-update refs/heads/symref: invalid arg ${SQ}foo${SQ} for old value" err
+ '
+
+ test_expect_success "stdin $type symref-update creates with zero old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates with no old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates dangling" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo refs/heads/nonexistent >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails with wrong old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "$b" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
+ grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected $b" err &&
+ test_must_fail git rev-parse --verify -q $c
+ '
+
+ test_expect_success "stdin $type symref-update updates dangling ref" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update updates dangling ref with old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/nonexistent" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails update dangling ref with wrong old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_must_fail git rev-parse refs/heads/nonexistent &&
+ git symbolic-ref refs/heads/symref refs/heads/nonexistent &&
+ format_command $type "symref-update refs/heads/symref" "$a" "ref" "refs/heads/wrongref" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ echo refs/heads/nonexistent >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update works with right old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "$a" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $m >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update works with no old value" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" >stdin &&
+ git update-ref --stdin $type --no-deref <stdin &&
+ echo $m >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update fails with empty old ref-target" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref $a &&
+ format_command $type "symref-update refs/heads/symref" "$m" "ref" "" >stdin &&
+ test_must_fail git update-ref --stdin $type --no-deref <stdin &&
+ echo $a >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update creates (with deref)" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref --stdin $type <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+ grep "$Z $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref with correct old-oid" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse $a)" >stdin &&
+ git update-ref --stdin $type <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref fails with wrong old-oid" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "$(git rev-parse refs/heads/target2)" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/regularref${SQ}: is at $(git rev-parse $a) but expected $(git rev-parse refs/heads/target2)" err &&
+ echo $(git rev-parse $a) >expect &&
+ git rev-parse refs/heads/regularref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref fails with invalid old-oid" '
+ test_when_finished "git update-ref -d refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" "oid" "not-a-ref-oid" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: symref-update refs/heads/regularref: invalid oid: not-a-ref-oid" err &&
+ echo $(git rev-parse $a) >expect &&
+ git rev-parse refs/heads/regularref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update existing symref with zero old-oid" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/symref" &&
+ git symbolic-ref refs/heads/symref refs/heads/target2 &&
+ format_command $type "symref-update refs/heads/symref" "$a" "oid" "$Z" >stdin &&
+ test_must_fail git update-ref --stdin $type <stdin 2>err &&
+ grep "fatal: cannot lock ref ${SQ}refs/heads/symref${SQ}: reference already exists" err &&
+ echo refs/heads/target2 >expect &&
+ git symbolic-ref refs/heads/symref >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref (with deref)" '
+ test_when_finished "git symbolic-ref -d refs/heads/symref" &&
+ test_when_finished "git update-ref -d --no-deref refs/heads/symref2" &&
+ git update-ref refs/heads/symref2 $a &&
+ git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 &&
+ format_command $type "symref-update refs/heads/symref" "$a" >stdin &&
+ git update-ref $type --stdin <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref2 >actual &&
+ test_cmp expect actual &&
+ echo refs/heads/symref2 >expect &&
+ git symbolic-ref --no-recurse refs/heads/symref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
+ test_expect_success "stdin $type symref-update regular ref to symref" '
+ test_when_finished "git symbolic-ref -d --no-recurse refs/heads/regularref" &&
+ git update-ref --no-deref refs/heads/regularref $a &&
+ format_command $type "symref-update refs/heads/regularref" "$a" >stdin &&
+ git update-ref $type --stdin <stdin &&
+ echo $a >expect &&
+ git symbolic-ref --no-recurse refs/heads/regularref >actual &&
+ test_cmp expect actual &&
+ test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual &&
+ grep "$(git rev-parse $a) $(git rev-parse $a)" actual
+ '
+
done
test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index ff77dcca6b..5a812ca3c0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -163,6 +163,7 @@ test_expect_success 'hook gets all queued symref updates' '
git update-ref refs/heads/branch $POST_OID &&
git symbolic-ref refs/heads/symref refs/heads/main &&
git symbolic-ref refs/heads/symrefd refs/heads/main &&
+ git symbolic-ref refs/heads/symrefu refs/heads/main &&
test_hook reference-transaction <<-\EOF &&
echo "$*" >>actual
@@ -190,10 +191,12 @@ test_expect_success 'hook gets all queued symref updates' '
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+ ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
committed
ref:refs/heads/main $ZERO_OID refs/heads/symref
ref:refs/heads/main $ZERO_OID refs/heads/symrefd
$ZERO_OID ref:refs/heads/main refs/heads/symrefc
+ ref:refs/heads/main ref:refs/heads/branch refs/heads/symrefu
EOF
git update-ref --no-deref --stdin <<-EOF &&
@@ -201,6 +204,7 @@ test_expect_success 'hook gets all queued symref updates' '
symref-verify refs/heads/symref refs/heads/main
symref-delete refs/heads/symrefd refs/heads/main
symref-create refs/heads/symrefc refs/heads/main
+ symref-update refs/heads/symrefu refs/heads/branch ref refs/heads/main
prepare
commit
EOF
--
2.43.GIT
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/7] refs: specify error for regular refs with `old_target`
2024-06-07 13:32 ` [PATCH v5 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
@ 2024-06-10 6:54 ` Patrick Steinhardt
2024-06-10 8:26 ` Karthik Nayak
0 siblings, 1 reply; 25+ messages in thread
From: Patrick Steinhardt @ 2024-06-10 6:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]
On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 194e74eb4d..fc57c9d220 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>
> /*
> * 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.
> + * fail with an error.
> */
> if (update->old_target) {
> - if (ref_update_check_old_target(referent.buf, update, err)) {
> - ret = TRANSACTION_GENERIC_ERROR;
> - goto out;
> - }
> + strbuf_addf(err, _("cannot lock ref '%s': "
> + "expected symref with target '%s': "
> + "but is a regular ref"),
> + ref_update_original_update_refname(update),
> + update->old_target);
> + ret = TRANSACTION_GENERIC_ERROR;
> + goto out;
Shouldn't the second colon be a comma? "expected symref, but is a
regular ref" reads way more natural to me than "expected symref: but is
a regular ref".
Other than that this series looks good to me now, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/7] refs: specify error for regular refs with `old_target`
2024-06-10 6:54 ` Patrick Steinhardt
@ 2024-06-10 8:26 ` Karthik Nayak
0 siblings, 0 replies; 25+ messages in thread
From: Karthik Nayak @ 2024-06-10 8:26 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Jun 07, 2024 at 03:32:59PM +0200, Karthik Nayak wrote:
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 194e74eb4d..fc57c9d220 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2491,14 +2491,16 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>
>> /*
>> * 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.
>> + * fail with an error.
>> */
>> if (update->old_target) {
>> - if (ref_update_check_old_target(referent.buf, update, err)) {
>> - ret = TRANSACTION_GENERIC_ERROR;
>> - goto out;
>> - }
>> + strbuf_addf(err, _("cannot lock ref '%s': "
>> + "expected symref with target '%s': "
>> + "but is a regular ref"),
>> + ref_update_original_update_refname(update),
>> + update->old_target);
>> + ret = TRANSACTION_GENERIC_ERROR;
>> + goto out;
>
> Shouldn't the second colon be a comma? "expected symref, but is a
> regular ref" reads way more natural to me than "expected symref: but is
> a regular ref".
>
It makes sense the way you put it, but we also print the 'target', so it
is something like "cannot lock ref 'refs/heads/branch1': expected symref with
target 'refs/heads/master': but is a regular ref". I felt here the colon
divides the error into three segments
1. States that we couldn't lock the file
2. States that we were expecting a symref with a particular target
3. States that the ref in question is actually a regular ref
Having said that, I'm not too bullish on this and happy to change it :)
> Other than that this series looks good to me now, thanks!
>
> Patrick
>
Thanks for all the reviews. Since this is the only change, I'm inclined
to leave it as is.
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2024-06-10 8:26 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <https://lore.kernel.org/r/20240530120940.456817-1-knayak@gitlab.com>
2024-06-05 10:29 ` [PATCH v4 0/7] update-ref: add symref support for --stdin Karthik Nayak
2024-06-06 8:22 ` Karthik Nayak
2024-06-06 11:03 ` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 " Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
2024-06-07 13:32 ` [PATCH v5 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
2024-06-10 6:54 ` Patrick Steinhardt
2024-06-10 8:26 ` Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-06-07 13:33 ` [PATCH v5 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 1/7] refs: create and use `ref_update_expects_existing_old_ref()` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 2/7] refs: specify error for regular refs with `old_target` Karthik Nayak
2024-06-06 11:02 ` Patrick Steinhardt
2024-06-06 14:20 ` Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 4/7] update-ref: add support for 'symref-delete' command Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 5/7] update-ref: add support for 'symref-create' command Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 6/7] reftable: pick either 'oid' or 'target' for new updates Karthik Nayak
2024-06-05 10:29 ` [PATCH v4 7/7] update-ref: add support for 'symref-update' command Karthik Nayak
2024-04-23 21:28 [PATCH v3 0/8] refs: add symref support to 'git-update-ref' Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 0/7] add symref-* commands to 'git-update-ref --stdin' Karthik Nayak
2024-04-26 15:24 ` [PATCH v4 3/7] update-ref: add support for 'symref-verify' command Karthik Nayak
2024-04-26 22:51 ` Junio C Hamano
2024-04-28 22:28 ` Karthik Nayak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).