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

[Nickolai Belakovski <nbelakovski@gmail.com>]

On Wed, Jan 23, 2019 at 10:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Missing sign-off?
>

My mistake, forgot -s on format-patch. Will remember to add it next go-around.

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

OK, should I shorten the function signature as well? Is it too long
because it's 101 characters and you're trying to stick to 100?

>
> > +
> > +static struct hashmap ref_to_worktree_map;
> > +static struct worktree **worktrees = NULL;
>
> No need to initialize static vars to 0/NULL.

OK

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

Sure.

> > @@ -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?

Yes, the parser used the atom argument in an earlier version of this
patch, but we since moved the map out of the atom since it only needs
to exist once globally. Even though we have a caching mechanism for
atoms it still seemed like a logical move to explicitly keep one
instance of the map globally. Will make the change, thanks for taking
a look at it through fresh eyes.