Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
On Thu, Jan 18, 2018 at 7:22 AM, Оля Тележнаяwrote: > 2018-01-18 2:04 GMT+03:00 Christian Couder : >> On Wed, Jan 17, 2018 at 10:49 PM, Jeff King wrote: >>> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote: >>> >> 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 think we have a little bit of chicken and egg there, though. I'm >>> having trouble reviewing the current work, because it's hard to evaluate >>> whether it's doing the right thing without seeing the end state. >> >> Yeah, to me it feels like you are at a middle point and there are many >> ways to go forward. > > OK. Maybe I misunderstood you and Jeff in our call, I thought that was > your idea to make a merge now, sorry. I will continue my work here. If you think you can now prepare a patch series that looks like it is going in the right direction, then yes you can do that. But after looking at the current patch series I agree with Peff that, as it doesn't look finished, it's difficult to tell if all the steps are good. In general it is a good idea to try to merge things as soon possible, but for that to be possible the state of the code that we want to be merged should look quite stable, and it doesn't look very stable to me.
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
2018-01-18 2:04 GMT+03:00 Christian Couder: > On Wed, Jan 17, 2018 at 10:49 PM, Jeff King wrote: >> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote: >> >>> >> 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 think we have a little bit of chicken and egg there, though. I'm >> having trouble reviewing the current work, because it's hard to evaluate >> whether it's doing the right thing without seeing the end state. > > Yeah, to me it feels like you are at a middle point and there are many > ways to go forward. OK. Maybe I misunderstood you and Jeff in our call, I thought that was your idea to make a merge now, sorry. I will continue my work here. > > As I wrote in another email though, I think it might be a good time to > consolidate new functionality by adding tests (and perhaps > documentation at the same time) for each new atom that is added to > ref-filter or cat-file. It will help you refactor the code and your > patch series later without breaking the new functionality. > >> So what >> I was suggesting in my earlier mails was that we actually _not_ try to >> merge this series, but use its components and ideas to build a new >> series that does things in a bit different order. > > Yeah, I think you will have to do that, but the tests that you can add > now for the new features will help you when you will build the new > series. > > And hopefully it will not be too much work to create this new series > as you will perhaps be able to just use the interactive rebase to > build it. > > I also don't think it's a big problem if the current patch series gets > quite long before you start creating a new series.
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
On Wed, Jan 17, 2018 at 10:49 PM, Jeff Kingwrote: > On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote: > >> >> 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 think we have a little bit of chicken and egg there, though. I'm > having trouble reviewing the current work, because it's hard to evaluate > whether it's doing the right thing without seeing the end state. Yeah, to me it feels like you are at a middle point and there are many ways to go forward. As I wrote in another email though, I think it might be a good time to consolidate new functionality by adding tests (and perhaps documentation at the same time) for each new atom that is added to ref-filter or cat-file. It will help you refactor the code and your patch series later without breaking the new functionality. > So what > I was suggesting in my earlier mails was that we actually _not_ try to > merge this series, but use its components and ideas to build a new > series that does things in a bit different order. Yeah, I think you will have to do that, but the tests that you can add now for the new features will help you when you will build the new series. And hopefully it will not be too much work to create this new series as you will perhaps be able to just use the interactive rebase to build it. I also don't think it's a big problem if the current patch series gets quite long before you start creating a new series.
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote: > >> 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 think we have a little bit of chicken and egg there, though. I'm having trouble reviewing the current work, because it's hard to evaluate whether it's doing the right thing without seeing the end state. So what I was suggesting in my earlier mails was that we actually _not_ try to merge this series, but use its components and ideas to build a new series that does things in a bit different order. Some of the patches here would end up thrown out, and some would get cherry-picked into the new series. And some would have some conflicts and need to get parts rewritten to fit into the new order. -Peff
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
2018-01-16 1:09 GMT+03:00 Jeff King: > 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).
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
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
Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions
On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote: > Split expand_atom function into 2 different functions, > expand_atom_into_fields prepares variable for further filling, > (new) expand_atom creates resulting string. > Need that for further reusing of formatting logic from ref-filter. This commit puzzled me, and I had to look ahead to try to figure out why we want this split (because on its face, the split is bad, since it duplicates the list). Later, the preparation step goes away, but we still are left with expand_atom(). That's because the preparation was all moved into ref-filter.c, where we rely on populate_value() to fill in the values, and then we pick them out with our own formats. 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). -Peff