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

From: Jeff King <peff@peff.net>
To: Nickolai Belakovski <nbelakovski@gmail.com>
Cc: git@vger.kernel.org, "Rafael Ascensão" <rafa.almas@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3 1/3] ref-filter: add worktreepath atom
Date: Thu, 20 Dec 2018 09:59:31 -0500	[thread overview]
Message-ID: <20181220145931.GB27361@sigill.intra.peff.net> (raw)
In-Reply-To: <CAC05384H1LgsGMO=ggUyfFTXrXAFcjUXDSdcmev9ActPt5081A@mail.gmail.com>

On Wed, Dec 19, 2018 at 11:09:59PM -0800, Nickolai Belakovski wrote:

> > Also, why are we replacing it with a single space? Wouldn't the empty
> > string be more customary (and work with the other "if empty, then do
> > this" formatting options)?
> 
> I was just following what was done for HEAD, but overall I agree that
> empty is preferable to single space, will change.

Ah, right, that makes more sense. I do still think for %(HEAD) it's a
little different because it is "+" or a single space, so always one
character.  Here we have some value or not, and in the "not" case for
such things we usually give an empty string (e.g., for %(push),
%(upstream), etc).

> To sum up the hashmap comments:
> -I hadn't thought to re-use the head_ref of worktree as the key.
> That's clever. I like the readability of having separate entries for
> key and value, but I can see the benefit of not having to do an extra
> allocation. I can make up for the readability hit with a comment.

Thanks, that makes sense.

> -Actually, for any valid use case there will only be one instance of
> the map since the entries of used_atom are cached, but regardless it
> makes sense to keep per-atom info in used_atom and global context
> somewhere else, so I'll make that change to make it a static variable
> outside of used_atom.

Ah, right, I forgot there was some magic around used_atom. I do still
agree that the separate static global makes things a little simpler.

> -Will change the lookup logic to remove the extra allocation. Since
> I'm letting the hashmap use its internal comparison function on the
> hash, I don't need to provide a comparison function.

I don't think that works. The default function is always_equal(), which
will treat two entries equal if they have the same hash value. I.e., any
collisions would be considered a match.

> Thanks for all the feedback, will try to turn these around quickly.

Great, thanks! I'll be on vacation for the next two weeks, so I may be
very slow to look at the next iteration. :)

-Peff