| Message ID | 20181224084756.49952-2-nbelakovski@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/3] ref-filter: add worktreepath atom | expand |
On Mon, Dec 24, 2018 at 12:47:54AM -0800, nbelakovski@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
On Thu, Jan 3, 2019 at 12:40 AM Jeff King <peff@peff.net> wrote: > On Mon, Dec 24, 2018 at 12:47:54AM -0800, nbelakovski@gmail.com wrote: > > +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. An interpolating here-doc would be even more natural: cat >expect <-EOF && master: $(pwd) master_worktree: $(pwd)/worktree_dir side: not checked out EOF > 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. As I understand it, this is exactly a case in which you would need to use $(pwd); namely, when coming up with an "expect" value. t/README talks about it.
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..240e7b80f8 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; @@ -34,6 +36,8 @@ static struct ref_msg { "ahead %d, behind %d" }; +static struct worktree **worktrees; + void setup_ref_filter_porcelain_msg(void) { msgs.gone = _("gone"); @@ -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 */ +}; + /* * An atom is a valid field atom listed below, possibly prefixed with * a "*" to denote deref_tag(). @@ -116,7 +125,8 @@ static struct used_atom { char *head; } u; } *used_atom; -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; /* * Expand string, append it to strbuf *sb, then return error code ret. @@ -420,6 +430,42 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a return 0; } +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); +} + +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; + + 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); + } + } + + has_worktree = 1; + + 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,20 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj return 0; } +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); +} + /* * Parse the object referred by ref, and grab needed value. */ @@ -1537,6 +1598,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 +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); + } } 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..d70517a6ae 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' ' + { + echo master: $PWD && + echo master_worktree: $PWD/worktree_dir && + echo side: not checked out + } > expect && + 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
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 | 72 +++++++++++++++++++++++++++++++++++++- t/t6302-for-each-ref-filter.sh | 15 ++++++++ 3 files changed, 91 insertions(+), 1 deletion(-)