Re: [PATCH 5/8] revision.c: better error reporting on ref from different worktrees

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: Git List <git@vger.kernel.org>, Elijah Newren <newren@gmail.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 5/8] revision.c: better error reporting on ref from different worktrees
Date: Sun, 23 Sep 2018 04:25:39 -0400	[thread overview]
Message-ID: <CAPig+cTZpgU_n78wG4W0PDvM7Xdr3-1DoHz4d25XBFUmUy7a+A@mail.gmail.com> (raw)
In-Reply-To: <20180922180500.4689-6-pclouds@gmail.com>

On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Make use of the new ref aliases to pass refs from another worktree
> around and access them from the current ref store instead. This does
> not change any functionality, but when a problem shows up, we would
> report something like

From this description, I had a hard time grasping that the first
example output is the desired one. Not necessarily worth a re-roll,
but had it said instead something like this:

    ...but when a problem arises, we would like the reported
    messages to mention full ref aliases, like this:

it would have been more obvious.

More below...

>     fatal: bad object worktrees/ztemp/HEAD
>     warning: reflog of 'main/HEAD' references pruned commits
>
> instead of
>
>     fatal: bad object HEAD
>     warning: reflog of 'HEAD' references pruned commits
>
> which does not really tell where the refs are from.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -487,6 +487,28 @@ int submodule_uses_worktrees(const char *path)
> +void strbuf_worktree_ref(const struct worktree *wt,
> +                        struct strbuf *sb,
> +                        const char *refname)
> +{
> +       if (wt && !wt->is_current) {
> +               if (is_main_worktree(wt))
> +                       strbuf_addstr(sb, "main/");
> +               else
> +                       strbuf_addf(sb, "worktrees/%s/", wt->id);
> +       }
> +       strbuf_addstr(sb, refname);
> +}

Seeing this use worktree->id to construct "worktrees/<id>/<refname>"
makes me wonder how the user is going to know the ID of a worktree in
order to specify such refs in the first place. For example, how is the
user going to know the <id> in "git rev-parse worktrees/<id>/HEAD"? I
don't think we print the worktree ID anywhere, do we? Certainly, "git
worktree list" doesn't show it, and "git worktree add" stopped showing
it as of 2c27002a0a (worktree: improve message when creating a new
worktree, 2018-04-24).

> diff --git a/worktree.h b/worktree.h
> @@ -108,4 +108,18 @@ extern const char *worktree_git_path(const struct worktree *wt,
> +/*
> + * Return a refname suitable for access from the current ref
> + * store. The result may be destroyed at the next call.
> + */

If you re-roll, perhaps: s/may/will/