On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:
> Michael Lohmann <mi.al.lohmann@xxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
