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

From: Jeff King <peff@peff.net>
Date: Dec 20, 2018, 14:59 UTC
Message-ID: <20181220145931.GB27361@sigill.intra.peff.net>
In-Reply-To: <CAC05384H1LgsGMO=ggUyfFTXrXAFcjUXDSdcmev9ActPt5081A@mail.gmail.com>

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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