From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: Re: [PATCH v2 4/4] teach config parsing to read from strbuf Date: Tue, 12 Mar 2013 07:18:06 -0400 Message-ID: <20130312111806.GF11340@sigill.intra.peff.net> References: <20130310165642.GA1136@sandbox-ub.fritz.box> <20130310170052.GE1136@sandbox-ub.fritz.box> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: Junio C Hamano , git@vger.kernel.org, Jens Lehmann , Ramsay Jones To: Heiko Voigt X-From: git-owner@vger.kernel.org Tue Mar 12 12:18:39 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1UFNE5-0004Pw-Qb for gcvg-git-2@plane.gmane.org; Tue, 12 Mar 2013 12:18:38 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754721Ab3CLLSK (ORCPT ); Tue, 12 Mar 2013 07:18:10 -0400 Received: from 75-15-5-89.uvs.iplsin.sbcglobal.net ([75.15.5.89]:49578 "EHLO peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752623Ab3CLLSJ (ORCPT ); Tue, 12 Mar 2013 07:18:09 -0400 Received: (qmail 26041 invoked by uid 107); 12 Mar 2013 11:19:49 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) (smtp-auth username relayok, mechanism cram-md5) by peff.net (qpsmtpd/0.84) with ESMTPA; Tue, 12 Mar 2013 07:19:49 -0400 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 12 Mar 2013 07:18:06 -0400 Content-Disposition: inline In-Reply-To: <20130310170052.GE1136@sandbox-ub.fritz.box> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote: > This can be used to read configuration values directly from gits > database. > > Signed-off-by: Heiko Voigt This is lacking motivation. IIRC, the rest of the story is something like "...so we can read .gitmodules directly from the repo" or something like that? > +struct config_strbuf { > + struct strbuf *strbuf; > + int pos; > +}; > > +static int config_strbuf_fgetc(struct config_source *conf) > +{ > + struct config_strbuf *str = conf->data; Yuck. If you used a union in the previous patch, then this could just go inline into the "struct config_source". > +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strbuf, void *data) Should this be a "const struct strbuf *strbuf"? For that matter, is there any reason not to take a bare pointer/len combination? It seems likely that callers would get the data from read_sha1_file, which means they have to stuff it into a strbuf for no good reason. > diff --git a/test-config.c b/test-config.c > new file mode 100644 > index 0000000..c650837 > --- /dev/null > +++ b/test-config.c > @@ -0,0 +1,40 @@ I'm slightly "meh" on this test-config program. Having to add a C test harness like this is a good indication that we are short-changing users of the shell API in favor of builtin C code. Your series does not actually add any callers of the new function. The obvious "patch 5/4" would be to plumb it into "git config --blob", and then we can just directly test it there (there could be other callers besides reading from a blob, of course, but I think the point of the series is to head in that direction). -Peff