Re: [PATCH v2 6/7] update-ref: add support for symref-update

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4333 bytes --]

On Fri, Apr 12, 2024 at 11:59:07AM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Add 'symref-update' to allow updates of symbolic refs in a transaction
> via the 'git-update-ref' command. The 'symref-update' command takes in a
> <new-ref>, which the <ref> will be updated to. If the <ref> doesn't
> exist it will be created.
> 
> It also optionally takes either an <old-ref> or <old-oid>. If the
> <old-ref> is provided, it checks to see if the <ref> ponints to the
> <old-ref> 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.
> 
> This command will also support deref mode, to ensure that we can update
> dereferenced regular refs to symrefs.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-update-ref.txt |   6 ++
>  builtin/update-ref.c             |  49 +++++++++++
>  refs.c                           |  24 ++----
>  refs/files-backend.c             |  15 ++--
>  refs/reftable-backend.c          |   7 +-
>  t/t1400-update-ref.sh            | 143 +++++++++++++++++++++++++++++++
>  6 files changed, 220 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
> index a5b1f42728..9710c9bc78 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-ref> [SP (<old-ref> | <old-oid>)] LF
>  	symref-create SP <ref> SP <new-ref> LF
>  	symref-delete SP <ref> [SP <old-ref>] LF
>  	symref-verify SP <ref> [SP <old-ref>] 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-ref> [NUL (<old-ref> | <old-oid>)] NUL
>  	symref-create SP <ref> NUL <new-ref> NUL
>  	symref-delete SP <ref> [NUL <old-ref>] NUL
>  	symref-verify SP <ref> [NUL <old-ref>] NUL
> @@ -123,6 +125,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-update::
> +	Set <ref> to <new-ref> after verifying <old-ref> or <old-oid>,
> +	if given. Can be used to delete or create symrefs too.
> +
>  symref-create::
>  	Create symbolic ref <ref> with <new-ref> after verifying
>  	it does not exist.  Can only be used in `no-deref` mode.
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 24556a28a8..809c1c7a76 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -238,6 +238,54 @@ 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)
> +{
> +	struct strbuf err = STRBUF_INIT;
> +	char *refname, *new_ref, *old_ref;
> +	struct object_id old_oid;
> +	int have_old = 0;
> +
> +	refname = parse_refname(&next);
> +	if (!refname)
> +		die("symref-update: missing <ref>");
> +
> +	new_ref = parse_next_refname(&next);
> +	if (!new_ref)
> +		die("symref-update %s: missing <new-ref>", refname);
> +	if (read_ref(new_ref, NULL))
> +		die("symref-update %s: invalid <new-ref>", refname);
> +
> +	old_ref = parse_next_refname(&next);
> +	/*
> +	 * Since the user can also send in an old-oid, we try to parse
> +	 * it as such too.
> +	 */
> +	if (old_ref && read_ref(old_ref, NULL)) {
> +		if (!repo_get_oid(the_repository, old_ref, &old_oid)) {
> +			old_ref = NULL;
> +			have_old = 1;
> +		} else
> +			die("symref-update %s: invalid <old-ref> or <old-oid>", refname);
> +	}

So we first try to parse it as a ref, and then as an object ID? Wouldn't
it preferable to try it the other way round and first check whether it
is a valid object ID? That would likely be cheaper, even though it may
be premature optimization.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]