Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2016, 18:30 UTC
Message-ID: <xmqqfupm3ya1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <e4b48a9a73ff33179556c0137caae85bfb4a48d9.1472457609.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 21 quoted lines

> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
>  	else if (!strcmp(key, "options.mainline"))
>  		opts->mainline = git_config_int(key, value);
> -	else if (!strcmp(key, "options.strategy"))
> +	else if (!strcmp(key, "options.strategy")) {
>  		git_config_string(&opts->strategy, key, value);
> -	else if (!strcmp(key, "options.gpg-sign"))
> +		sequencer_entrust(opts, (char *) opts->strategy);
> +	}
> +	else if (!strcmp(key, "options.gpg-sign")) {
>  		git_config_string(&opts->gpg_sign, key, value);
> +		sequencer_entrust(opts, (char *) opts->gpg_sign);
> +	}
>  	else if (!strcmp(key, "options.strategy-option")) {
>  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> +		opts->xopts[opts->xopts_nr++] =
> +			sequencer_entrust(opts, xstrdup(value));
>  	} else
>  		return error(_("Invalid key: %s"), key);

Hmm.

I would have expected a call to sequencer_opts_clear(&opts) once the machinery is done with the options structure, and among these places where an old value in opts->field is overwritten by a new one would get

	free(opt->field); opt->field = ... new value ...;

Perhaps there was a good reason to do it this way (one valid reason may be that there is _no_ good place to declare that opts is now done and it is safe to call sequencer_opts_clear() on it), but this looks backwards from the way things are usually done.