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

On Fri, Feb 1, 2019 at 2:20 PM Eric Sunshine [off-list ref] wrote:

On Fri, Feb 1, 2019 at 5:04 PM [off-list ref] wrote:

quoted

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 <redacted>
---

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.

quoted

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.

quoted

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.

quoted

+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.