On Fri, Feb 1, 2019 at 2:20 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Feb 1, 2019 at 5:04 PM <nbelakovski@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
