Re: Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
On Tue, Jul 15, 2014 at 03:37:21PM -0700, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: Can there be any caller that include and use submodule-config.h that does not need anythjing from submodule.h? Or vice versa? It just did not look like these two headers describe independent subsystems but they almost always have to go hand-in-hand. And if that is the case, perhaps it is not such a good idea to add it as a new header. That was where my question came from. The reason for a separate module was because we add quite some lines of code for it. $ wc -l submodule.c 1068 submodule.c $ wc -l submodule-config.c 435 submodule-config.c Because of this I would like to keep the c-files separate. OK. I do not feel too strongly. It just looked odd that a change needs to add a new header file without having to change the code in existing files at all. Any other thing that still needs fixing in the series, or is it now ready for 'next'? All comments addressed. From my side it should be ready for next. Cheers Heiko -- 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: Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
On Thu, Jul 17, 2014 at 12:53 PM, Heiko Voigt hvo...@hvoigt.net wrote: On Tue, Jul 15, 2014 at 03:37:21PM -0700, Junio C Hamano wrote: Any other thing that still needs fixing in the series, or is it now ready for 'next'? All comments addressed. From my side it should be ready for next. Thanks; I am running today's integration cycle with this in 'next' now. -- 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 v2 3/4] use new config API for worktree configurations of submodules
Heiko Voigt hvo...@hvoigt.net writes: Can there be any caller that include and use submodule-config.h that does not need anythjing from submodule.h? Or vice versa? It just did not look like these two headers describe independent subsystems but they almost always have to go hand-in-hand. And if that is the case, perhaps it is not such a good idea to add it as a new header. That was where my question came from. The reason for a separate module was because we add quite some lines of code for it. $ wc -l submodule.c 1068 submodule.c $ wc -l submodule-config.c 435 submodule-config.c Because of this I would like to keep the c-files separate. OK. I do not feel too strongly. It just looked odd that a change needs to add a new header file without having to change the code in existing files at all. Any other thing that still needs fixing in the series, or is it now ready for 'next'? -- 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: Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
On Wed, Jul 09, 2014 at 03:04:50PM -0700, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: On Tue, Jul 08, 2014 at 01:14:20PM -0700, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..03ea20d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -18,6 +18,7 @@ #include xdiff-interface.h #include ll-merge.h #include resolve-undo.h +#include submodule-config.h #include submodule.h #include argv-array.h Hmph. What is this change about? Nobody in checkout.c needs anything new, yet we add a new include? This is because I moved the parse_submodule_config_option() function into the submodule-config.c module. This was necessary so all parsed submodule values are stored in the cache with the null_sha1. We use static functions from this module to do this and thats thats the reason for the move. diff --git a/diff.c b/diff.c index f72769a..f692a3c 100644 --- a/diff.c ... Likewise. Same as above. Can there be any caller that include and use submodule-config.h that does not need anythjing from submodule.h? Or vice versa? It just did not look like these two headers describe independent subsystems but they almost always have to go hand-in-hand. And if that is the case, perhaps it is not such a good idea to add it as a new header. That was where my question came from. The reason for a separate module was because we add quite some lines of code for it. $ wc -l submodule.c 1068 submodule.c $ wc -l submodule-config.c 435 submodule-config.c Because of this I would like to keep the c-files separate. The header simply came from that. It is possible to use them individually but it is very likely that we will always use both. At the moment all occurrences of submodule-config.h also have submodule.h. The other way around it is not that way. I like it to have a header per c-module since you directly know where to look for definitions. Also the config topic can be considered a separate subsystem for submodules. And for me it feels that way. If we want to make it easier for the user it probably makes sense to move the declarations into submodule.h But then how should we document it? As api-submodule.txt? I do not have a strong opinion either way. I like it in the current shape but if you convince me it makes more sense with one header, I would also be fine with it. Cheers Heiko -- 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: Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
On Tue, Jul 08, 2014 at 01:14:20PM -0700, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..03ea20d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -18,6 +18,7 @@ #include xdiff-interface.h #include ll-merge.h #include resolve-undo.h +#include submodule-config.h #include submodule.h #include argv-array.h Hmph. What is this change about? Nobody in checkout.c needs anything new, yet we add a new include? This is because I moved the parse_submodule_config_option() function into the submodule-config.c module. This was necessary so all parsed submodule values are stored in the cache with the null_sha1. We use static functions from this module to do this and thats thats the reason for the move. diff --git a/diff.c b/diff.c index f72769a..f692a3c 100644 --- a/diff.c +++ b/diff.c @@ -13,6 +13,7 @@ #include utf8.h #include userdiff.h #include sigchain.h +#include submodule-config.h #include submodule.h #include ll-merge.h #include string-list.h Likewise. Same as above. It is somewhat unclear to me what real change that improves the life of end-users this series brings to us. The test-submodule-config test program obviously is new but that does not really count until we see real uses. Do you mean the API improvements? I split this series off from my recursive fetch series since this infrastructure is also needed by Jens recursive checkout series. I am currently working on finishing the recursive fetch series here[1]. So until now we do not have any improvements for the end-user but only in the API for the developer. With my series it is possible to easily lookup what configuration for which submodule is in which revision. That makes is possible to also implement the recursive fetch logic for renamed submodules[2]. We are also able to decide whether (and from where) a new submodules repository can possibly be cloned during recursive fetch. A clone on recursive fetch for new submodule makes sure we have everything available, so a recursive checkout later can work without the need for an extra fetch. Does that make the improvements in my series clear for you? I would wait until my recursive fetch series is ready so we have real uses. Since there are others (namely Jens or submodule support for 'git archive') that need it I think it makes sense to review and merge this separately into master so they have a stable API to code against. Cheers Heiko [1] https://github.com/hvoigt/git/commits/hv/fetch-submodules-recursive [2] https://github.com/hvoigt/git/commit/975c370856c3b8f96ab0c5a3ed754e3839f4de45 -- 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 v2 3/4] use new config API for worktree configurations of submodules
Heiko Voigt hvo...@hvoigt.net writes: On Tue, Jul 08, 2014 at 01:14:20PM -0700, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..03ea20d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -18,6 +18,7 @@ #include xdiff-interface.h #include ll-merge.h #include resolve-undo.h +#include submodule-config.h #include submodule.h #include argv-array.h Hmph. What is this change about? Nobody in checkout.c needs anything new, yet we add a new include? This is because I moved the parse_submodule_config_option() function into the submodule-config.c module. This was necessary so all parsed submodule values are stored in the cache with the null_sha1. We use static functions from this module to do this and thats thats the reason for the move. diff --git a/diff.c b/diff.c index f72769a..f692a3c 100644 --- a/diff.c ... Likewise. Same as above. Can there be any caller that include and use submodule-config.h that does not need anythjing from submodule.h? Or vice versa? It just did not look like these two headers describe independent subsystems but they almost always have to go hand-in-hand. And if that is the case, perhaps it is not such a good idea to add it as a new header. That was where my question came from. Does that make the improvements in my series clear for you? I would wait until my recursive fetch series is ready so we have real uses. Since there are others (namely Jens or submodule support for 'git archive') that need it I think it makes sense to review and merge this separately into master so they have a stable API to code against. Sure. If we have sufficient amount of client code to judge the goodness of the API design against, there is no need to wait until all possible client code becomes ready. -- 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 v2 3/4] use new config API for worktree configurations of submodules
Heiko Voigt hvo...@hvoigt.net writes: diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..03ea20d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -18,6 +18,7 @@ #include xdiff-interface.h #include ll-merge.h #include resolve-undo.h +#include submodule-config.h #include submodule.h #include argv-array.h Hmph. What is this change about? Nobody in checkout.c needs anything new, yet we add a new include? diff --git a/diff.c b/diff.c index f72769a..f692a3c 100644 --- a/diff.c +++ b/diff.c @@ -13,6 +13,7 @@ #include utf8.h #include userdiff.h #include sigchain.h +#include submodule-config.h #include submodule.h #include ll-merge.h #include string-list.h Likewise. It is somewhat unclear to me what real change that improves the life of end-users this series brings to us. The test-submodule-config test program obviously is new but that does not really count until we see real uses. -- 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
