Jeff King <p...@peff.net> writes: > On Thu, Jan 24, 2019 at 11:30:20AM -0800, Junio C Hamano wrote: > >> Jeff King <p...@peff.net> 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.
That sounds like a sensible longer-term strategy. Let's however leave it outside the scope of this change.
