{"thread":{"id":"47391","subject":"[PATCH] diff: add test showing regression to --relative","startedAt":"2017-12-07T00:35:26Z","lastAt":"2017-12-08T21:09:30Z","messageCount":15,"participants":["Jacob Keller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334296","messageId":"20171207003517.11729-1-jacob.e.keller@intel.com","threadId":"47391","inReplyTo":null,"subject":"[PATCH] diff: add test showing regression to --relative","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-12-07T00:35:17Z","receivedAt":"2017-12-07T00:35:26Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nSigned-off-by: Jacob Keller \n---\n t/t4045-diff-relative.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 3950f5034d31..41e4f59b2ffb 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -70,4 +70,9 @@ for type in diff numstat stat raw; do\n \tcheck_$type dir/file2 --relative=sub\n done\n \n+cd subdir\n+for type in diff numstat stat raw; do\n+\tcheck_$type file2 --relative\n+done\n+\n test_done\n-- \n2.15.1.477.g3ed0a2a61da8\n\n"},{"id":"334300","messageId":"20171207010421.GC1975@sigill.intra.peff.net","threadId":"47391","inReplyTo":"20171207003517.11729-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] diff: add test showing regression to --relative","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-07T01:04:21Z","receivedAt":"2017-12-07T01:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:\n\n> Subject: [PATCH] diff: add test showing regression to --relative\n\nSince we'd hopefully not ever merge that regression, I think this patch\nought to stand on its own. In which case it probably wants to say\nsomething like:\n\n diff: test --relative without a prefix\n\n We already test \"diff --relative=subdir\", but not that\n \"--relative\" by itself should use the current directory as\n its prefix.\n\n> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\n> index 3950f5034d31..41e4f59b2ffb 100755\n> --- a/t/t4045-diff-relative.sh\n> +++ b/t/t4045-diff-relative.sh\n> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do\n> \tcheck_$type dir/file2 --relative=sub\n> done\n> \n> +cd subdir\n> +for type in diff numstat stat raw; do\n> +\tcheck_$type file2 --relative\n> +done\n\nWe should avoid moving the cwd of the whole test script in\ncase we add tests later. Normally we'd do the cd inside a\nsubshell, but that's complicated by the wrapper (we wouldn't\nwant to increment the test counter just inside the subshell,\nfor instance).\n\nAdding \"cd ..\" is the smallest thing we could do to fix\nthat. But I think the more robust solution is to actually\nteach the check_* helper about doing the \"cd\" inside the\ntest_expect block. Or just pushing the helper down into the\ntest block and living with repeating the\n\"test_expect_success\" parts for each call.\n\n-Peff\n"},{"id":"334303","messageId":"CA+P7+xr8KobkY56-4qE4ZdVgC388L6=NKQc=3kTZvWz8XOD3ag@mail.gmail.com","threadId":"47391","inReplyTo":"20171207010421.GC1975@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add test showing regression to --relative","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T01:21:57Z","receivedAt":"2017-12-07T01:22:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Dec 6, 2017 at 5:04 PM, Jeff King wrote:\n> On Wed, Dec 06, 2017 at 04:35:17PM -0800, Jacob Keller wrote:\n>\n>> Subject: [PATCH] diff: add test showing regression to --relative\n>\n> Since we'd hopefully not ever merge that regression, I think this patch\n> ought to stand on its own. In which case it probably wants to say\n> something like:\n>\n> diff: test --relative without a prefix\n>\n> We already test \"diff --relative=subdir\", but not that\n> \"--relative\" by itself should use the current directory as\n> its prefix.\n>\n\nYea, I wasn't sure what the actual status of the changes were though,\nso I thought I'd send the actual fix I did. It's definitely up to\nChristian in determining what he thinks the best path forward is.\n\n>> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\n>> index 3950f5034d31..41e4f59b2ffb 100755\n>> --- a/t/t4045-diff-relative.sh\n>> +++ b/t/t4045-diff-relative.sh\n>> @@ -70,4 +70,9 @@ for type in diff numstat stat raw; do\n>> check_$type dir/file2 --relative=sub\n>> done\n>>\n>> +cd subdir\n>> +for type in diff numstat stat raw; do\n>> + check_$type file2 --relative\n>> +done\n>\n> We should avoid moving the cwd of the whole test script in\n> case we add tests later. Normally we'd do the cd inside a\n> subshell, but that's complicated by the wrapper (we wouldn't\n> want to increment the test counter just inside the subshell,\n> for instance).\n>\n> Adding \"cd ..\" is the smallest thing we could do to fix\n> that. But I think the more robust solution is to actually\n> teach the check_* helper about doing the \"cd\" inside the\n> test_expect block. Or just pushing the helper down into the\n> test block and living with repeating the\n> \"test_expect_success\" parts for each call.\n\n\nYea, I tried cd inside the subshell and it didn't work, so I did this\nas the quicker solution.\n\nI think the best method would be to update the check_* helper\nfunctions to take a dir parameter, and use that to do git -C \"dir\"\nwhen calling the diff commands.\n\nThanks,\nJake\n\n>\n> -Peff\n"},{"id":"334333","messageId":"20171207173034.12865-1-gitster@pobox.com","threadId":"47391","inReplyTo":"20171207003517.11729-1-jacob.e.keller@intel.com","subject":"[PATCH v2 0/7] reroll of cc/skip-to-optional-val","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T17:30:31Z","receivedAt":"2017-12-07T17:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'm sending out only the last three parts, as the changes necessary\nto 03/07 that incorrectly used the variant without _default() to\noverwrite options->prefix should be trivally obvious.\n\n05/07 uses the _default() variant so that the caller can react\ndifferently to \"--relative\" from \"--relative=\" correctly.\n\n06/07 is an update to t4045, so that the change made in 07/07\nbecomes readable.\n\n07/07 updates the test to verify the change in 05/07. I just made\nsure that these tests catch it if we deliberately reintroduce the\nbroken version from Christian's series on top.\n\nJunio C Hamano (3):\n diff: use skip-to-optional-val in parsing --relative\n t4045: reindent to make helpers readable\n t4045: test 'diff --relative' for real\n\n builtin/index-pack.c | 11 ++---\n diff.c | 50 +++++++--------------\n git-compat-util.h | 23 ++++++++++\n strbuf.c | 20 +++++++++\n t/t4045-diff-relative.sh | 111 ++++++++++++++++++++++++++++-------------------\n 5 files changed, 128 insertions(+), 87 deletions(-)\n\n-- \n2.15.1-480-gbc5668f98a\n\n"},{"id":"334334","messageId":"20171207173034.12865-3-gitster@pobox.com","threadId":"47391","inReplyTo":"20171207173034.12865-1-gitster@pobox.com","subject":"[PATCH v2 6/7] t4045: reindent to make helpers readable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T17:30:33Z","receivedAt":"2017-12-07T17:31:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n t/t4045-diff-relative.sh | 95 +++++++++++++++++++++++++-----------------------\n 1 file changed, 50 insertions(+), 45 deletions(-)\n\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 3950f5034d..fefd2f3f81 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -12,59 +12,64 @@ test_expect_success 'setup' '\n \tgit commit -m one\n '\n \n-check_diff() {\n-expect=$1; shift\n-cat >expected <actual &&\n-\ttest_cmp expected actual\n-\"\n+check_diff () {\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 diff -p $* HEAD^ >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n }\n \n-check_numstat() {\n-expect=$1; shift\n-cat >expected <expected &&\n-\tgit diff --numstat $* HEAD^ >actual &&\n-\ttest_cmp expected actual\n-\"\n+check_numstat () {\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 diff --numstat $* HEAD^ >actual &&\n+\t\ttest_cmp expected actual\n+\t\"\n }\n \n-check_stat() {\n-expect=$1; shift\n-cat >expected <actual &&\n-\ttest_i18ncmp expected actual\n-\"\n+check_stat () {\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 diff --stat $* HEAD^ >actual &&\n+\t\ttest_i18ncmp expected actual\n+\t\"\n }\n \n-check_raw() {\n-expect=$1; shift\n-cat >expected <actual &&\n-\ttest_cmp expected actual\n-\"\n+check_raw () {\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 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+for type in diff numstat stat raw\n+do\n \tcheck_$type file2 --relative=subdir/\n \tcheck_$type file2 --relative=subdir\n \tcheck_$type dir/file2 --relative=sub\n-- \n2.15.1-480-gbc5668f98a\n\n"},{"id":"334335","messageId":"20171207173034.12865-4-gitster@pobox.com","threadId":"47391","inReplyTo":"20171207173034.12865-1-gitster@pobox.com","subject":"[PATCH v2 7/7] t4045: test 'diff --relative' for real","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T17:30:34Z","receivedAt":"2017-12-07T17:31:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The existing tests only checked how well -relative= work,\nwithout testing --relative (without any value).\n\nSigned-off-by: Junio C Hamano \n---\n t/t4045-diff-relative.sh | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex fefd2f3f81..815cdd7295 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -25,7 +25,10 @@ check_diff () {\n \t+other content\n \tEOF\n \ttest_expect_success \"-p $*\" \"\n-\t\tgit diff -p $* HEAD^ >actual &&\n+\t\t(\n+\t\t\ttest -z \"$in_there\" || cd \"$in_there\"\n+\t\t\tgit diff -p $* HEAD^\n+\t\t) >actual &&\n \t\ttest_cmp expected actual\n \t\"\n }\n@@ -38,7 +41,10 @@ check_numstat () {\n \tEOF\n \ttest_expect_success \"--numstat $*\" \"\n \t\techo '1\t0\t$expect' >expected &&\n-\t\tgit diff --numstat $* HEAD^ >actual &&\n+\t\t(\n+\t\t\ttest -z \"$in_there\" || cd \"$in_there\"\n+\t\t\tgit diff --numstat $* HEAD^\n+\t\t) >actual &&\n \t\ttest_cmp expected actual\n \t\"\n }\n@@ -51,7 +57,10 @@ check_stat () {\n \t 1 file changed, 1 insertion(+)\n \tEOF\n \ttest_expect_success \"--stat $*\" \"\n-\t\tgit diff --stat $* HEAD^ >actual &&\n+\t\t(\n+\t\t\ttest -z \"$in_there\" || cd \"$in_there\"\n+\t\t\tgit diff --stat $* HEAD^\n+\t\t) >actual &&\n \t\ttest_i18ncmp expected actual\n \t\"\n }\n@@ -63,15 +72,22 @@ check_raw () {\n \t:000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A\t$expect\n \tEOF\n \ttest_expect_success \"--raw $*\" \"\n-\t\tgit diff --no-abbrev --raw $* HEAD^ >actual &&\n+\t\t(\n+\t\t\ttest -z \"$in_there\" || cd \"$in_there\"\n+\t\t\tgit diff --no-abbrev --raw $* HEAD^ >actual\n+\t\t) &&\n \t\ttest_cmp expected actual\n \t\"\n }\n \n for type in diff numstat stat raw\n do\n+\tin_there=\n \tcheck_$type file2 --relative=subdir/\n \tcheck_$type file2 --relative=subdir\n+\tin_there=subdir\n+\tcheck_$type file2 --relative\n+\tin_there=\n \tcheck_$type dir/file2 --relative=sub\n done\n \n-- \n2.15.1-480-gbc5668f98a\n\n"},{"id":"334336","messageId":"20171207173034.12865-2-gitster@pobox.com","threadId":"47391","inReplyTo":"20171207173034.12865-1-gitster@pobox.com","subject":"[PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T17:30:32Z","receivedAt":"2017-12-07T17:31:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n diff.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex cd032c6367..e99ac6ec8a 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_val_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-480-gbc5668f98a\n\n"},{"id":"334346","messageId":"CA+P7+xp0b-6936K3_2hN3LH4wh8k+Hkgukh6aC2CRF1QdQ_FUQ@mail.gmail.com","threadId":"47391","inReplyTo":"20171207173034.12865-2-gitster@pobox.com","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T19:03:26Z","receivedAt":"2017-12-07T19:03:52Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano wrote:\n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index cd032c6367..e99ac6ec8a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,\n> options->flags.rename_empty = 1;\n> else if (!strcmp(arg, \"--no-rename-empty\"))\n> options->flags.rename_empty = 0;\n> - else if (!strcmp(arg, \"--relative\"))\n> + else if (skip_to_optional_val_default(arg, \"--relative\", &arg, NULL)) {\n> options->flags.relative_name = 1;\n> - else if (skip_prefix(arg, \"--relative=\", &arg)) {\n> - options->flags.relative_name = 1;\n> - options->prefix = arg;\n> + if (arg)\n> + options->prefix = arg;\n> }\n>\n> /* xdiff options */\n> --\n> 2.15.1-480-gbc5668f98a\n>\n\nYea, this is exactly what I had imagined as the fix.\n\nThanks,\nJake\n"},{"id":"334348","messageId":"CA+P7+xoYEQ8sD+hSk+N95xAaRkMo15+kDRNB5rH5dgbGCfPieQ@mail.gmail.com","threadId":"47391","inReplyTo":"20171207173034.12865-4-gitster@pobox.com","subject":"Re: [PATCH v2 7/7] t4045: test 'diff --relative' for real","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T19:06:29Z","receivedAt":"2017-12-07T19:06:56Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano wrote:\n> The existing tests only checked how well -relative= work,\n> without testing --relative (without any value).\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t4045-diff-relative.sh | 24 ++++++++++++++++++++----\n> 1 file changed, 20 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\n> index fefd2f3f81..815cdd7295 100755\n> --- a/t/t4045-diff-relative.sh\n> +++ b/t/t4045-diff-relative.sh\n> @@ -25,7 +25,10 @@ check_diff () {\n> +other content\n> EOF\n> test_expect_success \"-p $*\" \"\n> - git diff -p $* HEAD^ >actual &&\n> + (\n> + test -z \"$in_there\" || cd \"$in_there\"\n> + git diff -p $* HEAD^\n> + ) >actual &&\n> test_cmp expected actual\n> \"\n> }\n> @@ -38,7 +41,10 @@ check_numstat () {\n> EOF\n> test_expect_success \"--numstat $*\" \"\n> echo '1 0 $expect' >expected &&\n> - git diff --numstat $* HEAD^ >actual &&\n> + (\n> + test -z \"$in_there\" || cd \"$in_there\"\n> + git diff --numstat $* HEAD^\n> + ) >actual &&\n> test_cmp expected actual\n> \"\n> }\n> @@ -51,7 +57,10 @@ check_stat () {\n> 1 file changed, 1 insertion(+)\n> EOF\n> test_expect_success \"--stat $*\" \"\n> - git diff --stat $* HEAD^ >actual &&\n> + (\n> + test -z \"$in_there\" || cd \"$in_there\"\n> + git diff --stat $* HEAD^\n> + ) >actual &&\n> test_i18ncmp expected actual\n> \"\n> }\n> @@ -63,15 +72,22 @@ check_raw () {\n> :000000 100644 0000000000000000000000000000000000000000 25c05ef3639d2d270e7fe765a67668f098092bc5 A $expect\n> EOF\n> test_expect_success \"--raw $*\" \"\n> - git diff --no-abbrev --raw $* HEAD^ >actual &&\n> + (\n> + test -z \"$in_there\" || cd \"$in_there\"\n> + git diff --no-abbrev --raw $* HEAD^ >actual\n> + ) &&\n\nYou could avoid the subshell by just passing $in_there to -C on the\ngit commands. Same for the other tests. If you quote it, -C\n'$in_there', then it will work regardless of if in_there is set or\nnot, (-C with an empty string doesn't cd anywhere). I think this is\ngenerally preferable for tests given it helps avoid unnecessary\nsubshells when testing on Windows..?\n\n> test_cmp expected actual\n> \"\n> }\n>\n> for type in diff numstat stat raw\n> do\n> + in_there=\n> check_$type file2 --relative=subdir/\n> check_$type file2 --relative=subdir\n> + in_there=subdir\n> + check_$type file2 --relative\n> + in_there=\n> check_$type dir/file2 --relative=sub\n> done\n>\n\nThis isn't quite what I had in mind for the directory parameter. I\npassed it as an extra argument, but I think this is probably more\nsensible.\n\n> --\n> 2.15.1-480-gbc5668f98a\n>\n"},{"id":"334369","messageId":"20171207211102.GA12850@sigill.intra.peff.net","threadId":"47391","inReplyTo":"20171207173034.12865-2-gitster@pobox.com","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-07T21:11:03Z","receivedAt":"2017-12-07T21:11:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:\n\n> Signed-off-by: Junio C Hamano \n> ---\n\nIt might be worth mentioning why this conversion is pulled out from the\nothers (because its \"default\" case is \"do not touch the pointer\").\n\nOther than that, it looks good to me.\n\n-Peff\n"},{"id":"334383","messageId":"xmqqpo7qb3n8.fsf@gitster.mtv.corp.google.com","threadId":"47391","inReplyTo":"20171207211102.GA12850@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T21:59:39Z","receivedAt":"2017-12-07T21:59:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:\n>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>\n> It might be worth mentioning why this conversion is pulled out from the\n> others (because its \"default\" case is \"do not touch the pointer\").\n\nI am not sure what you mean by \"pulled out from the others\". I did\nnot intend to keep these 3 additional patches permanently; rather, I\ndid them to help Christian's rerolling the series, and I do not think\nthis one should be separate from other ones that use the _default()\nvariant when that happens.\n\n\n"},{"id":"334386","messageId":"20171207222143.GA14845@sigill.intra.peff.net","threadId":"47391","inReplyTo":"xmqqpo7qb3n8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-07T22:21:43Z","receivedAt":"2017-12-07T22:21:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:\n> >\n> >> Signed-off-by: Junio C Hamano \n> >> ---\n> >\n> > It might be worth mentioning why this conversion is pulled out from the\n> > others (because its \"default\" case is \"do not touch the pointer\").\n> \n> I am not sure what you mean by \"pulled out from the others\". I did\n> not intend to keep these 3 additional patches permanently; rather, I\n> did them to help Christian's rerolling the series, and I do not think\n> this one should be separate from other ones that use the _default()\n> variant when that happens.\n\nAh, I see. I had thought you meant these to be applied on top.\n\n-Peff\n"},{"id":"334388","messageId":"xmqqlgieb25x.fsf@gitster.mtv.corp.google.com","threadId":"47391","inReplyTo":"20171207222143.GA14845@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T22:31:38Z","receivedAt":"2017-12-07T22:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:\n>> >\n>> >> Signed-off-by: Junio C Hamano \n>> >> ---\n>> >\n>> > It might be worth mentioning why this conversion is pulled out from the\n>> > others (because its \"default\" case is \"do not touch the pointer\").\n>> \n>> I am not sure what you mean by \"pulled out from the others\". I did\n>> not intend to keep these 3 additional patches permanently; rather, I\n>> did them to help Christian's rerolling the series, and I do not think\n>> this one should be separate from other ones that use the _default()\n>> variant when that happens.\n>\n> Ah, I see. I had thought you meant these to be applied on top.\n\nAh, I do not particularly mind doing things incrementally, either.\n\nIf this goes on top as a standalone patch, then the reason why it is\nseparate from the other users of _default() is not because the way\nit uses the null return is special, but because it was written by a\ndifferent author, I would think. \n\nThe reason why _default() variant exists is because its callers want\nto react differently to \"--foo\" from the way they react to \"--foo=\"\n(with an empty string as its value), and from that point of view,\nthis caller is not all that different from other ones, like the one\nthat parses --color-words Christian wrote in his initial round.\n\n"},{"id":"334401","messageId":"20171208090531.GC26199@sigill.intra.peff.net","threadId":"47391","inReplyTo":"xmqqlgieb25x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T09:05:31Z","receivedAt":"2017-12-08T09:05:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:\n\n> If this goes on top as a standalone patch, then the reason why it is\n> separate from the other users of _default() is not because the way\n> it uses the null return is special, but because it was written by a\n> different author, I would think.\n\nMostly I was just concerned that we missed a somewhat subtle bug in the\nfirst conversion, and I think it's worth calling out in the commit\nmessage why that callsite must be converted in a particular way. Whether\nthat happens in a separate commit or squashed, I don't care too much.\n\nBut I dunno. Maybe it is obvious now that the correct code is in there.\n;)\n\n-Peff\n"},{"id":"334498","messageId":"xmqqindh6i64.fsf@gitster.mtv.corp.google.com","threadId":"47391","inReplyTo":"20171208090531.GC26199@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T21:09:23Z","receivedAt":"2017-12-08T21:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:\n>\n>> If this goes on top as a standalone patch, then the reason why it is\n>> separate from the other users of _default() is not because the way\n>> it uses the null return is special, but because it was written by a\n>> different author, I would think.\n>\n> Mostly I was just concerned that we missed a somewhat subtle bug in the\n> first conversion, and I think it's worth calling out in the commit\n> message why that callsite must be converted in a particular way. Whether\n> that happens in a separate commit or squashed, I don't care too much.\n>\n> But I dunno. Maybe it is obvious now that the correct code is in there.\n> ;)\n\nIt probably is too obvious to me only because the use case like this\none that wanted to treat --foo and --foo=\"\" differently was the only\nreason why I pushed against Christian's original one that hardcoded\nthe equivalence without allowing what the _default() variant lets us\ndo, I think.\n"}]}