Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2014, 22:04 UTC
Message-ID: <xmqqegxu9ojh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140709195547.GA3081@sandbox-ub>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 33 quoted lines

> On Tue, Jul 08, 2014 at 01:14:20PM -0700, Junio C Hamano wrote:
>> Heiko Voigt <hvoigt@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.

Show 5 quoted lines

> 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.