{"thread":{"id":"57533","subject":"RFC: Using '--no-output-indicator-old' to only show new state","startedAt":"2022-03-10T19:41:11Z","lastAt":"2022-03-11T19:42:39Z","messageCount":11,"participants":["Linus Torvalds","Junio C Hamano","Ævar Arnfjörð Bjarmason","Konstantin Ryabitsev"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"451071","messageId":"CAHk-=wgh8emJn-+FtxN=m_SCPiP6cGKHU-5ozzV9tWBMxn+xcA@mail.gmail.com","threadId":"57533","inReplyTo":null,"subject":"RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-10T19:40:50Z","receivedAt":"2022-03-10T19:41:11Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"So after another round of doing\n\n git diff | grep -v '^-'\n\nto just show what the end result of a patch is, I decided that there\nhas to be a better way.\n\nOf course, to normal people, that \"better way\" is probably some GUI\ntool that shows diffs as \"before/after\" in two different frames next\nto each other, but I'm a grumpy old man (\"Get off my lawn\") and I do\neverything but read my email in a standard text-only terminal.\n\nSo this RFC is that better way for me.\n\nIt's a RFC because maybe people think this is stupid, but even more so\nbecasue I think it should also go with an actual new and more legible\nname - which this tech demonstration does *not* do, because I suspect\npeople have more opinions on the option name than they have on the\ntrivial technical parts.\n\nIOW, with this patch, you can do\n\n git show --no-output-indicator-old\n\nand it shows the diff but without the 'old' lines. That option name is\nhorrible to write, though, and even with command line completion I\nthink it migth be worth doing as\n\n git show --new\n\nor something like that.\n\nWhen would you want this? The case _I_ tend to use that \"grep -v '^-'\"\nthing is when I re-write a function, and a regular diff really is just\nvery messy and what I really want to see is just the end result to\nverify that \"yeah, this looks sane\".\n\nIn case people care, the immediate (current) cause of this was kernel\ncommit fe673d3f5bf1 (\"mm: gup: make fault_in_safe_writeable() use\nfixup_user_fault()\"), which really is a fairly messy diff, but what\nmatters is the much simplified end result of the\nfault_in_safe_writeable() function.\n\nBut in the commit message, I put a random git commit as an example\ninstead, just to make it easier to see what's up when you're a git\ndeveloper, not a kernel developer.\n\nAnd hey, there are probably smarter things that could be done: this\ndoesn't work with word-diffs, for example. Maybe people would want an\nequivalent \"don't show the deleted word\" instead. But this is really\nreally simple.\n\nAnd maybe there already was some better way to do this.\n\nComments?\n\n(And for consistency, this obviously also allows\n\"--no-output-indicator-new\", but I've never found _that_ particularly\nuseful. But GUI diff tools most certainly also tend to show the old\nstate on one side, so maybe it's would be reasonable to use as a\n\"compare the old state vs the new state\" in terminals side-by-side. I\njust tend to care much more about \"what is the end result\")\n\n Linus\n\n\nFrom 95e926c38267eb7ec8956ea47db55bf8080fb282 Mon Sep 17 00:00:00 2001\nFrom: Linus Torvalds \nDate: Thu, 10 Mar 2022 11:00:43 -0800\nSubject: [PATCH 1/2] Allow '--no-output-indicator-{old,new}' to disable diff\n output\n\nThis is particularly useful if you want to just see the end result of a\ndiff, without the original lines that have been removed (or the reverse).\n\nThat's a fairly common model in various GUI diff viewers, but it's not\nunusual to want to just see \"what is the end result of my changes\"\nwithout seeing (a) everything that didn't change and (b) the old removed\nstate.\n\nExample:\n\n git show --no-output-indicator-old 04bf052eef\n\nto see just the end result of the changes in commit 04bf052eef (\"grep:\nsimplify config parsing and option parsing\").\n\nThis is a technology presentation, I think it needs a shorter option\nname too if people agree that this is useful (this basically replaces my\nhacky\n\n git diff | grep -v '^-'\n\nthat I use for the same purpose, but the very long argument name\nobviously makes it no more convenient, although the end result is\nqualitatively better).\n\nSigned-off-by: Linus Torvalds \n---\n diff.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2bd5e0d817..895951b849 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1254,6 +1254,8 @@ static void emit_line_ws_markup(struct diff_options *o,\n \tconst char *ws = NULL;\n \tint sign = o->output_indicators[sign_index];\n \n+\tif (!sign)\n+\t\treturn;\n \tif (o->ws_error_highlight & ws_rule) {\n \t\tws = diff_get_color_opt(o, DIFF_WHITESPACE);\n \t\tif (!*ws)\n@@ -4986,6 +4988,10 @@ static int diff_opt_char(const struct option *opt,\n {\n \tchar *value = opt->value;\n \n+\tif (unset) {\n+\t\t*value = 0;\n+\t\treturn 0;\n+\t}\n \tBUG_ON_OPT_NEG(unset);\n \tif (arg[1])\n \t\treturn error(_(\"%s expects a character, got '%s'\"),\n@@ -5476,12 +5482,12 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW],\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate a new line instead of '+'\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\t\t 0, diff_opt_char),\n \t\tOPT_CALLBACK_F(0, \"output-indicator-old\",\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD],\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate an old line instead of '-'\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\t\t 0, diff_opt_char),\n \t\tOPT_CALLBACK_F(0, \"output-indicator-context\",\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_CONTEXT],\n \t\t\t N_(\"\"),\n-- \n2.35.1.459.gabbef95d73\n\n"},{"id":"451074","messageId":"xmqqo82dd0qv.fsf@gitster.g","threadId":"57533","inReplyTo":"CAHk-=wgh8emJn-+FtxN=m_SCPiP6cGKHU-5ozzV9tWBMxn+xcA@mail.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T20:13:44Z","receivedAt":"2022-03-10T20:13:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So after another round of doing\n>\n> git diff | grep -v '^-'\n>\n> to just show what the end result of a patch is, I decided that there\n> has to be a better way.\n>\n> Of course, to normal people, that \"better way\" is probably some GUI\n> tool that shows diffs as \"before/after\" in two different frames next\n> to each other, but I'm a grumpy old man (\"Get off my lawn\") and I do\n> everything but read my email in a standard text-only terminal.\n\nSounds like the \"apply --no-add\" in the opposite direction ;-) I\nwould find it handy myself, too, though I tend to read my patches\nafter applying to my tree so the postimage is usually an invocation\nof \"less\" away for me.\n\nI do not think it is a bad idea to have an option to give only the\npostimage and another option to give only the preimage. It would\nalso trivially allow people to show the side-by-side diff in GUI.\n"},{"id":"451076","messageId":"CAHk-=wg+n_-btzoyMqnDYsJxFYAyUh0Kb=TkpS8RiD_h3G=_cA@mail.gmail.com","threadId":"57533","inReplyTo":"xmqqo82dd0qv.fsf@gitster.g","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-10T20:40:06Z","receivedAt":"2022-03-10T20:40:28Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 10, 2022 at 12:13 PM Junio C Hamano wrote:\n>\n> Sounds like the \"apply --no-add\" in the opposite direction ;-)\n\nI was thinking more the opposite of \"--ours/theirs\" when merging, but\nyeah, I guess \"--no-add\" is technically even closer.\n\n> I would find it handy myself, too, though I tend to read my patches\n> after applying to my tree so the postimage is usually an invocation\n> of \"less\" away for me.\n\nObviously just looking at the file itself is always an option, and I\ndo that too. But I traditionally do that \"grep -v\" trick as I'm\nverifying the patch before sending it out (or before committing)\nbecause it's such a nice way to limit the output just to the changed\nparts.\n\n> I do not think it is a bad idea to have an option to give only the\n> postimage and another option to give only the preimage. It would\n> also trivially allow people to show the side-by-side diff in GUI.\n\nI suspect people doing GUI's are happy just parsing the '-' and '+'\nlines themselves, since they want both sides anyway.\n\nFor example, 'gitk' already has that diff/old/new checkbox, that does\nexactly what my patch does.\n\nAnd I doubt anybody wants gitk to re-run 'diff' just because somebody\nclicked another option - it's only used to visualize the diff that was\nalready done differently.\n\nOf course, I might be wrong. I didn't actually look at what 'gitk'\ndoes. Maybe it _does_ re-run diff when you click that thing.\n\nBut that gitk behavior - which I also do use - is probably the best\nway to explain the feature. It's just that I also want to get that\n\"New version\" behavior for plain \"git diff\" on the command line.\n\nI don't know what a good command line option would be, though. I'd\nlike it to be somehat short, because the whole point of this is to be\na convenience feature.\n\nSo \"--new/old\"? \"--pre/post\"?\n\nOr it could be something random, and tie it with the existing \"-U\"\noption, where \"-U+\" would be \"positive side only\", and \"-U5-\" would be\n\"5 context lines, negative side only\". Very dense and convenient,\nmaybe not all that intuitive?\n\n Linus\n"},{"id":"451079","messageId":"xmqqfsnpcxdm.fsf@gitster.g","threadId":"57533","inReplyTo":"CAHk-=wg+n_-btzoyMqnDYsJxFYAyUh0Kb=TkpS8RiD_h3G=_cA@mail.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T21:26:29Z","receivedAt":"2022-03-10T21:26:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So \"--new/old\"? \"--pre/post\"?\n>\n> Or it could be something random, and tie it with the existing \"-U\"\n> option, where \"-U+\" would be \"positive side only\", and \"-U5-\" would be\n> \"5 context lines, negative side only\". Very dense and convenient,\n> maybe not all that intuitive?\n\nI often use -W and the above would give us a natural extension, but\nI agree that is a bit too dense and totally unintuitive. As we use\nparse-options for patch output formatting options, my pick would be\n\"--new-only\" vs \"--old-only\" (if there were existing options that\nhas new/old in their names, \"--preimage-only\" and \"--postimage-only\"\nwould also work, but that's much longer), and let the option parser\naccept unique abbreviations like \"--new\" and \"--old\".\n\n"},{"id":"451085","messageId":"CAHk-=whRbuJJ1LzYN9F48JaS7EjuP3FkppHJXi1wAO_qLJQ2xw@mail.gmail.com","threadId":"57533","inReplyTo":"xmqqfsnpcxdm.fsf@gitster.g","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-10T22:13:10Z","receivedAt":"2022-03-10T22:13:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 10, 2022 at 1:26 PM Junio C Hamano wrote:\n>\n> I often use -W and the above would give us a natural extension, but\n> I agree that is a bit too dense and totally unintuitive. As we use\n> parse-options for patch output formatting options, my pick would be\n> \"--new-only\" vs \"--old-only\" (\n\nI was \"ok, that's really easy\" and did\n\n+ OPT_ALIAS(0, \"new-only\", \"no-output-indicator-old\"),\n+ OPT_ALIAS(0, \"old-only\", \"no-output-indicator-new\"),\n\nbut sadly the parse-options alias code isn't quite smart enough.\n\nDoing it with an explicit callback obviously works, but the \"unique\nabbreviations\" part doesn't actually work for me. I think it's due to\nPARSE_OPT_KEEP_UNKNOWN making the abbreviated options not work, but I\ndon't know tha option parsing code well enough.\n\nHere's the stupid patch that \"works\" but doesn't allow the shortened\nversion. Maybe somebody can point out what silly thing I did wrong.\n\n Linus\n\n\n diff.c | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 895951b849..cf168e727d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5000,6 +5000,17 @@ static int diff_opt_char(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_no_char(const struct option *opt,\n+\t\t\t const char *arg, int unset)\n+{\n+\tchar *value = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tBUG_ON_OPT_ARG(arg);\n+\t*value = 0;\n+\treturn 0;\n+}\n+\n static int diff_opt_color_moved(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -5493,6 +5504,14 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate a context instead of ' '\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\tOPT_CALLBACK_F(0, \"new-only\",\n+\t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD], NULL,\n+\t\t\t N_(\"show only new lines in diff\"),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n+\t\tOPT_CALLBACK_F(0, \"old-only\",\n+\t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW], NULL,\n+\t\t\t N_(\"show only old lines in diff\"),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n \n \t\tOPT_GROUP(N_(\"Diff rename options\")),\n \t\tOPT_CALLBACK_F('B', \"break-rewrites\", &options->break_opt, N_(\"[/]\"),\n"},{"id":"451103","messageId":"CAHk-=wj0ZfmTEhc4iPJSbn_FxzU94qZfK9WcgujKUcZK9a2UvQ@mail.gmail.com","threadId":"57533","inReplyTo":"CAHk-=whRbuJJ1LzYN9F48JaS7EjuP3FkppHJXi1wAO_qLJQ2xw@mail.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-11T00:01:48Z","receivedAt":"2022-03-11T00:02:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 10, 2022 at 2:13 PM Linus Torvalds\n wrote:\n>\n> Here's the stupid patch that \"works\" but doesn't allow the shortened\n> version. Maybe somebody can point out what silly thing I did wrong.\n\nI just created a short alias to do this. Maybe there's some smarter\noption, but this seems to work.\n\nI've updated the commit message - I kept the --no-output-indicator-xyz\nform since it really logically ends up being exactly that, but I guess\nthose changes could also be dropped.\n\nHmm?\n\n Linus\n\n\nFrom 1d207585369ec1708da5549176a79bb22c472ee2 Mon Sep 17 00:00:00 2001\nFrom: Linus Torvalds \nDate: Thu, 10 Mar 2022 11:00:43 -0800\nSubject: [PATCH] Allow '--new-only/--old-only' to only show the new/old parts\n of a diff\n\nThis is particularly useful if you want to just see the end result of a\ndiff, without the original lines that have been removed (or, less\ncommonly, the reverse).\n\nThis a fairly useful model in various GUI diff viewers, with 'gitk' for\nexample having the option to show the diff, the old state, or the new\nstate. It's not unusual to want to just see \"what is the end result of\nmy changes\" without seeing (a) everything that didn't change and (b) the\nold removed state.\n\nThe option name can also be abbreviated to just \"--new\" and \"--old\".\n\nExample:\n\n git show --new 04bf052eef\n\nto see just the end result of the changes in commit 04bf052eef (\"grep:\nsimplify config parsing and option parsing\").\n\nInternally this is simply implemented as setting the output indicator\nfor old and new lines, and you can in fact also do this with that fairly\ncumbersome interface (ie \"--no-output-indicator-old\" disables the old\nlines, and thus acts as \"--new\").\n\nSigned-off-by: Linus Torvalds \n---\n diff.c | 31 +++++++++++++++++++++++++++++--\n 1 file changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2bd5e0d817..f37f0b383a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1254,6 +1254,8 @@ static void emit_line_ws_markup(struct diff_options *o,\n \tconst char *ws = NULL;\n \tint sign = o->output_indicators[sign_index];\n \n+\tif (!sign)\n+\t\treturn;\n \tif (o->ws_error_highlight & ws_rule) {\n \t\tws = diff_get_color_opt(o, DIFF_WHITESPACE);\n \t\tif (!*ws)\n@@ -4986,6 +4988,10 @@ static int diff_opt_char(const struct option *opt,\n {\n \tchar *value = opt->value;\n \n+\tif (unset) {\n+\t\t*value = 0;\n+\t\treturn 0;\n+\t}\n \tBUG_ON_OPT_NEG(unset);\n \tif (arg[1])\n \t\treturn error(_(\"%s expects a character, got '%s'\"),\n@@ -4994,6 +5000,17 @@ static int diff_opt_char(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_no_char(const struct option *opt,\n+\t\t\t const char *arg, int unset)\n+{\n+\tchar *value = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tBUG_ON_OPT_ARG(arg);\n+\t*value = 0;\n+\treturn 0;\n+}\n+\n static int diff_opt_color_moved(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -5476,17 +5493,27 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW],\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate a new line instead of '+'\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\t\t 0, diff_opt_char),\n \t\tOPT_CALLBACK_F(0, \"output-indicator-old\",\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD],\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate an old line instead of '-'\"),\n-\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\t\t 0, diff_opt_char),\n \t\tOPT_CALLBACK_F(0, \"output-indicator-context\",\n \t\t\t &options->output_indicators[OUTPUT_INDICATOR_CONTEXT],\n \t\t\t N_(\"\"),\n \t\t\t N_(\"specify the character to indicate a context instead of ' '\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_char),\n+\t\tOPT_CALLBACK_F(0, \"new-only\",\n+\t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD], NULL,\n+\t\t\t N_(\"show only new lines in diff\"),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n+\t\tOPT_CALLBACK_F(0, \"old-only\",\n+\t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW], NULL,\n+\t\t\t N_(\"show only old lines in diff\"),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n+\t\tOPT_ALIAS(0, \"new\", \"new-only\"),\n+\t\tOPT_ALIAS(0, \"old\", \"old-only\"),\n \n \t\tOPT_GROUP(N_(\"Diff rename options\")),\n \t\tOPT_CALLBACK_F('B', \"break-rewrites\", &options->break_opt, N_(\"[/]\"),\n-- \n2.35.1.459.g1d20758536\n\n"},{"id":"451114","messageId":"220311.867d90j2vj.gmgdl@evledraar.gmail.com","threadId":"57533","inReplyTo":"CAHk-=wj0ZfmTEhc4iPJSbn_FxzU94qZfK9WcgujKUcZK9a2UvQ@mail.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-11T07:48:51Z","receivedAt":"2022-03-11T08:43:35Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 10 2022, Linus Torvalds wrote:\n\n> On Thu, Mar 10, 2022 at 2:13 PM Linus Torvalds\n> wrote:\n\nIf it's not too much trouble inline patches like those you send to the\nLKML would be preferred :) Re-arranging things a bit to quote them....\n\n> diff --git a/diff.c b/diff.c\n> index 2bd5e0d817..f37f0b383a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1254,6 +1254,8 @@ static void emit_line_ws_markup(struct diff_options *o,\n> \tconst char *ws = NULL;\n> \tint sign = o->output_indicators[sign_index];\n> \n> +\tif (!sign)\n> +\t\treturn;\n> \tif (o->ws_error_highlight & ws_rule) {\n> \t\tws = diff_get_color_opt(o, DIFF_WHITESPACE);\n> \t\tif (!*ws)\n> @@ -4986,6 +4988,10 @@ static int diff_opt_char(const struct option *opt,\n> {\n> \tchar *value = opt->value;\n> \n> +\tif (unset) {\n> +\t\t*value = 0;\n> +\t\treturn 0;\n> +\t}\n> \tBUG_ON_OPT_NEG(unset);\n> \tif (arg[1])\n> \t\treturn error(_(\"%s expects a character, got '%s'\"),\n> @@ -4994,6 +5000,17 @@ static int diff_opt_char(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static int diff_opt_no_char(const struct option *opt,\n> +\t\t\t const char *arg, int unset)\n> +{\n> +\tchar *value = opt->value;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\tBUG_ON_OPT_ARG(arg);\n> +\t*value = 0;\n> +\treturn 0;\n> +}\n> +\n> static int diff_opt_color_moved(const struct option *opt,\n> \t\t\t\tconst char *arg, int unset)\n> {\n> @@ -5476,17 +5493,27 @@ static void prep_parse_options(struct diff_options *options)\n> \t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW],\n> \t\t\t N_(\"\"),\n> \t\t\t N_(\"specify the character to indicate a new line instead of '+'\"),\n> -\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n> +\t\t\t 0, diff_opt_char),\n> \t\tOPT_CALLBACK_F(0, \"output-indicator-old\",\n> \t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD],\n> \t\t\t N_(\"\"),\n> \t\t\t N_(\"specify the character to indicate an old line instead of '-'\"),\n> -\t\t\t PARSE_OPT_NONEG, diff_opt_char),\n> +\t\t\t 0, diff_opt_char),\n> \t\tOPT_CALLBACK_F(0, \"output-indicator-context\",\n> \t\t\t &options->output_indicators[OUTPUT_INDICATOR_CONTEXT],\n> \t\t\t N_(\"\"),\n> \t\t\t N_(\"specify the character to indicate a context instead of ' '\"),\n> \t\t\t PARSE_OPT_NONEG, diff_opt_char),\n> +\t\tOPT_CALLBACK_F(0, \"new-only\",\n> +\t\t\t &options->output_indicators[OUTPUT_INDICATOR_OLD], NULL,\n> +\t\t\t N_(\"show only new lines in diff\"),\n> +\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n> +\t\tOPT_CALLBACK_F(0, \"old-only\",\n> +\t\t\t &options->output_indicators[OUTPUT_INDICATOR_NEW], NULL,\n> +\t\t\t N_(\"show only old lines in diff\"),\n> +\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_char),\n> +\t\tOPT_ALIAS(0, \"new\", \"new-only\"),\n> +\t\tOPT_ALIAS(0, \"old\", \"old-only\"),\n> \n\nFWIW the reason...\n\n>>\n>> Here's the stupid patch that \"works\" but doesn't allow the shortened\n>> version. Maybe somebody can point out what silly thing I did wrong.\n>\n> I just created a short alias to do this. Maybe there's some smarter\n> option, but this seems to work.\n\n..you needed to do that is because we pass PARSE_OPT_KEEP_UNKNOWN\nparse_options() there, which turns off our abbreviation discovery logic,\ni.e. where we'll take a --foo, --foob, --fooba if we have a --foobar\noption defined.\n\nLooking at it there appears to be no good reason for why it's so\noverzelous. If I remove the relevant PARSE_OPT_KEEP_UNKNOWN logic in\nparse-options.c our entire test suite passes, except for one obscure\npart where \"git format-patch --output=x\" needs to not abbreviate to \"git\nformat-patch --output-directory=x\".\n\nWhich, for reasons is something where we do option parsing in two\npasses, i.e. we hand the \"output\" option off to the revision walker.\n\nWe really should just teach those callsites to \"grab\" the revisions.c\noptions and do the parse in one pass, but in the meantime this is a less\ninvasive way to have that case work, which makes your code work without\nthe OPT_ALIAS() hunk:\n\n\tdiff --git a/builtin/log.c b/builtin/log.c\n\tindex c211d66d1d0..adacc65bc7e 100644\n\t--- a/builtin/log.c\n\t+++ b/builtin/log.c\n\t@@ -1811,7 +1811,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n\t \t\t\t PARSE_OPT_NONEG, subject_prefix_callback),\n\t \t\tOPT_CALLBACK_F('o', \"output-directory\", &output_directory,\n\t \t\t\t N_(\"dir\"), N_(\"store resulting files in \"),\n\t-\t\t\t PARSE_OPT_NONEG, output_directory_callback),\n\t+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NO_ABBREV,\n\t+\t\t\t output_directory_callback),\n\t \t\tOPT_CALLBACK_F('k', \"keep-subject\", &rev, NULL,\n\t \t\t\t N_(\"don't strip/add [PATCH]\"),\n\t \t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback),\n\tdiff --git a/parse-options.c b/parse-options.c\n\tindex 6e57744fd22..9d0c4694482 100644\n\t--- a/parse-options.c\n\t+++ b/parse-options.c\n\t@@ -332,7 +332,7 @@ static enum parse_opt_result parse_long_opt(\n\t \t\t\trest = NULL;\n\t \t\tif (!rest) {\n\t \t\t\t/* abbreviated? */\n\t-\t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n\t+\t\t\tif (!(options->flags & PARSE_OPT_NO_ABBREV) &&\n\t \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n\t is_abbreviated:\n\t \t\t\t\tif (abbrev_option &&\n\tdiff --git a/parse-options.h b/parse-options.h\n\tindex 685fccac137..f6372f60edb 100644\n\t--- a/parse-options.h\n\t+++ b/parse-options.h\n\t@@ -48,6 +48,7 @@ enum parse_opt_option_flags {\n\t \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n\t \tPARSE_OPT_COMP_ARG = 1 << 10,\n\t \tPARSE_OPT_CMDMODE = 1 << 11,\n\t+\tPARSE_OPT_NO_ABBREV = 1 << 12,\n\t };\n\t \n\t enum parse_opt_result {\n\nOf course that also makes --new and --old work (even --ne and --ol), but\nwe generally accept that in other places, so... :)\n\n> I've updated the commit message - I kept the --no-output-indicator-xyz\n> form since it really logically ends up being exactly that, but I guess\n> those changes could also be dropped.\n\nI see this interacts nicely with the -I option, at least in my testing,\ni.e. (with my change applied) this will ignore the first hunk of your\npatch:\n\n --new -p -I'sign|return'\n\nWhat this doesn't interact \"well\" with, or perhaps it's what we actually\nwant is e.g. -G:\n\n git log -1 -p -G'PARSE_OPT_NONEG, diff_opt_char' --new\n\nIf the user is expecting us to search through the displayed hunks this\nshould not show your change, but it does.\n\nFWIW I have a local patch I've been meaning to submit which extends -G\nto allow you to search through the full displayed context, right now we\nstrip it down to the \"+\" and \"-\" lines and regex match on that.\n\nSo you could do, in this case:\n\n git log --pickaxe-patch -G'^-.*diff_opt_char' -1\n\nTo search through your removed hunks.\n\nAll of which is to say that I think the core semantics you're\nimplementing here make sense, it's just worth thinking about, and we\nshould have tests for some of the edge cases for a non-RFC. I.e. it's\nnot obvious that we have a couple of diff \"passes\", and -G, -I and these\n--new-only and --old-only options search through different versions of\nthat diff.\n"},{"id":"451120","messageId":"patch-1.1-d2ae423d1ad-20220311T094315Z-avarab@gmail.com","threadId":"57533","inReplyTo":"220311.867d90j2vj.gmgdl@evledraar.gmail.com","subject":"[PATCH] parse-options: add per-option flag to stop abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-11T09:45:14Z","receivedAt":"2022-03-11T09:45:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"For cases like \"format-patch --output-directory=dir\" we disallow\nabbreviations entirely, because we'd like setup_revisions() to do a\nsubsequent option parsing pass, and it accepts \"--output=file\".\n\nThat's one reason for why the parse_options() in diff.c added in\n4a288478394 (diff.c: prepare to use parse_options() for parsing,\n2019-01-27) passes PARSE_OPT_KEEP_UNKNOWN. I.e. we need the\n\"--output=file\" to be retained in the arguments for passing it down to\nsetup_revisions().\n\nIt's not stated explicitly but I think \"--output\"\nv.s. \"--output-directory\" is the specific case the author had in mind\nwhen adding the PARSE_OPT_KEEP_UNKNOWN case in the pre-image. That was\nadded in an earlier commit in the same series,\nbaa4adc66ae (parse-options: disable option abbreviation with\nPARSE_OPT_KEEP_UNKNOWN, 2019-01-27).\n\nAs that commit shows we had to disallow \"git difftool --symlink\" as an\nabbreviation for \"git difftool --symlinks\" as a side-effect, even\nthough that abbreviation wouldn't have conflicted.\n\nSo let's tweak this rule to be more narrowly scoped, now we'll allow\nabbreviated options regardless of PARSE_OPT_KEEP_UNKNOWN being set for\nparse_options(), but we'll provide individual options an opt-out. By\nusing that opt-out for \"--output-directory\" we can have that case work\nwithout overzealously disallowing others.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\n> On Thu, Mar 10 2022, Linus Torvalds wrote:\n>\n>> On Thu, Mar 10, 2022 at 2:13 PM Linus Torvalds\n>> wrote:\n>[...]\n>>> Here's the stupid patch that \"works\" but doesn't allow the shortened\n>>> version. Maybe somebody can point out what silly thing I did wrong.\n>>\n>> I just created a short alias to do this. Maybe there's some smarter\n>> option, but this seems to work.\n>\n> ..you needed to do that is because we pass PARSE_OPT_KEEP_UNKNOWN\n> parse_options() there, which turns off our abbreviation discovery logic,\n> i.e. where we'll take a --foo, --foob, --fooba if we have a --foobar\n> option defined.\n>\n> Looking at it there appears to be no good reason for why it's so\n> overzelous. If I remove the relevant PARSE_OPT_KEEP_UNKNOWN logic in\n> parse-options.c our entire test suite passes, except for one obscure\n> part where \"git format-patch --output=x\" needs to not abbreviate to \"git\n> format-patch --output-directory=x\".\n>\n> Which, for reasons is something where we do option parsing in two\n> passes, i.e. we hand the \"output\" option off to the revision walker.\n>\n> We really should just teach those callsites to \"grab\" the revisions.c\n> options and do the parse in one pass, but in the meantime this is a less\n> invasive way to have that case work, which makes your code work without\n> the OPT_ALIAS() hunk\n\nIt even passes CI! I think this change makes sense, with/without the\nRFC topic.\n\n builtin/log.c | 3 ++-\n parse-options.c | 2 +-\n parse-options.h | 1 +\n t/t7800-difftool.sh | 5 +++++\n 4 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c211d66d1d0..adacc65bc7e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1811,7 +1811,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_NONEG, subject_prefix_callback),\n \t\tOPT_CALLBACK_F('o', \"output-directory\", &output_directory,\n \t\t\t N_(\"dir\"), N_(\"store resulting files in \"),\n-\t\t\t PARSE_OPT_NONEG, output_directory_callback),\n+\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NO_ABBREV,\n+\t\t\t output_directory_callback),\n \t\tOPT_CALLBACK_F('k', \"keep-subject\", &rev, NULL,\n \t\t\t N_(\"don't strip/add [PATCH]\"),\n \t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback),\ndiff --git a/parse-options.c b/parse-options.c\nindex 6e57744fd22..9d0c4694482 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -332,7 +332,7 @@ static enum parse_opt_result parse_long_opt(\n \t\t\trest = NULL;\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n-\t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n+\t\t\tif (!(options->flags & PARSE_OPT_NO_ABBREV) &&\n \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option &&\ndiff --git a/parse-options.h b/parse-options.h\nindex 685fccac137..f6372f60edb 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -48,6 +48,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n+\tPARSE_OPT_NO_ABBREV = 1 << 12,\n };\n \n enum parse_opt_result {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 096456292c0..ae47426f66e 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -582,6 +582,11 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged ch\n \tEOF\n \tgit difftool --dir-diff --symlinks \\\n \t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n+\ttest_cmp expect actual &&\n+\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tgit difftool --dir-diff --symlink \\\n+\t\t--extcmd \"./.git/CHECK_SYMLINKS\" branch HEAD &&\n \ttest_cmp expect actual\n '\n \n-- \n2.35.1.1334.gf8cd03b07c8\n\n"},{"id":"451156","messageId":"CAHk-=wjB_MZqG6QN8gcmdQ2Me9GWmr5HJ8TX8cj9fHck8y75XA@mail.gmail.com","threadId":"57533","inReplyTo":"patch-1.1-d2ae423d1ad-20220311T094315Z-avarab@gmail.com","subject":"Re: [PATCH] parse-options: add per-option flag to stop abbreviation","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-11T18:44:41Z","receivedAt":"2022-03-11T18:45:03Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Mar 11, 2022 at 1:45 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> So let's tweak this rule to be more narrowly scoped, now we'll allow\n> abbreviated options regardless of PARSE_OPT_KEEP_UNKNOWN being set for\n> parse_options(), but we'll provide individual options an opt-out. By\n> using that opt-out for \"--output-directory\" we can have that case work\n> without overzealously disallowing others.\n\nThanks, this looks much more obvious to me than the odd\nPARSE_OPT_KEEP_UNKNOWN logic was. And would seem to obviate the need\nfor my hacky short aliases.\n\n Linus\n"},{"id":"451158","messageId":"CAHk-=wgT9=3V60koHO40omnubeBK3bD8kX8WAx+gUHTWoNCn4Q@mail.gmail.com","threadId":"57533","inReplyTo":"220311.867d90j2vj.gmgdl@evledraar.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-11T19:15:10Z","receivedAt":"2022-03-11T19:22:15Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Mar 11, 2022 at 12:43 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> If it's not too much trouble inline patches like those you send to the\n> LKML would be preferred :)\n\nHeh. When I send patches to LKML inline I end up doing whitespace\ndamage to them.\n\nPart of the problem is that gmail - which is what I use for normal\nemail - makes it quite hard to send patches with the legacy tools I\nhave when you have set your account up to have all the security\nmeasures (2FA etc).\n\nIn fact, I think these days gmail actively blocks any clients it\ndoesn't trust, even if you were to do the special app key thing (which\nI have occasionally done).\n\nDespite that, I find gmail very convenient for my workflow, but the\ntwo issues I have:\n\n (a) I'd like to have \"inline attachments\" for the web interface so\nthat I could attach a text file that would *not* get whitespace\ndamaged\n\n (b) the mobile client should have a \"no html\" mode (for when I'm on\nthe road and reply to lkml)\n\nAnd I know a lot of people inside of google, but I gave up trying to\nget the message through to the gmail people (everybody I knew said\n\"it's a different group and we have our own issues with them\")\n\nYes, yes, I could use other tools - and other email setups - for\nsending out patches, but realistically these days I never actually\nsend out patches any more. These kinds of very occasional git patches\nare the exception, not the rule.\n\nSo, for example, I could trivially use \"torvalds@kernel.org\" to send\nout patches, but I've tried to avoid having multiple active email\naccounts and confusing people with different names etc.\n\nMy lkml patches are invariably examples (ie \"how about we do it this\nway\") and then I actually mostly *prefer* that they be\nwhitespace-damaged so that people won't apply them mindlessly. In\nfact, I often explicitly indent the patch to make it really obvious\nthat \"yes, this is a patch, but it's meant to be thought about, not\nused mindlessly\".\n\n> FWIW the reason... [...]\n> ..you needed to do that is because we pass PARSE_OPT_KEEP_UNKNOWN\n> parse_options() there, which turns off our abbreviation discovery logic,\n> i.e. where we'll take a --foo, --foob, --fooba if we have a --foobar\n> option defined.\n\nYeah, I had gotten to the PARSE_OPT_KEEP_UNKNOWN part, and then\ndecided I didn't understand why the option parsing code did that, and\nI wasn't going to touch it.\n\nI saw your patch to fix it, and it looked sane to me. Thanks.\n\n> We really should just teach those callsites to \"grab\" the revisions.c\n> options and do the parse in one pass,\n\nI was assuming the two-pass thing to avoid doing any callbacks before\ndeciding the option is unambiguous.\n\nNot that I see why that would be a big deal as long as you just error\nout anyway, but maybe some users end up ignoring errors?\n\nYou know that code better than I do in any case.\n\n> Of course that also makes --new and --old work (even --ne and --ol), but\n> we generally accept that in other places, so... :)\n\nI do wonder if abbreviation should have some limit (ie single-letter\nabbreviation sounds very suspect even if they are unique, and even\ntwo-letter ones sound odd), but it probably doesn't matter.\n\n> I see this interacts nicely with the -I option, at least in my testing,\n> i.e. (with my change applied) this will ignore the first hunk of your\n> patch:\n>\n> --new -p -I'sign|return'\n\nSo it wasn't really intentional - I saw it _purely_ as a \"filter\noutput\" thing, not as a \"filter the patch generation\".\n\nIn fact, I like the part where it often shows patch hunks that have no\nactual changes, because the only changes in that hunk were removals.\nTo me, that's a feature - it shows the end result of the patch in that\nplace.\n\nBut it also means:\n\n> What this doesn't interact \"well\" with, or perhaps it's what we actually\n> want is e.g. -G:\n\nYeah, exactly because I saw it as purely a \"filter the final output of\nthe patch\" thing, it means that things that filter on the *contents*\nof the patch aren't affected.\n\nSo that was my mental model, but yes, if your mental model is that\n\"-G\" acts on the output, then it's a mis-feature.\n\nIOW, to me, this would be a bigger decision about what kinds of\nsemantics you want.\n\nNow, the *real* downside of \"--new\" to me is that it really doesn't\nwork very well with --word-diff. That's where I think it would really\nmake sense to still work just fine. Particularly with\n\"--word-diff=color\" I think \"--new\" (and \"--old\") would make a lot of\nsense, but my patch very much did *not* change the word-diff output\nmachinery to take the output_indicators into account.\n\nA big reason for that is that while I occasionally use --word-diff\n(and find it very useful when I do), I've never touched that code, so\nit's entirely unfamiliar to me.\n\n Linus\n"},{"id":"451160","messageId":"20220311194232.huwvcsar5iglswne@meerkat.local","threadId":"57533","inReplyTo":"CAHk-=wgT9=3V60koHO40omnubeBK3bD8kX8WAx+gUHTWoNCn4Q@mail.gmail.com","subject":"Re: RFC: Using '--no-output-indicator-old' to only show new state","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2022-03-11T19:42:32Z","receivedAt":"2022-03-11T19:42:39Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Fri, Mar 11, 2022 at 11:15:10AM -0800, Linus Torvalds wrote:\n> So, for example, I could trivially use \"torvalds@kernel.org\" to send\n> out patches, but I've tried to avoid having multiple active email\n> accounts and confusing people with different names etc.\n\nFWIW, you can send mail through mail.kernel.org with your regular\ntorvalds@linux-foundation.org email. This is what akpm does, and it's kosher\nas far as DKIM/DMARC is concerned.\n\nIf you'd like to try that out, you can set that up with git-send-email:\nhttps://korg.docs.kernel.org/mail.html#sending-outgoing-mail\n\nJust set your sendemail.from=torvalds@linux-foundation.org instead of\ntorvalds@kernel.org.\n\n-K\n"}]}