* [PATCH 1/3] parse_config_key: use skip_prefix instead of starts_with
2017-02-24 21:06 ` Jeff King
@ 2017-02-24 21:07 ` Jeff King
2017-02-24 21:08 ` [PATCH 2/3] parse_config_key: allow matching single-level config Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2017-02-24 21:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git
This saves us having to repeatedly add in "section_len" (and
also avoids walking over the first part of the string
multiple times for a strlen() and strrchr()).
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index c6b874a7b..1b08a75a7 100644
--- a/config.c
+++ b/config.c
@@ -2536,11 +2536,10 @@ int parse_config_key(const char *var,
const char **subsection, int *subsection_len,
const char **key)
{
- int section_len = strlen(section);
const char *dot;
/* Does it start with "section." ? */
- if (!starts_with(var, section) || var[section_len] != '.')
+ if (!skip_prefix(var, section, &var) || *var != '.')
return -1;
/*
@@ -2552,12 +2551,12 @@ int parse_config_key(const char *var,
*key = dot + 1;
/* Did we have a subsection at all? */
- if (dot == var + section_len) {
+ if (dot == var) {
*subsection = NULL;
*subsection_len = 0;
}
else {
- *subsection = var + section_len + 1;
+ *subsection = var + 1;
*subsection_len = dot - *subsection;
}
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] parse_config_key: allow matching single-level config
2017-02-24 21:06 ` Jeff King
2017-02-24 21:07 ` [PATCH 1/3] parse_config_key: use skip_prefix instead of starts_with Jeff King
@ 2017-02-24 21:08 ` Jeff King
2017-02-24 21:20 ` Junio C Hamano
2017-02-24 21:08 ` [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection Jeff King
2017-02-24 21:18 ` [PATCH] refs: parse_hide_refs_config to use parse_config_key Junio C Hamano
3 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2017-02-24 21:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git
The parse_config_key() function was introduced to make it
easier to match "section.subsection.key" variables. It also
handles the simpler "section.key", and the caller is
responsible for distinguishing the two from its
out-parameters.
Most callers who _only_ want "section.key" would just use a
strcmp(var, "section.key"), since there is no parsing
required. However, they may still use parse_config_key() if
their "section" variable isn't a constant (an example of
this is in parse_hide_refs_config).
Using the parse_config_key is a bit clunky, though:
const char *subsection;
int subsection_len;
const char *key;
if (!parse_config_key(var, section, &subsection, &subsection_len, &key) &&
!subsection) {
/* matched! */
}
Instead, let's treat a NULL subsection as an indication that
the caller does not expect one. That lets us write:
const char *key;
if (!parse_config_key(var, section, NULL, NULL, &key)) {
/* matched! */
}
Existing callers should be unaffected, as passing a NULL
subsection would currently segfault.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 5 ++++-
config.c | 8 ++++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 61fc86e6d..647a78f3f 100644
--- a/cache.h
+++ b/cache.h
@@ -1819,8 +1819,11 @@ extern int git_config_include(const char *name, const char *value, void *data);
*
* (i.e., what gets handed to a config_fn_t). The caller provides the section;
* we return -1 if it does not match, 0 otherwise. The subsection and key
- * out-parameters are filled by the function (and subsection is NULL if it is
+ * out-parameters are filled by the function (and *subsection is NULL if it is
* missing).
+ *
+ * If the subsection pointer-to-pointer passed in is NULL, returns 0 only if
+ * there is no subsection at all.
*/
extern int parse_config_key(const char *var,
const char *section,
diff --git a/config.c b/config.c
index 1b08a75a7..13c8b21ea 100644
--- a/config.c
+++ b/config.c
@@ -2552,10 +2552,14 @@ int parse_config_key(const char *var,
/* Did we have a subsection at all? */
if (dot == var) {
- *subsection = NULL;
- *subsection_len = 0;
+ if (subsection) {
+ *subsection = NULL;
+ *subsection_len = 0;
+ }
}
else {
+ if (!subsection)
+ return -1;
*subsection = var + 1;
*subsection_len = dot - *subsection;
}
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
2017-02-24 21:08 ` [PATCH 2/3] parse_config_key: allow matching single-level config Jeff King
@ 2017-02-24 21:20 ` Junio C Hamano
2017-02-24 21:25 ` Junio C Hamano
2017-02-24 21:26 ` Jeff King
0 siblings, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-02-24 21:20 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
Jeff King <peff@peff.net> writes:
> The parse_config_key() function was introduced to make it
> easier to match "section.subsection.key" variables. It also
> handles the simpler "section.key", and the caller is
> responsible for distinguishing the two from its
> out-parameters.
>
> Most callers who _only_ want "section.key" would just use a
> strcmp(var, "section.key"), since there is no parsing
> required. However, they may still use parse_config_key() if
> their "section" variable isn't a constant (an example of
> this is in parse_hide_refs_config).
Perhaps "only" at the end of the title?
After grepping for call sites of this function, I think we can
simplify quite a few instances of:
if (parse_config_key(...) || !name)
return ...;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
2017-02-24 21:20 ` Junio C Hamano
@ 2017-02-24 21:25 ` Junio C Hamano
2017-02-24 21:26 ` Jeff King
1 sibling, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-02-24 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> The parse_config_key() function was introduced to make it
>> easier to match "section.subsection.key" variables. It also
>> handles the simpler "section.key", and the caller is
>> responsible for distinguishing the two from its
>> out-parameters.
>>
>> Most callers who _only_ want "section.key" would just use a
>> strcmp(var, "section.key"), since there is no parsing
>> required. However, they may still use parse_config_key() if
>> their "section" variable isn't a constant (an example of
>> this is in parse_hide_refs_config).
>
> Perhaps "only" at the end of the title?
which still stands. My initial reaction was "eh, I didn't know it
was an error to call the function for two-level names".
> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
>
> if (parse_config_key(...) || !name)
> return ...;
This is false. Sorry for the noise.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
2017-02-24 21:20 ` Junio C Hamano
2017-02-24 21:25 ` Junio C Hamano
@ 2017-02-24 21:26 ` Jeff King
1 sibling, 0 replies; 16+ messages in thread
From: Jeff King @ 2017-02-24 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git
On Fri, Feb 24, 2017 at 01:20:48PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The parse_config_key() function was introduced to make it
> > easier to match "section.subsection.key" variables. It also
> > handles the simpler "section.key", and the caller is
> > responsible for distinguishing the two from its
> > out-parameters.
> >
> > Most callers who _only_ want "section.key" would just use a
> > strcmp(var, "section.key"), since there is no parsing
> > required. However, they may still use parse_config_key() if
> > their "section" variable isn't a constant (an example of
> > this is in parse_hide_refs_config).
>
> Perhaps "only" at the end of the title?
Yeah, that would be an improvement.
> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
>
> if (parse_config_key(...) || !name)
> return ...;
I think you figured this out from your other response, but no, those are
the opposite case (it tricked me at first, too).
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection
2017-02-24 21:06 ` Jeff King
2017-02-24 21:07 ` [PATCH 1/3] parse_config_key: use skip_prefix instead of starts_with Jeff King
2017-02-24 21:08 ` [PATCH 2/3] parse_config_key: allow matching single-level config Jeff King
@ 2017-02-24 21:08 ` Jeff King
2017-02-24 23:28 ` Stefan Beller
2017-02-24 21:18 ` [PATCH] refs: parse_hide_refs_config to use parse_config_key Junio C Hamano
3 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2017-02-24 21:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git
This lets us avoid declaring some otherwise useless
variables.
Signed-off-by: Jeff King <peff@peff.net>
---
refs.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 21bc8c910..b9188908b 100644
--- a/refs.c
+++ b/refs.c
@@ -1034,11 +1034,10 @@ static struct string_list *hide_refs;
int parse_hide_refs_config(const char *var, const char *value, const char *section)
{
- const char *subsection, *key;
- int subsection_len;
+ const char *key;
if (!strcmp("transfer.hiderefs", var) ||
- (!parse_config_key(var, section, &subsection, &subsection_len, &key)
- && !subsection && !strcmp(key, "hiderefs"))) {
+ (!parse_config_key(var, section, NULL, NULL, &key) &&
+ !strcmp(key, "hiderefs"))) {
char *ref;
int len;
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection
2017-02-24 21:08 ` [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection Jeff King
@ 2017-02-24 23:28 ` Stefan Beller
0 siblings, 0 replies; 16+ messages in thread
From: Stefan Beller @ 2017-02-24 23:28 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
On Fri, Feb 24, 2017 at 1:08 PM, Jeff King <peff@peff.net> wrote:
> + (!parse_config_key(var, section, NULL, NULL, &key) &&
> + !strcmp(key, "hiderefs"))) {
Heh, see how fast my code gets replaced with even better code? ;)
All three patches look good,
Thanks,
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
2017-02-24 21:06 ` Jeff King
` (2 preceding siblings ...)
2017-02-24 21:08 ` [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection Jeff King
@ 2017-02-24 21:18 ` Junio C Hamano
2017-02-24 21:20 ` Jeff King
3 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-02-24 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
Jeff King <peff@peff.net> writes:
> On Fri, Feb 24, 2017 at 03:39:40PM -0500, Jeff King wrote:
>
>> This will start parsing "receive.foobar.hiderefs", which we don't want.
>> I think you need:
>>
>> !parse_config_key(var, section, &subsection, &subsection_len, &key) &&
>> !subsection &&
>> !strcmp(key, "hiderefs")
>>
>> Perhaps passing NULL for the subsection variable should cause
>> parse_config_key to return failure when there is a non-empty subsection.
>>
>> -Peff
>>
>> PS Outside of parse_config_key, this code would be nicer if it used
>> skip_prefix() instead of starts_with(). Since it's going away, I
>> don't think it matters, but I note that parse_config_key could
>> probably benefit from the same.
>
> While I'm thinking about it, here are patches to do that. The third one
> I'd probably squash into yours (after ordering it to the end).
>
> [1/3]: parse_config_key: use skip_prefix instead of starts_with
> [2/3]: parse_config_key: allow matching single-level config
> [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
While you were doing that, I was grepping the call sites for
parse_config_key() and made sure that all of them are OK when fed
two level names. Most of them follow this pattern:
if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
return -1;
and ones that do not immediately check !name does either eventually
do so or have separate codepaths for handlihng two- and three-level
names.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
2017-02-24 21:18 ` [PATCH] refs: parse_hide_refs_config to use parse_config_key Junio C Hamano
@ 2017-02-24 21:20 ` Jeff King
2017-02-24 21:24 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2017-02-24 21:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git
On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:
> > While I'm thinking about it, here are patches to do that. The third one
> > I'd probably squash into yours (after ordering it to the end).
> >
> > [1/3]: parse_config_key: use skip_prefix instead of starts_with
> > [2/3]: parse_config_key: allow matching single-level config
> > [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
>
> While you were doing that, I was grepping the call sites for
> parse_config_key() and made sure that all of them are OK when fed
> two level names. Most of them follow this pattern:
>
> if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
> return -1;
>
> and ones that do not immediately check !name does either eventually
> do so or have separate codepaths for handlihng two- and three-level
> names.
Yeah, I did that, too. :)
I don't think there are any other sites to convert. And I don't think we
can make the "!name" case easier (because some call-sites do want to
handle both types). And it's not like it gets much easier anyway (unlike
the opposite case, you _do_ need to declare the extra variables.
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
2017-02-24 21:20 ` Jeff King
@ 2017-02-24 21:24 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-02-24 21:24 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
Jeff King <peff@peff.net> writes:
> On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:
>
>> > While I'm thinking about it, here are patches to do that. The third one
>> > I'd probably squash into yours (after ordering it to the end).
>> >
>> > [1/3]: parse_config_key: use skip_prefix instead of starts_with
>> > [2/3]: parse_config_key: allow matching single-level config
>> > [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
>>
>> While you were doing that, I was grepping the call sites for
>> parse_config_key() and made sure that all of them are OK when fed
>> two level names. Most of them follow this pattern:
>>
>> if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
>> return -1;
>>
>> and ones that do not immediately check !name does either eventually
>> do so or have separate codepaths for handlihng two- and three-level
>> names.
>
> Yeah, I did that, too. :)
>
> I don't think there are any other sites to convert. And I don't think we
> can make the "!name" case easier (because some call-sites do want to
> handle both types). And it's not like it gets much easier anyway (unlike
> the opposite case, you _do_ need to declare the extra variables.
Yeah, because the rejection of !name as an error is not the only
reason these call sites have &name and &namelen---they want to use
that subsection name after that if() statement rejects malformed
input, so we cannot really lose them.
Thanks. All three looked good.
^ permalink raw reply [flat|nested] 16+ messages in thread