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 From 2721ce21e439ee0726dc69073acd7e0d2b2407b3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 13 Jun 2016 06:04:20 -0400 Subject: use string_list initializer consistently There are two types of string_lists: those that own the string memory, and those that don't. You can tell the difference by the strdup_strings flag, and one should use either STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an initializer. Historically, the normal all-zeros initialization has corresponded to the NODUP case. Many sites use no initializer at all, and that works as a shorthand for that case. But for a reader of the code, it can be hard to remember which is which. Let's be more explicit and actually have each site declare which type it means to use. This is a fairly mechanical conversion; I assumed each site was correct as-is, and just switched them all to NODUP. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/apply.c b/builtin/apply.c index 8e4da2e..955af4d 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -270,7 +270,7 @@ struct image { * the case where more than one patches touch the same file. */ -static struct string_list fn_table; +static struct string_list fn_table = STRING_LIST_INIT_NODUP; static uint32_t hash_line(const char *cp, size_t len) { @@ -1936,7 +1936,7 @@ static void prefix_patch(struct patch *p) * include/exclude */ -static struct string_list limit_by_name; +static struct string_list limit_by_name = STRING_LIST_INIT_NODUP; static int has_include; static void add_name_limit(const char *name, int exclude) { @@ -3582,7 +3582,7 @@ static int check_to_create(const char *new_name, int ok_if_exists) * it is perfectly fine, as the patch removes a/b to make room * to create a directory a/b so that a/b/c can be created. */ -static struct string_list symlink_changes; +static struct string_list symlink_changes = STRING_LIST_INIT_NODUP; #define SYMLINK_GOES_AWAY 01 #define SYMLINK_IN_RESULT 02 diff --git a/builtin/blame.c b/builtin/blame.c index 80d2431..de70153 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -56,7 +56,7 @@ static int show_progress; static struct date_mode blame_date_mode = { DATE_ISO8601 }; static size_t blame_date_width; -static struct string_list mailmap; +static struct string_list mailmap = STRING_LIST_INIT_NODUP; #ifndef DEBUG #define DEBUG 0 diff --git a/builtin/clone.c b/builtin/clone.c index 5f867e6..70d8213 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -49,8 +49,8 @@ static char *option_upload_pack = "git-upload-pack"; static int option_verbosity; static int option_progress = -1; static enum transport_family family; -static struct string_list option_config; -static struct string_list option_reference; +static struct string_list option_config = STRING_LIST_INIT_NODUP; +static struct string_list option_reference = STRING_LIST_INIT_NODUP; static int option_dissociate; static int max_jobs = -1; diff --git a/builtin/log.c b/builtin/log.c index dff3fbb..c11bd66 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -674,9 +674,9 @@ static int auto_number = 1; static char *default_attach = NULL; -static struct string_list extra_hdr; -static struct string_list extra_to; -static struct string_list extra_cc; +static struct string_list extra_hdr = STRING_LIST_INIT_NODUP; +static struct string_list extra_to = STRING_LIST_INIT_NODUP; +static struct string_list extra_cc = STRING_LIST_INIT_NODUP; static void add_header(const char *value) { diff --git a/builtin/remote.c b/builtin/remote.c index fda5c2e..e1cc55e 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -247,7 +247,7 @@ struct branch_info { enum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE } rebase; }; -static struct string_list branch_list; +static struct string_list branch_list = STRING_LIST_INIT_NODUP; static const char *abbrev_ref(const char *name, const char *prefix) { diff --git a/notes.c b/notes.c index e4e4854..df4660f 100644 --- a/notes.c +++ b/notes.c @@ -70,7 +70,7 @@ struct non_note { struct notes_tree default_notes_tree; -static struct string_list display_notes_refs; +static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP; static struct notes_tree **display_notes_trees; static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, diff --git a/submodule.c b/submodule.c index 4532b11..abc2ac2 100644 --- a/submodule.c +++ b/submodule.c @@ -17,7 +17,7 @@ static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; static int parallel_jobs = 1; -static struct string_list changed_submodule_paths; +static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP; static int initialized_fetch_ref_tips; static struct sha1_array ref_tips_before_fetch; static struct sha1_array ref_tips_after_fetch; diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index 2c8c8f1..37a1967 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -11,7 +11,7 @@ static int verbose = 0, dry_run = 0, quiet = 0; static char *string = NULL; static char *file = NULL; static int ambiguous; -static struct string_list list; +static struct string_list list = STRING_LIST_INIT_NODUP; static int length_callback(const struct option *opt, const char *arg, int unset) { -- cgit v0.10.2-6-g49f6