# Removing duplicated code between builtin-send-pack.c and transport.c 7 messages from 2009-04-22 to 2009-04-22. Participants: Andy Lester, Daniel Barkalow, Jeff King. Thread: https://gitlist.dev/t/19005 ## Andy Lester, 2009-04-22 17:01 Subject: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: <09511913-0ED3-41C0-A4F0-9F2D452C00D7@petdance.com> URL: https://gitlist.dev/e/09511913-0ED3-41C0-A4F0-9F2D452C00D7%40petdance.com ``` There's a ton of code duplicated between transport.c and builtin-send- pack.c, from print_push_status() and its static helpers. Is there a reason NOT to refactor it out of the builtin and use the transport? xoa -- Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance ``` ## Daniel Barkalow, 2009-04-22 18:24 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0904221407160.10753%40iabervon.org In-Reply-To: <09511913-0ED3-41C0-A4F0-9F2D452C00D7@petdance.com> ``` On Wed, 22 Apr 2009, Andy Lester wrote: > There's a ton of code duplicated between transport.c and builtin-send-pack.c, > from print_push_status() and its static helpers. > > Is there a reason NOT to refactor it out of the builtin and use the > transport? I think the builtin should actually just be deprecated and eventually removed. As far as I know, nothing actually runs "git send-pack" rather than calling send_pack(), but I left the builtin entry point, along with its helpers, just in case. If you're interested in reorganizing things there, I think it would be best to move send_pack() to a new send-pack.c, such that builtin-send-pack.c can go away entirely. -Daniel *This .sig left intentionally blank* ``` ## Jeff King, 2009-04-22 19:03 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: <20090422190337.GA13424@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090422190337.GA13424%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Apr 22, 2009 at 02:24:07PM -0400, Daniel Barkalow wrote: > On Wed, 22 Apr 2009, Andy Lester wrote: > > > There's a ton of code duplicated between transport.c and builtin-send-pack.c, > > from print_push_status() and its static helpers. > > > > Is there a reason NOT to refactor it out of the builtin and use the > > transport? > > I think the builtin should actually just be deprecated and eventually > removed. As far as I know, nothing actually runs "git send-pack" rather > than calling send_pack(), but I left the builtin entry point, along with > its helpers, just in case. > > If you're interested in reorganizing things there, I think it would be > best to move send_pack() to a new send-pack.c, such that > builtin-send-pack.c can go away entirely. I think there are actually three issues here: 1. send_pack() is a library-ish function used by transport.c (which in turn is called by push), and it is in builtin-send-pack.c. This is generally against git policy. 2. There are several static functions duplicated in transport.c and builtin-send-pack.c, which can be refactored to exist only once. In fact, I really don't see why your 64fcef2 didn't do that in the first place. It looks like they were cut and paste into transport.c; I don't see why you didn't just make them non-static and delete the original versions. 3. Nobody really uses "git send-pack" anymore, so it can perhaps be deprecated and eventually dropped. I think Andy was referring to (2), and I think that should be cleaned up, as the different versions have a tendency to diverge. Probably addressing (1) by moving send_pack() to transport.c makes sense as part of the same cleanup. I don't know that (3) really buys us much. Sure, it is probably useless, but we would need to keep it for historical compatibility for quite some time, anyway. -Peff ``` ## Andy Lester, 2009-04-22 19:06 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: URL: https://gitlist.dev/e/FF499E4E-B2F1-4795-B9F9-AD73CDDE417A%40petdance.com In-Reply-To: <20090422190337.GA13424@coredump.intra.peff.net> ``` On Apr 22, 2009, at 2:03 PM, Jeff King wrote: > I think Andy was referring to (2), and I think that should be cleaned > up, as the different versions have a tendency to diverge. Probably > addressing (1) by moving send_pack() to transport.c makes sense as > part > of the same cleanup. Yes, exactly. I was applying const to function parameters in builtin- send-pack.c, and discovered the duplication. I sure don't want to patch twice if we don't need to. So it sounds like what I'll do is start a send-pack.c and hoist out the common functions from builtin-send-pack.c and transport.c. xoxo, Andy -- Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance ``` ## Jeff King, 2009-04-22 19:10 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: <20090422191044.GC13424@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090422191044.GC13424%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Apr 22, 2009 at 02:06:22PM -0500, Andy Lester wrote: > Yes, exactly. I was applying const to function parameters in builtin- > send-pack.c, and discovered the duplication. I sure don't want to patch > twice if we don't need to. > > So it sounds like what I'll do is start a send-pack.c and hoist out the > common functions from builtin-send-pack.c and transport.c. I don't know if that is quite appropriate. I think the point of moving many of the duplicated functions into transport.c is that they are used by other transports, like http. So probably the transport-agnostic ones should stay in transport.c, and they should all get called in the same way, no matter what the transport. -Peff ``` ## Andy Lester, 2009-04-22 19:13 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: URL: https://gitlist.dev/e/ADA21A9C-96E3-4441-B925-5B623433F42E%40petdance.com In-Reply-To: <20090422191044.GC13424@coredump.intra.peff.net> ``` On Apr 22, 2009, at 2:10 PM, Jeff King wrote: > I don't know if that is quite appropriate. I think the point of moving > many of the duplicated functions into transport.c is that they are > used > by other transports, like http. So probably the transport-agnostic > ones > should stay in transport.c, and they should all get called in the same > way, no matter what the transport. My mistake. I'll remove the code from builtin-send-pack.c, improve it, and builtin-send-pack.c can get deprecated when/if anyone wants. -- Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance ``` ## Jeff King, 2009-04-22 19:14 Subject: Re: Removing duplicated code between builtin-send-pack.c and transport.c Message-ID: <20090422191439.GA13646@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090422191439.GA13646%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Apr 22, 2009 at 02:13:28PM -0500, Andy Lester wrote: >> I don't know if that is quite appropriate. I think the point of moving >> many of the duplicated functions into transport.c is that they are used >> by other transports, like http. So probably the transport-agnostic ones >> should stay in transport.c, and they should all get called in the same >> way, no matter what the transport. > > My mistake. I'll remove the code from builtin-send-pack.c, improve it, > and builtin-send-pack.c can get deprecated when/if anyone wants. That makes sense. Thanks. -Peff ```