Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
From: Jakub Narębski <hidden>
Date: 2016-08-30 14:55:02
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted hunk ↗ jump to hunk
The sequencer reads options from disk and stores them in its struct for use during sequencer's operations. With this patch, the memory is released afterwards, plugging a memory leak. Signed-off-by: Johannes Schindelin <redacted> --- sequencer.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)diff --git a/sequencer.c b/sequencer.c index b5be0f9..8d79091 100644 --- a/sequencer.c +++ b/sequencer.c@@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts *opts) free(opts->owned[i]); free(opts->owned); + free(opts->xopts); +
This looks like independent change, not related to using the sequencer_entrust() to store options read from disk in replay_opts struct to be able to free memory afterwards. I guess you wanted to avoid one line changes...
quoted hunk ↗ jump to hunk
strbuf_addf(&dir, "%s", get_dir(opts)); remove_dir_recursively(&dir, 0); strbuf_release(&dir);@@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
Sidenote: this patch would be easier to read if lines were reordered as below, but I don't think any slider heuristics could help achieve that automatically. Also, the patch might be invalid...
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);
+ sequencer_entrust(opts, (char *) opts->strategy);I wonder if the ability to free strings dup-ed by git_config_string() be something that is part of replay_opts, or rather remove_sequencer_state(), that is a list of free(opts->strategy); free(opts->gpg_sign); And of course for (i = 0; i < opts->xopts_nr; i++) free(opts->xopts[i]); free(opts->xopts); Though... free(NULL) is nop as per standard, but can we rely on it? If it is a problem, we can create xfree(ptr) being if(ptr)free(ptr); The *_entrust() mechanism is more generic, but do we use this general-ness? Well, it could be xstrdup or git_config_string doing entrust'ing...
+ }
- else if (!strcmp(key, "options.gpg-sign"))
+ 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));Nice.
} else
return error(_("Invalid key: %s"), key);