Removing duplicated code between builtin-send-pack.c and transport.c

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

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*

On Wed, Apr 22, 2009 at 02:24:07PM -0400, Daniel Barkalow wrote:

On Wed, 22 Apr 2009, Andy Lester wrote:

quoted

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

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

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

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

On Wed, Apr 22, 2009 at 02:13:28PM -0500, Andy Lester wrote:

quoted

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