{"thread":{"id":"47416","subject":"[PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","startedAt":"2017-12-09T20:40:31Z","lastAt":"2017-12-11T23:23:22Z","messageCount":11,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":7},"messages":[{"id":"334547","messageId":"20171209204013.10997-1-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":null,"subject":"[PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:07Z","receivedAt":"2017-12-09T20:40:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nWe often accept both a \"--key\" option and a \"--key=<val>\" option.\n\nThese options currently are parsed using something like:\n\nif (!strcmp(arg, \"--key\")) {\n\t/* do something */\n} else if (skip_prefix(arg, \"--key=\", &arg)) {\n\t/* do something with arg */\n}\n\nwhich is a bit cumbersome compared to just:\n\nif (skip_to_optional_arg(arg, \"--key\", &arg)) {\n\t/* do something with arg */\n}\n\nThis also introduces skip_to_optional_arg_default() for the few\ncases where something different should be done when the first\nargument is exactly \"--key\" than when it is exactly \"--key=\".\n\nIn general it is better for UI consistency and simplicity if\n\"--key\" and \"--key=\" do the same thing though, so that using\nskip_to_optional_arg() should be encouraged compared to\nskip_to_optional_arg_default().\n\nNote that these functions can be used to parse any \"key=value\"\nstring where \"key\" is also considered as valid, not just\ncommand line options.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-compat-util.h | 23 +++++++++++++++++++++++\n strbuf.c          | 22 ++++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n\nThe changes compared to v2 are:\n\n  - s/_val/_arg/ in the name of the functions\n  - s/val/arg/ in the name of the third argument of the functions\n  - works with NULL as third argument of the functions\n  - \"--relative\" is handled correctly in a separate new patch from Junio\n  - tests are added for \"--relative\" in a separate new patch from Jake\n  - a new test reindenting patch from Junio is added\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d581..68b2ad531e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -484,6 +484,29 @@ static inline int skip_prefix(const char *str, const char *prefix,\n \treturn 0;\n }\n \n+/*\n+ * If the string \"str\" is the same as the string in \"prefix\", then the \"arg\"\n+ * parameter is set to the \"def\" parameter and 1 is returned.\n+ * If the string \"str\" begins with the string found in \"prefix\" and then a\n+ * \"=\" sign, then the \"arg\" parameter is set to \"str + strlen(prefix) + 1\"\n+ * (i.e., to the point in the string right after the prefix and the \"=\" sign),\n+ * and 1 is returned.\n+ *\n+ * Otherwise, return 0 and leave \"arg\" untouched.\n+ *\n+ * When we accept both a \"--key\" and a \"--key=<val>\" option, this function\n+ * can be used instead of !strcmp(arg, \"--key\") and then\n+ * skip_prefix(arg, \"--key=\", &arg) to parse such an option.\n+ */\n+int skip_to_optional_arg_default(const char *str, const char *prefix,\n+\t\t\t\t const char **arg, const char *def);\n+\n+static inline int skip_to_optional_arg(const char *str, const char *prefix,\n+\t\t\t\t       const char **arg)\n+{\n+\treturn skip_to_optional_arg_default(str, prefix, arg, \"\");\n+}\n+\n /*\n  * Like skip_prefix, but promises never to read past \"len\" bytes of the input\n  * buffer, and returns the remaining number of bytes in \"out\" via \"outlen\".\ndiff --git a/strbuf.c b/strbuf.c\nindex 323c49ceb3..29169b8ef8 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -11,6 +11,28 @@ int starts_with(const char *str, const char *prefix)\n \t\t\treturn 0;\n }\n \n+int skip_to_optional_arg_default(const char *str, const char *prefix,\n+\t\t\t\t const char **arg, const char *def)\n+{\n+\tconst char *p;\n+\n+\tif (!skip_prefix(str, prefix, &p))\n+\t\treturn 0;\n+\n+\tif (!*p) {\n+\t\tif (arg)\n+\t\t\t*arg = def;\n+\t\treturn 1;\n+\t}\n+\n+\tif (*p != '=')\n+\t\treturn 0;\n+\n+\tif (arg)\n+\t\t*arg = p + 1;\n+\treturn 1;\n+}\n+\n /*\n  * Used as the default ->buf value, so that people can always assume\n  * buf is non NULL and ->buf is NUL terminated even for a freshly\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334548","messageId":"20171209204013.10997-2-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 2/7] index-pack: use skip_to_optional_arg()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:08Z","receivedAt":"2017-12-09T20:40:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nLet's simplify index-pack option parsing using\nskip_to_optional_arg().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/index-pack.c | 11 +++--------\n 1 file changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8ec459f522..4c51aec81f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1660,10 +1660,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfrom_stdin = 1;\n \t\t\t} else if (!strcmp(arg, \"--fix-thin\")) {\n \t\t\t\tfix_thin_pack = 1;\n-\t\t\t} else if (!strcmp(arg, \"--strict\")) {\n-\t\t\t\tstrict = 1;\n-\t\t\t\tdo_fsck_object = 1;\n-\t\t\t} else if (skip_prefix(arg, \"--strict=\", &arg)) {\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--strict\", &arg)) {\n \t\t\t\tstrict = 1;\n \t\t\t\tdo_fsck_object = 1;\n \t\t\t\tfsck_set_msg_types(&fsck_options, arg);\n@@ -1679,10 +1676,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tverify = 1;\n \t\t\t\tshow_stat = 1;\n \t\t\t\tstat_only = 1;\n-\t\t\t} else if (!strcmp(arg, \"--keep\")) {\n-\t\t\t\tkeep_msg = \"\";\n-\t\t\t} else if (starts_with(arg, \"--keep=\")) {\n-\t\t\t\tkeep_msg = arg + 7;\n+\t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n+\t\t\t\t; /* nothing to do */\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334549","messageId":"20171209204013.10997-3-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 3/7] diff: use skip_to_optional_arg()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:09Z","receivedAt":"2017-12-09T20:40:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nLet's simplify diff option parsing using skip_to_optional_arg().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 23 +++++++++--------------\n 1 file changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2ebe2227b4..464a53adb5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4508,17 +4508,12 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->output_format |= DIFF_FORMAT_NUMSTAT;\n \telse if (!strcmp(arg, \"--shortstat\"))\n \t\toptions->output_format |= DIFF_FORMAT_SHORTSTAT;\n-\telse if (!strcmp(arg, \"-X\") || !strcmp(arg, \"--dirstat\"))\n-\t\treturn parse_dirstat_opt(options, \"\");\n-\telse if (skip_prefix(arg, \"-X\", &arg))\n-\t\treturn parse_dirstat_opt(options, arg);\n-\telse if (skip_prefix(arg, \"--dirstat=\", &arg))\n+\telse if (skip_prefix(arg, \"-X\", &arg) ||\n+\t\t skip_to_optional_arg(arg, \"--dirstat\", &arg))\n \t\treturn parse_dirstat_opt(options, arg);\n \telse if (!strcmp(arg, \"--cumulative\"))\n \t\treturn parse_dirstat_opt(options, \"cumulative\");\n-\telse if (!strcmp(arg, \"--dirstat-by-file\"))\n-\t\treturn parse_dirstat_opt(options, \"files\");\n-\telse if (skip_prefix(arg, \"--dirstat-by-file=\", &arg)) {\n+\telse if (skip_to_optional_arg(arg, \"--dirstat-by-file\", &arg)) {\n \t\tparse_dirstat_opt(options, \"files\");\n \t\treturn parse_dirstat_opt(options, arg);\n \t}\n@@ -4540,13 +4535,13 @@ int diff_opt_parse(struct diff_options *options,\n \t\treturn stat_opt(options, av);\n \n \t/* renames options */\n-\telse if (starts_with(arg, \"-B\") || starts_with(arg, \"--break-rewrites=\") ||\n-\t\t !strcmp(arg, \"--break-rewrites\")) {\n+\telse if (starts_with(arg, \"-B\") ||\n+\t\t skip_to_optional_arg(arg, \"--break-rewrites\", NULL)) {\n \t\tif ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n \t\t\treturn error(\"invalid argument to -B: %s\", arg+2);\n \t}\n-\telse if (starts_with(arg, \"-M\") || starts_with(arg, \"--find-renames=\") ||\n-\t\t !strcmp(arg, \"--find-renames\")) {\n+\telse if (starts_with(arg, \"-M\") ||\n+\t\t skip_to_optional_arg(arg, \"--find-renames\", NULL)) {\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n \t\t\treturn error(\"invalid argument to -M: %s\", arg+2);\n \t\toptions->detect_rename = DIFF_DETECT_RENAME;\n@@ -4554,8 +4549,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n \t\toptions->irreversible_delete = 1;\n \t}\n-\telse if (starts_with(arg, \"-C\") || starts_with(arg, \"--find-copies=\") ||\n-\t\t !strcmp(arg, \"--find-copies\")) {\n+\telse if (starts_with(arg, \"-C\") ||\n+\t\t skip_to_optional_arg(arg, \"--find-copies\", NULL)) {\n \t\tif (options->detect_rename == DIFF_DETECT_COPY)\n \t\t\toptions->flags.find_copies_harder = 1;\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334550","messageId":"20171209204013.10997-4-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 4/7] diff: use skip_to_optional_arg_default()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:10Z","receivedAt":"2017-12-09T20:40:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nLet's simplify diff option parsing using\nskip_to_optional_arg_default().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 464a53adb5..28e1ab168f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4623,9 +4623,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-follow\")) {\n \t\toptions->flags.follow_renames = 0;\n \t\toptions->flags.default_follow_renames = 0;\n-\t} else if (!strcmp(arg, \"--color\"))\n-\t\toptions->use_color = 1;\n-\telse if (skip_prefix(arg, \"--color=\", &arg)) {\n+\t} else if (skip_to_optional_arg_default(arg, \"--color\", &arg, \"always\")) {\n \t\tint value = git_config_colorbool(NULL, arg);\n \t\tif (value < 0)\n \t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n@@ -4645,14 +4643,9 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n-\t} else if (!strcmp(arg, \"--color-words\")) {\n-\t\toptions->use_color = 1;\n-\t\toptions->word_diff = DIFF_WORDS_COLOR;\n-\t}\n-\telse if (skip_prefix(arg, \"--color-words=\", &arg)) {\n+\t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n-\t\toptions->word_regex = arg;\n \t}\n \telse if (!strcmp(arg, \"--word-diff\")) {\n \t\tif (options->word_diff == DIFF_WORDS_NONE)\n@@ -4691,15 +4684,10 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->flags.textconv_set_via_cmdline = 1;\n \t} else if (!strcmp(arg, \"--no-textconv\"))\n \t\toptions->flags.allow_textconv = 0;\n-\telse if (!strcmp(arg, \"--ignore-submodules\")) {\n-\t\toptions->flags.override_submodule_config = 1;\n-\t\thandle_ignore_submodules_arg(options, \"all\");\n-\t} else if (skip_prefix(arg, \"--ignore-submodules=\", &arg)) {\n+\telse if (skip_to_optional_arg_default(arg, \"--ignore-submodules\", &arg, \"all\")) {\n \t\toptions->flags.override_submodule_config = 1;\n \t\thandle_ignore_submodules_arg(options, arg);\n-\t} else if (!strcmp(arg, \"--submodule\"))\n-\t\toptions->submodule_format = DIFF_SUBMODULE_LOG;\n-\telse if (skip_prefix(arg, \"--submodule=\", &arg))\n+\t} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n \t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\n \t\treturn parse_ws_error_highlight_opt(options, arg);\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334551","messageId":"20171209204013.10997-5-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 5/7] diff: use skip_to_optional_arg_default() in parsing --relative","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:11Z","receivedAt":"2017-12-09T20:40:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nHelped-by: Jacob Keller <jacob.keller@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 28e1ab168f..3f14cdace6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->flags.rename_empty = 1;\n \telse if (!strcmp(arg, \"--no-rename-empty\"))\n \t\toptions->flags.rename_empty = 0;\n-\telse if (!strcmp(arg, \"--relative\"))\n+\telse if (skip_to_optional_arg_default(arg, \"--relative\", &arg, NULL)) {\n \t\toptions->flags.relative_name = 1;\n-\telse if (skip_prefix(arg, \"--relative=\", &arg)) {\n-\t\toptions->flags.relative_name = 1;\n-\t\toptions->prefix = arg;\n+\t\tif (arg)\n+\t\t\toptions->prefix = arg;\n \t}\n \n \t/* xdiff options */\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334552","messageId":"20171209204013.10997-6-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 6/7] diff: add tests for --relative without optional prefix value","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:12Z","receivedAt":"2017-12-09T20:40:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nWe already have tests for --relative, but they currently only test when\na prefix has been provided. This fails to test the case where --relative\nby itself should use the current directory as the prefix.\n\nTeach the check_$type functions to take a directory argument to indicate\nwhich subdirectory to run the git commands in. Add a new test which uses\nthis to test --relative without a prefix value.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n t/t4045-diff-relative.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 3950f5034d..7d68a6e2a5 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -13,6 +13,7 @@ test_expect_success 'setup' '\n '\n \n check_diff() {\n+dir=$1; shift\n expect=$1; shift\n cat >expected <<EOF\n diff --git a/$expect b/$expect\n@@ -24,50 +25,54 @@ index 0000000..25c05ef\n +other content\n EOF\n test_expect_success \"-p $*\" \"\n-\tgit diff -p $* HEAD^ >actual &&\n+\tgit -C '$dir' diff -p $* HEAD^ >actual &&\n \ttest_cmp expected actual\n \"\n }\n \n check_numstat() {\n+dir=$1; shift\n expect=$1; shift\n cat >expected <<EOF\n 1\t0\t$expect\n EOF\n test_expect_success \"--numstat $*\" \"\n \techo '1\t0\t$expect' >expected &&\n-\tgit diff --numstat $* HEAD^ >actual &&\n+\tgit -C '$dir' diff --numstat $* HEAD^ >actual &&\n \ttest_cmp expected actual\n \"\n }\n \n check_stat() {\n+dir=$1; shift\n expect=$1; shift\n cat >expected <<EOF\n  $expect | 1 +\n  1 file changed, 1 insertion(+)\n EOF\n test_expect_success \"--stat $*\" \"\n-\tgit diff --stat $* HEAD^ >actual &&\n+\tgit -C '$dir' diff --stat $* HEAD^ >actual &&\n \ttest_i18ncmp expected actual\n \"\n }\n \n check_raw() {\n+dir=$1; shift\n expect=$1; shift\n cat >expected <<EOF\n :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A\t$expect\n EOF\n test_expect_success \"--raw $*\" \"\n-\tgit diff --no-abbrev --raw $* HEAD^ >actual &&\n+\tgit -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&\n \ttest_cmp expected actual\n \"\n }\n \n for type in diff numstat stat raw; do\n-\tcheck_$type file2 --relative=subdir/\n-\tcheck_$type file2 --relative=subdir\n-\tcheck_$type dir/file2 --relative=sub\n+\tcheck_$type . file2 --relative=subdir/\n+\tcheck_$type . file2 --relative=subdir\n+\tcheck_$type . dir/file2 --relative=sub\n+\tcheck_$type subdir file2 --relative\n done\n \n test_done\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334553","messageId":"20171209204013.10997-7-chriscool@tuxfamily.org","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"[PATCH v3 7/7] t4045: reindent to make helpers readable","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-09T20:40:13Z","receivedAt":"2017-12-09T20:40:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4045-diff-relative.sh | 104 +++++++++++++++++++++++++----------------------\n 1 file changed, 56 insertions(+), 48 deletions(-)\n\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 7d68a6e2a5..4df55884c4 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -12,60 +12,68 @@ test_expect_success 'setup' '\n \tgit commit -m one\n '\n \n-check_diff() {\n-dir=$1; shift\n-expect=$1; shift\n-cat >expected <<EOF\n-diff --git a/$expect b/$expect\n-new file mode 100644\n-index 0000000..25c05ef\n---- /dev/null\n-+++ b/$expect\n-@@ -0,0 +1 @@\n-+other content\n-EOF\n-test_expect_success \"-p $*\" \"\n-\tgit -C '$dir' diff -p $* HEAD^ >actual &&\n-\ttest_cmp expected actual\n-\"\n+check_diff () {\n+\tdir=$1\n+\tshift\n+\texpect=$1\n+\tshift\n+\tcat >expected <<-EOF\n+\tdiff --git a/$expect b/$expect\n+\tnew file mode 100644\n+\tindex 0000000..25c05ef\n+\t--- /dev/null\n+\t+++ b/$expect\n+\t@@ -0,0 +1 @@\n+\t+other content\n+\tEOF\n+\ttest_expect_success \"-p $*\" \"\n+\t\tgit -C '$dir' diff -p $* HEAD^ >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n }\n \n-check_numstat() {\n-dir=$1; shift\n-expect=$1; shift\n-cat >expected <<EOF\n-1\t0\t$expect\n-EOF\n-test_expect_success \"--numstat $*\" \"\n-\techo '1\t0\t$expect' >expected &&\n-\tgit -C '$dir' diff --numstat $* HEAD^ >actual &&\n-\ttest_cmp expected actual\n-\"\n+check_numstat () {\n+\tdir=$1\n+\tshift\n+\texpect=$1\n+\tshift\n+\tcat >expected <<-EOF\n+\t1\t0\t$expect\n+\tEOF\n+\ttest_expect_success \"--numstat $*\" \"\n+\t\techo '1\t0\t$expect' >expected &&\n+\t\tgit -C '$dir' diff --numstat $* HEAD^ >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n }\n \n-check_stat() {\n-dir=$1; shift\n-expect=$1; shift\n-cat >expected <<EOF\n- $expect | 1 +\n- 1 file changed, 1 insertion(+)\n-EOF\n-test_expect_success \"--stat $*\" \"\n-\tgit -C '$dir' diff --stat $* HEAD^ >actual &&\n-\ttest_i18ncmp expected actual\n-\"\n+check_stat () {\n+\tdir=$1\n+\tshift\n+\texpect=$1\n+\tshift\n+\tcat >expected <<-EOF\n+\t $expect | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\tEOF\n+\ttest_expect_success \"--stat $*\" \"\n+\t\tgit -C '$dir' diff --stat $* HEAD^ >actual &&\n+\t\ttest_i18ncmp expected actual\n+\t\"\n }\n \n-check_raw() {\n-dir=$1; shift\n-expect=$1; shift\n-cat >expected <<EOF\n-:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A\t$expect\n-EOF\n-test_expect_success \"--raw $*\" \"\n-\tgit -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&\n-\ttest_cmp expected actual\n-\"\n+check_raw () {\n+\tdir=$1\n+\tshift\n+\texpect=$1\n+\tshift\n+\tcat >expected <<-EOF\n+\t:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A\t$expect\n+\tEOF\n+\ttest_expect_success \"--raw $*\" \"\n+\t\tgit -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n }\n \n for type in diff numstat stat raw; do\n-- \n2.15.1.361.g8b07d831d0\n\n"},{"id":"334566","messageId":"20171210143118.GB19453@sigill.intra.peff.net","threadId":"47416","inReplyTo":"20171209204013.10997-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-10T14:31:19Z","receivedAt":"2017-12-10T14:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:\n\n> The changes compared to v2 are:\n> \n>   - s/_val/_arg/ in the name of the functions\n>   - s/val/arg/ in the name of the third argument of the functions\n>   - works with NULL as third argument of the functions\n\nThis whole series looks OK to me, but this third point made me wonder:\nwhat would be the use of allowing NULL for the \"arg\" parameter?\n\nI didn't see any use of this in the series, and I'm having trouble\nfiguring out how it would be useful. E.g., if I do:\n\n  if (skip_to_optional_arg(arg, \"--foo\", NULL))\n     ...\n\nwhat can I do in \"...\"? I know we matched _some_ type of \"--foo\", but I\ncannot know whether it was \"--foo\" or \"--foo=bar\", nor what \"bar\" is. It\ncould only be used by some kind of vague validator to say \"well, at\nleast this looks like an option that I _could_ parse if I wanted to\".\n\nSo I guess I don't mind it, as it does the most reasonable thing it can\nwhen passed NULL, but I would be surprised if we ever actually exercise\nthe code path.\n\n-Peff\n"},{"id":"334567","messageId":"20171210143914.GA19894@sigill.intra.peff.net","threadId":"47416","inReplyTo":"20171210143118.GB19453@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-10T14:39:15Z","receivedAt":"2017-12-10T14:39:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 10, 2017 at 09:31:18AM -0500, Jeff King wrote:\n\n> On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:\n> \n> > The changes compared to v2 are:\n> > \n> >   - s/_val/_arg/ in the name of the functions\n> >   - s/val/arg/ in the name of the third argument of the functions\n> >   - works with NULL as third argument of the functions\n> \n> This whole series looks OK to me, but this third point made me wonder:\n> what would be the use of allowing NULL for the \"arg\" parameter?\n> \n> I didn't see any use of this in the series, and I'm having trouble\n> figuring out how it would be useful. E.g., if I do:\n> \n>   if (skip_to_optional_arg(arg, \"--foo\", NULL))\n>      ...\n> \n> what can I do in \"...\"? I know we matched _some_ type of \"--foo\", but I\n> cannot know whether it was \"--foo\" or \"--foo=bar\", nor what \"bar\" is. It\n> could only be used by some kind of vague validator to say \"well, at\n> least this looks like an option that I _could_ parse if I wanted to\".\n> \n> So I guess I don't mind it, as it does the most reasonable thing it can\n> when passed NULL, but I would be surprised if we ever actually exercise\n> the code path.\n\nAnd of course as soon as I sent this, I went back and double-checked.\nAnd indeed I totally missed this call:\n\n  +       else if (starts_with(arg, \"-B\") ||\n  +                skip_to_optional_arg(arg, \"--break-rewrites\", NULL)) {\n\t  if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n\nSo that's kind-of weird, because we are parsing \"-B\", etc, and then\nexpecting it to be _reparsed_ by diff_scoreopt_parse. So the two\ncallsites must always match. IMHO this ought to do either:\n\n  - we should just ask diff_scoreopt_parser to tell us if this was a\n    valid option that it understood\n\nor\n\n  - parse up to the \"=\", and then ask the scoreopt parser to parse the\n    remainder. This would require us passing 'B'/'C'/'M' to the\n    function ourselves, I think that's a better pattern. It means we\n    could reuse the parser for things like config values if we wanted to\n    (our current diff.renames is a bool, but it would not be\n    unreasonable for it to take a score).\n\nNone of that is a mess of your creation, though, so I'm OK punting on it\nfor now.\n\n-Peff\n"},{"id":"334589","messageId":"CAP8UFD26ZJQWtfzETHXjPoKWyL0f3qEuimohVysYo0WtmpSyBg@mail.gmail.com","threadId":"47416","inReplyTo":"20171210143914.GA19894@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-11T05:56:03Z","receivedAt":"2017-12-11T06:04:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 10, 2017 at 3:39 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, Dec 10, 2017 at 09:31:18AM -0500, Jeff King wrote:\n>\n>> On Sat, Dec 09, 2017 at 09:40:07PM +0100, Christian Couder wrote:\n>>\n>> > The changes compared to v2 are:\n>> >\n>> >   - s/_val/_arg/ in the name of the functions\n>> >   - s/val/arg/ in the name of the third argument of the functions\n>> >   - works with NULL as third argument of the functions\n>>\n>> This whole series looks OK to me, but this third point made me wonder:\n>> what would be the use of allowing NULL for the \"arg\" parameter?\n>>\n>> I didn't see any use of this in the series, and I'm having trouble\n>> figuring out how it would be useful. E.g., if I do:\n>>\n>>   if (skip_to_optional_arg(arg, \"--foo\", NULL))\n>>      ...\n>>\n>> what can I do in \"...\"? I know we matched _some_ type of \"--foo\", but I\n>> cannot know whether it was \"--foo\" or \"--foo=bar\", nor what \"bar\" is. It\n>> could only be used by some kind of vague validator to say \"well, at\n>> least this looks like an option that I _could_ parse if I wanted to\".\n>>\n>> So I guess I don't mind it, as it does the most reasonable thing it can\n>> when passed NULL, but I would be surprised if we ever actually exercise\n>> the code path.\n>\n> And of course as soon as I sent this, I went back and double-checked.\n> And indeed I totally missed this call:\n>\n>   +       else if (starts_with(arg, \"-B\") ||\n>   +                skip_to_optional_arg(arg, \"--break-rewrites\", NULL)) {\n>           if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n\nYeah, calls like this were discussed in:\nhttps://public-inbox.org/git/xmqqh8t6o9me.fsf@gitster.mtv.corp.google.com/\n\n> So that's kind-of weird, because we are parsing \"-B\", etc, and then\n> expecting it to be _reparsed_ by diff_scoreopt_parse. So the two\n> callsites must always match. IMHO this ought to do either:\n>\n>   - we should just ask diff_scoreopt_parser to tell us if this was a\n>     valid option that it understood\n>\n> or\n>\n>   - parse up to the \"=\", and then ask the scoreopt parser to parse the\n>     remainder. This would require us passing 'B'/'C'/'M' to the\n>     function ourselves, I think that's a better pattern. It means we\n>     could reuse the parser for things like config values if we wanted to\n>     (our current diff.renames is a bool, but it would not be\n>     unreasonable for it to take a score).\n>\n> None of that is a mess of your creation, though, so I'm OK punting on it\n> for now.\n\nYeah, this could be part of the #leftoverbits.\n"},{"id":"334640","messageId":"xmqqmv2o4zoc.fsf@gitster.mtv.corp.google.com","threadId":"47416","inReplyTo":"20171210143914.GA19894@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-11T23:23:15Z","receivedAt":"2017-12-11T23:23:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>\n>   +       else if (starts_with(arg, \"-B\") ||\n>   +                skip_to_optional_arg(arg, \"--break-rewrites\", NULL)) {\n> \t  if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)\n>\n> So that's kind-of weird, because we are parsing \"-B\", etc, and then\n> expecting it to be _reparsed_ by diff_scoreopt_parse. So the two\n> callsites must always match.\n\nCorrect.  diff_scoreopt_parse() can be coaxed to fit better within\nthis if/else if/... cascade by making it take a pointer to .break_opt\nfield and have it return \"did I handle the -B/break-rewrites?\" etc.,\nbut otherwise, this shows that skip_to_optional_arg() has impedance\nmismatch with its current API.  And the NULL thing serves as a reminder\nthat skip_to_optional_arg() is used _only_ as a prefix check and not\nfor parsing.\n\n\n"}]}