Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)

> Hi Kaartic,
> 
>>
>> I think I didn't mention I've set `commit.gpgsign` to `true` for that
>> repo, did I?
> 
> Hah! I had troubles to associate the correct line in my versions of Git's
> source code (the line numbers alone are only reliable if you also have a
> commit from which the Git binaries were built), 

> but I did this free() at
> (https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L1975:
> 
> 	if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> 		if (!starts_with(buf.buf, "-S"))
> 			strbuf_reset(&buf);
> 		else {
> 			free(opts->gpg_sign);
> 			^^^^^^^^^^^^^^^^^^^^^
> 			opts->gpg_sign = xstrdup(buf.buf + 2);
> 		}
> 		strbuf_reset(&buf);
> 	}
> 

> The culprit is that we have these really unclear ownership rules, where it
> is not at all clear who is responsible for releasing allocated memory:
> caller or callee? (Hannes would not rightfully point out that this would
> be a non-issue if we would switch to C++). In C, the common pattern is to
> use `const char *` for users that are *not* supposed to take ownership,
> and `char *` for users that are supposed to take custody. And in this
> instance, `gpg_sign` should be owned by `struct replay_opts` because of
> this (https://github.com/git/git/blob/cd54ea2b18/sequencer.h#L38):
> 
> 	char *gpg_sign;
> 
> Technically, using `char *` (instead of `const char *`) does not say
> "you're now responsible for de-allocating this memory", of course, but in
> practice it is often used like this (and the signature of `free(void *)`
> certainly encourages that type of interpreting the `const` qualifier).
> 

> By the way, Kaartic, thank you so much for focusing on correctness before
> focusing on style issues. I know it is harder to review patches for
> correctness than it is to concentrate on style issues, but in my mind it
> is not only much more work, but also a lot more valuable.
>