From 7a7a517a2f6264a893ed47f8daf02cb221aca67c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 13 Jun 2016 01:39:12 -0400 Subject: parse_opt_string_list: stop allocating new strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parse_opt_string_list callback is basically a thin wrapper to string_list_append() any string options we get. However, it calls: string_list_append(v, xstrdup(arg)); which duplicates the option value. This is wrong for two reasons: 1. If the string list has strdup_strings set, then we are making an extra copy, which is simply leaked. 2. If the string list does not have strdup_strings set, then we pass memory ownership to the string list, but it does not realize this. If we later call string_list_clear(), which can happen if "--no-foo" is passed, then we will leak all of the existing entries. Instead, we should just pass the argument straight to string_list_append, and it can decide whether to copy or not based on its strdup_strings flag. It's possible that some (buggy) caller could be relying on this extra copy (e.g., because it parses some options from an allocated argv array and then frees the array), but it's not likely. For one, we generally only use parse_options on the argv given to us in main(). And two, such a caller is broken anyway, because other option types like OPT_STRING() do not make such a copy. This patch brings us in line with them. Noticed-by: Nguyễn Thái Ngọc Duy Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/parse-options-cb.c b/parse-options-cb.c index 5ab6ed6..4e82901 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -127,7 +127,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset) if (!arg) return -1; - string_list_append(v, xstrdup(arg)); + string_list_append(v, arg); return 0; } -- cgit v0.10.2-6-g49f6 From 7c4b169585ebe650783051c4b7a7b17de62836ad Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 13 Jun 2016 01:39:20 -0400 Subject: interpret-trailers: don't duplicate option strings There's no need to do so; the argv strings will last until the end of the program. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 46838d2..b75e953 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -19,7 +19,7 @@ static const char * const git_interpret_trailers_usage[] = { int cmd_interpret_trailers(int argc, const char **argv, const char *prefix) { int trim_empty = 0; - struct string_list trailers = STRING_LIST_INIT_DUP; + struct string_list trailers = STRING_LIST_INIT_NODUP; struct option options[] = { OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")), -- cgit v0.10.2-6-g49f6 From 64093fc06a871f71316211a2aea6bb46c49b20ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 13 Jun 2016 01:39:28 -0400 Subject: blame,shortlog: don't make local option variables static There's no need for these option variables to be static, except that they are referenced by the options array itself, which is static. But having all of this static is simply unnecessary and confusing (and inconsistent with most other commands, which either use a static global option list or a true function-local one). Note that in some cases we may need to actually initialize the variables (since we cannot rely on BSS to do so). This is a net improvement to readability, though, as we can use the more verbose initializers for our string_lists. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/blame.c b/builtin/blame.c index 6cac59c..9b1701d 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2503,12 +2503,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix) char *final_commit_name = NULL; enum object_type type; - static struct string_list range_list; - static int output_option = 0, opt = 0; - static int show_stats = 0; - static const char *revs_file = NULL; - static const char *contents_from = NULL; - static const struct option options[] = { + struct string_list range_list = STRING_LIST_INIT_NODUP; + int output_option = 0, opt = 0; + int show_stats = 0; + const char *revs_file = NULL; + const char *contents_from = NULL; + const struct option options[] = { OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")), OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")), OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")), diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 007cc66..cb3e89c 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -221,11 +221,11 @@ void shortlog_init(struct shortlog *log) int cmd_shortlog(int argc, const char **argv, const char *prefix) { - static struct shortlog log; - static struct rev_info rev; + struct shortlog log = { STRING_LIST_INIT_NODUP }; + struct rev_info rev; int nongit = !startup_info->have_repository; - static const struct option options[] = { + const struct option options[] = { OPT_BOOL('n', "numbered", &log.sort_by_number, N_("sort output according to the number of commits per author")), OPT_BOOL('s', "summary", &log.summary, -- cgit v0.10.2-6-g49f6