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
