On Fri, Aug 16, 2013 at 08:48:43AM +0200, Jens Lehmann wrote:
> > This patch addresses the issue by returning from the function if
> > 'value' is null before the call to xstrdup is made.
>
> Hmm, I'm not sure silently ignoring the misconfiguration is the best
> way to go. A submodule config having a path setting without a value
> is broken (while a submodule setting without a subsection configures
> something else, so the "|| !name" below is fine). So I believe we
> should complain to the user when "value" is NULL.
Yeah, the usual behavior is to catch it and return config_error_nonbool
(because a key without a value is an implicit-true boolean). Most code
does this via git_config_string, but what the submodule code is doing is
too tricky and custom to use that stock function.
> On the other hand this should only happen for the three options we do
> parse, as some users (e.g. git-submodule.sh) use other configurations
> for which a missing value may be fine. Please see the "lacks value"
> errors in read_merge_config() of ll-merge.c for an example of how to
> deal with that.
Those spots in ll-merge should probably be using config_error_nonbool,
if only for consistency with the rest of the code base.
> > - if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> > + if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
> > return 0;
I think this is also the wrong place to make the check, anyway. It is
saying that all values of submodule.X.Y must be non-NULL. But that is
not true. The submodule.X.fetchRecurseSubmodules option can be a
boolean, and:
[submodule "foo"]
fetchRecurseSubmodules
is perfectly valid (and is broken by this patch).
-Peff
--
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