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

[Jeff King <peff@peff.net>]

On Thu, Jan 24, 2019 at 11:30:20AM -0800, Junio C Hamano wrote:

> Jeff King <peff@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.

-Peff