Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path
- From
Petr Baudis <pasky@suse.cz>
- Date
- Sep 12, 2008, 21:58 UTC
- Message-ID
- <20080912215814.GG10544@machine.or.cz>
- In-Reply-To
- <7vsks5njmg.fsf@gitster.siamese.dyndns.org>
On Fri, Sep 12, 2008 at 02:23:51PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Show 24 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;This looks strange, but I think I see what do you mean. I will use
if (strcmp(subkey, ".path") || subkey == key + strlen("submodule.") - 1)Show 7 quoted lines
> > + if (strcmp(value, info->path)) > > + return 0; > > This will segfault on a misconfigured: > > [submodule "xyzzy"] > path
Thanks.
Show 5 quoted lines
> > + /* 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.
Show 9 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.
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.
Show 28 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?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