patch, 4 partsrefs: improve handling of special refs

Hi,

there are a bunch of "special" refs in Git that sometimes behave like a normal reference and sometimes they don't. These references are written to directly via the filesystem without going through the reference backend, but the expectation is that those references can then be read by things like git-rev-parse(1).

We do not currently have a single source of truth for what those special refs are, and we also don't have clear rules for how they should be written to. This works in the context of the files backend, because it is able to read back such manually-written loose references just fine. But once the reftable backend lands this will stop working.

This patch series tries to improve this state by doing two things:

    1. We explicitly mark these references as special by introducing a
       new `is_special_ref()` function. This serves as documentation,
       but will also cause us to explicitly read all of these special
       refs via loose files regardless of the actual backend.

    2. We document a new rule around writing refs. Namely, normal
       references are _always_ written via the reference backend,
       whereas special references are _always_ written directly via the
       filesystem. This rule is not enforced anywhere, but at least it's
       now made more explicit.

The last patch fixes one of the instances where we treat a reference inconsistently by converting it to a normal reference. We can eventually migrate more of the special refs to become normal refs as we deem fit, but I consider this to be out of scope for this patch series.

These patches improve compatibility with the new reftable backend.

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 64 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         |  9 ++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 17 +++++-----
 6 files changed, 86 insertions(+), 39 deletions(-)

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

On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:

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?

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

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

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

On Thu, Nov 30, 2023 at 08:42:48AM +0100, Patrick Steinhardt wrote:

Thanks for thinking through these. I agree with your reasoning and think that this is fine as-is.

(Off-topic, but can you please trim your replies to only include the quoted parts/context that you're replying to?)

Thanks, Taylor

Some refs in Git are more special than others due to reasons explained in the next commit. These refs are read via `refs_read_special_head()`, but this function doesn't behave the same as when we try to read a normal ref. Most importantly, we do not propagate `failure_errno` in the case where the reference does not exist, which is behaviour that we rely on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              | 6 +++++-
 t/t1403-show-ref.sh | 9 +++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:

Do we need to set errno to 0 here? Looking at the implementation of strbuf_read_file(), it looks like we return early in two cases. Either open() fails, in which errno is set for us, or strbuf_read() fails, in which case we set errno to whatever it was right after the failed read (preventing the subsequent close() call from tainting the value of errno).

So I think in either case, we have the right value in errno, and don't need to worry about setting it to "0" ahead of time.

I don't think that it matters here, but do we need to worry about cleaning up .git/FETCH_HEAD for future tests?

Thanks, Taylor

On Wed, Nov 29, 2023 at 04:51:13PM -0500, Taylor Blau wrote:

True. I'll drop this when rerolling.

There's so many tests where I wish that we did a better job of cleanup, so I agree that it is sensible to clean up after ourselves. Will drop when rerolling.

Patrick

We have some references that are more special than others. The reason for them being special is that they either do not follow the usual format of references, or that they are written to the filesystem directly by the respective owning subsystem and thus circumvent the reference backend.

This works perfectly fine right now because the reffiles backend will know how to read those refs just fine. But with the prospect of gaining a new reference backend implementation we need to be a lot more careful here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit. But the current list of special refs is missing a lot of refs that really should be treated specially. Right now, we only treat `FETCH_HEAD` and `MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current instances of special refs to fix the reading path.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:

Is there a reason that we don't want to declare this statically? If we did, I think we could drop one const, since the strings would instead reside in the .rodata section.

Not that it matters for this case, but it may be worth declaring i to be an unsigned type, since it's used as an index into an array. size_t seems like an appropriate choice there.

Do we care about case sensitivity here? Definitely not on case-sensitive filesystems, but I'm not sure about case-insensitive ones. For instance, on macOS, I can do:

    $ git rev-parse hEAd

and get the same value as "git rev-parse HEAD" (on my Linux workstation, this fails as expected).

I doubt that there are many users in the wild asking to resolve reBASe-APPLY/xyz, but I think that after this patch that would no longer work as-is, so we may want to replace this with istarts_with() instead.

Thanks, Taylor

On Wed, Nov 29, 2023 at 04:59:35PM -0500, Taylor Blau wrote:

Not really, no.

Hm. We do use `int` almost everywhere when iterating through an array via `ARRAY_SIZE`, but ultimately I don't mind whether it's `int`, `unsigned` or `size_t`.

In practice I'd argue that nobody is ever going to ask for something in `rebase-apply/` outside of Git internals or scripts, and I'd expect these to always use proper casing. So I rather lean towards a "no, we don't care about case sensitivity".

Patrick

Hi Patrick

Thanks for working on this. I've left a couple of thought below.

On 29/11/2023 08:14, Patrick Steinhardt wrote:

I'd really like to get away from treating these files as refs. I think their use as refs is purely historic and predates the reflog and possibly ORIG_HEAD. These days I'm not sure there is a good reason to be running

     git rev-parse rebase-merge/orig-head

One reason for not wanting to treat them as refs is that we do not handle multi-level refs that do not begin with "refs/" consistently.

     git update-ref foo/bar HEAD

succeeds and creates .git/foo/bar but

     git update-ref -d foo/bar

fails with

     error: refusing to update ref with bad name 'foo/bar'

To me it would make sense to refuse to create 'foo/bar' but allow an existing ref named 'foo/bar' to be deleted but the current behavior is the opposite of that.

I'd be quite happy to see us refuse to treat anything that fails

     if (starts_with(refname, "refs/") || refname_is_safe(refname))

as a ref but I don't know how much pain that would cause.

Is there any reason to treat this specially in the long term? It points to a tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is effectively a "normal" ref.

Should we be treating this as a ref? I thought it was written as an implementation detail of the autostash implementation rather than to provide a ref for users and scripts.

Best Wishes

Phillip

On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:

Well, we already do use these internally as references, but I don't disagree with you. I think the current state is extremely confusing, which is why my first approach was to simply document what falls into the category of these "special" references.

In my mind, this patch series here is a first step towards addressing the problem more generally. For now it is more or less only documenting _what_ is a special ref and why they are special, while also ensuring that these refs are compatible with the reftable backend. But once this lands, I'd certainly want to see us continue to iterate on this.

Most importantly, I'd love to see us address two issues:

  - Start to refuse writing these special refs via the refdb so that
    the rules I've now layed out are also enforced. This would also
    address your point about things being inconsistent.

  - Gradually reduce the list of special refs so that they are reduced
    to a bare minimum and so that most refs are simply that, a normal
    ref.

No, I'd love to see this and others converted to become a normal ref eventually. The goal of this patch series was mostly to document what we already have, and address those cases which are inconsistent with the new rules. But I'd be happy to convert more of these special refs to become normal refs after it lands.

Yes, we have to in the context of the reftable backend. There's a bunch of tests that exercise our ability to parse this as a ref, and they would otherwise fail with the reftable backend.

Patrick

Hi Patrick

On 01/12/2023 06:43, Patrick Steinhardt wrote:

I should have been clearer that I was talking about the refs starting "rebase-*" rather than FETCH_HEAD and MERGE_HEAD. As a user find it convenient to be able to run "git fetch ... && git log -p FETCH_HEAD" even if the implementation is a bit ugly. As far as I can see we do not use "rebase-(apply|merge)/(orig-head|amend|autostash)" as a ref in our code or tests.

That's certainly a good place to start

That sounds like a good plan

That's great

Ah, looking at the the man page for "git merge" it seems we do actually document the existence of MERGE_AUTOSTASH so it is not just an implementation detail after all.

Best Wishes

Phillip

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem and via the refdb, which violates the newly established rules for how special refs must be treated. This works alright in practice with the reffiles reference backend, but will cause bugs once we gain additional backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  2 +-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 8 insertions(+), 29 deletions(-)

On Wed, Nov 29, 2023 at 09:14:24AM +0100, Patrick Steinhardt wrote:

Very nice :-).

Thanks, Taylor

On Wed, Nov 29, 2023 at 09:14:07AM +0100, Patrick Steinhardt wrote:

These all look pretty good to me. I had a few minor questions on the first three patches, but I don't think they necessarily require a reroll.

Thanks, Taylor

On Wed, Nov 29, 2023 at 05:14:39PM -0500, Taylor Blau wrote:

I agree that none of the comments really require a reroll, but I'll address them if there will be another iteration of this patch series.

Thanks for your review! 

Patrick

On Thu, Nov 30, 2023 at 08:46:54AM +0100, Patrick Steinhardt wrote:

No problem on either. I doubt that there will be another iteration of this series since it is already good, so no need to worry too much about these changes.

Thanks, Taylor

Hi,

this is the second version of my patch series that aims to improve our handling of special refs. This patch series is in preparation for the upcoming reftable backend.

Changes compared to v1:

  - Patch 1: Stop needlessly resetting `errno` before calling
    `strbuf_read_file()`. Also, clean up state we create in our test
    repos properly.

  - Patch 3: The discussion with Phillip made me reevaluate my claim
    that "rebase-apply/" and "rebase-merge/" contain special refs.
    Indeed they don't, all files in there seem to be exclusively
    read and written via the filesystem without ever going via the
    refdb. I've thus dropped them from the set of special refs.

  - Patch 4: The array of static refs is now static.

Thanks for your reviews!

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++-------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 59 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         | 10 +++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 17 ++++++-----
 6 files changed, 82 insertions(+), 39 deletions(-)

Range-diff against v1:
1:  35b74eb972 = 1:  1db3eb3945 wt-status: read HEAD and ORIG_HEAD via the refdb
2:  691552a17e ! 2:  24032a62e5 refs: propagate errno when reading special refs fails
    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
      	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
      
     -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
    -+	errno = 0;
    -+
     +	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
     +		*failure_errno = errno;
      		goto done;
    @@ t/t1403-show-ref.sh: test_expect_success '--exists with directory fails with gen
     +'
     +
     +test_expect_success '--exists with existing special ref' '
    ++	test_when_finished "rm .git/FETCH_HEAD" &&
     +	git rev-parse HEAD >.git/FETCH_HEAD &&
     +	git show-ref --exists FETCH_HEAD
     +'
3:  0e38103114 ! 3:  3dd9089fd5 refs: complete list of special refs
    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
     +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
     +	 *   heads.
     +	 *
    -+	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
    -+	 *   rebases, where keeping it closely together feels sensible.
    -+	 *
     +	 * There are some exceptions that you might expect to see on this list
     +	 * but which are handled exclusively via the reference backend:
     +	 *
     +	 * - CHERRY_PICK_HEAD
    ++	 *
     +	 * - HEAD
    ++	 *
     +	 * - ORIG_HEAD
     +	 *
    ++	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
    ++	 *   rebases, including some reference-like files. These are
    ++	 *   exclusively read and written via the filesystem and never go
    ++	 *   through the refdb.
    ++	 *
     +	 * Writing or deleting references must consistently go either through
     +	 * the filesystem (special refs) or through the reference backend
     +	 * (normal ones).
     +	 */
    -+	const char * const special_refs[] = {
    ++	static const char * const special_refs[] = {
     +		"AUTO_MERGE",
     +		"BISECT_EXPECTED_REV",
     +		"FETCH_HEAD",
     +		"MERGE_AUTOSTASH",
     +		"MERGE_HEAD",
     +	};
    -+	int i;
    ++	size_t i;
     +
     +	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
     +		if (!strcmp(refname, special_refs[i]))
     +			return 1;
     +
    -+	/*
    -+	 * git-rebase(1) stores its state in `rebase-apply/` or
    -+	 * `rebase-merge/`, including various reference-like bits.
    -+	 */
    -+	if (starts_with(refname, "rebase-apply/") ||
    -+	    starts_with(refname, "rebase-merge/"))
    -+		return 1;
    -+
     +	return 0;
     +}
     +
4:  c7ab26fb7e ! 4:  4a4447a3f5 bisect: consistently write BISECT_EXPECTED_REV via the refdb
    @@ refs.c: static int is_special_ref(const char *refname)
      	 * but which are handled exclusively via the reference backend:
      	 *
     +	 * - BISECT_EXPECTED_REV
    ++	 *
      	 * - CHERRY_PICK_HEAD
    + 	 *
      	 * - HEAD
    - 	 * - ORIG_HEAD
     @@ refs.c: static int is_special_ref(const char *refname)
      	 */
    - 	const char * const special_refs[] = {
    + 	static const char * const special_refs[] = {
      		"AUTO_MERGE",
     -		"BISECT_EXPECTED_REV",
      		"FETCH_HEAD",

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996

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

Patrick Steinhardt <ps@pks.im> writes:

"via the refdb" -> "via the refs API" or something here and on the title, and possibly elsewhere in the proposed log messages and in-code comments in patches in this series, as I've never seen a word "refdb" used in the context of this project.

I agree it is bad manners to be intimate with the implementation details of the how files-backend stores HEAD and ORIG_HEAD.

Good to see a rewrite being careful like this.

This made me wonder if we have changed behaviour when on an unborn branch. In such a case, the original most likely would have read "ref: blah" in "head" and compared it with "rebase_amend", which would be a good way to ensure they would not match. I would not know offhand what the updated code would do, but head_oid would be uninitialized in such a case, so ...?

On 12/12/2023 20:24, Junio C Hamano wrote:

Hmm, I have never thought of the 'pseudo-refs' as being a part of the 'reference database' at all. ;)

We seem to have pseudo-refs, special pseudo-refs and (recently) ex-pseudo-refs!

This patch (well series) changes the 'status' of some, *but not all*, pseudo-refs; some graduate to full-blown refs stored as part of *a* reference database (ie reftable).

As far as I recall, this has not been discussed on the ML. Why are only some chosen to become 'full' refs and others not? This is not discussed in any of the commit messages.

The '.invalid' HEAD hack featured in a recent completion patch as well. [If this is because the JAVA implementation does it this way, I think it needs some thought before including it in the Git project].

Anyway, I haven't had the time to study the details here, so please ignore my uninformed ramblings!

ATB, Ramsay Jones

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

Me neither, but once you start thinking about getting rid of the need to use one-file-per-ref filesystem, being able to maintain all refs, including the pseudo refs, in one r/w store backend, becomes a very tempting goal. From that point of view, I do not have problem with the idea to move _all_ pseudorefs to reftable.

But I do have reservations on what Patrick, and the code he inherited from Han-Wen, calls "special refs" (which is not defined in the glossary at all), namely, refs.c:is_special_ref() and its callers. Neither am I very much sympathetic to the hardcoded list of "known" pseudorefs, refs.c:pseudorefs[]. I cannot quite see why we need anything more than

    any string that passes refs.c:is_pseudoref_syntax() is a
    pseudoref, is per worktree, and ref backends can store them like
    any other refs.  Many of them have specific meaning and uses
    (e.g. HEAD is "the current branch").

Enumerating existing pseudorefs in files backend may need to opendir(".git") + readdir() filtered with is_pseudoref_syntax(), and a corresponding implementation for reftable backend may be much simpler (because there won't be "other cruft" stored there, unlike files backend that needs to worry about files that are not refs, like ".git/config" file.

Yeah, that leaves bad taste in my mouth, too.

On Tue, Dec 12, 2023 at 04:36:24PM -0800, Junio C Hamano wrote:

Yes, we're in agreement.

I do not want to add "special refs" to the glossary because ultimately they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD. Once we're there we can of course discuss whether we want to explicitly point them out in the glossary and give them a special name.

I'm taking an iterative approach to things, which means that we're at times going to be in an in-between state. I want to avoid changing too many things at once and overwhelming potential reviewers. But I realize that I should've done a better job of explaining that this patch series is not the end goal, but rather a step towards that goal.

The patch series at hand merely records the status quo and rectifies any inconsistencies we have with accessing such "special" refs. The natural next step here would be to reduce the list of special refs (like e.g. we do in patch 4) so that in the end it will only contain those refs which really are special (FETCH_HEAD, MERGE_HEAD).

Please let me know in case you strongly disagree with my iterative approach, or whether the issue is rather that I didn't make myself sufficiently clear.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

OK, I somehow got a (wrong) impression that you are very close to the finish line. If the intention is to leave many others still in the "special" category (for only the reasons of inertia), with a vision that some selected few must remain "special" with their own good reasons, then I am perfectly fine.

Thanks.

On Wed, Dec 13, 2023 at 07:15:33AM -0800, Junio C Hamano wrote:

You mean with the reftable backend? I indeed am quite close, I've just finished the last prerequisite ("extensions.refFormat" and related tooling) today. I will send that patch series upstream for review once my patches that fix repo initialization with git-clone(1) land in the "next" branch. The current state at [1] passes CI, even though there will of course still be bugs which aren't covered by the test suite.

So once all prerequisites that are currently in flight plus the pending "extensions.refFormat" series have landed I will send the reftable backend implementation in for review. If things continue to go smoothly I expect that this may happen at the end of January/start of February.

Anyway. This patch series here is in fact already sufficient to make reftables work with those special refs. The only thing that we require in this context is that refs are either exclusively routed through the filesystem, or exclusively routed through the ref API. If that property holds then things work just fine.

But still, I do want to clean up the remaining special refs regardless of that, even though it is not a mandatory prerequisite. I find that the current state is just plain confusing with all these special cases, and I'd really love for it to be simplified. Also, I think there is benefit in having those refs in reftables because it does allow for proper atomic updates.

Okay.

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/58

Patrick Steinhardt <ps@pks.im> writes:

It would be a major achievement if we just stop bypassing refs API to read/write ORIG_HEAD and friends, even if we are still using only the files backend. And I meant that I got an impression that you are very close to that, regardless of the readiness of the reftable support.

On Tue, Dec 12, 2023 at 12:24:24PM -0800, Junio C Hamano wrote:

[snip]

The code flow goes as following:

  1. We call `read_ref_full()`, which itself calls
     `refs_resolve_ref_unsafe()`.

  2. It calls `refs_read_raw_ref()`, which succeeds and finds the
     symref.

  3. We notice that this is a symref and that `RESOLVE_REF_NO_RECURSE`
     is set. We thus clear the object ID and return the name of the
     symref target.

  4. Back in `read_ref_full()` we see that `refs_resolve_ref_unsafe()`
     returns the symref target, which we interpret as successful lookup.
     We thus return `0`.

  5. Now we look up "rebase-merge/{amend,orig-head}" and end up
     comparing whatever they contain with the cleared OID resolved from
     HEAD/ORIG_HEAD.

So the OID would not be uninitialized but the zero OID. Now:

  - "rebase-merge/amend" always contains the result of `repo_get_oid()`
    and never contains the zero OID.

  - "rebase-merge/orig-head" may contain the zero OID when there was no
    ORIG_HEAD at the time of starting a rebase or in case it did not
    resolve

So... if ORIG_HEAD was rewritten to be a symref pointing into nirvana between starting the rebase and calling into "wt-status.c", and when ORIG_HEAD didn't exist at the time of starting the rebase, then we might now wrongly report that splitting was in progress.

In other cases the old code was actually doing the wrong thing. Suppose that ORIG_HEAD was a dangling symref, then we'd have written the zero OID into "rebase-merge/orig-head". But when calling into "wt-status.c" now we read the still-dangling symref value and notice that the zero OID is different than "ref: refs/heads/dangling".

I dunno. It feels like this is one of many cases where as you start to think deeply about how things behave you realize that it's been broken all along. On the other hand, I doubt there was even a single user who ever experienced this issue. It often just needs to be correct enough.

I think the best way to go about this is to check for `REF_ISSSYMREF` and exit early in that case. We only want to compare direct refs, so this is closer to the old behaviour and should even fix edge cases like the above.

Will update.

Patrick

Some refs in Git are more special than others due to reasons explained in the next commit. These refs are read via `refs_read_special_head()`, but this function doesn't behave the same as when we try to read a normal ref. Most importantly, we do not propagate `failure_errno` in the case where the reference does not exist, which is behaviour that we rely on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

Patrick Steinhardt <ps@pks.im> writes:

Hmph, I thought, judging from what [1/4] did, that your plan was to use the refs API, instead of peeking directly into the filesystem, to access these pseudo refs, and am a bit puzzled if it makes all that much difference to fix errno handling while still reading directly from the filesystem. Perhaps such a conversion happens in later steps of this series (or a follow on series after this series lands)?

On Tue, Dec 12, 2023 at 12:28:49PM -0800, Junio C Hamano wrote:

Yes, that's ultimately the goal. The patch series here is only setting the stage by recording what we have, and addressing cases where we are inconsistently accessing refs via the filesystem _and_ via the ref API. Once this lands I do want create a follow up patch series that converts all refs to be non-special to the extent possible.

I say "to the extent possible" though because in the end there will be two refs that we must continue to treat specially: FETCH_HEAD and MERGE_HEAD, which we were treating special before this patch series already. Both of these are not normal refs because they may contain multiple values with annotations, so they will need to stay special.

Patrick

We have some references that are more special than others. The reason for them being special is that they either do not follow the usual format of references, or that they are written to the filesystem directly by the respective owning subsystem and thus circumvent the reference backend.

This works perfectly fine right now because the reffiles backend will know how to read those refs just fine. But with the prospect of gaining a new reference backend implementation we need to be a lot more careful here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit. But the current list of special refs is missing a lot of refs that really should be treated specially. Right now, we only treat `FETCH_HEAD` and `MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current instances of special refs to fix the reading path.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem and via the refdb, which violates the newly established rules for how special refs must be treated. This works alright in practice with the reffiles reference backend, but will cause bugs once we gain additional backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

Hi,

this is the third version of my patch series to improve handling of special refs.

Changes combpared to v2:

  - Patch 1: We're now more careful around missing and symbolic refs.

  - Patch 3: Document in the commit message that the extended list of
    special refs is not intended to stay like this forever.

Thanks for your reviews and the discussion!

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++-------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 59 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         | 10 +++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 22 +++++++++-----
 6 files changed, 87 insertions(+), 39 deletions(-)

Range-diff against v2:
1:  1db3eb3945 ! 1:  aea9410bd9 wt-status: read HEAD and ORIG_HEAD via the refdb
    @@ Commit message
         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.
    +    There are some subtleties involved here:
    +
    +      - We pass `RESOLVE_REF_READING` so that a missing ref will cause
    +        `read_ref_full()` to return an error.
    +
    +      - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve
    +        symrefs. The old code didn't resolve symrefs either, and we only
    +        ever write object IDs into the refs in "rebase-merge/".
    +
    +      - In the same spirit we verify that successfully-read refs are not
    +        symbolic refs.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)
     -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
     +	struct object_id head_oid, orig_head_oid;
     +	char *rebase_amend, *rebase_orig_head;
    ++	int head_flags, orig_head_flags;
      
      	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
      	    !s->branch || strcmp(s->branch, "HEAD"))
    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)
      
     -	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))
    ++	if (read_ref_full("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++			  &head_oid, &head_flags) ||
    ++	    read_ref_full("ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++			  &orig_head_oid, &orig_head_flags))
    ++		return 0;
    ++	if (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)
     +		return 0;
     +
      	rebase_amend = read_line_from_git_path("rebase-merge/amend");
2:  24032a62e5 = 2:  455ab69177 refs: propagate errno when reading special refs fails
3:  3dd9089fd5 ! 3:  81ac092281 refs: complete list of special refs
    @@ Commit message
     
         We're already mostly good with regard to the first item, except for
         `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
    -    But the current list of special refs is missing a lot of refs that
    -    really should be treated specially. Right now, we only treat
    -    `FETCH_HEAD` and `MERGE_HEAD` specially here.
    +    But the current list of special refs is missing some refs that really
    +    should be treated specially. Right now, we only treat `FETCH_HEAD` and
    +    `MERGE_HEAD` specially here.
     
         Introduce a new function `is_special_ref()` that contains all current
         instances of special refs to fix the reading path.
     
    +    Note that this is only a temporary measure where we record and rectify
    +    the current state. Ideally, the list of special refs should in the end
    +    only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may
    +    reference multiple objects and can contain annotations, so they indeed
    +    are special.
    +
         Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
4:  4a4447a3f5 = 4:  3244678161 bisect: consistently write BISECT_EXPECTED_REV via the refdb

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996

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.

There are some subtleties involved here:

  - We pass `RESOLVE_REF_READING` so that a missing ref will cause
    `read_ref_full()` to return an error.

  - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve
    symrefs. The old code didn't resolve symrefs either, and we only
    ever write object IDs into the refs in "rebase-merge/".

  - In the same spirit we verify that successfully-read refs are not
    symbolic refs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wt-status.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Some refs in Git are more special than others due to reasons explained in the next commit. These refs are read via `refs_read_special_head()`, but this function doesn't behave the same as when we try to read a normal ref. Most importantly, we do not propagate `failure_errno` in the case where the reference does not exist, which is behaviour that we rely on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

We have some references that are more special than others. The reason for them being special is that they either do not follow the usual format of references, or that they are written to the filesystem directly by the respective owning subsystem and thus circumvent the reference backend.

This works perfectly fine right now because the reffiles backend will know how to read those refs just fine. But with the prospect of gaining a new reference backend implementation we need to be a lot more careful here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit. But the current list of special refs is missing some refs that really should be treated specially. Right now, we only treat `FETCH_HEAD` and `MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current instances of special refs to fix the reading path.

Note that this is only a temporary measure where we record and rectify the current state. Ideally, the list of special refs should in the end only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may reference multiple objects and can contain annotations, so they indeed are special.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem and via the refdb, which violates the newly established rules for how special refs must be treated. This works alright in practice with the reffiles reference backend, but will cause bugs once we gain additional backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

Patrick Steinhardt <ps@pks.im> writes:

With the clear understanding that we plan to make those other than FETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[] eventually not special at all, this round looked quite sensible to me.

Let's merge it down to 'next'.

Thanks.

On Wed, Dec 20, 2023 at 11:28:51AM -0800, Junio C Hamano wrote:

Thanks. We (myself or a fellow team member at GitLab) will work on converting the remaining not-so-special refs once this topic lands on the `master` branch. Unless of course somebody else picks it up before we do.

Patrick