From 0d4cc1b45bf063b3a46654098a9fb85f82f386a7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 06:25:26 -0500 Subject: [PATCH 1/6] give "nbuf" strbuf a more meaningful name It's a common pattern in our code to read paths from stdin, separated either by newlines or NULs, and unquote as necessary. In each of these five cases we use "nbuf" to temporarily store the unquoted value. Let's give it the more meaningful name "unquoted", which makes it easier to understand the purpose of the variable. While we're at it, let's also static-initialize all of our strbufs. It's not wrong to call strbuf_init, but it increases the cognitive load on the reader, who might wonder "do we sometimes avoid initializing them? why?". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/check-attr.c | 13 ++++++------- builtin/check-ignore.c | 13 ++++++------- builtin/checkout-index.c | 11 ++++++----- builtin/hash-object.c | 11 ++++++----- builtin/update-index.c | 11 ++++++----- 5 files changed, 30 insertions(+), 29 deletions(-) diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 087325ef69..53a5a18c16 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt, static void check_attr_stdin_paths(const char *prefix, int cnt, struct git_attr_check *check) { - struct strbuf buf, nbuf; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; strbuf_getline_fn getline_fn; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; - strbuf_init(&buf, 0); - strbuf_init(&nbuf, 0); while (getline_fn(&buf, stdin) != EOF) { if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } check_attr(prefix, cnt, check, buf.buf); maybe_flush_or_die(stdout, "attribute to stdout"); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); } static NORETURN void error_with_usage(const char *msg) diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 4f0b09e2db..1d73d3ca3d 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir, static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) { - struct strbuf buf, nbuf; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; char *pathspec[2] = { NULL, NULL }; strbuf_getline_fn getline_fn; int num_ignored = 0; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; - strbuf_init(&buf, 0); - strbuf_init(&nbuf, 0); while (getline_fn(&buf, stdin) != EOF) { if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } pathspec[0] = buf.buf; num_ignored += check_ignore(dir, prefix, @@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) maybe_flush_or_die(stdout, "check-ignore to stdout"); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); return num_ignored; } diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index ed888a5b9e..d8d7bd3854 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) } if (read_from_stdin) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; strbuf_getline_fn getline_fn; if (all) @@ -261,16 +262,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) while (getline_fn(&buf, stdin) != EOF) { char *p; if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } p = prefix_path(prefix, prefix_length, buf.buf); checkout_file(p, prefix); free(p); } - strbuf_release(&nbuf); + strbuf_release(&unquoted); strbuf_release(&buf); } diff --git a/builtin/hash-object.c b/builtin/hash-object.c index 3bc5ec1d21..d3cb4e5345 100644 --- a/builtin/hash-object.c +++ b/builtin/hash-object.c @@ -58,20 +58,21 @@ static void hash_object(const char *path, const char *type, const char *vpath, static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, int literally) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; while (strbuf_getline_lf(&buf, stdin) != EOF) { if (buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } hash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags, literally); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); } int cmd_hash_object(int argc, const char **argv, const char *prefix) diff --git a/builtin/update-index.c b/builtin/update-index.c index 7c5c143de5..f052fafb54 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1075,16 +1075,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) } if (read_from_stdin) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; setup_work_tree(); while (getline_fn(&buf, stdin) != EOF) { char *p; if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } p = prefix_path(prefix, prefix_length, buf.buf); update_one(p); @@ -1092,7 +1093,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) chmod_path(set_executable_bit, p); free(p); } - strbuf_release(&nbuf); + strbuf_release(&unquoted); strbuf_release(&buf); } From 9096ee162b7e4e426b1719bb43f9e42e84c95816 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 06:25:43 -0500 Subject: [PATCH 2/6] checkout-index: simplify "-z" option parsing Now that we act as a simple bool, there's no need to use a custom callback. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index d8d7bd3854..3b913d1b37 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt, return 0; } -static int option_parse_z(const struct option *opt, - const char *arg, int unset) -{ - nul_term_line = !unset; - return 0; -} - static int option_parse_prefix(const struct option *opt, const char *arg, int unset) { @@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) { OPTION_CALLBACK, 'u', "index", &newfd, NULL, N_("update stat information in the index file"), PARSE_OPT_NOARG, option_parse_u }, - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, - N_("paths are separated with NUL character"), - PARSE_OPT_NOARG, option_parse_z }, + OPT_BOOL('z', NULL, &nul_term_line, + N_("paths are separated with NUL character")), OPT_BOOL(0, "stdin", &read_from_stdin, N_("read list of paths from the standard input")), OPT_BOOL(0, "temp", &to_tempfile, From 5ed5f671c4193f2b9d47c08e989574903f3bf9cc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 06:26:16 -0500 Subject: [PATCH 3/6] checkout-index: handle "--no-prefix" option We use a custom callback to parse "--prefix", but it does not handle the "unset" case. As a result, passing "--no-prefix" will cause a segfault. We can fix this by switching it to an OPT_STRING, which makes "--no-prefix" counteract a previous "--prefix". Note that this assigns NULL, so we bump our default-case initialization to lower in the main function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 3b913d1b37..43beddee13 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -142,14 +142,6 @@ static int option_parse_u(const struct option *opt, return 0; } -static int option_parse_prefix(const struct option *opt, - const char *arg, int unset) -{ - state.base_dir = arg; - state.base_dir_len = strlen(arg); - return 0; -} - static int option_parse_stage(const struct option *opt, const char *arg, int unset) { @@ -191,9 +183,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) N_("read list of paths from the standard input")), OPT_BOOL(0, "temp", &to_tempfile, N_("write the content to temporary files")), - OPT_CALLBACK(0, "prefix", NULL, N_("string"), - N_("when creating files, prepend "), - option_parse_prefix), + OPT_STRING(0, "prefix", &state.base_dir, N_("string"), + N_("when creating files, prepend ")), OPT_CALLBACK(0, "stage", NULL, NULL, N_("copy out the files from named stage"), option_parse_stage), @@ -204,7 +195,6 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) usage_with_options(builtin_checkout_index_usage, builtin_checkout_index_options); git_config(git_default_config, NULL); - state.base_dir = ""; prefix_length = prefix ? strlen(prefix) : 0; if (read_cache() < 0) { @@ -217,6 +207,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) state.quiet = quiet; state.not_new = not_new; + if (!state.base_dir) + state.base_dir = ""; + state.base_dir_len = strlen(state.base_dir); + if (state.base_dir_len || to_tempfile) { /* when --prefix is specified we do not * want to update cache. From 6a6df8aa45aa68df737d6c59d7abec499e9451da Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 06:29:36 -0500 Subject: [PATCH 4/6] checkout-index: handle "--no-index" option The parsing of "--index" is done in a callback, but it does not handle an "unset" option. We don't necessarily expect anyone to use this, but the current behavior is to treat it exactly like "--index", which would probably be surprising. Instead, let's just turn it into an OPT_BOOL, and handle it after we're done parsing. This makes "--no-index" just work (it cancels a previous "--index"). As a bonus, this makes the logic easier to follow. The old code opened the index during the option parsing, leaving the reader to wonder if there was some timing issue (there isn't; none of the other options care that we've opened it). And then if we found that "--prefix" had been given, we had to rollback the index. Now we can simply avoid opening it in the first place. Note that it might make more sense for checkout-index to complain when "--index --prefix=foo" is given (rather than silently ignoring "--index"), but since it has been that way since 415e96c ([PATCH] Implement git-checkout-cache -u to update stat information in the cache., 2005-05-15), it's safer to leave it as-is. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 43beddee13..f8179a751b 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] = { static struct lock_file lock_file; -static int option_parse_u(const struct option *opt, - const char *arg, int unset) -{ - int *newfd = opt->value; - - state.refresh_cache = 1; - state.istate = &the_index; - if (*newfd < 0) - *newfd = hold_locked_index(&lock_file, 1); - return 0; -} - static int option_parse_stage(const struct option *opt, const char *arg, int unset) { @@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) int read_from_stdin = 0; int prefix_length; int force = 0, quiet = 0, not_new = 0; + int index_opt = 0; struct option builtin_checkout_index_options[] = { OPT_BOOL('a', "all", &all, N_("check out all files in the index")), @@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) N_("no warning for existing files and files not in index")), OPT_BOOL('n', "no-create", ¬_new, N_("don't checkout new files")), - { OPTION_CALLBACK, 'u', "index", &newfd, NULL, - N_("update stat information in the index file"), - PARSE_OPT_NOARG, option_parse_u }, + OPT_BOOL('u', "index", &index_opt, + N_("update stat information in the index file")), OPT_BOOL('z', NULL, &nul_term_line, N_("paths are separated with NUL character")), OPT_BOOL(0, "stdin", &read_from_stdin, @@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) state.base_dir = ""; state.base_dir_len = strlen(state.base_dir); - if (state.base_dir_len || to_tempfile) { - /* when --prefix is specified we do not - * want to update cache. - */ - if (state.refresh_cache) { - rollback_lock_file(&lock_file); - newfd = -1; - } - state.refresh_cache = 0; + /* + * when --prefix is specified we do not want to update cache. + */ + if (index_opt && !state.base_dir_len && !to_tempfile) { + state.refresh_cache = 1; + state.istate = &the_index; + newfd = hold_locked_index(&lock_file, 1); } /* Check out named files first */ From 22396175267b7bcef97a02036831152539429bec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 22:18:24 -0500 Subject: [PATCH 5/6] checkout-index: disallow "--no-stage" option We do not really expect people to use "--no-stage", but if they do, git currently segfaults. We could instead have it undo the effects of a previous "--stage", but this gets tricky around the "to_tempfile" flag. We cannot simply reset it to 0, because we don't know if it was set by a previous "--stage=all" or an explicit "--temp" option. We could solve this by setting a flag and resolving to_tempfile later, but it's not worth the effort. Nobody actually wants to use "--no-stage"; we are just trying to fix a potential segfault here. While we're in the area, let's improve the user-facing messages for this option. The error string should be translatable, and we should give some hint in the "-h" output about what can go in the argument field. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout-index.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index f8179a751b..92c69672e9 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -141,7 +141,7 @@ static int option_parse_stage(const struct option *opt, if ('1' <= ch && ch <= '3') checkout_stage = arg[0] - '0'; else - die("stage should be between 1 and 3 or all"); + die(_("stage should be between 1 and 3 or all")); } return 0; } @@ -173,9 +173,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) N_("write the content to temporary files")), OPT_STRING(0, "prefix", &state.base_dir, N_("string"), N_("when creating files, prepend ")), - OPT_CALLBACK(0, "stage", NULL, NULL, + { OPTION_CALLBACK, 0, "stage", NULL, "1-3|all", N_("copy out the files from named stage"), - option_parse_stage), + PARSE_OPT_NONEG, option_parse_stage }, OPT_END() }; From 1f3c79a9d6c3278753e7fb63b48569a9ff8632df Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 31 Jan 2016 06:35:46 -0500 Subject: [PATCH 6/6] apply, ls-files: simplify "-z" parsing As a short option, we cannot handle negation. Thus a callback handling "unset" is overkill, and we can just use OPT_SET_INT instead to handle setting the option. Anybody who adds "--nul" synonym to this later would need to be careful not to break "--no-nul", which should mean that lines are terminated with LF at the end. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/apply.c | 16 +++------------- builtin/ls-files.c | 14 +++----------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index deb1364fa8..d61ac65dab 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt, return 0; } -static int option_parse_z(const struct option *opt, - const char *arg, int unset) -{ - if (unset) - line_termination = '\n'; - else - line_termination = 0; - return 0; -} - static int option_parse_space_change(const struct option *opt, const char *arg, int unset) { @@ -4546,9 +4536,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_) N_( "attempt three-way merge if a patch does not apply")), OPT_FILENAME(0, "build-fake-ancestor", &fake_ancestor, N_("build a temporary index based on embedded index information")), - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, - N_("paths are separated with NUL character"), - PARSE_OPT_NOARG, option_parse_z }, + /* Think twice before adding "--nul" synonym to this */ + OPT_SET_INT('z', NULL, &line_termination, + N_("paths are separated with NUL character"), '\0'), OPT_INTEGER('C', NULL, &p_context, N_("ensure at least lines of context match")), { OPTION_CALLBACK, 0, "whitespace", &whitespace_option, N_("action"), diff --git a/builtin/ls-files.c b/builtin/ls-files.c index b6a7cb0c7c..467699bcf5 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = { NULL }; -static int option_parse_z(const struct option *opt, - const char *arg, int unset) -{ - line_terminator = unset ? '\n' : '\0'; - - return 0; -} - static int option_parse_exclude(const struct option *opt, const char *arg, int unset) { @@ -408,9 +400,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) struct exclude_list *el; struct string_list exclude_list = STRING_LIST_INIT_NODUP; struct option builtin_ls_files_options[] = { - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, - N_("paths are separated with NUL character"), - PARSE_OPT_NOARG, option_parse_z }, + /* Think twice before adding "--nul" synonym to this */ + OPT_SET_INT('z', NULL, &line_terminator, + N_("paths are separated with NUL character"), '\0'), OPT_BOOL('t', NULL, &show_tag, N_("identify the file status with tags")), OPT_BOOL('v', NULL, &show_valid_bit,