On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/convert.c
> > +++ b/convert.c
> > @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> > * External conversion drivers are configured using
> > * "filter.<name>.variable".
> > */
> > - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
> > + if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> > return 0;
>
> Hm, I actually find the preimage more readable here.
The big thing to me is getting rid of the manual "6" there, which is bad
for maintainability (it must match the length of "filter", and there is
no compile-time verification).
> Rename match_config_key() to something like parse_config_key()?
> match_ makes it sound like its main purpose is to match it against a
> pattern, but really it is more about decomposing into constituent
> parts.
There is already a git_config_parse_key, but it does something else. I
wanted to avoid confusion there. And I was trying to indicate that this
was not just about parsing, but also about matching the section.
Basically I was trying to encapsulate the current technique and have as
little code change as possible. But maybe:
struct config_key k;
parse_config_key(&k, var);
if (strcmp(k.section, "filter") || k.subsection))
return 0;
would be a better start (or having git_config do the first two lines
itself before triggering the callback).
> Rename ep to something like 'key' or 'filtertype'? Without the
> explicit string processing, it is not obvious what ep is the end of.
Ah, so that is what "ep" stands for. I was thinking it is a terrible
variable name, but I was trying to keep the patch minimal.
> > - if (prefixcmp(var, "diff."))
> > - return NULL;
> > - dot = strrchr(var, '.');
> > - if (dot == var + 4)
> > - return NULL;
> > - if (strcmp(type, dot+1))
> > + if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> > + strcmp(type, key))
> > return NULL;
> >
> > - name = var + 5;
> > - namelen = dot - name;
> > drv = userdiff_find_by_namelen(name, namelen);
>
> What happens in the !name case? (Honest question --- I haven't checked.)
Segfault, I expect. Thanks for catching.
I actually wrote this correctly once, coupled with patch 4, but screwed
it up when teasing it apart into two patches. It should be:
if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
!name ||
strcmp(type, key))
return NULL;
Patch 4 replaces this with correct code (as it moves it into
userdiff_config).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html