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

Johannes Schindelin [off-list ref] writes:

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.

Hi Junio,

On Tue, 30 Aug 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

@@ -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,

It is part of sequencer_remove_state().

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 ...;

That is *exactly* what we *cannot* do.

You see, the replay_opts struct is *not necessarily* populated by
read_populate_opts(). So we really cannot tell whether we are free to
free(opt->field) or whether some other code still wants to reference that
memory in the end (or for that matter, whether it was malloc()ed).

That is the *precise* reason for the existence of sequencer_entrust().

Ciao,
Dscho