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

From: Junio C Hamano <gitster@pobox.com>
To: nbelakovski@gmail.com
Cc: git@vger.kernel.org, peff@peff.net, rafa.almas@gmail.com,
	avarab@gmail.com
Subject: Re: [PATCH v6 1/3] ref-filter: add worktreepath atom
Date: Wed, 23 Jan 2019 10:57:19 -0800	[thread overview]
Message-ID: <xmqq36pjcicw.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190122232301.95971-2-nbelakovski@gmail.com> (nbelakovski's message of "Tue, 22 Jan 2019 15:22:59 -0800")

nbelakovski@gmail.com writes:

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

Missing sign-off?

> +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);

Overlong line.

> +}

> +
> +static struct hashmap ref_to_worktree_map;
> +static struct worktree **worktrees = NULL;

No need to initialize static vars to 0/NULL.

> @@ -438,6 +456,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;
> +}

It is kind of interesting that a function for parsing an "atom" in
"format" does not look at none of its arguments at all ;-)  The fact
that "%(worktreepath)" atom got noticed by verify_ref_format() alone
is of interest for this function.

The helper iterates over all worktrees, registers them in a hashmap
ref_to_worktree_map, indexed by the head reference.

OK.

> +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);
> +}

We do not need a strbuf to do the above, do we?

	hashmap_entry_init(...);
	lookup_result = hashmap_get(...);
	if (lookup_result)
		return xstrdup(lookup_result->wt->path);
	else
		return xstrdup("");

or something like that, perhaps?

> @@ -1562,6 +1624,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;
> +		}

I am wondering if get_worktree_path() being called should be the
triggering event for lazy initialization worktree_atom_parser() is
doing in this patch, instead of verify_ref_format() seeing the
"%(worktreepath)" atom.  Is there any codepath that wants to make
sure the lazy initialization is done without/before populate_value()
sees a use of the "%(worktreepath)" atom?  If so, such a plan would
not work, but otherwise, we can do the following:

 - rename worktree_atom_parser() to lazy_init_worktrees() or
   something, and remove all of its unused parameters.

 - remove parser callback for "worktreepath" from valid_atom[].

 - call lazy_inti_worktrees() at the beginning of
   get_worktree_path().

Hmm?