On Mon, Dec 24, 2018 at 12:47:54AM -0800, nbelakov...@gmail.com 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