git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Heiko Voigt <hvoigt@hvoigt.net>
Cc: git@vger.kernel.org, Jens Lehmann <jens.lehmann@web.de>,
	Jonathan Nieder <jrnieder@gmail.com>, Jeff King <peff@peff.net>,
	"W. Trevor King" <wking@tremily.us>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Karsten Blees <karsten.blees@gmail.com>
Subject: Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
Date: Wed, 09 Jul 2014 15:04:50 -0700	[thread overview]
Message-ID: <xmqqegxu9ojh.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20140709195547.GA3081@sandbox-ub> (Heiko Voigt's message of "Wed, 9 Jul 2014 21:55:47 +0200")

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.

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

  reply	other threads:[~2014-07-09 22:05 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-28  9:58 [PATCH v2 0/4] submodule config lookup API Heiko Voigt
2014-06-28 10:01 ` [PATCH v2 1/4] implement submodule config cache for lookup of submodule names Heiko Voigt
2014-06-28 10:02 ` [PATCH v2 2/4] extract functions for submodule config set and lookup Heiko Voigt
2014-06-28 10:03 ` [PATCH v2 3/4] use new config API for worktree configurations of submodules Heiko Voigt
2014-07-08 20:14   ` Junio C Hamano
2014-07-09 19:55     ` Heiko Voigt
2014-07-09 22:04       ` Junio C Hamano [this message]
2014-07-14 20:57         ` Heiko Voigt
2014-07-15 22:37           ` Junio C Hamano
2014-07-17 19:53             ` Heiko Voigt
2014-07-17 20:08               ` Junio C Hamano
2014-06-28 10:04 ` [PATCH v2 4/4] do not die on error of parsing fetchrecursesubmodules option Heiko Voigt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqegxu9ojh.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=hvoigt@hvoigt.net \
    --cc=jens.lehmann@web.de \
    --cc=jrnieder@gmail.com \
    --cc=karsten.blees@gmail.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    --cc=wking@tremily.us \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).