So where does that leave us for this series? We could move hashmap back into used_atom, but if a user entered --format="%(worktreepath)%(worktreepath:)" we'd end up freeing worktrees twice. Not that that should stop us - that scenario is one where user input isn't sensible and personally I don't think it's necessary to protect against such things (unless the user was reasonably confused, but I don't see that as the case here). I agree with Jeff that a ref-filter "context" would help. And in more ways than one, it could help us decide ahead of time whether to check if a ref is a branch or a tag before doing a hashmap lookup or just skip the check (i.e. if there are no tags within the context, the check would only add cost). But I do believe that that would be outside the scope of this series. I think leaving it as globals is a tiny bit safer and also makes it easier to pack it into a context if/when we decide to do that work, but as always I'm open to other interpretations. On Thu, Jan 24, 2019 at 1:26 PM Jeff King <peff@xxxxxxxx> wrote: > > On Thu, Jan 24, 2019 at 11:30:20AM -0800, Junio C Hamano wrote: > > > Jeff King <peff@xxxxxxxx> writes: > > > > > What if you have other atoms that need worktrees? E.g., does > > > %(worktreepath:foo) use the same used_atom slot? What if we have another > > > worktree-related atom? > > > ... > > > And that one is a good example where we _do_ need the global, because we > > > already have multiple atoms pulling from it. > > > > I guess that we broke the original atom design by mistake when we > > added ":<modifiers>" support. There should have been one layer of > > indirection that binds the instances of the same atom with different > > modifiers together---I agree with you that we cannot avoid globals > > without fixing that mistake first. > > Yes, that's one way to fix it. > > I actually think the biggest mistake is having that used_atoms list in > the first place, as we iterate over it several times asking "can we fill > this in yet?". The way pretty.c does it is just to incrementally parse > the commit, saving intermediate results. And in cat-file.c, we figure > out what we need ahead of time in a single pass, and then just fill it > in for each object (which sort of works out the same, but doesn't > require that the parsing needed for item X is a strict superset of item > Y). > > So I'd much rather see us parse the format into a real tree of nodes, > and figure out (once) which properties of each object are required to > fulfill that. Then for each object, we grab those properties, and then > walk the tree to generate the output string. > > -Peff
