Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 12, 2008, 21:23 UTC
- Message-ID
- <7vsks5njmg.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20080912210857.31628.7605.stgit@localhost>
Petr Baudis <pasky@suse.cz> 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?
Show 10 quoted lines
> +{
> + 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;> + if (strcmp(value, info->path)) > + return 0;
This will segfault on a misconfigured:
[submodule "xyzzy"]
path> + /* Found the key to change. */
> + if (info->key) {
> + error("multiple submodules live at path `%s'", info->path);Why is this "error()", not "warning()"?
Show 5 quoted lines
> + /* 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.
Show 26 quoted lines
> +}
> +
> +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?
> +extern char *submodule_by_path(const char *path); > + > +#endif