nbelakovski@xxxxxxxxx writes:
> From: Nickolai Belakovski <nbelakovski@xxxxxxxxx>
>
> 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?
