Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path

[Junio C Hamano <gitster@pobox.com>]

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?

> +{
> +	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()"?

> +		/* 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.

> +}
> +
> +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