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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Jul 14, 2014, 20:57 UTC
Message-ID: <20140714205759.GA3682@sandbox-ub>
In-Reply-To: <xmqqegxu9ojh.fsf@gitster.dls.corp.google.com>

On Wed, Jul 09, 2014 at 03:04:50PM -0700, Junio C Hamano wrote:

Show 43 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > 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.

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