On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:
> That works, but I don't think it's where we want to end up in the long
> run. Because:
>
> 1. We still have the set of formats duplicated between expand_atom()
> and the "preparation" step. It's just that the preparation is now
> in ref-filter.c. What happens if ref-filter.c learns new formatting
> placeholders (or options for those placeholders) that cat-file.c
> doesn't, or vice versa? The two have to be kept in sync.
>
> 2. We're missing out on all of the other placeholders that ref-filter
> knows about. Not all of them are meaningful (e.g., %(refname)
> wouldn't make sense here), but part of our goal is to support the
> same set of placeholders as much as possible. Some obvious ones
> that ought to work for cat-file: %(objectname:short), %(if), and
> things like %(subject) when the appropriate object type is used.
>
> In other words, I think the endgame is that expand_atom() isn't there at
> all, and we're calling the equivalent of format_ref_item() for each
> object (except that in a unified formatting world, it probably doesn't
> have the word "ref" in it, since that's just one of the items a caller
> might pass in).
I read carefully up through about patch 6, and then skimmed through the
rest. I think we really want to push this in the direction of more
unification, as above. I know that the patches here may be a step on the
way, but I had a hard time evaluating each patch to see if it was
leading us in the right direction.
I think what would help for reviewing this is:
1. Start with a cover letter that makes it clear what the end state of
the series is, and why that's the right place to end up.
2. Include a rough roadmap of the patches in the cover letter.
Hopefully they should group into a few obvious steps (like "in
patches 1-5, we're teaching ref-filter to support everything that
cat-file can do, then in 6-10 we're preparing cat-file for the
conversion, and then in 11 we do the conversion"). If it doesn't
form a coherent narrative, then it may be worth stepping back and
thinking about combining or reordering the patches in a different
way, so that the progression becomes more plain.
I think one of the things that makes the progression here hard to
understand (for me, anyway) is that it's "inside out" of what I'd
expect. There's a lot of code movement happening first, and then
refactoring and simplifying after that. So between those two steps,
there's a lot of interim ugliness (e.g., having to reach across
module boundaries to look at expand_data). It's hard to tell when
looking at each individual patch how necessary the ugliness is, and
whether and when it's going to go away later in the series.
There's a lot of good work here, and you've figured out a lot about how
the two systems function. I think we just need to rearrange things a bit
to make sure each step is moving in the right direction.
-Peff
