On Fri, Sep 12, 2008 at 02:23:51PM -0700, Junio C Hamano wrote:
> Petr Baudis <pasky@xxxxxxx> writes:
>
> > +static int gitmodules_worker(const char *key, const char *value, void *info_)
>
> Won't you ever have different kind of work in the future?
> find_submodule_by_path(), perhaps?
Good point.
> > +{
> > + struct gitmodules_info *info = info_;
> > + const char *subkey;
> > +
> > + if (prefixcmp(key, "submodule."))
> > + return 0;
> > +
> > + subkey = strrchr(key, '.');
> > + if (!subkey)
> > + return 0;
>
> This cannot happen; you made sure the thing begins with "submodule."
> already.
>
> > + if (strcmp(subkey, ".path"))
> > + return 0;
>
> This will miss a misconfigured "submodule.path" (two level).
>
> I can understand if this part were:
>
> subkey = strrchr(key, '.');
> if (!subkey || subkey == key + strlen("submodule.") - 1)
> return 0;
This looks strange, but I think I see what do you mean. I will use
if (strcmp(subkey, ".path") || subkey == key + strlen("submodule.") - 1)
> > + if (strcmp(value, info->path))
> > + return 0;
>
> This will segfault on a misconfigured:
>
> [submodule "xyzzy"]
> path
Thanks.
> > + /* Found the key to change. */
> > + if (info->key) {
> > + error("multiple submodules live at path `%s'", info->path);
>
> Why is this "error()", not "warning()"?
Matter of taste, I suppose. I have changed this to warning(), though it
is dubious configuration at best.
> > + /* The last one is supposed to win. */
> > + free(info->key);
> > + }
> > + info->key = xstrdup(key);
> > + return 0;
>
> Have to wonder if it makes easier for the users if this function kept only
> "xyzzy" out of "submodule.xyzzy.path", not the whole thing. Cannot judge
> without actual callers.
They follow up in the next patches. ;-) They use the submodule name
only to access the configuration again, so this format is the most
convenient for them.
> > +}
> > +
> > +char *submodule_by_path(const char *path)
> > +{
> > + struct gitmodules_info info = { path, NULL };
> > +
> > + config_exclusive_filename = ".gitmodules";
> > + if (git_config(gitmodules_worker, &info))
> > + die("cannot process .gitmodules");
> > + if (!info.key)
> > + die("the submodule of `%s' not found in .gitmodules", path);
> > + config_exclusive_filename = NULL;
> > +
> > + return info.key;
> > +}
> > diff --git a/submodule.h b/submodule.h
> > new file mode 100644
> > index 0000000..bc74fa0
> > --- /dev/null
> > +++ b/submodule.h
> > @@ -0,0 +1,8 @@
> > +#ifndef SUBMODULE_H
> > +#define SUBMODULE_H
> > +
> > +/* Find submodule living at given path in .gitmodules and return the key
> > + * of its path config variable (dynamically allocated). */
>
> Style?
Would you seriously find it prettier with the newline?
$ git grep '^[^/]*[a-z][^/]*\*/$' *.c | wc -l
37
--
Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC. -- Bill Gates
--
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
