Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well

[Jeff King <peff@peff.net>]

On Sat, Feb 10, 2018 at 04:59:52PM +0700, Duy Nguyen wrote:

> On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:
> > This takes advantage of the fact that "worktrees/$WORKTREE/HEAD" will
> > currently resolve as a ref, which may not be true in the future with
> > different ref backends.  I don't think it locks us in to supporting
> > resolving other worktree HEADs with this syntax, as I view the
> > user-visible error message as more of a pointer to a pathname that the
> > user will need to fix.  If the underlying ref storage changes, naturally
> > both this code and the hint may need to change to match.
> 
> I'm leaning more about something like this patch below (which does not
> even build, just to demonstrate). A couple points:
> 
> - Instead of doing the hacky refs worktrees/foo/HEAD, we get the
>   correct ref store for each worktree
> - We have an API for getting all (non-broken) worktrees. I realize for
>   fsck, we may even want to examine semi-broken worktrees as well, but
>   get_worktrees() can take a flag to accomplish that if needed.
> - As you can see, I print %p from the ref store instead of something
>   human friendly. This is something I was stuck at last time. I need
>   better ref store description (or even the worktree name)

Yeah, I think this is the right approach. We know about worktrees
internally, and we should be able to iterate over their ref stores, even
if we have no way to succinctly name the resulting ref.

> - This ref_name() function makes me think if we should have an
>   extended sha1 syntax for accessing per-worktree refs from a
>   different worktree, something like HEAD@{worktree:foo} to resolve to
>   worktrees/foo/HEAD. Then we have a better description here that can
>   actually be used from command line, as a regular ref, if needed.

Yeah, I agree this would be very useful. I peeked at how bad it would be
to hanlde this. The parsing is pretty easy in get_oid_basic(), but you'd
have to plumb through the ref_store in quite a few places:

 - interpret_nth_prior_checkout() would probably want to use the
   worktree's HEAD (for "@{worktree:foo}@{-1}")

 - dwim_ref() would need to know about the ref store

 - that implies that substitute_branch_name() knows about it, too

So it may turn into a big job. But I think it's moving in the right
direction. And it may not be the end of the world if all features do not
work from day one (e.g., if HEAD@{worktree:foo} works, but
HEAD@{worktree:foo}@{upstream} does not yet, with plans to eventually
make that work).

-Peff