On Friday 22 December 2017 05:19 PM, Johannes Schindelin wrote:
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),
Should have mentioned that, sorry.
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);
}
Seems you got the right one, regardless. :)
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).
Nice explanation.
I spent a little quality time with the code and came up with a patch that
I will send out in a moment.
That relieves Philip of the burden, I guess. :)
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.
Though it's good to hear these words and I do like to focus on
correctness, there wasn't much I did to focus on correctness in this
case ;-) It was you actually, after seeing such a clear explanation!.
I just used Git built from 'next' for my personal use and encountered
the issue I stated in the start of this sub-thread.
--
Kaartic
