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

[Patrick Steinhardt <ps@pks.im>]

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

On Sun, Apr 21, 2024 at 03:00:11PM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> [snip]
> >> +	/*
> >> +	 * 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
> 
> I think the issue is `repo_get_oid` would also parse a refname to an
> OID. Whereas we want to first check and keep refnames and only if it
> isn't a refname, we'd want to parse it as an OID.

Okay. The question is whether this matches precedence rules that we have
in other places. Namely, whether a reference name that looks like an
object ID overrides an object with the same name. Testing it:

```
$ rm -rf repo/
$ git init --ref-format=files repo
Initialized empty Git repository in /tmp/repo/.git/
$ cd repo/
$ git commit --allow-empty --message first
[main (root-commit) 09293d8] first
$ git commit --allow-empty --message second
[main 1588e76] second

$ git update-ref $(git rev-parse HEAD~) HEAD
$ cat .git/09293d82c434cdc1f7f286cf7b90cf35a6e57c43
1588e76ce7ef1ab25ee6f846a7b5d7032f83a69e

$ git rev-parse 09293d82c434cdc1f7f286cf7b90cf35a6e57c43
warning: refname '09293d82c434cdc1f7f286cf7b90cf35a6e57c43' is ambiguous.
Git normally never creates a ref that ends with 40 hex characters
because it will be ignored when you just specify 40-hex. These refs
may be created by mistake. For example,

  git switch -c $br $(git rev-parse ...)

where "$br" is somehow empty and a 40-hex ref is created. Please
examine these refs and maybe delete them. Turn this message off by
running "git config advice.objectNameWarning false"
09293d82c434cdc1f7f286cf7b90cf35a6e57c43
```

So the object ID has precedence over the reference with the same name.
Unless I'm mistaken, your proposed order would reverse that, wouldn't
it?

> Also, why do you say it is cheaper?

Checking whether a string can be parsed as an object ID should be faster
than having to ask the ref backend whether it knows any reference with
that name. So it should be fast for `repo_get_oid()` to bail out in case
the provided string doesn't look like an object ID.

Patrick

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