From afaef55e230c6fcfb4e8a27c1b970f7e6400a7f6 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 9 Dec 2017 21:40:07 +0100 Subject: [PATCH 1/7] git-compat-util: introduce skip_to_optional_arg() We often accept both a "--key" option and a "--key=" option. These options currently are parsed using something like: if (!strcmp(arg, "--key")) { /* do something */ } else if (skip_prefix(arg, "--key=", &arg)) { /* do something with arg */ } which is a bit cumbersome compared to just: if (skip_to_optional_arg(arg, "--key", &arg)) { /* do something with arg */ } This also introduces skip_to_optional_arg_default() for the few cases where something different should be done when the first argument is exactly "--key" than when it is exactly "--key=". In general it is better for UI consistency and simplicity if "--key" and "--key=" do the same thing though, so that using skip_to_optional_arg() should be encouraged compared to skip_to_optional_arg_default(). Note that these functions can be used to parse any "key=value" string where "key" is also considered as valid, not just command line options. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-compat-util.h | 23 +++++++++++++++++++++++ strbuf.c | 22 ++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index cedad4d581..68b2ad531e 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -484,6 +484,29 @@ static inline int skip_prefix(const char *str, const char *prefix, return 0; } +/* + * If the string "str" is the same as the string in "prefix", then the "arg" + * parameter is set to the "def" parameter and 1 is returned. + * If the string "str" begins with the string found in "prefix" and then a + * "=" sign, then the "arg" parameter is set to "str + strlen(prefix) + 1" + * (i.e., to the point in the string right after the prefix and the "=" sign), + * and 1 is returned. + * + * Otherwise, return 0 and leave "arg" untouched. + * + * When we accept both a "--key" and a "--key=" option, this function + * can be used instead of !strcmp(arg, "--key") and then + * skip_prefix(arg, "--key=", &arg) to parse such an option. + */ +int skip_to_optional_arg_default(const char *str, const char *prefix, + const char **arg, const char *def); + +static inline int skip_to_optional_arg(const char *str, const char *prefix, + const char **arg) +{ + return skip_to_optional_arg_default(str, prefix, arg, ""); +} + /* * Like skip_prefix, but promises never to read past "len" bytes of the input * buffer, and returns the remaining number of bytes in "out" via "outlen". diff --git a/strbuf.c b/strbuf.c index 323c49ceb3..29169b8ef8 100644 --- a/strbuf.c +++ b/strbuf.c @@ -11,6 +11,28 @@ int starts_with(const char *str, const char *prefix) return 0; } +int skip_to_optional_arg_default(const char *str, const char *prefix, + const char **arg, const char *def) +{ + const char *p; + + if (!skip_prefix(str, prefix, &p)) + return 0; + + if (!*p) { + if (arg) + *arg = def; + return 1; + } + + if (*p != '=') + return 0; + + if (arg) + *arg = p + 1; + return 1; +} + /* * Used as the default ->buf value, so that people can always assume * buf is non NULL and ->buf is NUL terminated even for a freshly From 72885a6d5181ca090039917b6ce76b952ebb59b1 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 9 Dec 2017 21:40:08 +0100 Subject: [PATCH 2/7] index-pack: use skip_to_optional_arg() Let's simplify index-pack option parsing using skip_to_optional_arg(). Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 8ec459f522..4c51aec81f 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1660,10 +1660,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) from_stdin = 1; } else if (!strcmp(arg, "--fix-thin")) { fix_thin_pack = 1; - } else if (!strcmp(arg, "--strict")) { - strict = 1; - do_fsck_object = 1; - } else if (skip_prefix(arg, "--strict=", &arg)) { + } else if (skip_to_optional_arg(arg, "--strict", &arg)) { strict = 1; do_fsck_object = 1; fsck_set_msg_types(&fsck_options, arg); @@ -1679,10 +1676,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) verify = 1; show_stat = 1; stat_only = 1; - } else if (!strcmp(arg, "--keep")) { - keep_msg = ""; - } else if (starts_with(arg, "--keep=")) { - keep_msg = arg + 7; + } else if (skip_to_optional_arg(arg, "--keep", &keep_msg)) { + ; /* nothing to do */ } else if (starts_with(arg, "--threads=")) { char *end; nr_threads = strtoul(arg+10, &end, 0); From 948cbe6703d0d1e3ba65fd10037bafd1b8b87696 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 9 Dec 2017 21:40:09 +0100 Subject: [PATCH 3/7] diff: use skip_to_optional_arg() Let's simplify diff option parsing using skip_to_optional_arg(). Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- diff.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/diff.c b/diff.c index 2ebe2227b4..464a53adb5 100644 --- a/diff.c +++ b/diff.c @@ -4508,17 +4508,12 @@ int diff_opt_parse(struct diff_options *options, options->output_format |= DIFF_FORMAT_NUMSTAT; else if (!strcmp(arg, "--shortstat")) options->output_format |= DIFF_FORMAT_SHORTSTAT; - else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat")) - return parse_dirstat_opt(options, ""); - else if (skip_prefix(arg, "-X", &arg)) - return parse_dirstat_opt(options, arg); - else if (skip_prefix(arg, "--dirstat=", &arg)) + else if (skip_prefix(arg, "-X", &arg) || + skip_to_optional_arg(arg, "--dirstat", &arg)) return parse_dirstat_opt(options, arg); else if (!strcmp(arg, "--cumulative")) return parse_dirstat_opt(options, "cumulative"); - else if (!strcmp(arg, "--dirstat-by-file")) - return parse_dirstat_opt(options, "files"); - else if (skip_prefix(arg, "--dirstat-by-file=", &arg)) { + else if (skip_to_optional_arg(arg, "--dirstat-by-file", &arg)) { parse_dirstat_opt(options, "files"); return parse_dirstat_opt(options, arg); } @@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options, return stat_opt(options, av); /* renames options */ - else if (starts_with(arg, "-B") || starts_with(arg, "--break-rewrites=") || - !strcmp(arg, "--break-rewrites")) { + else if (starts_with(arg, "-B") || + skip_to_optional_arg(arg, "--break-rewrites", NULL)) { if ((options->break_opt = diff_scoreopt_parse(arg)) == -1) return error("invalid argument to -B: %s", arg+2); } - else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") || - !strcmp(arg, "--find-renames")) { + else if (starts_with(arg, "-M") || + skip_to_optional_arg(arg, "--find-renames", NULL)) { if ((options->rename_score = diff_scoreopt_parse(arg)) == -1) return error("invalid argument to -M: %s", arg+2); options->detect_rename = DIFF_DETECT_RENAME; @@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options, else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) { options->irreversible_delete = 1; } - else if (starts_with(arg, "-C") || starts_with(arg, "--find-copies=") || - !strcmp(arg, "--find-copies")) { + else if (starts_with(arg, "-C") || + skip_to_optional_arg(arg, "--find-copies", NULL)) { if (options->detect_rename == DIFF_DETECT_COPY) options->flags.find_copies_harder = 1; if ((options->rename_score = diff_scoreopt_parse(arg)) == -1) From cf81f94da45db8a030073e6ef3370f8498e9ce78 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 9 Dec 2017 21:40:10 +0100 Subject: [PATCH 4/7] diff: use skip_to_optional_arg_default() Let's simplify diff option parsing using skip_to_optional_arg_default(). Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- diff.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/diff.c b/diff.c index 464a53adb5..28e1ab168f 100644 --- a/diff.c +++ b/diff.c @@ -4623,9 +4623,7 @@ int diff_opt_parse(struct diff_options *options, else if (!strcmp(arg, "--no-follow")) { options->flags.follow_renames = 0; options->flags.default_follow_renames = 0; - } else if (!strcmp(arg, "--color")) - options->use_color = 1; - else if (skip_prefix(arg, "--color=", &arg)) { + } else if (skip_to_optional_arg_default(arg, "--color", &arg, "always")) { int value = git_config_colorbool(NULL, arg); if (value < 0) return error("option `color' expects \"always\", \"auto\", or \"never\""); @@ -4645,15 +4643,10 @@ int diff_opt_parse(struct diff_options *options, if (cm < 0) die("bad --color-moved argument: %s", arg); options->color_moved = cm; - } else if (!strcmp(arg, "--color-words")) { + } else if (skip_to_optional_arg_default(arg, "--color-words", &options->word_regex, NULL)) { options->use_color = 1; options->word_diff = DIFF_WORDS_COLOR; } - else if (skip_prefix(arg, "--color-words=", &arg)) { - options->use_color = 1; - options->word_diff = DIFF_WORDS_COLOR; - options->word_regex = arg; - } else if (!strcmp(arg, "--word-diff")) { if (options->word_diff == DIFF_WORDS_NONE) options->word_diff = DIFF_WORDS_PLAIN; @@ -4691,15 +4684,10 @@ int diff_opt_parse(struct diff_options *options, options->flags.textconv_set_via_cmdline = 1; } else if (!strcmp(arg, "--no-textconv")) options->flags.allow_textconv = 0; - else if (!strcmp(arg, "--ignore-submodules")) { - options->flags.override_submodule_config = 1; - handle_ignore_submodules_arg(options, "all"); - } else if (skip_prefix(arg, "--ignore-submodules=", &arg)) { + else if (skip_to_optional_arg_default(arg, "--ignore-submodules", &arg, "all")) { options->flags.override_submodule_config = 1; handle_ignore_submodules_arg(options, arg); - } else if (!strcmp(arg, "--submodule")) - options->submodule_format = DIFF_SUBMODULE_LOG; - else if (skip_prefix(arg, "--submodule=", &arg)) + } else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log")) return parse_submodule_opt(options, arg); else if (skip_prefix(arg, "--ws-error-highlight=", &arg)) return parse_ws_error_highlight_opt(options, arg); From 1efad51197f52ba9fb928b92a7f92514ab02a97f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 9 Dec 2017 21:40:11 +0100 Subject: [PATCH 5/7] diff: use skip_to_optional_arg_default() in parsing --relative Helped-by: Jacob Keller Helped-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/diff.c b/diff.c index 28e1ab168f..3f14cdace6 100644 --- a/diff.c +++ b/diff.c @@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options, options->flags.rename_empty = 1; else if (!strcmp(arg, "--no-rename-empty")) options->flags.rename_empty = 0; - else if (!strcmp(arg, "--relative")) + else if (skip_to_optional_arg_default(arg, "--relative", &arg, NULL)) { options->flags.relative_name = 1; - else if (skip_prefix(arg, "--relative=", &arg)) { - options->flags.relative_name = 1; - options->prefix = arg; + if (arg) + options->prefix = arg; } /* xdiff options */ From 6d7c17ec9d4b6c384648d5bc28bc234e3083c66e Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Sat, 9 Dec 2017 21:40:12 +0100 Subject: [PATCH 6/7] diff: add tests for --relative without optional prefix value We already have tests for --relative, but they currently only test when a prefix has been provided. This fails to test the case where --relative by itself should use the current directory as the prefix. Teach the check_$type functions to take a directory argument to indicate which subdirectory to run the git commands in. Add a new test which uses this to test --relative without a prefix value. Signed-off-by: Jacob Keller Signed-off-by: Junio C Hamano --- t/t4045-diff-relative.sh | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 3950f5034d..22fcacfa33 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -13,6 +13,7 @@ test_expect_success 'setup' ' ' check_diff() { +dir=$1; shift expect=$1; shift cat >expected <actual && + git -C '$dir' diff -p $* HEAD^ >actual && test_cmp expected actual " } check_numstat() { +dir=$1; shift expect=$1; shift cat >expected <expected && - git diff --numstat $* HEAD^ >actual && + git -C '$dir' diff --numstat $* HEAD^ >actual && test_cmp expected actual " } check_stat() { +dir=$1; shift expect=$1; shift cat >expected <actual && + git -C '$dir' diff --stat $* HEAD^ >actual && test_i18ncmp expected actual " } check_raw() { +dir=$1; shift expect=$1; shift cat >expected <actual && + git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual && test_cmp expected actual " } -for type in diff numstat stat raw; do - check_$type file2 --relative=subdir/ - check_$type file2 --relative=subdir - check_$type dir/file2 --relative=sub +for type in diff numstat stat raw +do + check_$type . file2 --relative=subdir/ + check_$type . file2 --relative=subdir + check_$type subdir file2 --relative + check_$type . dir/file2 --relative=sub done test_done From f1e4fb2462894585d88c190aa9f16826f45ebbeb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 9 Dec 2017 21:40:13 +0100 Subject: [PATCH 7/7] t4045: reindent to make helpers readable Signed-off-by: Junio C Hamano --- t/t4045-diff-relative.sh | 104 +++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh index 22fcacfa33..6471a68701 100755 --- a/t/t4045-diff-relative.sh +++ b/t/t4045-diff-relative.sh @@ -12,60 +12,68 @@ test_expect_success 'setup' ' git commit -m one ' -check_diff() { -dir=$1; shift -expect=$1; shift -cat >expected <actual && - test_cmp expected actual -" +check_diff () { + dir=$1 + shift + expect=$1 + shift + cat >expected <<-EOF + diff --git a/$expect b/$expect + new file mode 100644 + index 0000000..25c05ef + --- /dev/null + +++ b/$expect + @@ -0,0 +1 @@ + +other content + EOF + test_expect_success "-p $*" " + git -C '$dir' diff -p $* HEAD^ >actual && + test_cmp expected actual + " } -check_numstat() { -dir=$1; shift -expect=$1; shift -cat >expected <expected && - git -C '$dir' diff --numstat $* HEAD^ >actual && - test_cmp expected actual -" +check_numstat () { + dir=$1 + shift + expect=$1 + shift + cat >expected <<-EOF + 1 0 $expect + EOF + test_expect_success "--numstat $*" " + echo '1 0 $expect' >expected && + git -C '$dir' diff --numstat $* HEAD^ >actual && + test_cmp expected actual + " } -check_stat() { -dir=$1; shift -expect=$1; shift -cat >expected <actual && - test_i18ncmp expected actual -" +check_stat () { + dir=$1 + shift + expect=$1 + shift + cat >expected <<-EOF + $expect | 1 + + 1 file changed, 1 insertion(+) + EOF + test_expect_success "--stat $*" " + git -C '$dir' diff --stat $* HEAD^ >actual && + test_i18ncmp expected actual + " } -check_raw() { -dir=$1; shift -expect=$1; shift -cat >expected <actual && - test_cmp expected actual -" +check_raw () { + dir=$1 + shift + expect=$1 + shift + cat >expected <<-EOF + :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A $expect + EOF + test_expect_success "--raw $*" " + git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual && + test_cmp expected actual + " } for type in diff numstat stat raw