From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 31 Jan 2019 21:42:14 GMT
Subject: Re: [PATCH v6 1/3] ref-filter: add worktreepath atom
Message-ID: <xmqqef8s1p3d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190124212608.GD16114@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> 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.

That sounds like a sensible longer-term strategy.  Let's however
leave it outside the scope of this change.