diff mbox series

[v2,3/7] update-ref: add support for symref-delete

Message ID 20240412095908.1134387-4-knayak@gitlab.com (mailing list archive)
State New, archived
Headers show
Series update-ref: add symref oriented commands | expand

Commit Message

karthik nayak April 12, 2024, 9:59 a.m. UTC 
From: Karthik Nayak <karthik.188@gmail.com>

Similar to the previous commit, add 'symref-delete' to allow deletions
of symbolic refs in a transaction via the 'git-update-ref' command. The
'symref-delete' command can when given with an <old-ref>, deletes the
provided <ref> only when it points to <old-ref>.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-update-ref.txt |  6 +++++
 builtin/fetch.c                  |  2 +-
 builtin/receive-pack.c           |  3 ++-
 builtin/update-ref.c             | 34 +++++++++++++++++++++++++++-
 refs.c                           | 16 +++++++++----
 refs.h                           |  4 +++-
 refs/files-backend.c             |  2 +-
 refs/reftable-backend.c          |  2 +-
 t/t1400-update-ref.sh            | 39 ++++++++++++++++++++++++++++++++
 9 files changed, 97 insertions(+), 11 deletions(-)

Comments

Christian Couder April 18, 2024, 2:52 p.m. UTC | #1 
On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Similar to the previous commit, add 'symref-delete' to allow deletions
> of symbolic refs in a transaction via the 'git-update-ref' command. The
> 'symref-delete' command can when given with an <old-ref>, deletes the
> provided <ref> only when it points to <old-ref>.

I have a similar question as with the previous patch about what
happens if <old-ref> looks like an oid and <ref> is a regular ref
pointing to it.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
> index 749aaa7892..ef22a1a2f4 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-ref>] LF
>         symref-verify SP <ref> [SP <old-ref>] 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-ref>] NUL
>         symref-verify SP <ref> [NUL <old-ref>] NUL
>         option SP <opt> NUL
>         start NUL

Also I wonder if there is a test where <old-ref> is an empty string, so where:

    symref-delete SP <ref> SP LF

or:

    symref-delete SP <ref> NUL NUL

are used?
Patrick Steinhardt April 19, 2024, 9:40 a.m. UTC | #2 
On Fri, Apr 12, 2024 at 11:59:04AM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
[snip]
> @@ -302,6 +302,37 @@ 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_ref;
> +
> +	if (!(update_flags & REF_NO_DEREF))
> +                die("symref-delete: cannot operate with deref mode");

Again, I'm a bit on the fence regarding this restriction. I feel like it
ought to be possible to delete both plain and symbolic refs in a single
git-update-ref(1) command.

> +	refname = parse_refname(&next);
> +	if (!refname)
> +		die("symref-delete: missing <ref>");
> +
> +        old_ref = parse_next_refname(&next);

This line is indented with spaces and not tabs.

[snip]
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1715,6 +1715,45 @@ test_expect_success "stdin ${type} symref-verify fails for mistaken null value"
>  	test_cmp expect actual
>  '
>  
> +test_expect_success "stdin ${type} symref-delete fails without --no-deref" '
> +	git symbolic-ref refs/heads/symref $a &&
> +	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
> +	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} fails symref-delete with no ref" '
> +	create_stdin_buf ${type} "symref-delete " &&
> +	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} fails symref-delete with too many arguments" '
> +	create_stdin_buf ${type} "symref-delete 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-delete refs/heads/symref: extra input:  $a" err
> +	fi
> +'
> +
> +test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" '
> +	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
> +	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
> +	grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err &&

You can use "${SQ}" to insert single quotes.

Patrick

> +	git symbolic-ref refs/heads/symref >expect &&
> +	echo $a >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success "stdin ${type} symref-delete ref works with right old value" '
> +	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
> +	git update-ref --stdin ${type} --no-deref <stdin &&
> +	test_must_fail git rev-parse --verify -q $b
> +'
> +
>  done
>  
>  test_done
> -- 
> 2.43.GIT
>
karthik nayak April 21, 2024, 10:43 a.m. UTC | #3 
Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Similar to the previous commit, add 'symref-delete' to allow deletions
>> of symbolic refs in a transaction via the 'git-update-ref' command. The
>> 'symref-delete' command can when given with an <old-ref>, deletes the
>> provided <ref> only when it points to <old-ref>.
>
> I have a similar question as with the previous patch about what
> happens if <old-ref> looks like an oid and <ref> is a regular ref
> pointing to it.
>

We parse refs passed as <old-ref> and this would fail.

>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
>> index 749aaa7892..ef22a1a2f4 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-ref>] LF
>>         symref-verify SP <ref> [SP <old-ref>] 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-ref>] NUL
>>         symref-verify SP <ref> [NUL <old-ref>] NUL
>>         option SP <opt> NUL
>>         start NUL
>
> Also I wonder if there is a test where <old-ref> is an empty string, so where:
>
>     symref-delete SP <ref> SP LF
>
> or:
>
>     symref-delete SP <ref> NUL NUL
>
> are used?

I didn't add such tests, will add.
karthik nayak April 21, 2024, 10:45 a.m. UTC | #4 
Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Apr 12, 2024 at 11:59:04AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
> [snip]
>> @@ -302,6 +302,37 @@ 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_ref;
>> +
>> +	if (!(update_flags & REF_NO_DEREF))
>> +                die("symref-delete: cannot operate with deref mode");
>
> Again, I'm a bit on the fence regarding this restriction. I feel like it
> ought to be possible to delete both plain and symbolic refs in a single
> git-update-ref(1) command.
>

Yup this is still possible since we have the 'no-deref' option.

>> +	refname = parse_refname(&next);
>> +	if (!refname)
>> +		die("symref-delete: missing <ref>");
>> +
>> +        old_ref = parse_next_refname(&next);
>
> This line is indented with spaces and not tabs.
>

There was a bunch of this, I'll have them all fixed.

> [snip]
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -1715,6 +1715,45 @@ test_expect_success "stdin ${type} symref-verify fails for mistaken null value"
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success "stdin ${type} symref-delete fails without --no-deref" '
>> +	git symbolic-ref refs/heads/symref $a &&
>> +	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
>> +	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} fails symref-delete with no ref" '
>> +	create_stdin_buf ${type} "symref-delete " &&
>> +	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} fails symref-delete with too many arguments" '
>> +	create_stdin_buf ${type} "symref-delete 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-delete refs/heads/symref: extra input:  $a" err
>> +	fi
>> +'
>> +
>> +test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" '
>> +	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
>> +	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
>> +	grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err &&
>
> You can use "${SQ}" to insert single quotes.
>
> Patrick
>

Neat, this is much better, thanks!
diff mbox series

Patch

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 749aaa7892..ef22a1a2f4 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-ref>] LF
 	symref-verify SP <ref> [SP <old-ref>] 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-ref>] NUL
 	symref-verify SP <ref> [NUL <old-ref>] NUL
 	option SP <opt> NUL
 	start NUL
@@ -119,6 +121,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-delete::
+	Delete <ref> after verifying it exists with <old-ref>, if
+	given.
+
 symref-verify::
 	Verify symbolic <ref> against <old-ref> but do not change it.
 	If <old-ref> is missing, the ref must not exist.  Can only be
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 66840b7c5b..d02592efca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1383,7 +1383,7 @@  static int prune_refs(struct display_state *display_state,
 		if (transaction) {
 			for (ref = stale_refs; ref; ref = ref->next) {
 				result = ref_transaction_delete(transaction, ref->name, NULL, 0,
-								"fetch: prune", &err);
+								NULL, "fetch: prune", &err);
 				if (result)
 					goto cleanup;
 			}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ebea1747cb..6b728baaac 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)) {
+					   0, NULL,
+					   "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 4ae6bdcb12..3be9ae0d00 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -294,7 +294,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))
+				   update_flags, NULL, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = default_flags;
@@ -302,6 +302,37 @@  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_ref;
+
+	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_ref = parse_next_refname(&next);
+	if (old_ref && read_ref(old_ref, NULL))
+		die("symref-delete %s: invalid <old-ref>", refname);
+
+	if (*next != line_termination)
+		die("symref-delete %s: extra input: %s", refname, next);
+
+	if (ref_transaction_delete(transaction, refname, NULL,
+				   update_flags | REF_SYMREF_UPDATE,
+				   old_ref, msg, &err))
+		die("%s", err.buf);
+
+	update_flags = default_flags;
+	free(refname);
+	free(old_ref);
+	strbuf_release(&err);
+}
+
 static void parse_cmd_verify(struct ref_transaction *transaction,
 			     const char *next, const char *end)
 {
@@ -445,6 +476,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 124b294c9f..6d98d9652d 100644
--- a/refs.c
+++ b/refs.c
@@ -981,7 +981,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) ||
+				   flags, NULL, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
@@ -1220,6 +1220,7 @@  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_ref);
+		free((void *)transaction->updates[i]->new_ref);
 		free(transaction->updates[i]);
 	}
 	free(transaction->updates);
@@ -1252,6 +1253,8 @@  struct ref_update *ref_transaction_add_update(
 	if (update->flags & REF_SYMREF_UPDATE) {
 		if (old_ref)
 			update->old_ref = xstrdup(old_ref);
+		if (new_ref)
+			update->new_ref = xstrdup(new_ref);
 	} else {
 		if (flags & REF_HAVE_NEW)
 			oidcpy(&update->new_oid, new_oid);
@@ -1317,14 +1320,17 @@  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,
+			   unsigned int flags,
+			   const char *old_ref,
+			   const char *msg,
 			   struct strbuf *err)
 {
-	if (old_oid && is_null_oid(old_oid))
+	if (!(flags & REF_SYMREF_UPDATE) && old_oid &&
+	    is_null_oid(old_oid))
 		BUG("delete called with old_oid set to zeros");
 	return ref_transaction_update(transaction, refname,
 				      null_oid(), old_oid,
-				      NULL, NULL, flags,
+				      NULL, old_ref, flags,
 				      msg, err);
 }
 
@@ -2748,7 +2754,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, flags, 0, msg, &err);
 		if (ret) {
 			warning(_("could not delete reference %s: %s"),
 				item->string, err.buf);
diff --git a/refs.h b/refs.h
index a988e672ff..60e6a21a31 100644
--- a/refs.h
+++ b/refs.h
@@ -758,7 +758,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,
+			   unsigned int flags,
+			   const char *old_ref,
+			   const char *msg,
 			   struct strbuf *err);
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8421530bde..f74ea308b5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2501,7 +2501,7 @@  static int lock_ref_for_update(struct files_ref_store *refs,
 
 	files_assert_main_repository(refs, "lock_ref_for_update");
 
-	if ((update->flags & REF_HAVE_NEW) && is_null_oid(&update->new_oid))
+	if ((update->flags & REF_HAVE_NEW) && null_new_value(update))
 		update->flags |= REF_DELETING;
 
 	if (head_ref) {
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 7a03922c7b..935bf407df 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1122,7 +1122,7 @@  static int write_transaction_table(struct reftable_writer *writer, void *cb_data
 		if (u->flags & REF_LOG_ONLY)
 			continue;
 
-		if (u->flags & REF_HAVE_NEW && is_null_oid(&u->new_oid)) {
+		if (u->flags & REF_HAVE_NEW && null_new_value(u)) {
 			struct reftable_ref_record ref = {
 				.refname = (char *)u->refname,
 				.update_index = ts,
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d8ffda4096..cf01c5d867 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1715,6 +1715,45 @@  test_expect_success "stdin ${type} symref-verify fails for mistaken null value"
 	test_cmp expect actual
 '
 
+test_expect_success "stdin ${type} symref-delete fails without --no-deref" '
+	git symbolic-ref refs/heads/symref $a &&
+	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
+	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} fails symref-delete with no ref" '
+	create_stdin_buf ${type} "symref-delete " &&
+	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} fails symref-delete with too many arguments" '
+	create_stdin_buf ${type} "symref-delete 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-delete refs/heads/symref: extra input:  $a" err
+	fi
+'
+
+test_expect_success "stdin ${type} symref-delete ref fails with wrong old value" '
+	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$m" &&
+	test_must_fail git update-ref --stdin ${type} --no-deref <stdin 2>err &&
+	grep "fatal: cannot lock ref '"'"'refs/heads/symref'"'"'" err &&
+	git symbolic-ref refs/heads/symref >expect &&
+	echo $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success "stdin ${type} symref-delete ref works with right old value" '
+	create_stdin_buf ${type} "symref-delete refs/heads/symref" "$a" &&
+	git update-ref --stdin ${type} --no-deref <stdin &&
+	test_must_fail git rev-parse --verify -q $b
+'
+
 done
 
 test_done