Re: [PATCH] Move try_merge_command and checkout_fast_forward to libgit.a

From: Jeff King <peff@peff.net>
Date: Oct 26, 2012, 12:02 UTC
Message-ID: <20121026120253.GA1455@sigill.intra.peff.net>
In-Reply-To: <CACsJy8D1_c9NP_nwvp6uJyKikG+_jqnm1hVEs_t_wnsYTLZ4Yg@mail.gmail.com>

On Thu, Oct 25, 2012 at 07:50:26PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 19 quoted lines

> On Thu, Oct 25, 2012 at 4:45 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Oct 23, 2012 at 09:24:51AM +0700, Nguyen Thai Ngoc Duy wrote:
> >
> >> These functions are called in sequencer.c, which is part of
> >> libgit.a. This makes libgit.a potentially require builtin/merge.c for
> >> external git commands.
> >>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >> ---
> >>  I made some unrelated changes in sequencer.c and this problem shown
> >>  up. merge-recursive.c is probably not the best place for these
> >>  functions. I just don't want to create merge.c for them.
> >
> > I'm fine with this conceptually, but merge-recursive really is the wrong
> > place. I'd much rather see a new merge.c to collect merge-related helper
> > functions that are not strategy-specific.
> 
> OK. I checked around for similar issues and found these used by
> libgit.a but stay in builtin/ instead:

Yeah, we have traditionally been kind of lazy about the distinction, because it doesn't really matter for our build system (i.e., libgit.a is not _really_ a library, but just a convenience in the build process). So one option is just not caring about these. I'm also fine with fixing.

> estimate_bisect_steps: bisect.c and builtin/rev-list.c
> print_commit_list: bisect.c and builtin/rev-list.c
> 
>  -> move them to bisect.c? another candidate is revision.c.

I'd probably say bisect.c for the first, and commit.c for the latter (that is where commit_list functions are defined, and it is really about that).

Show 6 quoted lines

> fetch_pack: transport.c and builtin/fetch-pack.c
> send_pack: transport.c and builtin/send-pack.c
> 
>  -> move them to transport.c? or new files fetch-pack.c and
> send-pack.c? I haven't check how many functions they may pull
> together.

I think I'd rather have fetch-pack.c and send-pack.c than putting them into transport.c (which is getting kind of bloated already).

> setup_diff_pager: diff-no-index.c and builtin/diff.c
> 
>  -> to diff-lib.c?

I'm not sure (to be honest, I am not even sure of the intended difference between diff.c and diff-lib.c). That function is really not for general diffing, but for diff-like commands. Probably it would be OK in diff.c.

-Peff