[v5,1/3] ref-filter: add worktreepath atom
diff mbox series

Message ID 20190106002619.54741-2-nbelakovski@gmail.com
State New
Headers show
Series
  • Untitled series #62625
Related show

Commit Message

Nickolai Belakovski Jan. 6, 2019, 12:26 a.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.
---
 Documentation/git-for-each-ref.txt |  5 +++
 ref-filter.c                       | 71 ++++++++++++++++++++++++++++++++++++++
 t/t6302-for-each-ref-filter.sh     | 15 ++++++++
 3 files changed, 91 insertions(+)

Comments

Junio C Hamano Jan. 7, 2019, 6:20 p.m. UTC | #1
nbelakovski@gmail.com writes:

> +static struct hashmap ref_to_worktree_map;
> +static struct worktree **worktrees = NULL;
> +
>  /*
>   * An atom is a valid field atom listed below, possibly prefixed with
>   * a "*" to denote deref_tag().
> @@ -420,6 +438,34 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
>  	return 0;
>  }
>  
> +static int worktree_atom_parser(const struct ref_format *format,
> +				struct used_atom *atom,
> +				const char *arg,
> +				struct strbuf *unused_err)
> +{
> +	int i;
> +
> +	if (worktrees)
> +		return 0;

OK, so verify_ref_format() etc. will trigger this to be called via
valid_atom[].parser when "%(worktreepath)" is seen in the user
format, and then this grabs all the worktrees and record their paths
in the hashmap.  This if() statement makes sure that it happens only
once.

> +	worktrees = get_worktrees(0);
> +
> +	hashmap_init(&ref_to_worktree_map, ref_to_worktree_map_cmpfnc, NULL, 0);
> +
> +	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(&ref_to_worktree_map, entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static struct {
>  	const char *name;
>  	info_source source;
> @@ -461,6 +507,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, FIELD_STR, worktree_atom_parser },
>  	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
>  	{ "end", SOURCE_NONE },
>  	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
> @@ -1500,6 +1547,21 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
>  	return 0;
>  }
>  
> +static char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
> +{
> +	struct strbuf val = STRBUF_INIT;
> +	struct hashmap_entry entry;
> +	struct ref_to_worktree_entry *lookup_result;

And then this will be called from populate_value() for each of the
ref.  It looks up the worktree that has checked out this branch, if
any, from the hashmap, and yields the path to it.

When seeing a tag or note ref, by definition that's not something we
can have checked out in any worktree.  I wonder if it is worth to
optimize further by omitting this lookup when ref is not a local
branch?

IOW, with a typical number of worktrees and refs, how costly would ...

> +	hashmap_entry_init(&entry, strhash(ref->refname));
> +	lookup_result = hashmap_get(&ref_to_worktree_map, &entry, ref->refname);

... this sequence of calls be.

> +
> +	if (lookup_result)
> +		strbuf_addstr(&val, lookup_result->wt->path);
> +
> +	return strbuf_detach(&val, NULL);
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -1537,6 +1599,10 @@ 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")) {
> +			v->s = get_worktree_path(atom, ref);
> +			continue;
> +		}
>  		else if (starts_with(name, "symref"))
>  			refname = get_symref(atom, ref);
>  		else if (starts_with(name, "upstream")) {
> @@ -2020,6 +2086,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 (worktrees)
> +	{

Have this opening brace at the end of the previous line, i.e.

	if (worktrees) {

> +		hashmap_free(&ref_to_worktree_map, 1);
> +		free_worktrees(worktrees);
> +	}
>  }

What's the point of ref_array_clear()?  What does the caller of this
function really want?  Is it merely to release the resources
consumed?  If so, then this is good enough, but then the existing
calls to FREE_AND_NULL() for releasing resources in the function is
overkill.

Or is it envisioned that we are preparing a clean slate so that
another call, possibly after the external environment changed, can
be made into this machinery (i.e. imagine we lift ref-filter.c code
and link it to a long running service process; after serving one
for-each-ref request, a new worktree or a new branch may get
created, and then we may get another for-each-ref request)?  If that
is the case, then the added code breaks that hope, as it leaves a
dangling pointer in the worktrees variable.

>  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
Nickolai Belakovski Jan. 18, 2019, 10:17 p.m. UTC | #2
On Mon, Jan 7, 2019 at 10:20 AM Junio C Hamano <gitster@pobox.com> wrote:
> When seeing a tag or note ref, by definition that's not something we
> can have checked out in any worktree.  I wonder if it is worth to
> optimize further by omitting this lookup when ref is not a local
> branch?
>
> IOW, with a typical number of worktrees and refs, how costly would ...
>
> > +     hashmap_entry_init(&entry, strhash(ref->refname));
> > +     lookup_result = hashmap_get(&ref_to_worktree_map, &entry, ref->refname);
>
> ... this sequence of calls be.
>

It certainly wouldn't be free. Every check would compute the hash of
the refname and do a lookup into the hash table. If the bucket it
looked up was empty that'd be it, but if it were non-empty a few more
comparisons would happen.

I think avoiding this would be check, we can simply check ref->kind ==
FILTER_REFS_BRANCHES ahead of calling into get_worktree_path and
provide an empty string otherwise.

> >               free_array_item(array->items[i]);
> >       FREE_AND_NULL(array->items);
> >       array->nr = array->alloc = 0;
> > +     if (worktrees)
> > +     {
>
>
> What's the point of ref_array_clear()?  ...  If that
> is the case, then the added code breaks that hope, as it leaves a
> dangling pointer in the worktrees variable.
>

Discussion of the point of ref_array_clear would be out of scope of
this change, but your point is well taken that setting worktrees to
NULL would be consistent with the rest of the function. Will
implement.
Nickolai Belakovski Jan. 18, 2019, 10:20 p.m. UTC | #3
On Fri, Jan 18, 2019 at 2:17 PM Nickolai Belakovski
<nbelakovski@gmail.com> wrote:
>
>
> I think avoiding this would be check, we can simply check ref->kind ==
> FILTER_REFS_BRANCHES ahead of calling into get_worktree_path and
> provide an empty string otherwise.
>

*would be check -> would be cheap

Patch
diff mbox series

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 901faef1bf..caba1c23b8 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -209,6 +209,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 5de616befe..e7ca45f39b 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,22 @@  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 hashmap ref_to_worktree_map;
+static struct worktree **worktrees = NULL;
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -420,6 +438,34 @@  static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 	return 0;
 }
 
+static int worktree_atom_parser(const struct ref_format *format,
+				struct used_atom *atom,
+				const char *arg,
+				struct strbuf *unused_err)
+{
+	int i;
+
+	if (worktrees)
+		return 0;
+
+	worktrees = get_worktrees(0);
+
+	hashmap_init(&ref_to_worktree_map, ref_to_worktree_map_cmpfnc, NULL, 0);
+
+	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(&ref_to_worktree_map, entry);
+		}
+	}
+
+	return 0;
+}
+
 static struct {
 	const char *name;
 	info_source source;
@@ -461,6 +507,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, FIELD_STR, worktree_atom_parser },
 	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
 	{ "end", SOURCE_NONE },
 	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
@@ -1500,6 +1547,21 @@  static int get_object(struct ref_array_item *ref, int deref, struct object **obj
 	return 0;
 }
 
+static char *get_worktree_path(const struct used_atom *atom, const struct ref_array_item *ref)
+{
+	struct strbuf val = STRBUF_INIT;
+	struct hashmap_entry entry;
+	struct ref_to_worktree_entry *lookup_result;
+
+	hashmap_entry_init(&entry, strhash(ref->refname));
+	lookup_result = hashmap_get(&ref_to_worktree_map, &entry, ref->refname);
+
+	if (lookup_result)
+		strbuf_addstr(&val, lookup_result->wt->path);
+
+	return strbuf_detach(&val, NULL);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1537,6 +1599,10 @@  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")) {
+			v->s = get_worktree_path(atom, ref);
+			continue;
+		}
 		else if (starts_with(name, "symref"))
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
@@ -2020,6 +2086,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 (worktrees)
+	{
+		hashmap_free(&ref_to_worktree_map, 1);
+		free_worktrees(worktrees);
+	}
 }
 
 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