threads / discuss / 19005
Subject: Removing duplicated code between builtin-send-pack.c and transport.c
Andy LesterThere'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 BarkalowRe: Removing duplicated code between builtin-send-pack.c and transport.c
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 KingRe: Removing duplicated code between builtin-send-pack.c and transport.c
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 LesterRe: Removing duplicated code between builtin-send-pack.c and transport.c
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 KingRe: Removing duplicated code between builtin-send-pack.c and transport.c
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 LesterRe: Removing duplicated code between builtin-send-pack.c and transport.c
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 KingRe: Removing duplicated code between builtin-send-pack.c and transport.c
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