{"thread":{"id":"19005","subject":"Removing duplicated code between builtin-send-pack.c and transport.c","startedAt":"2009-04-22T17:01:16Z","lastAt":"2009-04-22T19:14:39Z","messageCount":7,"participants":["Andy Lester","Daniel Barkalow","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"111966","messageId":"09511913-0ED3-41C0-A4F0-9F2D452C00D7@petdance.com","threadId":"19005","inReplyTo":null,"subject":"Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2009-04-22T17:01:16Z","receivedAt":"2009-04-22T17:01:16Z","isPatch":false,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"There's a ton of code duplicated between transport.c and builtin-send- \npack.c, from print_push_status() and its static helpers.\n\nIs there a reason NOT to refactor it out of the builtin and use the  \ntransport?\n\nxoa\n\n--\nAndy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance\n"},{"id":"111973","messageId":"alpine.LNX.1.00.0904221407160.10753@iabervon.org","threadId":"19005","inReplyTo":"09511913-0ED3-41C0-A4F0-9F2D452C00D7@petdance.com","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-04-22T18:24:07Z","receivedAt":"2009-04-22T18:24:07Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 22 Apr 2009, Andy Lester wrote:\n\n> There's a ton of code duplicated between transport.c and builtin-send-pack.c,\n> from print_push_status() and its static helpers.\n> \n> Is there a reason NOT to refactor it out of the builtin and use the \n> transport? \n\nI think the builtin should actually just be deprecated and eventually \nremoved. As far as I know, nothing actually runs \"git send-pack\" rather \nthan calling send_pack(), but I left the builtin entry point, along with \nits helpers, just in case.\n\nIf you're interested in reorganizing things there, I think it would be \nbest to move send_pack() to a new send-pack.c, such that \nbuiltin-send-pack.c can go away entirely.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"111977","messageId":"20090422190337.GA13424@coredump.intra.peff.net","threadId":"19005","inReplyTo":"alpine.LNX.1.00.0904221407160.10753@iabervon.org","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-22T19:03:37Z","receivedAt":"2009-04-22T19:03:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2009 at 02:24:07PM -0400, Daniel Barkalow wrote:\n\n> On Wed, 22 Apr 2009, Andy Lester wrote:\n> \n> > There's a ton of code duplicated between transport.c and builtin-send-pack.c,\n> > from print_push_status() and its static helpers.\n> > \n> > Is there a reason NOT to refactor it out of the builtin and use the \n> > transport? \n> \n> I think the builtin should actually just be deprecated and eventually \n> removed. As far as I know, nothing actually runs \"git send-pack\" rather \n> than calling send_pack(), but I left the builtin entry point, along with \n> its helpers, just in case.\n> \n> If you're interested in reorganizing things there, I think it would be \n> best to move send_pack() to a new send-pack.c, such that \n> builtin-send-pack.c can go away entirely.\n\nI think there are actually three issues here:\n\n  1. send_pack() is a library-ish function used by transport.c (which in\n     turn is called by push), and it is in builtin-send-pack.c. This is\n     generally against git policy.\n\n  2. There are several static functions duplicated in transport.c and\n     builtin-send-pack.c, which can be refactored to exist only once.\n     In fact, I really don't see why your 64fcef2 didn't do that in the\n     first place. It looks like they were cut and paste into\n     transport.c; I don't see why you didn't just make them non-static\n     and delete the original versions.\n\n  3. Nobody really uses \"git send-pack\" anymore, so it can perhaps be\n     deprecated and eventually dropped.\n\nI think Andy was referring to (2), and I think that should be cleaned\nup, as the different versions have a tendency to diverge. Probably\naddressing (1) by moving send_pack() to transport.c makes sense as part\nof the same cleanup.\n\nI don't know that (3) really buys us much. Sure, it is probably useless,\nbut we would need to keep it for historical compatibility for quite some\ntime, anyway.\n\n-Peff\n"},{"id":"111978","messageId":"FF499E4E-B2F1-4795-B9F9-AD73CDDE417A@petdance.com","threadId":"19005","inReplyTo":"20090422190337.GA13424@coredump.intra.peff.net","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2009-04-22T19:06:22Z","receivedAt":"2009-04-22T19:06:22Z","isPatch":false,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"\nOn Apr 22, 2009, at 2:03 PM, Jeff King wrote:\n\n> I think Andy was referring to (2), and I think that should be cleaned\n> up, as the different versions have a tendency to diverge. Probably\n> addressing (1) by moving send_pack() to transport.c makes sense as  \n> part\n> of the same cleanup.\n\n\nYes, exactly.  I was applying const to function parameters in builtin- \nsend-pack.c, and discovered the duplication.  I sure don't want to  \npatch twice if we don't need to.\n\nSo it sounds like what I'll do is start a send-pack.c and hoist out  \nthe common functions from builtin-send-pack.c and transport.c.\n\nxoxo,\nAndy\n\n--\nAndy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance\n"},{"id":"111980","messageId":"20090422191044.GC13424@coredump.intra.peff.net","threadId":"19005","inReplyTo":"FF499E4E-B2F1-4795-B9F9-AD73CDDE417A@petdance.com","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-22T19:10:45Z","receivedAt":"2009-04-22T19:10:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2009 at 02:06:22PM -0500, Andy Lester wrote:\n\n> Yes, exactly.  I was applying const to function parameters in builtin- \n> send-pack.c, and discovered the duplication.  I sure don't want to patch \n> twice if we don't need to.\n>\n> So it sounds like what I'll do is start a send-pack.c and hoist out the \n> common functions from builtin-send-pack.c and transport.c.\n\nI don't know if that is quite appropriate. I think the point of moving\nmany of the duplicated functions into transport.c is that they are used\nby other transports, like http. So probably the transport-agnostic ones\nshould stay in transport.c, and they should all get called in the same\nway, no matter what the transport.\n\n-Peff\n"},{"id":"111982","messageId":"ADA21A9C-96E3-4441-B925-5B623433F42E@petdance.com","threadId":"19005","inReplyTo":"20090422191044.GC13424@coredump.intra.peff.net","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2009-04-22T19:13:28Z","receivedAt":"2009-04-22T19:13:28Z","isPatch":false,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"\nOn Apr 22, 2009, at 2:10 PM, Jeff King wrote:\n\n> I don't know if that is quite appropriate. I think the point of moving\n> many of the duplicated functions into transport.c is that they are  \n> used\n> by other transports, like http. So probably the transport-agnostic  \n> ones\n> should stay in transport.c, and they should all get called in the same\n> way, no matter what the transport.\n\n\nMy mistake.  I'll remove the code from builtin-send-pack.c, improve  \nit, and builtin-send-pack.c can get deprecated when/if anyone wants.\n\n--\nAndy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance\n"},{"id":"111984","messageId":"20090422191439.GA13646@coredump.intra.peff.net","threadId":"19005","inReplyTo":"ADA21A9C-96E3-4441-B925-5B623433F42E@petdance.com","subject":"Re: Removing duplicated code between builtin-send-pack.c and transport.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-22T19:14:39Z","receivedAt":"2009-04-22T19:14:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2009 at 02:13:28PM -0500, Andy Lester wrote:\n\n>> I don't know if that is quite appropriate. I think the point of moving\n>> many of the duplicated functions into transport.c is that they are used\n>> by other transports, like http. So probably the transport-agnostic ones\n>> should stay in transport.c, and they should all get called in the same\n>> way, no matter what the transport.\n>\n> My mistake.  I'll remove the code from builtin-send-pack.c, improve it, \n> and builtin-send-pack.c can get deprecated when/if anyone wants.\n\nThat makes sense. Thanks.\n\n-Peff\n"}]}