Re: [PATCH 1/8] submodule-config: keep update strategy around

[Jeff King <peff@peff.net>]

On Wed, Feb 03, 2016 at 03:09:06PM -0800, Junio C Hamano wrote:

> > +	} else if (!strcmp(item.buf, "update")) {
> > +		if (!value)
> > +			ret = config_error_nonbool(var);
> > +		else if (!me->overwrite && submodule->update != NULL)
> > +			warn_multiple_config(me->commit_sha1, submodule->name,
> > +					     "update");
> > +		else {
> > +			free((void *) submodule->update);
> > +			submodule->update = xstrdup(value);
> 
> This is a tangent, but whenever I see us needing to cast the
> constness away to call free(), it makes me wonder how much value we
> are getting by marking the field as a pointer to "const" in the
> first place.  With this patch alone, we cannot really see it,
> because we cannot see how it is used.

I suspect we may have just inherited this from other config code. We
typically use a const pointer to declare config strings, because
git_config_string() takes a pointer to a const pointer for its
destination. And we do that because some strings need BSS defaults, and
we cannot assign a string literal to a non-const pointer without a cast
(and if we did, it is inviting somebody to accidentally free() the
string literal).

So _somebody_ generally has to cast to cover all situations. But here we
are not using git_config_string() at all, so I think we could get away
with a non-const pointer.

As a tangent to your tangent, another problem with git_config_string()
and const pointers is that we never free the "old" value for a string,
and we leak it.  It's usually OK in practice because people do not have
unbounded repetition of their config keys. We don't free because it
would be an error to do so for a default-assigned string literal. But
for any string variable which defaults to NULL, it would be fine.

IOW, git_config_string (and probably git_config_pathname) could be
implemented like this:

  int git_config_string(char **dest, const char *var, const char *value)
  {
	if (!value)
		return config_error_nonbool(var);
	free(*dest);
	*dest = xstrdup(value);
	return 0;
  }

and then:

  char *git_foo;
  ...
      return git_config_string(&git_foo, var, value);

would do the right thing. But I'm not sure how much that would ripple
through the code-base. Any string who wants:

  const char *git_foo = "default";

would have to use another function (and keep the leak), or convert its
uses to treat NULL the same as "default". I have a suspicion that _most_
sites would prefer it the non-const way, and it would be a net win. But
I think you'd have to replace the function, then convert all of the
variables used (which the compiler will helpfully point out to you
because of the type mismatch), and then see how many are broken (which
the compiler will also tell you).

-Peff