On Tue, Nov 05, 2024 at 08:11:49AM +0100, Patrick Steinhardt wrote:
> On Mon, Oct 21, 2024 at 09:34:40PM +0800, shejialuo wrote:
> > We have already set up the infrastructure to check the consistency for
> > refs, but we do not support multiple worktrees. As we decide to add more
> > checks for ref content, we need to set up support for multiple
> > worktrees.
>
> I don't quite follow that logic: the fact that we perform more checks
> for the ref content doesn't necessarily mean that we also have to check
> worktree refs. We rather want to do that so that we get feature parity
> with git-fsck(1) eventually, don't we?
>
Yes, I agree. I come across why I wrote such message. Actually, in the
very early implementation, I didn't consider about worktree situation
for the "escape". And I thought I should add support for worktree. So, I
made a mistake.
[snip]
> > @@ -3558,6 +3560,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
> > } else if (S_ISREG(iter->st.st_mode) ||
> > S_ISLNK(iter->st.st_mode)) {
> > strbuf_reset(&target_name);
> > +
> > + if (!is_main_worktree(wt))
> > + strbuf_addf(&target_name, "worktrees/%s/", wt->id);
> > strbuf_addf(&target_name, "%s/%s", refs_check_dir,
> > iter->relative_path);
> >
>
> Hm. Isn't it somewhat duplicate to pass both the prepended target name
> _and_ the worktree to the callback? I imagine that we'd have to
> eventually strip the worktree prefix to find the correct ref, unless we
> end up using the main ref store to look up the ref.
>
Actually, the worktree won't be passed to the callback. The
`fsck_refs_fn` function will never use worktree `wt`. The reason why I
use `wt` is that we need to print _full_ path information to the user
when error happens for the situation where worktree A and worktree B has
the same ref name "refs/worktree/foo".
I agree that we will strip the worktree prefix to find the correct ref
in the file system. This is done by the following statement:
strbuf_addf(&sb, "%s/%s", ref_store->gitdir, refs_check_dir);
For worktree, `ref_store->gitdir` will automatically be
`.git/worktrees/<id>`.
In the v5, I didn't print the full path and we even didn't need the
parameter `wt`. However, if we want to print the following info:
worktrees/<id>/refs/worktree/a
So, just because we need the `worktrees/<id>` information. Actually, we
could also get the information by using "ref_store->gitdir" and
"ref_store->repo->gitdir". However, this is cumbersome and it's a bad
idea. So I change the prototype of "fsck_fn" to add a new parameter
"struct worktree *".
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 07c57fd541..46dcaec654 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -13,6 +13,7 @@
> > #include "../lockfile.h"
> > #include "../chdir-notify.h"
> > #include "../statinfo.h"
> > +#include "../worktree.h"
> > #include "../wrapper.h"
> > #include "../write-or-die.h"
> > #include "../trace2.h"
> > @@ -1754,8 +1755,13 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
> > }
> >
> > static int packed_fsck(struct ref_store *ref_store UNUSED,
> > - struct fsck_options *o UNUSED)
> > + struct fsck_options *o UNUSED,
> > + struct worktree *wt)
> > {
> > +
> > + if (!is_main_worktree(wt))
> > + return 0;
> > +
> > return 0;
> > }
>
> It's somewhat funny to have this condition here, but it does make sense
> overall as worktrees never have packed refs in the first place.
>
Yes, there is no packed-refs in the worktree. And we need to prevent
calling multiple times.
> Patrick
Thanks,
Jialuo
