diff mbox series

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

Message ID 20190201220420.36216-2-nbelakovski@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v7,1/3] ref-filter: add worktreepath atom | expand

Commit Message

Nickolai Belakovski Feb. 1, 2019, 10:04 p.m. UTC 
From: Nickolai Belakovski <nbelakovski@gmail.com>

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>
---
 Documentation/git-for-each-ref.txt |  5 +++
 ref-filter.c                       | 78 ++++++++++++++++++++++++++++++++++++++
 t/t6302-for-each-ref-filter.sh     | 15 ++++++++
 3 files changed, 98 insertions(+)

Comments

Eric Sunshine Feb. 1, 2019, 10:20 p.m. UTC | #1 
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.
Junio C Hamano Feb. 1, 2019, 10:31 p.m. UTC | #2 
nbelakovski@gmail.com writes:

> +static void lazy_init_worktree_map(void)
> +{
> +	if (ref_to_worktree_map.worktrees)
> +		return;
> +
> +	ref_to_worktree_map.worktrees = get_worktrees(0);
> +	hashmap_init(&(ref_to_worktree_map.map), ref_to_worktree_map_cmpfnc, NULL, 0);
> +	populate_worktree_map(&(ref_to_worktree_map.map), ref_to_worktree_map.worktrees);
> +}
> +
> +static char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
> +{
> +	struct hashmap_entry entry;
> +	struct ref_to_worktree_entry *lookup_result;
> +
> +	lazy_init_worktree_map();
> +
> +	hashmap_entry_init(&entry, strhash(ref->refname));
> +	lookup_result = hashmap_get(&(ref_to_worktree_map.map), &entry, ref->refname);
> +
> +	if (lookup_result)
> +		return xstrdup(lookup_result->wt->path);
> +	else
> +		return xstrdup("");
> +}

Makes more sense than the previous round; much simpler to have
lazy-init in this function.

Thanks.
Nickolai Belakovski Feb. 1, 2019, 10:41 p.m. UTC | #3 
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.
Junio C Hamano Feb. 4, 2019, 6:14 p.m. UTC | #4 
Nickolai Belakovski <nbelakovski@gmail.com> writes:

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

Would that be a problem, though?  You now have a singleton hashmap
that is a file-scope global not tied to any particular atom, so...?
Nickolai Belakovski Feb. 18, 2019, 10:09 a.m. UTC | #5 
Well, it sounded like we didn't like the ":" extender from another
conversation on this thread. Do you think this patch should move back
in that direction?

On Tue, Feb 5, 2019 at 3:14 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nickolai Belakovski <nbelakovski@gmail.com> writes:
>
> > 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.
>
> Would that be a problem, though?  You now have a singleton hashmap
> that is a file-scope global not tied to any particular atom, so...?
diff mbox series

Patch

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 774cecc7ed..6dcd39f6f6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -214,6 +214,11 @@  symref::
 	`:lstrip` and `:rstrip` options in the same way as `refname`
 	above.
 
+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.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 422a9c9ae3..e46608476d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -20,6 +20,8 @@ 
 #include "commit-slab.h"
 #include "commit-graph.h"
 #include "commit-reach.h"
+#include "worktree.h"
+#include "hashmap.h"
 
 static struct ref_msg {
 	const char *gone;
@@ -75,6 +77,27 @@  static struct expand_data {
 	struct object_info info;
 } oi, oi_deref;
 
+struct ref_to_worktree_entry {
+	struct hashmap_entry ent; /* must be the first member! */
+	struct worktree *wt; /* key is wt->head_ref */
+};
+
+static int ref_to_worktree_map_cmpfnc(const void *unused_lookupdata,
+				      const void *existing_hashmap_entry_to_test,
+				      const void *key,
+				      const void *keydata_aka_refname)
+{
+	const struct ref_to_worktree_entry *e = existing_hashmap_entry_to_test;
+	const struct ref_to_worktree_entry *k = key;
+	return strcmp(e->wt->head_ref,
+		keydata_aka_refname ? keydata_aka_refname : k->wt->head_ref);
+}
+
+static struct ref_to_worktree_map {
+	struct hashmap map;
+	struct worktree **worktrees;
+} ref_to_worktree_map;
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -480,6 +503,7 @@  static struct {
 	{ "flag", SOURCE_NONE },
 	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
 	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	{ "worktreepath", SOURCE_NONE },
 	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
 	{ "end", SOURCE_NONE },
 	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
@@ -1525,6 +1549,48 @@  static int get_object(struct ref_array_item *ref, int deref, struct object **obj
 	return 0;
 }
 
+static void populate_worktree_map(struct hashmap *map, struct worktree **worktrees)
+{
+	int i;
+
+	for (i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->head_ref) {
+			struct ref_to_worktree_entry *entry;
+			entry = xmalloc(sizeof(*entry));
+			entry->wt = worktrees[i];
+			hashmap_entry_init(entry, strhash(worktrees[i]->head_ref));
+
+			hashmap_add(map, entry);
+		}
+	}
+}
+
+static void lazy_init_worktree_map(void)
+{
+	if (ref_to_worktree_map.worktrees)
+		return;
+
+	ref_to_worktree_map.worktrees = get_worktrees(0);
+	hashmap_init(&(ref_to_worktree_map.map), ref_to_worktree_map_cmpfnc, NULL, 0);
+	populate_worktree_map(&(ref_to_worktree_map.map), ref_to_worktree_map.worktrees);
+}
+
+static char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
+{
+	struct hashmap_entry entry;
+	struct ref_to_worktree_entry *lookup_result;
+
+	lazy_init_worktree_map();
+
+	hashmap_entry_init(&entry, strhash(ref->refname));
+	lookup_result = hashmap_get(&(ref_to_worktree_map.map), &entry, ref->refname);
+
+	if (lookup_result)
+		return xstrdup(lookup_result->wt->path);
+	else
+		return xstrdup("");
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -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")) {
+			if (ref->kind == FILTER_REFS_BRANCHES)
+				v->s = get_worktree_path(atom, ref);
+			else
+				v->s = xstrdup("");
+			continue;
+		}
 		else if (starts_with(name, "symref"))
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
@@ -2045,6 +2118,11 @@  void ref_array_clear(struct ref_array *array)
 		free_array_item(array->items[i]);
 	FREE_AND_NULL(array->items);
 	array->nr = array->alloc = 0;
+	if (ref_to_worktree_map.worktrees) {
+		hashmap_free(&(ref_to_worktree_map.map), 1);
+		free_worktrees(ref_to_worktree_map.worktrees);
+		ref_to_worktree_map.worktrees = NULL;
+	}
 }
 
 static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fc067ed672..87e0222ea1 100755
--- 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_must_fail git for-each-ref --merged HEAD --no-merged HEAD
 '
 
+test_expect_success '"add" a worktree' '
+	mkdir worktree_dir &&
+	git worktree add -b master_worktree worktree_dir master
+'
+
+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
+'
+
 test_done