Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 30, 2023, 07:42 UTC
Message-ID: <ZWg8-HW6yCa6-tnS@tanuki>
In-Reply-To: <ZWew3CP4QJ4XDnHj@nand.local>

On Wed, Nov 29, 2023 at 04:45:00PM -0500, Taylor Blau wrote:

Show 57 quoted lines

> On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:
> > We read both the HEAD and ORIG_HEAD references directly from the
> > filesystem in order to figure out whether we're currently splitting a
> > commit. If both of the following are true:
> >
> >   - HEAD points to the same object as "rebase-merge/amend".
> >
> >   - ORIG_HEAD points to the same object as "rebase-merge/orig-head".
> >
> > Then we are currently splitting commits.
> >
> > The current code only works by chance because we only have a single
> > reference backend implementation. Refactor it to instead read both refs
> > via the refdb layer so that we'll also be compatible with alternate
> > reference backends.
> >
> > Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> > because we didn't resolve symrefs before either, and in practice none of
> > the refs in "rebase-merge/" would be symbolic. We thus don't want to
> > resolve symrefs with the new code either to retain the old behaviour.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  wt-status.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/wt-status.c b/wt-status.c
> > index 9f45bf6949..fe9e590b80 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
> >  static int split_commit_in_progress(struct wt_status *s)
> >  {
> >  	int split_in_progress = 0;
> > -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +	struct object_id head_oid, orig_head_oid;
> > +	char *rebase_amend, *rebase_orig_head;
> >
> >  	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> >  	    !s->branch || strcmp(s->branch, "HEAD"))
> >  		return 0;
> >
> > -	head = read_line_from_git_path("HEAD");
> > -	orig_head = read_line_from_git_path("ORIG_HEAD");
> > +	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> > +	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
> 
> Switching to read_ref_full() here is going to have some slightly
> different behavior than just reading out the contents of
> "$GIT_DIR/HEAD", but I think that it should be OK.
> 
> Before we would not have complained, if, for example, the contents of
> "$GIT_DIR/HEAD" were malformed, but now we will. I think that's OK,
> especially given that if that file is bogus, we'll have other problems
> before we get here ;-).
> 
> Are there any other gotchas that we should be thinking about?

Not that I can think of. As you say, a repository with malformed HEAD will run into other problems anyway. And `read_ref_full()` would return errors if these refs were malformed, which would cause us to exit early from anyway. So unless "rebase-merge/amend" and "rebase-merge/orig-head" contained the same kind of garbage we'd retain the same behaviour as before, and that shouldn't really be happening.

One interesting bit is that we don't set `RESOLVE_REF_READING`, so `read_ref_full()` may return successfully even if the ref doesn't exist. But in practice this is fine given that the resulting oid would be cleared in that case.

Patrick

Show 23 quoted lines

> > +		return 0;
> > +
> >  	rebase_amend = read_line_from_git_path("rebase-merge/amend");
> >  	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> >
> > -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> > +	if (!rebase_amend || !rebase_orig_head)
> >  		; /* fall through, no split in progress */
> >  	else if (!strcmp(rebase_amend, rebase_orig_head))
> > -		split_in_progress = !!strcmp(head, rebase_amend);
> > -	else if (strcmp(orig_head, rebase_orig_head))
> > +		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> > +	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
> 
> I did a double take at these strcmp(oid_to_hex(...)) calls, but I think
> that they are the best that we can do given that we're still reading the
> contents of "rebase-merge/amend" and "rebase-merge/orig-head" directly.
> 
> I suppose we could go the other way and turn their contents into
> object_ids and then use oidcmp(), but it doesn't seem worth it IMHO.
> 
> Thanks,
> Taylor