Re: [PATCH v6 4/9] ref: support multiple worktrees check for refs

[shejialuo <shejialuo@gmail.com>]

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