Re: [PATCH v6 4/9] ref: support multiple worktrees check for refs
- From
shejialuo <shejialuo@gmail.com>
- Date
- Nov 5, 2024, 12:52 UTC
- Message-ID
- <ZyoVA-p4JXPaKTny@ArchLinux>
- In-Reply-To
- <ZynFNQ8SnvTJlVdN@pks.im>
On Tue, Nov 05, 2024 at 08:11:49AM +0100, Patrick Steinhardt wrote:
Show 11 quoted lines
> 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]
Show 16 quoted lines
> > @@ -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 *".
Show 30 quoted lines
> > 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