Re: [PATCH v3 1/2] gpg-interface: refactor the free-and-xmemdupz pattern

[Junio C Hamano <gitster@pobox.com>]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> This commit introduces a static replace_cstring() function.  This
> function simplifies the continuous pattern of free-and-xmemdupz() for
> GPG status line parsing.
>
> The benefit of having it in a single helper function is that it helps
> avoid the need for duplicate code that does the same thing.  It also
> helps avoid potential memleaks if parsing of new status lines are
> introduced in the future.
>
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
> ---
>  gpg-interface.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Looks quite cleanly done and clearly explained.  Thanks.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index d60115ca40..b4c4443287 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -105,6 +105,17 @@ static struct {
>  	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
>  };
>  
> +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;
> +}
> +
>  static void parse_gpg_output(struct signature_check *sigc)
>  {
>  	const char *buf = sigc->gpg_status;
> @@ -136,21 +147,18 @@ static void parse_gpg_output(struct signature_check *sigc)
>  				/* Do we have key information? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {
>  					next = strchrnul(line, ' ');
> -					free(sigc->key);
> -					sigc->key = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->key, line, next);
>  					/* Do we have signer information? */
>  					if (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
>  						line = next + 1;
>  						next = strchrnul(line, '\n');
> -						free(sigc->signer);
> -						sigc->signer = xmemdupz(line, next - line);
> +						replace_cstring(&sigc->signer, line, next);
>  					}
>  				}
>  				/* Do we have fingerprint? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
>  					next = strchrnul(line, ' ');
> -					free(sigc->fingerprint);
> -					sigc->fingerprint = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->fingerprint, line, next);
>  
>  					/* Skip interim fields */
>  					for (j = 9; j > 0; j--) {
> @@ -162,7 +170,8 @@ static void parse_gpg_output(struct signature_check *sigc)
>  
>  					next = strchrnul(line, '\n');
>  					free(sigc->primary_key_fingerprint);
> -					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
> +					replace_cstring(&sigc->primary_key_fingerprint,
> +							line, next);
>  				}
>  
>  				break;