On Mon, Nov 18 2019, Junio C Hamano wrote:
> I wonder if it is simpler to define it next to 'next'. Yes, this
> new variable is used only within this block, but it also gets used
> only in conjunction with that other variable.
Done in v3 (sorry for the messed up versioning going from v0 to v2!).
> A bigger question is, when this happens, what value do we want to
> leave in sigc->primary_key_fingerprint? As we can see from the
> original code that makes sure the old value in the field will not
> leak by first free()ing, it seems that it is possible in this code
> that the field may not be NULL, but we just saw that on _our_
> signature verification system, the primary key is not available.
> Shouldn't we be nulling it out, after free()ing possibly leftover
> value in the field?
I investigated the code paths to `primary_key_fingerprint` and deduced
that it's only ever touched when GPG_STATUS_FINGERPRINT is encountered
and a primary fingerprint is extracted. However, v3 will NULL it even
when no primary fingerprint is found.
>> + xmemdupz(line,
>> + next - line);
>> + }
>
> Avoid such an unnatural line-wrapping that makes the result harder
> to read.
Sorry about that! I figured that some projects prefer to always trust
in the code formatter; so I just left it be. Now I know that human
decisions are allowed :)
> A short helper
>
> static void replace_cstring(const char **field,
> const char *line, const char *next)
> {
> free(*field);
> if (line && next)
> *field = xmemdupz(line, next - line);
> else
> *field = NULL;
> }
>
> may have quite a lot of uses in this function, not only for this
> field.
Implemented. I wasn't sure whether to do it in a separate commit or
not, but #git-devel suggested that I do; so that's what I did.
