Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

From: Jeff King <peff@peff.net>
Date: Jan 15, 2018, 22:09 UTC
Message-ID: <20180115220946.GF4778@sigill.intra.peff.net>
In-Reply-To: <20180115213335.GB4778@sigill.intra.peff.net>

On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:

Show 21 quoted lines

> 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