Re: [PATCH] Move try_merge_command and checkout_fast_forward to libgit.a
On Thu, Oct 25, 2012 at 07:50:26PM +0700, Nguyen Thai Ngoc Duy wrote: > On Thu, Oct 25, 2012 at 4:45 PM, Jeff King 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 > >> --- > >> 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). > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Move try_merge_command and checkout_fast_forward to libgit.a
On Thu, Oct 25, 2012 at 4:45 PM, Jeff King 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 >> --- >> 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: 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. 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. setup_diff_pager: diff-no-index.c and builtin/diff.c -> to diff-lib.c? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Move try_merge_command and checkout_fast_forward to libgit.a
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 > --- > 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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
