Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 15, 2024, 11:36 UTC
Message-ID: <ZaUYyEAdr4oTImL-@tanuki>
In-Reply-To: <xmqqzfxa9usx.fsf@gitster.g>

On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:

Show 34 quoted lines

> Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> 
> >> but we may want to tighten the original's use of repo_get
> >> > -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> >> > -		die("--merge without MERGE_HEAD?");
> >> > -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> >> 
> >> ... so that we won't be confused in a repository that has a tag
> >> whose name happens to be MERGE_HEAD.  We probably should be using
> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
> >
> > Here I am really at the limit of my understanding of the core functions.
> > Is this roughly what you had in mind? From my testing it seems to do the
> > job, but I don't understand the details "refs_resolve_ref_unsafe"...
> 
> Perhaps there are others who are more familiar with the ref API than
> I am, but I was just looking at refs.h today and wonder if something
> along the lines of this ...
> 
>     if (read_ref_full("MERGE_HEAD",
>     		      RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> 		      &oid, NULL))
> 	die("no MERGE_HEAD");
>     if (is_null_oid(&oid))
> 	die("MERGE_HEAD is a symbolic ref???");
> 
> ... would be simpler.
> 
> With the above, we are discarding the refname read_ref_full()
> obtains from its refs_resolve_ref_unsafe(), but I think that is
> totally fine.  We expect it to be "MERGE_HEAD" in the good case.
> The returned value can be different from "MERGE_HEAD" if it is
> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
> trusted, we should catch that case with the NULL-ness check on oid.

Yeah, this should be fine.

Even though I have stared at our refs API for extended periods of time during the last months I still have to look up this part of the API almost every time. I find it to be hard to use because you not only have to pay attention to the return value, but also to the flags in some edge cases. I wouldn't be surprised if there were many callsites that get this wrong.

> Which would mean that we do not need a separate "other_head"
> variable, and instead can use "MERGE_HEAD".

There is no need for this, is there? We have already resolved the ref to an object ID, so why not use that via `add_pending_oid()`?

Patrick