Re: [PATCH] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: - There may be pieces of usefully reusable code buried in builtin/*.o; - By definition, any code (piece of data or function definition) in builtin/*.o cannot be used in standalone binaries, because all of builtin/*.o expect to link with git.o and expect their cmd_foo() getting called from main in it; - By moving the useful reusable pieces ont of builtin/*.o and adding them to libgit.a, these pieces become usable from standalone binaries as well. What if these reusable pieces should not be used by standalone binaries? I am not sure what you mean. A piece is either reusable or not. When would one piece _be_ reusable and should *not* be used in one context but not in another? There are distinctions between being useful and usable, but I think the zeroth order of approximation when thinking about this is to think builtin/*.o as set of subroutines called by git.c::main(). These set of subroutines may call out to more generic helper functions that are usable from anywhere both within builtins and also from standalone. They may also call to their own helper functions that were originally designed to support only their use by the original caller from somewhere in builtin/*.o (most commonly in the same file, marked as static). The general direction, if we want to have an improve libgit.a, should be to see if the functions and their data that are private to builtin/*.o can be used from standalone, either as they are or with more generalization, and turn them from helpers specific to one cmd_foo() into more generally useful library-ish functions. There may be pieces in the callchain from that entry point to cmd_foo() that are implementation details of git.c::main(); for example the loop that does command dispatching to check with builtins, external commands that begin with git-, and aliases, is one of them, and would not be usable (nor it is useful) outside the context of git aggregate binary. But there are things that ought to be usable that are currently in builtin/*.o, which prevents them from being used by standalone binaries. If a remote helper binary that is standalone wants to call create_note(), it is not sufficient to make it non-static in builtin/notes.o, for example. But if it is moved outside builtin/notes.o, it becomes usable. I think the git notes, being one of the most recent additions, haven't gone through enough round of refactoring to come to the best separation between library-ish part (i.e. could be in notes.o, even though it is mostly about the underlying data structure manipulation and contains no higher-level operations like actually creating and copying, which might want to be in a separate source notes-lib.o) and its CLI implementation builtin/notes.o. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? Again, I do not know what you mean by cannot here. My tentative answer to that question is the eventual goal should be not to have any code in that class, and that is a reasonable goal we can achieve once we refactor what ought to be reusable out of builtin/*.o. What are the examples you have in mind, code that we want to forbid standalone from using? -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 12:33 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: - There may be pieces of usefully reusable code buried in builtin/*.o; - By definition, any code (piece of data or function definition) in builtin/*.o cannot be used in standalone binaries, because all of builtin/*.o expect to link with git.o and expect their cmd_foo() getting called from main in it; - By moving the useful reusable pieces ont of builtin/*.o and adding them to libgit.a, these pieces become usable from standalone binaries as well. What if these reusable pieces should not be used by standalone binaries? I am not sure what you mean. A piece is either reusable or not. It can be reusable for A, but not for B. A being the 'git' binary, B being other standalone binaries. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? Again, I do not know what you mean by cannot here. My tentative answer to that question is the eventual goal should be not to have any code in that class, and that is a reasonable goal we can achieve once we refactor what ought to be reusable out of builtin/*.o. What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. Nothing would need that, if you are talking about the current codebase, I would agree that nothing would link to it. But that is not a good justification for closing door to others that come later who may want to have a standalone that would want to use it. Think about rewriting filter-branch.sh in C but not as a built-in, for example. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 12:58 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. Nothing would need that, if you are talking about the current codebase, I would agree that nothing would link to it. But that is not a good justification for closing door to others that come later who may want to have a standalone that would want to use it. Think about rewriting filter-branch.sh in C but not as a built-in, for example. Why would anybody rewrite filter-branch, and not make it a builtin? It should be a builtin. That's the whole point of builtins. Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Moreover, if you are going to argue that we shouldn't be closing the door [...] Felipe, you saying if you are going to argue ... to anybody else is kind of ironic. Why is it every thread I see you in, you're being a dick and arguing for some theoretical thing that nobody else cares about? This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. Linus -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, but you have not explained why it should be standalone, when it's clear it should be a builtin. If we assume your argument is valid for the hypothetical 'git-filter-branch', if that's the case, then it would be even more reasonable to assume that there will be other standalone binaries that would want to use all sort of functions from ./builtin/*.o. Let's put an example: reset_index(). Some standalone program wants to use that function. What do you we do? The shortest route is to make it non-static, and add it to builtin.h. But that would not be enough, we need the infrastructure prepared for that; link libgit.a with ./builtin/*.o. I don't think that's the way to go, but your line of argumentation leads directly there; if we are worrying about anything that any potential standalone program could want, then we should worry about reset_index() not being easily accessible to them. IMO we should be clear and say no; standalone programs should not access copy_notes_for_rewrite(), that's for builtins. If we move all the code that potential standalone programs could want to libgit.a, it wouldn't be a library at all, and it would basically contain everything. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 1:14 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Moreover, if you are going to argue that we shouldn't be closing the door [...] Felipe, you saying if you are going to argue ... to anybody else is kind of ironic. Supposing the other side's argument is correct is a standard discussing technique. Why is it every thread I see you in, you're being a dick and arguing for some theoretical thing that nobody else cares about? I don't know. I've sent 800 patches in the last three months (patches, not email comments), and you pick this one to reply to. Maybe because you enjoy insulting people? This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. Why don't you ask Junio to let it go? If it's irrelevant, than it doesn't matter if this patch is applied or not. You say it's bike-shedding, that implies that Junio likes red, and I like blue, and both are equally useless. So let's go for blue then. Presumably Junio doesn't agree with you, he does truly think it should be red, in fact, he doesn't think it's just a color, it's something important, and I agree, and apparently other people in the mailing list also agree, as most of them have voiced their opinion that red is the color. Now, do you have something of value to say which of the two options should be, or are you just going to engage in double standards and personal attacks? If you truly think this is bikeshedding, at least be fair and complain about that to the people that argue for red, not just the ones that argue for blue. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. ... That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. And there is no justification behind that _should_ claim; you are not making any technical argument to explain it. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, Of course it has to be hypothetical; I already said with the current code no standalone does use it---it is not arranged to be doable so there is no user. If you want to have examples of future possible callers, they have to be hypothetical---the future by definition hasn't happened. But that does not mean hypothetical is impractical nor useless. There are out-of-tree programs like cgit that will not be built-in but already link with libgit.a. Moving things that can be used by outside people out of builtin/*.o to libgit.a would allow uses that you and I cannot imagine offhand. I do not see a reason for us to forbid a filter-branch replacement out of tree as a standalone. I do not see a point in continuing to discuss this (or any design level issues) with you. You seem to go into a wrong direction to break the design of the overall system, not in a direction to improve anything. I do not know, and at this point I do not care, if you are doing so deliberately to sabotage Git. Just stop. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 2:24 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. ... That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. And there is no justification behind that _should_ claim; you are not making any technical argument to explain it. The first argument of init_copy_notes_for_rewrite() is the name of the builtin command. There hardly could be any more justification. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, Of course it has to be hypothetical; I already said with the current code no standalone does use it---it is not arranged to be doable so there is no user. If you want to have examples of future possible callers, they have to be hypothetical---the future by definition hasn't happened. But that does not mean hypothetical is impractical nor useless. So? It's still hypothetical, which is what I said. What are you complaining about? About the fact that I made a correct assessment? There are out-of-tree programs like cgit that will not be built-in but already link with libgit.a. Moving things that can be used by outside people out of builtin/*.o to libgit.a would allow uses that you and I cannot imagine offhand. I do not see a reason for us to forbid a filter-branch replacement out of tree as a standalone. Yeah, I already ran that argument, and you conveniently chose to escape the next logical conclusion that I already put forward: --- a/Makefile +++ b/Makefile @@ -990,6 +990,8 @@ BUILTIN_OBJS += builtin/verify-pack.o BUILTIN_OBJS += builtin/verify-tag.o BUILTIN_OBJS += builtin/write-tree.o +LIB_OBJS += $(BUILTIN_OBJS) + GITLIBS = $(LIB_FILE) $(XDIFF_LIB) EXTLIBS = I don't think that's the right direction. I do not see a point in continuing to discuss this (or any design level issues) with you. You seem to go into a wrong direction to break the design of the overall system, not in a direction to improve anything. I do not know, and at this point I do not care, if you are doing so deliberately to sabotage Git. Just stop. That's your opinion, and it's not shared by others (outside of the Git project). If you were right and Git was moving in the right direction, there would be no need for libgit2. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Linus Torvalds torva...@linux-foundation.org writes: This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. The proposal to move sequencer.c to builtins/sequencer.c and then adding a filter in Makefile to exclude so that git-sequencer is not built is it wouldn't improve anything one way or the other. It is to throw in something into a set to which it does not belong, and then working around that mistake with another kludge. The problem that triggered the wrong solution actually is real, however. A function that sequencer.c (in libgit.a so that it could be used by standalone) may want to use in the future currently lives in builtin/notes.c. If you add a call to that function to sequencer.c without doing anything else, standalones like git-upload-pack will stop linking correctly. The git-upload-pack wants the revision traversal machinery in revision.o, which in turn wants to be able to see log-tree.o, which in turn wants to link with sequencer.o to see one global variable (there may be other dependencies). All of these objects are currently in libgit.a so that both builtins and standalones can use them. Moving sequencer.c to builtin/ is not even a solution. Linking git-upload-pack will still pull in builtin/notes.o along with cmd_notes(), which is not called from main(); as you remember, cmd_foo() in all builtin/*.o are designed to be called from git.c::main(). There is only one right solution. If a useful function is buried in builtin/*.o as a historical accident (i.e. it started its life as a helper for that particular command, and nobody else used it from outside so far) and that makes it impossible to use the function from outside builtin/*.o, refactor the function and its callers and move it to libgit.a. So I do not think this is not even a bikeshedding. Just one side being right, and the other side continuing to repeat nonsense without listening. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 2:59 PM, Junio C Hamano gits...@pobox.com wrote: Linus Torvalds torva...@linux-foundation.org writes: This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. The proposal to move sequencer.c to builtins/sequencer.c and then adding a filter in Makefile to exclude so that git-sequencer is not built is it wouldn't improve anything one way or the other. It is to throw in something into a set to which it does not belong, In your opinion. and then working around that mistake with another kludge. In your opinion. You continually use absolutist rhetoric to try to convince yourself and others that what you say is absolute 100% fact. But it's not, it's your opinion. So I do not think this is not even a bikeshedding. Just one side being right, and the other side continuing to repeat nonsense without listening. George W. Bush said history would prove him right, but saying so doesn't make it so. At least he had the decency to acknowledge that other people had different valid opinions. builtin/lib.a makes perfect sense, and it's the first logical step in moving libgit.a towards libgit2. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 07:30:31PM +0200, Vincent van Ravesteijn wrote: I think that libgit.a should contain all code to be able to carry out all functions of git. The stuff in builtin/ is just a command-line user interface. Whether or not sequencer should be in builtin depends on whether the sequencer is only part of this command-line user interface. One code organization issue I have not seen mentioned is that there is more CLI than what is in builtin, and libgit.a does more than simply provide code for the sources in builtin/. There are also external commands shipped in git.git that are not linked against git.c or the other builtins. Once upon a time, all commands were that way, and that was the origin of libgit.a: the set of common code used by all of the C commands in git.git. Over time, those commands became builtins (mostly to keep the size of the libexec dir down). These days there are only a handful of external commands left, mostly ones that have startup time overhead from the dynamic loader (e.g., remote-curl, http-push, imap-send). That is what libgit.a _is_ now. I do not mean to imply any additional judgement on what it could be. But if the goal is to make libgit.a functions that programs outside git.git would want, and nothing else, we may want to additionally split out a libgit-internal.a consisting of code that is used by multiple externals in git, but which would not be appropriate for clients to use. For example, I think most of http.c is in that boat, as it is full of wrappers for curl code that are of enough quality to reuse within git, but a little too half-baked to be part of a stable API. We can always link directly against http.o, too, of course. The point of putting the files into a static library is that it makes the link faster, and if there are only a handful of such links, it may not be worth the effort. -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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 4:45 PM, Jeff King p...@peff.net wrote: That is what libgit.a _is_ now. I do not mean to imply any additional judgement on what it could be. But if the goal is to make libgit.a functions that programs outside git.git would want, and nothing else, we may want to additionally split out a libgit-internal.a consisting of code that is used by multiple externals in git, but which would not be appropriate for clients to use. That might make sense, but that still doesn't clarify what belongs in ./*.o, and what belongs in ./builtin/*.o. And right now that creates a mess where you have code shared between ./builtin/*.o that is defined in cache.h (overlay_tree_on_cache), and some in builtin.h (init_copy_notes_for_rewrite). And it's not clear what should be done when code in ./*.o needs to access functionality in ./builtin/*.o, specially if that code is only useful for git builtins, and nothing else. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote: On Mon, Jun 10, 2013 at 4:45 PM, Jeff King p...@peff.net wrote: That is what libgit.a _is_ now. I do not mean to imply any additional judgement on what it could be. But if the goal is to make libgit.a functions that programs outside git.git would want, and nothing else, we may want to additionally split out a libgit-internal.a consisting of code that is used by multiple externals in git, but which would not be appropriate for clients to use. That might make sense, but that still doesn't clarify what belongs in ./*.o, and what belongs in ./builtin/*.o. And right now that creates a mess where you have code shared between ./builtin/*.o that is defined in cache.h (overlay_tree_on_cache), and some in builtin.h (init_copy_notes_for_rewrite). And it's not clear what should be done when code in ./*.o needs to access functionality in ./builtin/*.o, specially if that code is only useful for git builtins, and nothing else. My general impression of the goal of our current code organization is: 1. builtin/*.c should each contain a single builtin command and its supporting static functions. Each file gets linked into git.o to make the main git executable. 2. ./*.c is one of: a. Shared code usable by externals and builtins, which gets linked into libgit.a b. An external command itself, with its own main(). It gets linked against libgit.a. 3. Functions in libgit.a should be defined in a header file specific to their module (e.g., refs.h). cache.h picks up the slack for things that are general, or too small to get their own header file, or otherwise don't group well. I said it was a goal, because I know that we do not follow that in many places, so it is certainly easy to find counter-examples (and nor do I think it cannot be changed; I am just trying to describe the current rationale). Under that organization, there is no place for code that does not go into libgit.a, but is not a builtin command in itself. There was never a need in the past, because libgit.a was a bit of a dumping ground for linkable functions, and nobody cared that it had everything and the kitchen sink. If we want to start caring, then we probably need to create a separate kitchen sink-like library, with the rule that things in libgit.a cannot depend on it. In other words, a support library for Git's commands, for the parts that are not appropriate to expose as part of a library API. -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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 5:06 PM, Jeff King p...@peff.net wrote: On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote: On Mon, Jun 10, 2013 at 4:45 PM, Jeff King p...@peff.net wrote: That is what libgit.a _is_ now. I do not mean to imply any additional judgement on what it could be. But if the goal is to make libgit.a functions that programs outside git.git would want, and nothing else, we may want to additionally split out a libgit-internal.a consisting of code that is used by multiple externals in git, but which would not be appropriate for clients to use. That might make sense, but that still doesn't clarify what belongs in ./*.o, and what belongs in ./builtin/*.o. And right now that creates a mess where you have code shared between ./builtin/*.o that is defined in cache.h (overlay_tree_on_cache), and some in builtin.h (init_copy_notes_for_rewrite). And it's not clear what should be done when code in ./*.o needs to access functionality in ./builtin/*.o, specially if that code is only useful for git builtins, and nothing else. My general impression of the goal of our current code organization is: 1. builtin/*.c should each contain a single builtin command and its supporting static functions. Each file gets linked into git.o to make the main git executable. We already know this is not the case. Maybe this should be fixed by moving all the shared code between builtins to libgit.a, but maybe we already know at some level this is not wise, and that's why we haven't done so. If we want to start caring, then we probably need to create a separate kitchen sink-like library, with the rule that things in libgit.a cannot depend on it. In other words, a support library for Git's commands, for the parts that are not appropriate to expose as part of a library API. Yes, that's clearly what we should be doing, which is precisely what my patch that creates builtin/lib.a does. So we have two options: a) Do what we clearly should do; create builtin/lib.a, and move code there that is specific to builtin commands. b) Do what we think we have been doing; and move _all_ shared code to libgit.a (which shouldn't be called libgit, because it's not really a library), and cleanup builtin/*.c so they don't share anything among themselves directly. I vote for a), not only because we already know that's what we _should_ do, but because we are basically already there. Leaving things as they are is not really an option; that's a mess. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Jeff King p...@peff.net writes: My general impression of the goal of our current code organization is: 1. builtin/*.c should each contain a single builtin command and its supporting static functions. Each file gets linked into git.o to make the main git executable. Correct; that is what we aimed for when we made builtin-*.c (later moved to builtin/*.c). Some builtin/*.c files can contain more than one cmd_foo implementations, so single is not a solid rule, and it does not have to be, because all of them are expected to be linked into the main binary together with git.c to be called from main(). And as you hinted, if some global data or functions in it turns out to be useful for standalone binaries, their definitions must migrate out of buitlin/*.c to ./*.c files, because standalone binaries with their own main() definition cannot be linked with builtin/*.o, the latter of which requires to be linked with git.o with its own main(). 2. ./*.c is one of: a. Shared code usable by externals and builtins, which gets linked into libgit.a b. An external command itself, with its own main(). It gets linked against libgit.a. 3. Functions in libgit.a should be defined in a header file specific to their module (e.g., refs.h). cache.h picks up the slack for things that are general, or too small to get their own header file, or otherwise don't group well. I said it was a goal, because I know that we do not follow that in many places, so it is certainly easy to find counter-examples (and nor do I think it cannot be changed; I am just trying to describe the current rationale). Under that organization, there is no place for code that does not go into libgit.a, but is not a builtin command in itself. There was never a need in the past, because libgit.a was a bit of a dumping ground for linkable functions, and nobody cared that it had everything and the kitchen sink. The rationale behind libgit.a was so that make targets for the standalone binaries (note: all of them were standalone in the beginning) do not have to list *.o files that each of them needs to be linked with. It was primary done as a convenient way to have the linker figure out the dependency and link only what was needed. -- 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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 6:41 PM, Junio C Hamano gits...@pobox.com wrote: For the particular case of trying to make sequencer.o, which does not currently have dependencies on builtin/*.o, depend on something that is in builtin/notes.o, the link phase of standalone that wants anything from revision.o (which is pretty much everything ;-) goes like this: upload-pack.c wants handle_revision_opt etc. revision.c provides handle_revision_opt wants name_decoration etc. log-tree.c provides name_decoration wants append_signoff sequencer.c provides append_signoff So sequencer.o _is_ meant to be usable from standalone and belongs to libgit.a Not after my patch. It moves append_signoff *out* of sequencer, which in fact has nothing to do with the sequencer in the first place. If sequencer.o wants to call init_copy_notes_for_rewrite() and its friends [*1*] that are currently in builtin/notes.o, first the called function(s) should be moved outside builtin/notes.o to notes.o or somewhere more library-ish place to be included in libgit.a, which is meant to be usable from standalone. [Footnote] *1* ... which is a very reasonable thing to do. But moving sequencer.o to builtin/sequencer.o is *not* the way to do this. By now we all know what is the *CURRENT* way to do this; in other words, the status quo, which is BTW all messed up, because builtin/*.o objects depend on each other already. We are discussing the way it *SHOULD* be. Why aren't you leaning on that? -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: *1* ... which is a very reasonable thing to do. But moving sequencer.o to builtin/sequencer.o is *not* the way to do this. By now we all know what is the *CURRENT* way to do this; in other words, the status quo, which is BTW all messed up, because builtin/*.o objects depend on each other already. builtin/*.o are allowed to depend on each other. They are by definition builtins, meant to be linked into a single binary. We are discussing the way it *SHOULD* be. Why aren't you leaning on that? And I do not see the reason why builtin/*.o should not depend on each other. It is not messed up at all. They are meant to be linked into a single binary---that is what being built-in is. A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o by moving parts that do not have to be in the single git binary but are also usable in standalone binaries out of them. And that is what I just suggested. -- 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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 7:04 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: *1* ... which is a very reasonable thing to do. But moving sequencer.o to builtin/sequencer.o is *not* the way to do this. By now we all know what is the *CURRENT* way to do this; in other words, the status quo, which is BTW all messed up, because builtin/*.o objects depend on each other already. builtin/*.o are allowed to depend on each other. They are by definition builtins, meant to be linked into a single binary. That's not what John Keeping said[1]. I'm going to assume he was wrong, but I don't think that's relevant for my point. Either way, the meaning of builtin/ should probably be explained somewhere. We are discussing the way it *SHOULD* be. Why aren't you leaning on that? And I do not see the reason why builtin/*.o should not depend on each other. It is not messed up at all. They are meant to be linked into a single binary---that is what being built-in is. A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o by moving parts that do not have to be in the single git binary but are also usable in standalone binaries out of them. And that is what I just suggested. But init_copy_notes_for_rewrite() can *not* be used by anything other than git builtins. Standalone binaries will never use such a function, therefore it doesn't belong in libgit.a. Another example is alias_lookup(). They belong in builtin/lib.a. [1] http://article.gmane.org/gmane.comp.version-control.git/227017 -- Felipe Contreras -- 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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 8:53 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: And I do not see the reason why builtin/*.o should not depend on each other. It is not messed up at all. They are meant to be linked into a single binary---that is what being built-in is. A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o by moving parts that do not have to be in the single git binary but are also usable in standalone binaries out of them. Actually, as long as these pieces are currently used by builtins, moving them (e.g. init_copy_notes_for_rewrite()) out of builtin/*.o will not make these parts not to be in the single git binary at all, so the above is grossly misstated. - There may be pieces of usefully reusable code buried in builtin/*.o; - By definition, any code (piece of data or function definition) in builtin/*.o cannot be used in standalone binaries, because all of builtin/*.o expect to link with git.o and expect their cmd_foo() getting called from main in it; - By moving the useful reusable pieces ont of builtin/*.o and adding them to libgit.a, these pieces become usable from standalone binaries as well. What if these reusable pieces should not be used by standalone binaries? And that is the reason why slimming builtin/*.o is the way it *SHOULD* be. Another thing to think about is looking at pieces of data and functions defined in each *.o files and moving things around within them. For example, looking at the dependency chain I quoted earlier for sequencer.o to build upload-pack, which is about responding to git fetch on the sending side: upload-pack.c wants handle_revision_opt etc. revision.c provides handle_revision_opt wants name_decoration etc. log-tree.c provides name_decoration wants append_signoff sequencer.c provides append_signoff It is already crazy. There is no reason for the pack sender to be linking with the sequencer interpreter machinery. If the function definition (and possibly other ones) are split into separate source files (still in libgit.a), git-upload-pack binary does not have to pull in the whole sequencer.c at all. Agreed, which is precisely why my patches move that code out of sequencer.c. Maybe log-tree.c is not the right destination, but it is a step in the right direction. Coming back to the categorization Peff earlier made in the thread, I think I am OK with adding new two subdirectories to the root level, i.e. builtin/- the ones that define cmd_foo() As is the case right now. commands/ - the ones that has main() for standalone commands Good. libsrc/ - the ones that go in libgit.a lib/ is probably descriptive enough. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? I'd wager it belongs to builtin/ and should be linked to builtin/lib.a. Maybe you would like to have a separate builtin/lib/ directory, but I think that's overkill. We may also want to add another subdirectory to hold scripted Porcelains, but the primary topic of this thread is what to do about the C library, so it is orthogonal in that sense, but if we were to go in the group things in subdirectories to slim the root level direction, it may be worth considering doing so at the same time. Agreed. Plus there's completions, shell prompt, and other script-like tools that shouldn't really belong in contrib/, and probably installed by default. -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
--
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] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
--
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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
--
Felipe Contreras
--
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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 9:56 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
I already explained this multiple times; code from ./*.o can't access
code from ./builtin/*.o.
--
Felipe Contreras
--
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] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
How is it only useful for builtin commands? By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).
But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features. Copying notes around in the notes tree is
generally useful so why shouldn't it be in notes.c with the other note
manipulation functions? The current API may not be completely suitable
but that doesn't mean that it cannot be extracted into notes.c. In
fact, other than the commit message saying Notes added by 'git notes
copy' I don't see what's wrong with the current functions being
extracted as-is.
--
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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 11:02 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
How is it only useful for builtin commands? By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).
Which is precisely the point of this patch. If everything is for
builtin commands, then we don't have a git library, and git.a should
contain everything under builtin/*.o.
But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features.
No, we don't. Everything under ./*.o goes to libgit.a, and everything
under ./builtin/*.o goes to 'git'. So builtin/commit.o can access code
from builtin/notes.o, but sequencer.o can't.
--
Felipe Contreras
--
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] build: get rid of the notion of a git library
John Keeping wrote: How is it only useful for builtin commands? By that logic everything belongs in builtin/ because it's all only used by builtin commands (I realise that is what you're arguing towards). sequencer.c is merely a common API for builtins to invoke continuations: i.e. stop the program persisting enough state to let to user continue, allow the user to do whatever conflict resolutions using whatever tools, allow the user to continue the original operation. sequencer.c provides a uniform UI (--continue|--skip|--abort), and a uniform way to persist state (.git/sequencer/todo). It mainly abstracts out the boring details of reading/writing the todo lines. Currently, sequencer.c has no callers other than those in builtin/revert.c. In its current shape, it's incapable of being used by anything else: while an external ruby script (possibly a rebase implementation) could call into the sequencer to persist state, I don't think it is going to happen anytime soon. We might get a proper public api sequencer sometime in the distant future, but don't confuse that with the current shape of sequencer.c. But we make a distinction between things that are specific to one command (especially argument parsing and user interaction) and more generally useful features. Copying notes around in the notes tree is generally useful so why shouldn't it be in notes.c with the other note manipulation functions? The current API may not be completely suitable but that doesn't mean that it cannot be extracted into notes.c. In fact, other than the commit message saying Notes added by 'git notes copy' I don't see what's wrong with the current functions being extracted as-is. Sure, notes could have a better public api and so could a lot of other things: worktree operations like reset and checkout come to mind. The problem is that we seem to be at some frozen in some sort of stalemate, and some reorganization is definitely required. What would you suggest? -- 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] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 11:22:06AM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 11:02 AM, John Keeping j...@keeping.me.uk wrote: But we make a distinction between things that are specific to one command (especially argument parsing and user interaction) and more generally useful features. No, we don't. Everything under ./*.o goes to libgit.a, and everything under ./builtin/*.o goes to 'git'. So builtin/commit.o can access code from builtin/notes.o, but sequencer.o can't. I would argue that it was a mistake not to extract these functions from builtin/notes.c to notes.c when builtin/commit.c started using them. Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. -- 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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra artag...@gmail.com wrote: John Keeping wrote: Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. So libgit.a is a collection of everything that is shared between builtins? Does that correspond to reality? $ ls *.h | sed 's/.h$/.c/' | xargs file An example violation: builtin/log.c uses functions defined in builtin/shortlog.c. What is the point of all this separation, if no external scripts are ever going to use libgit.a? And all the functions should be static, which doesn't seem to be the case: 03c0 T add_files_to_cache 0530 T interactive_add 0410 T run_add_interactive 1920 T textconv_object 05b0 T fmt_merge_msg 0090 T fmt_merge_msg_config 0c00 T init_db 0b40 T set_git_dir_init 0360 T overlay_tree_on_cache 0500 T report_path_error 11a0 T copy_note_for_rewrite 1210 T finish_copy_notes_for_rewrite 1060 T init_copy_notes_for_rewrite T prune_packed_objects 0510 T shortlog_add_commit 06b0 T shortlog_init 0780 T shortlog_output T stripspace -- Felipe Contreras -- 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] build: get rid of the notion of a git library
Ramkumar Ramachandra wrote: So libgit.a is a collection of everything that is shared between builtins? That is not to say that we shouldn't share things between builtins. We can do it in builtin/lib.a, as Felipe has demonstrated here [1]. [1]: http://article.gmane.org/gmane.comp.version-control.git/226975 -- 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] build: get rid of the notion of a git library
Op 9-6-2013 17:40, Felipe Contreras schreef:
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
I think that libgit.a should contain all code to be able to carry out
all functions of git. The stuff in builtin/ is just a command-line user
interface. Whether or not sequencer should be in builtin depends on
whether the sequencer is only part of this command-line user interface.
I think that the sequencer code is at the moment unusable if you do not
use the code in builtin/ so that would advocate to move it into
builtin/. If sequencer is in libgit, and I write my own (graphical) user
interface, I expect to be able to use it.
Vincent
--
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] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra artag...@gmail.com wrote: John Keeping wrote: Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. So libgit.a is a collection of everything that is shared between builtins? Does that correspond to reality? I think that's *precisely* what libgit.a is. It doesn't currently correspond exactly to reality, but that's mostly for historic reasons (see below). $ ls *.h | sed 's/.h$/.c/' | xargs file An example violation: builtin/log.c uses functions defined in builtin/shortlog.c. What is the point of all this separation, if no external scripts are ever going to use libgit.a? Why do we structure code in a certain way at all? The reason libgit.a was introduced (according to commit 0a02ce7) is: This introduces the concept of git library objects that the real programs use, and makes it easier to add such things to a libgit.a. And all the functions should be static, which doesn't seem to be the case: 03c0 T add_files_to_cache 0530 T interactive_add 0410 T run_add_interactive 1920 T textconv_object 05b0 T fmt_merge_msg 0090 T fmt_merge_msg_config 0c00 T init_db 0b40 T set_git_dir_init 0360 T overlay_tree_on_cache 0500 T report_path_error 11a0 T copy_note_for_rewrite 1210 T finish_copy_notes_for_rewrite 1060 T init_copy_notes_for_rewrite T prune_packed_objects 0510 T shortlog_add_commit 06b0 T shortlog_init 0780 T shortlog_output T stripspace A quick check with git log -S... shows that most of these have barely been touched since the builtin/ directory was created. So the reason they're not static is most likely because no one has tidied them up since the division between builtins was introduced. It is a fact of life that as we live and work with a system we realise that there may be a better way of doing something. This doesn't mean that someone needs to immediately convert everything to the new way, it is often sufficient to do new things in the new way and slowly move existing things across as and when they are touched for other reasons. -- 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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 12:30 PM, Vincent van Ravesteijn v...@lyx.org wrote:
Op 9-6-2013 17:40, Felipe Contreras schreef:
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
I think that libgit.a should contain all code to be able to carry out all
functions of git. The stuff in builtin/ is just a command-line user
interface. Whether or not sequencer should be in builtin depends on whether
the sequencer is only part of this command-line user interface.
The sequencer is only part of the command-line user interface.
I think that the sequencer code is at the moment unusable if you do not use
the code in builtin/ so that would advocate to move it into builtin/. If
sequencer is in libgit, and I write my own (graphical) user interface, I
expect to be able to use it.
As do I, but it appears all other Git developers disagree; libgit.a is
not really a library, and will never be used by anything other than
Git's core.
Hence this patch.
--
Felipe Contreras
--
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] build: get rid of the notion of a git library
On Sun, Jun 9, 2013 at 12:32 PM, John Keeping j...@keeping.me.uk wrote: On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra artag...@gmail.com wrote: John Keeping wrote: Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. So libgit.a is a collection of everything that is shared between builtins? Does that correspond to reality? I think that's *precisely* what libgit.a is. We don't care what libgit.a is; the important thing is what it *should* be. A quick check with git log -S... shows that most of these have barely been touched since the builtin/ directory was created. So the reason they're not static is most likely because no one has tidied them up since the division between builtins was introduced. It is a fact of life that as we live and work with a system we realise that there may be a better way of doing something. This doesn't mean that someone needs to immediately convert everything to the new way, it is often sufficient to do new things in the new way and slowly move existing things across as and when they are touched for other reasons. Really? builtin/ls-files.c:307:13: error: static declaration of ‘overlay_tree_on_cache’ follows non-static declaration static void overlay_tree_on_cache(const char *tree_name, const char *prefix) ^ In file included from builtin/ls-files.c:8:0: ./cache.h:1318:6: note: previous declaration of ‘overlay_tree_on_cache’ was here void overlay_tree_on_cache(const char *tree_name, const char *prefix); ^ builtin/ls-files.c:361:12: error: static declaration of ‘report_path_error’ follows non-static declaration static int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix) ^ In file included from builtin/ls-files.c:8:0: ./cache.h:1317:5: note: previous declaration of ‘report_path_error’ was here int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix); ^ make: *** [builtin/ls-files.o] Error 1 make: *** Waiting for unfinished jobs builtin/add.c:184:12: error: static declaration of ‘add_files_to_cache’ follows non-static declaration static int add_files_to_cache(const char *prefix, const char **pathspec, int flags) ^ In file included from builtin/add.c:6:0: ./cache.h:1283:5: note: previous declaration of ‘add_files_to_cache’ was here int add_files_to_cache(const char *prefix, const char **pathspec, int flags); ^ builtin/add.c:280:12: error: static declaration of ‘run_add_interactive’ follows non-static declaration static int run_add_interactive(const char *revision, const char *patch_mode, ^ In file included from ./builtin.h:7:0, from builtin/add.c:7: ./commit.h:187:12: note: previous declaration of ‘run_add_interactive’ was here extern int run_add_interactive(const char *revision, const char *patch_mode, ^ builtin/add.c:309:12: error: static declaration of ‘interactive_add’ follows non-static declaration static int interactive_add(int argc, const char **argv, const char *prefix, int patch) ^ In file included from ./builtin.h:7:0, from builtin/add.c:7: ./commit.h:186:12: note: previous declaration of ‘interactive_add’ was here extern int interactive_add(int argc, const char **argv, const char *prefix, int patch); ^ builtin/add.c:184:12: warning: ‘add_files_to_cache’ defined but not used [-Wunused-function] static int add_files_to_cache(const char *prefix, const char **pathspec, int flags) ^ make: *** [builtin/add.o] Error 1 builtin/fmt-merge-msg.c:19:12: error: static declaration of ‘fmt_merge_msg_config’ follows non-static declaration static int fmt_merge_msg_config(const char *key, const char *value, void *cb) ^ In file included from builtin/fmt-merge-msg.c:9:0: ./fmt-merge-msg.h:5:12: note: previous declaration of ‘fmt_merge_msg_config’ was here extern int fmt_merge_msg_config(const char *key, const char *value, void *cb); ^ builtin/fmt-merge-msg.c:592:12: error: static declaration of ‘fmt_merge_msg’ follows non-static declaration static int fmt_merge_msg(struct strbuf *in, struct strbuf *out, ^ In file included from builtin/fmt-merge-msg.c:1:0: ./builtin.h:26:12: note: previous declaration of ‘fmt_merge_msg’ was here extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out, ^ make: *** [builtin/fmt-merge-msg.o] Error 1 builtin/init-db.c:316:12: error: static declaration of ‘set_git_dir_init’ follows non-static declaration static int set_git_dir_init(const char *git_dir, const char *real_git_dir, ^ In file included from builtin/init-db.c:6:0: ./cache.h:421:12: note: previous declaration of ‘set_git_dir_init’ was here extern int set_git_dir_init(const
[PATCH] build: get rid of the notion of a git library
There's no libgit, and there will never be, every object file in Git is the same, and there's wish to organize them in any way; they are *all* for the 'git' binary and its builtin commands. So let's shatter any hopes of ever having a library, and be clear about it; both the top-level objects (./*.o) and the builtin objects (./builtin/*.o) go into git.a, which is not a library, merely a convenient way to stash objects together. This way there will not be linking issues when top-level objects try to access functions of builtin objects. LIB_OBJS and LIB_H imply a library, but there isn't one, and never will be; so give them proper names; just a bunch of headers and objects. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- Makefile | 564 --- 1 file changed, 283 insertions(+), 281 deletions(-) diff --git a/Makefile b/Makefile index 03524d0..63451b1 100644 --- a/Makefile +++ b/Makefile @@ -435,8 +435,8 @@ XDIFF_OBJS = VCSSVN_OBJS = GENERATED_H = EXTRA_CPPFLAGS = -LIB_H = -LIB_OBJS = +HEADERS = +OBJS = PROGRAM_OBJS = PROGRAMS = SCRIPT_PERL = @@ -629,270 +629,270 @@ endif export PERL_PATH export PYTHON_PATH -LIB_FILE = libgit.a +GIT_LIB = git.a XDIFF_LIB = xdiff/lib.a VCSSVN_LIB = vcs-svn/lib.a GENERATED_H += common-cmds.h -LIB_H += advice.h -LIB_H += archive.h -LIB_H += argv-array.h -LIB_H += attr.h -LIB_H += bisect.h -LIB_H += blob.h -LIB_H += branch.h -LIB_H += builtin.h -LIB_H += bulk-checkin.h -LIB_H += bundle.h -LIB_H += cache-tree.h -LIB_H += cache.h -LIB_H += color.h -LIB_H += column.h -LIB_H += commit.h -LIB_H += compat/bswap.h -LIB_H += compat/cygwin.h -LIB_H += compat/mingw.h -LIB_H += compat/obstack.h -LIB_H += compat/poll/poll.h -LIB_H += compat/precompose_utf8.h -LIB_H += compat/terminal.h -LIB_H += compat/win32/dirent.h -LIB_H += compat/win32/pthread.h -LIB_H += compat/win32/syslog.h -LIB_H += connected.h -LIB_H += convert.h -LIB_H += credential.h -LIB_H += csum-file.h -LIB_H += decorate.h -LIB_H += delta.h -LIB_H += diff.h -LIB_H += diffcore.h -LIB_H += dir.h -LIB_H += exec_cmd.h -LIB_H += fetch-pack.h -LIB_H += fmt-merge-msg.h -LIB_H += fsck.h -LIB_H += gettext.h -LIB_H += git-compat-util.h -LIB_H += gpg-interface.h -LIB_H += graph.h -LIB_H += grep.h -LIB_H += hash.h -LIB_H += help.h -LIB_H += http.h -LIB_H += kwset.h -LIB_H += levenshtein.h -LIB_H += line-log.h -LIB_H += line-range.h -LIB_H += list-objects.h -LIB_H += ll-merge.h -LIB_H += log-tree.h -LIB_H += mailmap.h -LIB_H += merge-blobs.h -LIB_H += merge-recursive.h -LIB_H += mergesort.h -LIB_H += notes-cache.h -LIB_H += notes-merge.h -LIB_H += notes.h -LIB_H += object.h -LIB_H += pack-revindex.h -LIB_H += pack.h -LIB_H += parse-options.h -LIB_H += patch-ids.h -LIB_H += pathspec.h -LIB_H += pkt-line.h -LIB_H += progress.h -LIB_H += prompt.h -LIB_H += quote.h -LIB_H += reachable.h -LIB_H += reflog-walk.h -LIB_H += refs.h -LIB_H += remote.h -LIB_H += rerere.h -LIB_H += resolve-undo.h -LIB_H += revision.h -LIB_H += run-command.h -LIB_H += send-pack.h -LIB_H += sequencer.h -LIB_H += sha1-array.h -LIB_H += sha1-lookup.h -LIB_H += shortlog.h -LIB_H += sideband.h -LIB_H += sigchain.h -LIB_H += strbuf.h -LIB_H += streaming.h -LIB_H += string-list.h -LIB_H += submodule.h -LIB_H += tag.h -LIB_H += tar.h -LIB_H += thread-utils.h -LIB_H += transport.h -LIB_H += tree-walk.h -LIB_H += tree.h -LIB_H += unpack-trees.h -LIB_H += url.h -LIB_H += userdiff.h -LIB_H += utf8.h -LIB_H += varint.h -LIB_H += vcs-svn/fast_export.h -LIB_H += vcs-svn/line_buffer.h -LIB_H += vcs-svn/repo_tree.h -LIB_H += vcs-svn/sliding_window.h -LIB_H += vcs-svn/svndiff.h -LIB_H += vcs-svn/svndump.h -LIB_H += walker.h -LIB_H += wildmatch.h -LIB_H += wt-status.h -LIB_H += xdiff-interface.h -LIB_H += xdiff/xdiff.h -LIB_H += xdiff/xdiffi.h -LIB_H += xdiff/xemit.h -LIB_H += xdiff/xinclude.h -LIB_H += xdiff/xmacros.h -LIB_H += xdiff/xprepare.h -LIB_H += xdiff/xtypes.h -LIB_H += xdiff/xutils.h - -LIB_OBJS += abspath.o -LIB_OBJS += advice.o -LIB_OBJS += alias.o -LIB_OBJS += alloc.o -LIB_OBJS += archive.o -LIB_OBJS += archive-tar.o -LIB_OBJS += archive-zip.o -LIB_OBJS += argv-array.o -LIB_OBJS += attr.o -LIB_OBJS += base85.o -LIB_OBJS += bisect.o -LIB_OBJS += blob.o -LIB_OBJS += branch.o -LIB_OBJS += bulk-checkin.o -LIB_OBJS += bundle.o -LIB_OBJS += cache-tree.o -LIB_OBJS += color.o -LIB_OBJS += column.o -LIB_OBJS += combine-diff.o -LIB_OBJS += commit.o -LIB_OBJS += compat/obstack.o -LIB_OBJS += compat/terminal.o -LIB_OBJS += config.o -LIB_OBJS += connect.o -LIB_OBJS += connected.o -LIB_OBJS += convert.o -LIB_OBJS += copy.o -LIB_OBJS += credential.o -LIB_OBJS += csum-file.o -LIB_OBJS += ctype.o -LIB_OBJS += date.o -LIB_OBJS += decorate.o -LIB_OBJS += diffcore-break.o -LIB_OBJS += diffcore-delta.o -LIB_OBJS += diffcore-order.o -LIB_OBJS += diffcore-pickaxe.o -LIB_OBJS += diffcore-rename.o -LIB_OBJS += diff-delta.o -LIB_OBJS += diff-lib.o -LIB_OBJS += diff-no-index.o -LIB_OBJS += diff.o -LIB_OBJS +=
Re: [PATCH] build: get rid of the notion of a git library
Felipe Contreras wrote: There's no libgit, and there will never be, every object file in Git is the same, and there's wish to organize them in any way; they are *all* for the 'git' binary and its builtin commands. Nice joke patch to illustrate your point ;) On a more serious note, please be a little more patient while everyone copes with what you're attempting. I've already made it clear that I'm in favor of moving forward with your plan to lib'ify git. The problem is that you're sending your changes in fragmented comments and diffs, and nobody is able to piece together what the big picture is. Please write one cogent email (preferably with code included) explaining your plan. -- 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] build: get rid of the notion of a git library
On Sat, Jun 8, 2013 at 1:02 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
Felipe Contreras wrote:
There's no libgit, and there will never be, every object file in Git is
the same, and there's wish to organize them in any way; they are *all*
for the 'git' binary and its builtin commands.
Nice joke patch to illustrate your point ;)
It's not a joke. This is seriously the direction the others say is the
correct one.
One direction or the other, the problem that top-level objects can't
access code from builtin objects must be fixed. And if the others
don't want to fix it by properly splitting code between library-like
objects, and builtin objects, there's only one other way to fix it;
this way.
On a more serious note, please be a little more patient while everyone
copes with what you're attempting.
I don't think patience will help. What do you suggest? Wait until the
problem fixes itself? (I'll be waiting until the end times). Wait
until somebody changed their opinion by themselves? (I don't see that
happening).
I've already made it clear that
I'm in favor of moving forward with your plan to lib'ify git.
Unfortunately you are the only one.
The
problem is that you're sending your changes in fragmented comments and
diffs, and nobody is able to piece together what the big picture is.
Please write one cogent email (preferably with code included)
explaining your plan.
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
But to be honest, I don't really care, all I want is the problem of
the bogus split to be solved. One way to solve it is going the proper
library way, but the other is to stash everything together into git.a.
Both ways solve the problem.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
@@ -14,6 +14,7 @@
#include merge-recursive.h
#include refs.h
#include argv-array.h
+#include builtin.h
#define GIT_REFLOG_ACTION GIT_REFLOG_ACTION
@@ -102,6 +103,17 @@ static int has_conforming_footer(struct strbuf
*sb, struct strbuf *sob,
return 1;
}
+static void copy_notes(const char *name)
+{
+ struct notes_rewrite_cfg *cfg;
+
+ cfg = init_copy_notes_for_rewrite(name);
+ if (!cfg)
+ return;
+
+ finish_copy_notes_for_rewrite(cfg);
+}
+
static void remove_sequencer_state(void)
{
struct strbuf seq_dir = STRBUF_INIT;
@@ -997,6 +1009,8 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)
return res;
}
+ copy_notes(cherry-pick);
+
/*
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
What happens?
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
It is not the first time, nor the last that top-level code needs
builtin code, and the solution is easy; organize the code. Alas, this
simple solution reject on the basis that we shouldn't organize the
code, because the code is not meant to be organized.
Cheers.
--
Felipe Contreras
--
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
