Re: [PATCH v7 1/3] ref-filter: add worktreepath atom

[Nickolai Belakovski <nbelakovski@gmail.com>]

On Fri, Feb 1, 2019 at 2:20 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Feb 1, 2019 at 5:04 PM <nbelakovski@gmail.com> wrote:
> > Add an atom providing the path of the linked worktree where this ref is
> > checked out, if it is checked out in any linked worktrees, and empty
> > string otherwise.
> >
> > Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
> > ---
> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> > @@ -214,6 +214,11 @@ symref::
> > +worktreepath::
> > +       The absolute path to the worktree in which the ref is checked
> > +       out, if it is checked out in any linked worktree. Empty string
> > +       otherwise.
>
> This may have been asked previously, but is there a reason this name
> was chosen over the more extensible "worktree:" with "path" as a
> modifier (i.e. "worktree:path")? I scanned the thread a couple weeks
> ago and did see mention of "worktree:path" but did not find any
> followup. I ask because it's conceivable that someone in the future
> might want to retrieve other information about the worktree beyond its
> path (such as whether it's bare or detached, etc.). By using the form
> "worktree:<foo>", we leave that door open. (I'm not suggesting that
> this patch series needs to implement fetching of any of the other
> worktree properties, but just asking if "worktree:<foo>" should be
> considered.)
>

There's been a little back and forth on it, but my understanding is
that using the colon separator bypasses the caching mechanism in the
atoms, so every instance of "worktree:path" in a format string would
require a lookup. Future atoms should be along the lines of
"worktreeisdetached", "worktreeisbare", etc. This is consistent with
several of the other atoms, like objecttype/size/name,
comitter/name/email/date.

> > diff --git a/ref-filter.c b/ref-filter.c
> > @@ -1562,6 +1628,13 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
> >                 if (starts_with(name, "refname"))
> >                         refname = get_refname(atom, ref);
> > +               else if (starts_with(name, "worktreepath")) {
>
> I think this was brought up previously, but shouldn't this be strcmp()
> rather than starts_with()?
>
> (starts_with() would be appropriate, if you went with the suggested
> "worktree:<foo>".)

Not sure about it being brought up previously. starts_with seemed
consistent with other uses but now I see there's several other
instance of strcmp in populate value. Seems like a reasonable thing to
change. I had previously implemented "worktree:<foo>" and must've left
it alone after we went with worktreepath.

>
> > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> > @@ -441,4 +441,19 @@ test_expect_success '--merged is incompatible with --no-merged' '
> > +test_expect_success '"add" a worktree' '
> > +       mkdir worktree_dir &&
> > +       git worktree add -b master_worktree worktree_dir master
> > +'
>
> I don't think 'mkdir' is needed since "git worktree add" should create
> the directory itself.
>
> > +test_expect_success 'validate worktree atom' '
> > +       cat >expect <<-EOF &&
> > +       master: $(pwd)
> > +       master_worktree: $(pwd)/worktree_dir
> > +       side: not checked out
> > +       EOF
> > +       git for-each-ref --format="%(refname:short): %(if)%(worktreepath)%(then)%(worktreepath)%(else)not checked out%(end)" refs/heads/ >actual &&
> > +       test_cmp expect actual
> > +'
>
> If this is the only test using that newly-created worktree, it might
> make sense to squash the two tests together.

Sure, can do, on both points.