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

[Eric Sunshine <sunshine@sunshineco.com>]

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

> 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>".)

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