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

[Karthik Nayak <karthik.188@gmail.com>]

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

Patrick Steinhardt <ps@pks.im> writes:

> 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?
>

I wasn't talking about a reference being mistaken for a object ID,
rather I was talking about how a reference will be `rev-parse`'d into an
object ID.

So instead if we did something like:

```
if (old_target) {
	if (!repo_get_oid(the_repository, old_target, &old_oid)) {
		old_target = NULL;
		have_old = 1;
	} else if (read_ref(old_target, NULL)) {
	} else {
		die("symref-update %s: invalid <old-target> or <old-oid>", refname);
	}
}
```

The problem is that now:

$ git init repo && cd repo
$ git commit --allow-empty -m"c1"
[master (root-commit) af416de] c1
$ git commit --allow-empty -m"c2"
[master 52e95b2] c2
$ git symbolic-ref refs/heads/symref refs/heads/master
$ git branch b1 master~1
$ git branch b2 master
$ cat .git/refs/heads/symref
ref: refs/heads/master
$ git update-ref --no-deref --stdin
symref-update refs/heads/symref refs/heads/b1 refs/heads/b2
$ cat .git/refs/heads/symref
ref: refs/heads/b1

This shouldn't have worked because symref was pointing to master, but
this did work, because `refs/heads/b2` was rev-parse'd to '52e95b2'
which is also what master is at.

The issue is that we call 'repo_get_oid', which takes in a commitish and
this could even be a reference. But ideally in 'symref' commands we want
to first treat refnames as refnames and not have them parsed as an OID.

Since, I'm not moving to 'refs:<target>' with the existing commands.
This is no longer an issue.

>> 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

Yes. But for a valid refname, the `repo_get_oid()` is querying the
reference backend similar to `read_ref()`. Also `read_ref` also does
initial checks with `check_refname_format` which I think doesn't check
the ref backend.

Thanks

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