On Mon, Dec 24, 2018 at 12:47:54AM -0800, nbelakovski@xxxxxxxxx wrote:
> [...]
Thanks for keeping with this. I think we're getting quite close, though
I did find a few small-ish issues.
> @@ -34,6 +36,8 @@ static struct ref_msg {
> "ahead %d, behind %d"
> };
>
> +static struct worktree **worktrees;
> +
Maybe define this near "struct hashmap ref_to_worktree_map" so it's
more obvious that the two are related?
> @@ -75,6 +79,11 @@ 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 */
> +};
Indent with spaces?
> -static int used_atom_cnt, need_tagged, need_symref;
> +static int used_atom_cnt, need_tagged, need_symref, has_worktree;
> +static struct hashmap ref_to_worktree_map;
Makes sense. I thought at first has_worktree was a flag that we might
care about between parsing and formatting, but it's really just a flag
to say "we lazy-loaded the worktree list".
> +static int worktree_hashmap_cmpfnc(const void *unused_lookupdata, const void *existing_hashmap_entry_to_test,
> + const void *unused_key, const void *keydata_aka_refname)
> +{
> + const struct ref_to_worktree_entry *e = existing_hashmap_entry_to_test;
> + return strcmp(e->wt->head_ref, keydata_aka_refname);
> +}
So from the discussion in the cover letter, this needs to be more like:
static int worktree_hashmap_cmpfnc(const void *unused_lookupdata,
const void *ve1, const void *ve2,
const void *keydata_aka_refname)
{
const struct ref_to_worktree_entry *e1 = ve1, *e2 = ve2;
return strcmp(e1->wt->head_ref, keydata_aka_refname ?
keydata_aka_refname :
e2->wt->head_ref);
}
> +static int worktree_atom_parser(const struct ref_format *format,
> + struct used_atom *atom,
> + const char *arg,
> + struct strbuf *unused_err)
> +{
> + int i;
> + if (has_worktree)
> + return 0;
Minor style nit, but please put a space between the declarations and the
start of the code (not strictly necessary for a short function which has
no other linebreaks, like the cmpfunc above, but here I think it's
confusing not to).
> + worktrees = get_worktrees(0);
> +
> + hashmap_init(&ref_to_worktree_map, worktree_hashmap_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);
> + }
> + }
Makes sense to load the map.
> +static const 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);
> +
> + strbuf_addstr(&val, lookup_result ? lookup_result->wt->path : "");
> +
> + return strbuf_detach(&val, NULL);
> +}
And that makes sense to look up an item in it. Good.
Adding an empty string to a strbuf is a noop, so that part might more
clearly be written as just:
if (lookup_result)
strbuf_addstr(&val, lookup_result->wt->path);
We return a "const char *" here, but the result is always allocated. Do
we leak the result? Or should this return a "char *"?
I think there are a lot of other atoms that leak currently, but that is
being fixed in another topic that is currently in pu.
> @@ -2020,6 +2085,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 (has_worktree)
> + {
> + hashmap_free(&ref_to_worktree_map, 1);
> + free_worktrees(worktrees);
> + }
Here we free everything, but we don't unset has_worktree. So anybody
trying to format more refs afterward would see our freed worktree list.
We probably want:
has_worktree = 0;
here. Or simpler still, I think get_worktrees() will always return a
non-NULL list (even if it is empty). So you could just drop has_worktree
entirely, and use:
if (worktrees)
return; /* already loaded */;
in the loading function, and:
free_worktrees(worktrees);
worktrees = NULL;
here.
> +test_expect_success '"add" a worktree' '
> + mkdir worktree_dir &&
> + git worktree add -b master_worktree worktree_dir master
> +'
> +
> +test_expect_success 'validate worktree atom' '
> + {
> + echo master: $PWD &&
> + echo master_worktree: $PWD/worktree_dir &&
> + echo side: not checked out
> + } > expect &&
Minor style nit: use "} >expect" without the extra space.
This checks the actual directories. Good. I can never remember the rules
for when to use $PWD versus $(pwd) on Windows. We may run afoul of the
distinction here.
-Peff
