2018-01-16 1:09 GMT+03:00 Jeff King <peff@xxxxxxxx>: > 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. I absolutely agree, I want to merge current edits and then continue migrating process. And hopefully second part of the function will also be removed. >> 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). Agree! I want to merge current edits, then create format.h file and make some renames, then finish migrating process to new format.h and support all new meaningful tags. > > 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 As I said, I am sure that I will continue working on that, so this is not the end state. So I am not sure that I am able to write finalizing messages for now. But, if we merge current edits, it will be much easier for me to continue working on that (next patch would be about creating format.h and I am afraid of some merge conflicts if I will develop absolutely all process from start to finish in my branch, it takes time. It's not a big problem, but, if we find final goal worthwhile, so maybe we could go to it step-by-step).
