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

[Petr Baudis <pasky@suse.cz>]

On Fri, Sep 12, 2008 at 02:23:51PM -0700, Junio C Hamano wrote:
> 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?

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