From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tanay Abhra Subject: Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string Date: Mon, 30 Jun 2014 20:09:18 +0530 Message-ID: <53B17696.3060808@gmail.com> References: <1403520105-23250-1-git-send-email-tanayabh@gmail.com> <1403520105-23250-4-git-send-email-tanayabh@gmail.com> <53ABD78E.3050800@gmail.com> <53B16748.8080703@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Git List , Ramkumar Ramachandra , Matthieu Moy To: Karsten Blees , Eric Sunshine X-From: git-owner@vger.kernel.org Mon Jun 30 16:39:38 2014 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 1X1ck0-0005Oa-Tv for gcvg-git-2@plane.gmane.org; Mon, 30 Jun 2014 16:39:33 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752438AbaF3Oj2 (ORCPT ); Mon, 30 Jun 2014 10:39:28 -0400 Received: from mail-pd0-f174.google.com ([209.85.192.174]:38433 "EHLO mail-pd0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751609AbaF3Oj1 (ORCPT ); Mon, 30 Jun 2014 10:39:27 -0400 Received: by mail-pd0-f174.google.com with SMTP id y10so8327786pdj.19 for ; Mon, 30 Jun 2014 07:39:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=RqjNvfoOYT78u8Sz2CLN6NIHxjB6cjFu4J1kuag34sU=; b=WfC+c3NfU5r9QowbKjjqGom+P7sZ7Ppp4f6JVi9dHgszepStH6LMpV2fUfrAnFuoJw aweygUPsp8tcZ/c0uwWUSF1YumX3C0TVFYd4lLICmlmLvMTpQufX2hbky/N8zuUTpbqf rtTARS3iGKVP8UTDnJ0keJRxW6m/TliKrXbNkhQPp5YvtspwwVdauqNBMAIqc+J3mbrh PXxsZUpCZK4jgtMg06obiyofZKFn7nvndrCgdA8OVXLsr+M6zmUMelFBNdi6a3VXTLi0 a/ykSNubkldb6yjt1Lm9SABHSeSqrSvzELYcElWvaVa73LpGQi38LHdAIRjg+sD/5KTA sJ6g== X-Received: by 10.66.139.233 with SMTP id rb9mr52028159pab.5.1404139166962; Mon, 30 Jun 2014 07:39:26 -0700 (PDT) Received: from [127.0.0.1] ([117.254.218.95]) by mx.google.com with ESMTPSA id av2sm28195903pbc.16.2014.06.30.07.39.22 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 30 Jun 2014 07:39:26 -0700 (PDT) User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 In-Reply-To: <53B16748.8080703@gmail.com> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On 6/30/2014 7:04 PM, Karsten Blees wrote: > Am 29.06.2014 13:01, schrieb Eric Sunshine: >> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra wrote: >>> On 6/25/2014 1:24 PM, Eric Sunshine wrote: >>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote: >>>>> Use git_config_get_string instead of git_config to take advantage of >>>>> the config hash-table api which provides a cleaner control flow. >>>>> >>>>> Signed-off-by: Tanay Abhra >>>>> --- >>>>> notes-utils.c | 31 +++++++++++++++---------------- >>>>> 1 file changed, 15 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/notes-utils.c b/notes-utils.c >>>>> index a0b1d7b..fdc9912 100644 >>>>> --- a/notes-utils.c >>>>> +++ b/notes-utils.c >>>>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char *v) >>>>> return NULL; >>>>> } >>>>> >>>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb) >>>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c) >>>>> { >>>>> - struct notes_rewrite_cfg *c = cb; >>>>> - if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) { >>>>> - c->enabled = git_config_bool(k, v); >>>>> - return 0; >>>>> - } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) { >>>>> + struct strbuf key = STRBUF_INIT; >>>>> + const char *v; >>>>> + strbuf_addf(&key, "notes.rewrite.%s", c->cmd); >>>>> + >>>>> + if (!git_config_get_string(key.buf, &v)) >>>>> + c->enabled = git_config_bool(key.buf, v); >>>>> + >>>>> + if (!c->mode_from_env && !git_config_get_string("notes.rewritemode", &v)) { >>>>> if (!v) >>>>> - return config_error_nonbool(k); >>>>> + config_error_nonbool("notes.rewritemode"); >>>> >>>> There's a behavior change here. In the original code, the callback >>>> function would return -1, which would cause the program to die() if >>>> the config.c:die_on_error flag was set. The new code merely emits an >>>> error. >>> >>> Is this change serious enough? Can I ignore it? > > IMO its better to Fail Fast than continue with some invalid config (which > may lead to more severe errors such as data corruption / data loss). Noted but, what I am trying to do with the rewrite is emit an error and not set the value if the value found is a NULL. The only change is that program will not crash in this case and warn the user not set a NULL value for a non boolean key. This won't lead to severe errors as the value will not be set if found value is a NULL. >> >> I don't know. Even within this single function there is no consistency >> about whether such problems should die() or just emit a message and >> continue. For instance: >> >> - if "notes.rewritemode" is bool, it die()s. >> >> - if "notes.rewritemode" doesn't specify a recognized mode, it >> error()s but continues >> > > I think this would also die in git_parse_source(): > ... > if (get_value(fn, data, var) < 0) > break; > } > if (cf->die_on_error) > die("bad config file line %d in %s", cf->linenr, cf->name); > ... > > (AFAICT, die_on_error is always true, except if invoked via 'git-config > --blob', which isn't used anywhere...) > Noted. > This, however, raises another issue: switching to the config cache looses > file/line-precise error reporting for semantic errors. I don't know if > this feature is important enough to do something about it, though. A > message of the form "Key 'xyz' is bad" should usually enable a user to > locate the problematic file and line. > Hmn, but during the config cache construction we parse key-value pairs through git_config() which still warns users about semantic errors. This happened yesterday only when I was writing tests for the new API. Thanks for the review. Cheers, Tanay Abhra.