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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux