From: Tanay Abhra <tanayabh@gmail.com>
To: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: git@vger.kernel.org, Ramkumar Ramachandra <artagnon@gmail.com>
Subject: Re: [PATCH v8 1/2] add `config_set` API for caching config-like files
Date: Fri, 11 Jul 2014 22:01:41 +0530 [thread overview]
Message-ID: <53C0116D.3010404@gmail.com> (raw)
In-Reply-To: <vpqlhs02cz7.fsf@anie.imag.fr>
On 7/11/2014 7:51 PM, Matthieu Moy wrote:
> Hi,
>
> I had a closer look at error management (once more, sorry: I should have
> done this earlier...), and it seems to me that:
>
> * Not all errors are managed properly
>
> * Most error cases are untested
>
> Among the cases I can think of:
>
> * Syntax error when parsing the file
>
> * Non-existant file
>
> * Unreadable file (chmod -r)
>
I had seen that there were checks for Syntax error or Non-existant files in
t1300-repo-config, for example,
# malformed configuration files
test_expect_success 'barf on syntax error' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key garbage
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'barf on incomplete section header' '
cat >.git/config <<-\EOF &&
# broken section line
[section
key = value
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 2 " error
'
test_expect_success 'barf on incomplete string' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key = "value string
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
test_must_fail git config --file non-existing-config -l
'
They cover the same parsing code which I used for constructing the cache.
Still, more tests will not harm anyone, I will add more testcases accordingly
for the corner cases you raised. :)
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
>> +
>> + Parses the file and adds the variable-value pairs to the `config_set`,
>> + dies if there is an error in parsing the file.
>
> The return value is undocumented.
>
> If I read correctly, the only codepath from this to a syntax error sets
> die_on_error, hence "dies if there is an error in parsing the file" is
> correct.
>
> Still, there are errors like "unreadable file" or "no such file" that do
> not die (nor emit any error message, which is not very good for the
> user), and lead to returning -1 here.
>
> I'm not sure this distinction is right (why die on syntax error and
> continue running on unreadable file?).
>
> In any case, it should be documented and tested. I'll send a fixup patch
> with a few more example tests (probably insufficient).
>
I am not sure of this myself, I will have to look into it.
>> +static int git_config_check_init(void)
>> +{
>> + int ret = 0;
>> + if (the_config_set.hash_initialized)
>> + return 0;
>> + configset_init(&the_config_set);
>> + ret = git_config(config_hash_callback, &the_config_set);
>> + if (ret >= 0)
>> + return 0;
>> + else {
>> + hashmap_free(&the_config_set.config_hash, 1);
>> + the_config_set.hash_initialized = 0;
>> + return -1;
>> + }
>> +}
>
> We have the same convention for errors here. But a more serious issue is
> that the return value of this function is ignored most of the time.
>
> It seems git_config should never return a negative value, as it calls
> git_config_with_options -> git_config_early, which checks for file
> existance and permission before calling git_config_from_file. Indeed,
> Git's tests still pass after this:
>
> --- a/config.c
> +++ b/config.c
> @@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
>
> int git_config(config_fn_t fn, void *data)
> {
> - return git_config_with_options(fn, data, NULL, 1);
> + int ret = git_config_with_options(fn, data, NULL, 1);
> + if (ret < 0)
> + die("Negative return value in git_config");
> + return ret;
> }
>
> Still, we can imagine cases like race condition between access_or_die()
> and git_config_from_file() in
>
> if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> ret += git_config_from_file(fn, git_etc_gitconfig(),
> data);
> found += 1;
> }
>
> where the function would indeed return -1. In any case, either we
> consider that git_config should never return -1, and we should die in
> this case, or we consider that it may happen, and that the "else" branch
> of the if/else above is not dead code, and then we can't just ignore the
> return value.
>
> I think we should just do something like this:
>
> diff --git a/config.c b/config.c
> index 74adbbd..5c023e8 100644
> --- a/config.c
> +++ b/config.c
> @@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
> return 1;
> }
>
> -static int git_config_check_init(void)
> +static void git_config_check_init(void)
> {
> int ret = 0;
> if (the_config_set.hash_initialized)
> @@ -1437,11 +1437,8 @@ static int git_config_check_init(void)
> ret = git_config(config_hash_callback, &the_config_set);
> if (ret >= 0)
> return 0;
> - else {
> - hashmap_free(&the_config_set.config_hash, 1);
> - the_config_set.hash_initialized = 0;
> - return -1;
> - }
> + else
> + die("Unknown error when parsing one of the configuration files.");
> }
>
> If not, a comment should explain what the "else" branch corresponds to,
> and why/when the return value can be safely ignored.
>
Yes, this is much better, I will make the necessary changes, thanks.
next prev parent reply other threads:[~2014-07-11 16:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-11 3:34 [PATCH v8 0/3] git config cache & special querying api utilizing the cache Tanay Abhra
2014-07-11 3:34 ` [PATCH v8 1/2] add `config_set` API for caching config-like files Tanay Abhra
2014-07-11 14:21 ` Matthieu Moy
2014-07-11 16:31 ` Tanay Abhra [this message]
2014-07-11 16:59 ` Matthieu Moy
2014-07-15 10:54 ` Tanay Abhra
2014-07-15 11:32 ` Matthieu Moy
2014-07-11 17:25 ` Junio C Hamano
2014-07-11 3:34 ` [PATCH v8 2/2] test-config: add tests for the config_set API Tanay Abhra
2014-07-11 14:24 ` Matthieu Moy
2014-07-11 14:27 ` [fixup PATCH 1/2] Call configset_clear Matthieu Moy
2014-07-11 14:27 ` [PATCH 2/2] Better tests for error cases Matthieu Moy
2014-07-11 18:18 ` [PATCH v8 2/2] test-config: add tests for the config_set API Junio C Hamano
2014-07-15 11:10 ` Tanay Abhra
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=53C0116D.3010404@gmail.com \
--to=tanayabh@gmail.com \
--cc=Matthieu.Moy@grenoble-inp.fr \
--cc=artagnon@gmail.com \
--cc=git@vger.kernel.org \
/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).