patchconfig.c: change the function signature of `git_config_string()`

`git_config_string()` output parameter `dest` is declared as a const which is unnecessary as the caller of the function is given a strduped string which can be modified without causing any harm.

Thus, remove the const from the function signature.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 cache.h  | 2 +-
 config.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:

You are correct that it is unnecessary. However, this patch alone is not sufficient because of the way const-ness in C works. If I have:

  static const char *some_global;

then with your patch, calling:

  git_config_string(&some_global, var, value);

will complain that we are passing a pointer to "const char *", not a pointer to "char *". And indeed, compiling with your patch introduces a ton of compiler warnings.

We would have to convert each of the variables we pass to it to:

  static char *some_global;

That's not so bad, but:

  static char *some_global = "some_default_value";

is wrong. Such a global sometimes points to const storage (i.e., initially), and sometimes to allocated storage (if it was loaded from config). We simply keep the latter as a const pointer (since we would not bother to free it at the end of the program anyway), and that decision influences git_config_string, which is just a helper for setting such variables anyway.

So I would not mind lifting this unnecessary restriction on git_config_string, but I do not see a way to do it without making the rest of the code much uglier (and I do not see a particular advantage in modifying git_config_string here that would make it worth the trouble).

-Peff

On 7/22/2014 4:37 PM, Jeff King wrote:

I had also thought that the compiler would raise lot of warnings but it didn't on the first compile. Now I checked again and now it complains a lot, maybe it because I was tinkering with my config.mak, dunno.

Yes, you are right. This patch is the conclusion of discussion in [1]. I used the same function signature as git_config_string for git_config_get_string() which lead to some ugly casts like

+git_config_get_string("imap.folder", (const char**)&imap_folder);

in imap-send.c patch and others. What should we do about such cases, I used either an intermediate variable or casts but Junio commented that it would be better if the dest parameter was a non-const and that it was a weakness of the config-set API that demanded the dest to be a const pointer.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/253948/

Jeff King <peff@peff.net> writes:

Tanay: are you not compiling with gcc -Wall -Werror?

(see my earlier message, just create a file config.mak containing

  CFLAGS += -Wdeclaration-after-statement -Wall -Werror

)

OK, it seems I got convinced too quickly by Junio ;-). The function produces a char * that can be modified, but it also receives a value, and the function should keep the "const" to allow passing "const char *".

On 7/22/2014 5:14 PM, Matthieu Moy wrote:

Yes, I was. Dunno why it didn't work then.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Don't blame me. I never suggested to touch that existing function, with existing call sites.

Junio C Hamano <gitster@pobox.com> writes:

I don't understand what you mean. The new git_config_get_string() function is meant to be used in essentially every places where git_config_string() is currently used, so removing the const from git_config_get_string() raises the same issue as changing the existing function.

Dropping the const means we won't be able to write

const char *v = "default"; ... git_config_get_string(&v, ...);

Jeff King <peff@peff.net> writes:

I do not think changing the existing one is warranted, either.

Given that C's type system does not allow us to pass "const char *" as "char *" without cast (and vice versa), a function that takes an out parameter that points at the location to store the pointer to a string (instead of returning the pointer to a string as value) has to take either one of these forms:

	get_mutable(char **dest);
        get_const(const char **dest);

and half the callers need to pass their variables with a cast, i.e.

        char *mutable_string;
	const char *const_string;

	get_mutable((char **)&const_string);
        get_mutable(&mutable_string);
	get_const(&const_string);
	get_const((const char **)&mutable_string);

If we have to cast some [*1*], I'd prefer to see the type describe what it really is, and I think a function that gives a newly allocated storage for the callers' own use is better described by taking a pointer to non-const "char *" location. So I'd encourage a new function being introduced that does not have existing callers to use that as the criterion to decide which to take.

Changing the existing function with existing calling site needs two other pros-and-cons evaluation, in addition to the above "does the type describe what it really is?", which are "is it worth the churn?" and "does the end result make more sense than the original?"

[Footnote]

*1* We have safe_create_leading_directories_const() that works around this for input parameter around its _const less counterpart, which is ugly but livable solution.

Junio C Hamano <gitster@pobox.com> writes:

I think it would actually be a reasonable solution to avoid casting here and there on the caller side.

Another option would be to _return_ a non-const char * instead of outputing it as a by-address parameter. We'd lose the consiseness of

   return git_config_get_string(...)

(but in cases where the return value is ignored, we do not really care)

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

"Ugly" primarily refers to the fact that we are forced to do this in the first place by the language. I agree with you, especially if we have very many call sites, and I suspect config-get-string actually would.

Here, too, I agree that it is the most C-ish interface.