Re: [PATCH 02/10] builtin/refs.h: get worktrees without reading head info

[shejialuo <shejialuo@gmail.com>]

On Tue, Jan 07, 2025 at 06:57:08AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > In "packed-backend.c", there are some functions such as "create_snapshot"
> > and "next_record" which would check the correctness of the content of
> > the "packed-ref" file. When anything is bad, the program will die.
> 
> So you're saying, `create_snapshot()` and `next_record()` exit the
> program on any error. Okay that seems to be valid.
> 
> > It may seem that we have nothing relevant to above feature, because we
> > are going to read and parse the raw "packed-ref" file without creating
> > the snapshot and using the ref iterator to check the consistency.
> >
> > However, when using "get_worktrees" in "builtin/refs", we will parse the
> > head information. If the referent of the "HEAD" is inside the
> > "packed-ref", we will call "create_snapshot" and "next_record" functions
> > to parse the "packed-ref" to get the head information. And if there are
> > something wrong, the program will die.
> >
> > Although this behavior has no harm for the program, it will
> > short-circuit the program. When the users execute "git refs verify" or
> > "git fsck", we don't want to simply die the program but rather show the
> > warnings or errors as many as possible to info the users. So, we should
> > avoiding reading the head info.
> >
> 
> This is a bit tricky here. If the information for the `HEAD` ref is
> incorrect in the packed-refs, git would exit early. Which is what we're
> trying to avoid in this patch, by using the `get_worktrees_internal()`
> function.
> 

I think my commit message may confuse you here. The information of the
"HEAD" ref will never be stored in the "packed-refs", but if we need to
read the head information, we need to parse the "packed-refs" via
"create_snapshot" method. Even though the corresponding referent is
correct (and even if it is not correct, it won't let the program die),
"create_snapshot" will call "verify_buffer_safe" to check whether there
is a newline in the last line of the file. If not, it will die.

However, this is a bad thing. For example, if the HEAD points to
"refs/heads/main", now we need to use the code path from packed-backend,
we have to call "create_snapshot", the program will die. And we cannot
tell the user the other faults.

```packed-refs
<good_oid> refs/heads/main\n
<bad_oid> <bad_refname>\n
<oid> refs/heads/a
```

So, the motivation here is that we should not read HEAD at all when we
are doing consistency checking to make the code totally independent of
the "create_snapshot" and "next_record".

> However, I would question if this is the right approach. Shouldn't
> `get_worktree()` failing indicate that the repository is invalid? In
> that case does it really make sense to allow the user to even run `git
> refs verify`? Isn't the prerequisite for running the `git-refs(1)`
> command a valid repository?
> 

As I have talked about above, even though the referent of "HEAD" is
good, "get_worktree()" will still fail because of some fatal errors in
"packed-refs" file. I don't think that the repository is invalid in this
situation.

Put it further more, in what situations, the users want to execute "git
refs verify" or "git-fsck". From my intuitive thinking, the users will
execute these check commands when something fails. They want to know
why. So we should execute these commands when the repository is invalid
to tell the user what may be wrong. And this is the value of these two
commands.

Thanks,
Jialuo