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

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