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

From: Nickolai Belakovski <nbelakovski@gmail.com>
Date: Jan 23, 2019, 23:34 UTC
Message-ID: <CAC05384+KjC=4_ZF9BrxweMUjwpkaGXNqRNSnwif6yci6TxMMw@mail.gmail.com>
In-Reply-To: <xmqq36pjcicw.fsf@gitster-ct.c.googlers.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.

Show 11 quoted lines

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

Show 6 quoted lines

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

OK

Show 12 quoted lines

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

Show 29 quoted lines

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