{"thread":{"id":"47392","subject":"[PATCH v2] diff: fix --relative without arguments","startedAt":"2017-12-07T00:59:40Z","lastAt":"2017-12-07T19:01:39Z","messageCount":3,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"334299","messageId":"20171207005929.24109-1-jacob.e.keller@intel.com","threadId":"47392","inReplyTo":null,"subject":"[PATCH v2] diff: fix --relative without arguments","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-12-07T00:59:29Z","receivedAt":"2017-12-07T00:59:40Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nRecently, commit f7923a5ece00 (\"diff: use skip_to_optional_val()\",\n2017-12-04) changed how we parsed some diff options, preferring\nskip_to_optional_val instead of a more complex skip_prefix() setup.\n\nUnfortunately, this breaks --relative, because of the semantics of\nskip_to_optional_val. If no optional_val is given, then the function\nputs the empty string \"\" into the arg. Unfortunately, relative passes in\noptions->prefix as the location for the optional argument, and expects\nthis value to remain unchanged if no optional value was given.\n\nThis results in breaking --relative when no arguments are provided. This\ncascades into many failures of code flow that rely on this.\n\nPartially revert the commit, and restore --relative option parsing to\nthe previous working code.\n\nSince no tests exist for --relative without options, add a new test\nwhich will help ensure no future regressions.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nActually perform the revert of --relative handling.\n\n diff.c                   | 6 +++++-\n t/t4045-diff-relative.sh | 5 +++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex b62e4b61b564..1919afb4f104 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4581,8 +4581,12 @@ 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 (skip_to_optional_val(arg, \"--relative\", &options->prefix))\n+\telse if (!strcmp(arg, \"--relative\"))\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}\n \n \t/* xdiff options */\n \telse if (!strcmp(arg, \"--minimal\"))\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":"334323","messageId":"xmqq4lp2ftjt.fsf@gitster.mtv.corp.google.com","threadId":"47392","inReplyTo":"20171207005929.24109-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v2] diff: fix --relative without arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T15:26:30Z","receivedAt":"2017-12-07T15:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Recently, commit f7923a5ece00 (\"diff: use skip_to_optional_val()\",\n> 2017-12-04) changed how we parsed some diff options, preferring\n> skip_to_optional_val instead of a more complex skip_prefix() setup.\n\nI'd expect a moral equivalent of this squashed in when Christian\nrerolls the skip-to-optional-val series (with helped-by: attribution\nto you, probably).  It does not make much sense to leave the initial\nbreakage like this one in the history.\n\nThanks.\n"},{"id":"334345","messageId":"CA+P7+xpAFJcvcQ9PN5ZsnQxf8GpSG2yYKSYEg5wHebyCLtvdsw@mail.gmail.com","threadId":"47392","inReplyTo":"xmqq4lp2ftjt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] diff: fix --relative without arguments","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-07T19:01:12Z","receivedAt":"2017-12-07T19:01:39Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Dec 7, 2017 at 7:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> From: Jacob Keller <jacob.keller@gmail.com>\n>>\n>> Recently, commit f7923a5ece00 (\"diff: use skip_to_optional_val()\",\n>> 2017-12-04) changed how we parsed some diff options, preferring\n>> skip_to_optional_val instead of a more complex skip_prefix() setup.\n>\n> I'd expect a moral equivalent of this squashed in when Christian\n> rerolls the skip-to-optional-val series (with helped-by: attribution\n> to you, probably).  It does not make much sense to leave the initial\n> breakage like this one in the history.\n>\n> Thanks.\n\nQuite correct. I'll be sending a patch with just new tests shortly.\n\nThanks,\nJake\n"}]}