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

From: Jeff King <peff@peff.net>
Date: Apr 22, 2009, 19:03 UTC
Message-ID: <20090422190337.GA13424@coredump.intra.peff.net>
In-Reply-To: <alpine.LNX.1.00.0904221407160.10753@iabervon.org>

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

Show 16 quoted lines

> 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