{"thread":{"id":"59688","subject":"[PATCH] t4013: add expected failure for \"log --patch --no-patch\"","startedAt":"2023-05-03T13:41:40Z","lastAt":"2023-05-13T05:40:19Z","messageCount":42,"participants":["Sergey Organov","Junio C Hamano","Felipe Contreras","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"476508","messageId":"20230503134118.73504-1-sorganov@gmail.com","threadId":"59688","inReplyTo":null,"subject":"[PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-03T13:41:18Z","receivedAt":"2023-05-03T13:41:40Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"--patch followed by --no-patch is to be a no-op according to the \"git\nlog\" manual page. In reality this sequence breaks --raw output\nthough (and who knows what else?)\n\nAdd a test_expected_failure case for the issue.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t4013-diff-various.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5de1d190759f..f876b0cc8ec3 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n+# This should succeed as --patch followed by --no-patch sequence is to\n+# be a no-op according to the manual page. In reality it breaks --raw\n+# though. Needs to be fixed.\n+test_expect_failure '--no-patch cancels --patch only' '\n+\tgit log --raw master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --patch --no-patch --raw >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -m matches pure log' '\n \tgit log master >result &&\n \tprocess_diffs result >expected &&\n-- \n2.25.1\n\n"},{"id":"476522","messageId":"xmqqsfcdtkt0.fsf@gitster.g","threadId":"59688","inReplyTo":"20230503134118.73504-1-sorganov@gmail.com","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T16:57:15Z","receivedAt":"2023-05-03T16:57:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> --patch followed by --no-patch is to be a no-op according to the \"git\n> log\" manual page.\n\nI briefly wondered if it is a bug in the documentation.  But it is\nclear (at least to me) that \"git log -p --stat --no-patch\" wants to\nshow only \"--stat\", and when \"git log -p --raw\" shows both patch and\nraw, I do not think of a reason why \"git log -p --raw --no-patch\"\nshould not behave similarly.\n\n> Add a test_expected_failure case for the issue.\n\nThat is unsatisfactory, though.  Can you back-burner it and send in\na fix with the same test flipping expect_failure to expect_success\ninstead?\n\nThanks.\n\n>\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  t/t4013-diff-various.sh | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 5de1d190759f..f876b0cc8ec3 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode\n>  diff-tree -R --stat --compact-summary initial mode\n>  EOF\n>  \n> +# This should succeed as --patch followed by --no-patch sequence is to\n> +# be a no-op according to the manual page. In reality it breaks --raw\n> +# though. Needs to be fixed.\n> +test_expect_failure '--no-patch cancels --patch only' '\n> +\tgit log --raw master >result &&\n> +\tprocess_diffs result >expected &&\n> +\tgit log --patch --no-patch --raw >result &&\n> +\tprocess_diffs result >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'log -m matches pure log' '\n>  \tgit log master >result &&\n>  \tprocess_diffs result >expected &&\n"},{"id":"476527","messageId":"874jote2zl.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqqsfcdtkt0.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-03T17:31:10Z","receivedAt":"2023-05-03T17:31:46Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> --patch followed by --no-patch is to be a no-op according to the \"git\n>> log\" manual page.\n>\n> I briefly wondered if it is a bug in the documentation.  But it is\n> clear (at least to me) that \"git log -p --stat --no-patch\" wants to\n> show only \"--stat\", and when \"git log -p --raw\" shows both patch and\n> raw, I do not think of a reason why \"git log -p --raw --no-patch\"\n> should not behave similarly.\n>\n>> Add a test_expected_failure case for the issue.\n>\n> That is unsatisfactory, though.  Can you back-burner it and send in\n> a fix with the same test flipping expect_failure to expect_success\n> instead?\n\nNo problem from my side, but are you sure?\n\n - test_expect_failure [<prereq>] <message> <script>\n\n   This is NOT the opposite of test_expect_success, but is used\n   to mark a test that demonstrates a known breakage.\n\nDon't we need exactly this in this particular case? Demonstrate a known\nbreakage?\n\nI'm confused.\n\n>\n> Thanks.\n>\n>>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>>  t/t4013-diff-various.sh | 11 +++++++++++\n>>  1 file changed, 11 insertions(+)\n>>\n>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>> index 5de1d190759f..f876b0cc8ec3 100755\n>> --- a/t/t4013-diff-various.sh\n>> +++ b/t/t4013-diff-various.sh\n>> @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode\n>>  diff-tree -R --stat --compact-summary initial mode\n>>  EOF\n>>  \n>> +# This should succeed as --patch followed by --no-patch sequence is to\n>> +# be a no-op according to the manual page. In reality it breaks --raw\n>> +# though. Needs to be fixed.\n>> +test_expect_failure '--no-patch cancels --patch only' '\n>> +\tgit log --raw master >result &&\n>> +\tprocess_diffs result >expected &&\n>> +\tgit log --patch --no-patch --raw >result &&\n>> +\tprocess_diffs result >actual &&\n>> +\ttest_cmp expected actual\n>> +'\n>> +\n>>  test_expect_success 'log -m matches pure log' '\n>>  \tgit log master >result &&\n>>  \tprocess_diffs result >expected &&\n\nThanks,\n-- Sergey Organov\n\n\n"},{"id":"476534","messageId":"xmqqmt2lqofb.fsf@gitster.g","threadId":"59688","inReplyTo":"874jote2zl.fsf@osv.gnss.ru","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T18:07:20Z","receivedAt":"2023-05-03T18:07:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> No problem from my side, but are you sure?\n\nAbsolutely.\n\nI've seen people just say \"we document a failed one\" and leave it at\nthat, without attempting to fix.  I am trying to see if pushing back\nat first would serve as a good way to encourage these known failure\nto be fixed, without accumulating too many expect_failure in our\ntest suite, which will waste cycles at CI runs (which do not need to\nbe reminded something is known to be broken).  I will try not to do\nthis when I do not positively know the author of such a patch is\ncapable enough to provide a fix, though, and you are unlucky enough\nto have shown your abilities in the past ;-)\n\nThanks.\n\n\n"},{"id":"476536","messageId":"6452a8c4ea448_682294ed@chronos.notmuch","threadId":"59688","inReplyTo":"xmqqmt2lqofb.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-03T18:32:36Z","receivedAt":"2023-05-03T18:32:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> I am trying to see if pushing back at first would serve as a good way\n> to encourage these known failure to be fixed, without accumulating too\n> many expect_failure in our test suite, which will waste cycles at CI\n> runs\n\n> (which do not need to be reminded something is known to be broken)\n\nWe don't?\n\nThere's plenty of things that are broken in git that people have\nforgotten.\n\nIf wasting cycles on CI runs is your concern, I would gladly write a\npatch to skipp all the test_expect_failure tests.\n\nI would rather have documented all the things are known to be broken\ntoday than not, because in a month that might not be the case.\n\n-- \nFelipe Contreras\n"},{"id":"476556","messageId":"87ttwtci05.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqqmt2lqofb.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-03T19:49:46Z","receivedAt":"2023-05-03T19:49:53Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> No problem from my side, but are you sure?\n>\n> Absolutely.\n>\n> I've seen people just say \"we document a failed one\" and leave it at\n> that, without attempting to fix.  I am trying to see if pushing back\n> at first would serve as a good way to encourage these known failure\n> to be fixed, without accumulating too many expect_failure in our\n> test suite, which will waste cycles at CI runs (which do not need to\n> be reminded something is known to be broken).  I will try not to do\n> this when I do not positively know the author of such a patch is\n> capable enough to provide a fix, though, and you are unlucky enough\n> to have shown your abilities in the past ;-)\n\nThanks for the credit, but as my recent attempts to fix 2 obvious\ndeficiencies in Git CI (one of them being my own) failed quite\nmiserably, I figure I have no idea how these things in CI are to be\ntreated, so I prefer to leave a fix to somebody else, who actually groks\nwhat makes sense in the Git UI, and what doesn't.\n\nThat said, in case you still need the test with expect_success, below is\none rerolled.\n\nThanks,\n-- Sergey Organov\n\n--- >8 ---\n\nSubject: [PATCH] t4013: add test for \"log --patch --no-patch\"\n\n--patch followed by --no-patch is to be a no-op according to the \"git\nlog\" manual page. In reality this sequence breaks --raw output\nthough (and who knows what else?)\n\nAdd test case for the issue.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t4013-diff-various.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5de1d190759f..32907bf142fc 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n+# This should succeed as --patch followed by --no-patch sequence is to\n+# be a no-op according to the manual page. In reality it breaks --raw\n+# though. Needs to be fixed.\n+test_expect_success '--no-patch cancels --patch only' '\n+\tgit log --raw master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --patch --no-patch --raw >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -m matches pure log' '\n \tgit log master >result &&\n \tprocess_diffs result >expected &&\n-- \n2.25.1\n\n"},{"id":"476598","messageId":"xmqqttwskse5.fsf@gitster.g","threadId":"59688","inReplyTo":"xmqqmt2lqofb.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T15:50:26Z","receivedAt":"2023-05-04T15:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> No problem from my side, but are you sure?\n>\n> Absolutely.\n>\n> I've seen people just say \"we document a failed one\" and leave it at\n> that, without attempting to fix.  I am trying to see if pushing back\n> at first would serve as a good way to encourage these known failure\n> to be fixed, without accumulating too many expect_failure in our\n> test suite, which will waste cycles at CI runs (which do not need to\n> be reminded something is known to be broken).  I will try not to do\n> this when I do not positively know the author of such a patch is\n> capable enough to provide a fix, though, and you are unlucky enough\n> to have shown your abilities in the past ;-)\n\nI ended up spending some time digging history and remembered that\n\"--no-patch\" was added as a synonym to \"-s\" by d09cd15d (diff: allow\n--no-patch as synonym for -s, 2013-07-16).  These\n\n    git diff -p --stat --no-patch HEAD^ HEAD\n    git diff -p --raw --no-patch HEAD^ HEAD\n\nwould show no output from the diff machinery, patches, diffstats,\nraw object names, etc.\n\nAnd this turns out to be a prime example why the approach to ask\ncontributors do more, would help the project overall.  What I should\nhave done, instead of asking for the test with its expect_failure\nturned into expect_success *and* a fix to the code to make the new\ntest work, was to ask to see if it is really a bug in the behaviour\nor if the documentation is wrong.  Then your reaction wouldn't have\nbeen \"are you sure?\".  It hopefully would have been \"ah, the intent\nis not documented correctly, and here is a documentation patch to\nfix it.\"\n\nWhen a command does not behave the way one thinks it should, being\ncurious is good.  Reporting it as a potential bug is also good.  But\nit would help the project more if it was triaged before reporting it\nas a potential bug, if the reporter is capable of doing so.  Those\nwho encounter behaviour unexpected to them are more numerous than\nthose who can report it as a potential bug (many people are not\nequipped to write a good bug report), and those who can triage and\ndiagnose a bug report are fewer.  Those who can come up with a\nsolution is even more scarse.\n\nThanks.\n"},{"id":"476604","messageId":"xmqq1qjwj7go.fsf@gitster.g","threadId":"59688","inReplyTo":"xmqqsfcdtkt0.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T18:07:51Z","receivedAt":"2023-05-04T18:07:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> --patch followed by --no-patch is to be a no-op according to the \"git\n>> log\" manual page.\n>\n> I briefly wondered if it is a bug in the documentation.\n> ... when \"git log -p --raw\" shows both patch and raw, I do not\n> think of a reason why \"git log -p --raw --no-patch\" should not\n> behave similarly.\n\nSo, to tie the loose ends, \"log -p --raw --no-patch\" and \"log -p\n--stat --no-patch\" do behave similarly.  Where my reaction was\nmistaken was that I did not read the manual page myself that clearly\nsaid it is the same as \"-s\" that suppresses diff output (where \"diff\noutput\" is not limited to \"patch\"---diffstat is also output of \"diff\"),\nand incorrectly thought that \"--no-patch\" would countermand only\n\"--patch\" and nothing else.\n\nIn Documentation/diff-options.txt we have this snippet:\n\n    -s::\n    --no-patch::\n            Suppress diff output. Useful for commands like `git show` that\n            show the patch by default, or to cancel the effect of `--patch`.\n\nI imagine that argument could be made that the last half-sentence\ncan be read to say that the option is usable ONLY to cancel the\neffect of `--patch` without cancelling the effect of anything else.\n\nBut that smells like a bit of stretch, as \"like\" in \"commands like\"\nis a sign, at least to me, that it gives a few examples without\nattempting to be exhaustive (meaning that it is too much to read\n\"ONLY\" that is not written in \"or to cancel the effect of\")..\n\nHere is my attempt to make it tighter to avoid getting mis-read:\n\n    Suppress all output from the diff machinery.  Useful for\n    commands like `git show` that show the patch by default to\n    squelch their output, or to cancel the effect of options like\n    `--patch`, `--stat` earlier on the command line in an alias.\n\n"},{"id":"476605","messageId":"87o7n03qgq.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqqttwskse5.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-04T18:24:05Z","receivedAt":"2023-05-04T18:24:13Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n>>> No problem from my side, but are you sure?\n>>\n>> Absolutely.\n>>\n>> I've seen people just say \"we document a failed one\" and leave it at\n>> that, without attempting to fix.  I am trying to see if pushing back\n>> at first would serve as a good way to encourage these known failure\n>> to be fixed, without accumulating too many expect_failure in our\n>> test suite, which will waste cycles at CI runs (which do not need to\n>> be reminded something is known to be broken).  I will try not to do\n>> this when I do not positively know the author of such a patch is\n>> capable enough to provide a fix, though, and you are unlucky enough\n>> to have shown your abilities in the past ;-)\n>\n> I ended up spending some time digging history and remembered that\n> \"--no-patch\" was added as a synonym to \"-s\" by d09cd15d (diff: allow\n> --no-patch as synonym for -s, 2013-07-16).  These\n>\n>     git diff -p --stat --no-patch HEAD^ HEAD\n>     git diff -p --raw --no-patch HEAD^ HEAD\n>\n> would show no output from the diff machinery, patches, diffstats,\n> raw object names, etc.\n\n[-s meaning \"silent\" at that time? If so, making --no-patch a synonym for\n\"silent\", and then documenting -s a synonym for --no-patch sounds like\nquite a twitch.]\n\nAnyway, this seems pretty irrelevant to the test case. Even\nif we spell --no-patch as -s,\n\n git diff -s --raw HEAD^ HEAD\n\nshould produce what? To me it should be the same as\n\n git diff --raw HEAD^ HEAD\n\nas -s turns off everything, and then --raw is turned on. In reality this\nis not the case though, and that's what the test case is about.\n\nNotice that\n\n git diff -s --patch\n\ndoes produce the patch output, whereas\n\n git diff -s --raw\n git diff -s --stat\n\nproduce none. Sounds like nonsense.\n\n> And this turns out to be a prime example why the approach to ask\n> contributors do more, would help the project overall. What I should\n> have done, instead of asking for the test with its expect_failure\n> turned into expect_success *and* a fix to the code to make the new\n> test work, was to ask to see if it is really a bug in the behaviour or\n> if the documentation is wrong. Then your reaction wouldn't have been\n> \"are you sure?\". It hopefully would have been \"ah, the intent is not\n> documented correctly, and here is a documentation patch to fix it.\n\nYep, documentation then needs to be fixed as well to match the\nintention, but this is unrelated to the test-case, see above.\n\n> When a command does not behave the way one thinks it should, being\n> curious is good.  Reporting it as a potential bug is also good.  But\n> it would help the project more if it was triaged before reporting it\n> as a potential bug, if the reporter is capable of doing so.  Those\n> who encounter behaviour unexpected to them are more numerous than\n> those who can report it as a potential bug (many people are not\n> equipped to write a good bug report), and those who can triage and\n> diagnose a bug report are fewer.  Those who can come up with a\n> solution is even more scarse.\n\nI'm afraid the solution I'd come up with won't be welcomed. If I'd start\nto \"fix\" it, it'd be likely set of independent options:\n\n --patch --no-patch\n --raw   --no-raw\n --stat  --no-stat\n\nand then\n\n -s being just a shortcut for \"--no-raw --no-patch --no-stat\"\n\nEasy to understand, simple to implement, straightforward to document,\nall intentions are perfectly obvious. But then these are to be new\noptions to keep backward compatibility, and... No, thanks.\n\nOverall, as I neither able to make sense of the current set of\nintentions, nor even figure out what they are in the first place, I'm\nnot the right person to fix implementation of these intentions, or even\nfigure out for sure if a fix is needed.\n\nThanks,\n-- Sergey Organov\n"},{"id":"476606","messageId":"87jzxo3qco.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqq1qjwj7go.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-04T18:26:31Z","receivedAt":"2023-05-04T18:26:41Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n>>> --patch followed by --no-patch is to be a no-op according to the \"git\n>>> log\" manual page.\n>>\n>> I briefly wondered if it is a bug in the documentation.\n>> ... when \"git log -p --raw\" shows both patch and raw, I do not\n>> think of a reason why \"git log -p --raw --no-patch\" should not\n>> behave similarly.\n>\n> So, to tie the loose ends, \"log -p --raw --no-patch\" and \"log -p\n> --stat --no-patch\" do behave similarly.  Where my reaction was\n> mistaken was that I did not read the manual page myself that clearly\n> said it is the same as \"-s\" that suppresses diff output (where \"diff\n> output\" is not limited to \"patch\"---diffstat is also output of \"diff\"),\n> and incorrectly thought that \"--no-patch\" would countermand only\n> \"--patch\" and nothing else.\n>\n> In Documentation/diff-options.txt we have this snippet:\n>\n>     -s::\n>     --no-patch::\n>             Suppress diff output. Useful for commands like `git show` that\n>             show the patch by default, or to cancel the effect of `--patch`.\n>\n> I imagine that argument could be made that the last half-sentence\n> can be read to say that the option is usable ONLY to cancel the\n> effect of `--patch` without cancelling the effect of anything else.\n>\n> But that smells like a bit of stretch, as \"like\" in \"commands like\"\n> is a sign, at least to me, that it gives a few examples without\n> attempting to be exhaustive (meaning that it is too much to read\n> \"ONLY\" that is not written in \"or to cancel the effect of\")..\n>\n> Here is my attempt to make it tighter to avoid getting mis-read:\n>\n>     Suppress all output from the diff machinery.  Useful for\n>     commands like `git show` that show the patch by default to\n>     squelch their output, or to cancel the effect of options like\n>     `--patch`, `--stat` earlier on the command line in an alias.\n\nThis is fine, but is irrelevant to the test-case. Please refer to my\nanswer to your previous reply on the issue for details.\n\nThanks,\n-- Sergey Organov\n\n"},{"id":"476607","messageId":"xmqqpm7fizsl.fsf@gitster.g","threadId":"59688","inReplyTo":"87o7n03qgq.fsf@osv.gnss.ru","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T20:53:30Z","receivedAt":"2023-05-04T20:54:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> [-s meaning \"silent\" at that time? If so, making --no-patch a synonym for\n> \"silent\", and then documenting -s a synonym for --no-patch sounds like\n> quite a twitch.]\n\nI thought it stood for \"squelch\", but it turns out that it was\nintroduced by f4f21ce3 (git-diff-tree: clean up output, 2005-05-06),\nas a \"silent\" mode, way before anything else in the thread has\nhappened.\n\nIn Git v1.8.4, \"--no-patch\" was added as a synonym for \"-s\", and\never since we documented that they are equivalents.\n\n> Anyway, this seems pretty irrelevant to the test case. Even\n> if we spell --no-patch as -s,\n>\n>  git diff -s --raw HEAD^ HEAD\n>\n> should produce what? To me it should be the same as\n>\n>  git diff --raw HEAD^ HEAD\n>\n> as -s turns off everything, and then --raw is turned on.\n\nAh, yeah, I missed that part of your sample command.  It does seem\nquite puzzling, and the bad (or good?) part of the story is that my\nfresh build of v1.8.4 behaves exactly the same way.  And with \"-s\"\nwe can try versions before v1.8.4 (the only change that version made\nwas to introduce \"--no-patch\" as its synonym).  It seems it behaved\nthat way at least back to v1.5.3 (which happens to be the oldest\nversion I consider worth comparing with more modern versions).\n\n> Notice that\n>\n>  git diff -s --patch\n>\n> does produce the patch output, whereas\n>\n>  git diff -s --raw\n>  git diff -s --stat\n>\n> produce none.\n\nYes, you're right.\n\n> Sounds like nonsense.\n\nSounds like a bug to me.  I wonder how it came about.  Did we forget\nto add support of the equivalent of what \"-s --patch\" does, when we\nadded \"--raw\" and \"--stat\", perhaps?\n\n> I'm afraid the solution I'd come up with won't be welcomed. If I'd start\n> to \"fix\" it, it'd be likely set of independent options:\n>\n>  --patch --no-patch\n>  --raw   --no-raw\n>  --stat  --no-stat\n>\n> and then\n>\n>  -s being just a shortcut for \"--no-raw --no-patch --no-stat\"\n\nIf I were writing Git from scratch without any existing users, that\nwould be how I would design it (modulo that I would make sure we\nhave some mechanism to make it easier for developers who may add\na new output <format> to ensure that \"-s\" also implies \"--no-<format>\"\nfor the new <format> they are adding to the mix).\n\nThe fact that this wasn't brought up until now may mean that nobody\nwould notice if we redefined the definition of \"--no-patch\" to\nbehave that way, as long as \"-s\" keeps its original meaning.  \n\nI dunno.\n\nThanks.\n"},{"id":"476609","messageId":"xmqqjzxnixqr.fsf_-_@gitster.g","threadId":"59688","inReplyTo":"xmqqpm7fizsl.fsf@gitster.g","subject":"Re* [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T21:37:48Z","receivedAt":"2023-05-04T21:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> ... it'd be likely set of independent options:\n\n>>  --patch --no-patch\n>>  --raw   --no-raw\n>>  --stat  --no-stat\n>>\n>> and then\n>>\n>>  -s being just a shortcut for \"--no-raw --no-patch --no-stat\"\n>\n> If I were writing Git from scratch without any existing users, that\n> would be how I would design it (modulo that I would make sure we\n> have some mechanism to make it easier for developers who may add\n> a new output <format> to ensure that \"-s\" also implies \"--no-<format>\"\n> for the new <format> they are adding to the mix).\n>\n> The fact that this wasn't brought up until now may mean that nobody\n> would notice if we redefined the definition of \"--no-patch\" to\n> behave that way, as long as \"-s\" keeps its original meaning.  \n>\n> I dunno.\n\nI haven't run any tests (not just your new one, but existing ones)\nbut at least \"git diff -s --stat\" and \"git diff -s --raw\" do countermand\nthe earlier \"-s\" with this patch.  I am not signing it off because I\nstarted from the options[] array in add_diff_options() and tweaked\nthose I happened to notice, and haven't checked if we need to adjust\nother entries in the array.\n\n diff.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex 1e83aaee6b..2d8025a9f7 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -4929,6 +4929,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)\n \t\tBUG(\"%s should not get here\", opt->long_name);\n \n \toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->stat_name_width = name_width;\n \toptions->stat_graph_width = graph_width;\n \toptions->stat_width = width;\n@@ -4947,6 +4948,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \t * The caller knows a dirstat-related option is given from the command\n \t * line; allow it to say \"return this_function();\"\n \t */\n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->output_format |= DIFF_FORMAT_DIRSTAT;\n \treturn 1;\n }\n@@ -5502,9 +5504,9 @@ struct option *add_diff_options(const struct option *opts,\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),\n \t\tOPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n \t\t\t N_(\"generate diffs with <n> lines context\")),\n-\t\tOPT_BIT_F(0, \"raw\", &options->output_format,\n+\t\tOPT_BITOP(0, \"raw\", &options->output_format,\n \t\t\t  N_(\"generate the diff in raw format\"),\n-\t\t\t  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BITOP(0, \"patch-with-raw\", &options->output_format,\n \t\t\t  N_(\"synonym for '-p --raw'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,\n@@ -5513,12 +5515,12 @@ struct option *add_diff_options(const struct option *opts,\n \t\t\t  N_(\"synonym for '-p --stat'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,\n \t\t\t  DIFF_FORMAT_NO_OUTPUT),\n-\t\tOPT_BIT_F(0, \"numstat\", &options->output_format,\n+\t\tOPT_BITOP(0, \"numstat\", &options->output_format,\n \t\t\t  N_(\"machine friendly --stat\"),\n-\t\t\t  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),\n-\t\tOPT_BIT_F(0, \"shortstat\", &options->output_format,\n+\t\t\t  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),\n+\t\tOPT_BITOP(0, \"shortstat\", &options->output_format,\n \t\t\t  N_(\"output only the last line of --stat\"),\n-\t\t\t  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_CALLBACK_F('X', \"dirstat\", options, N_(\"<param1,param2>...\"),\n \t\t\t       N_(\"output the distribution of relative amount of changes for each sub-directory\"),\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,\n"},{"id":"476610","messageId":"xmqqfs8bith1.fsf_-_@gitster.g","threadId":"59688","inReplyTo":"xmqqjzxnixqr.fsf_-_@gitster.g","subject":"[PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T23:10:02Z","receivedAt":"2023-05-04T23:10:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I haven't run any tests (not just your new one, but existing ones)\n> but ...\n\nAnd of course, not writing tests fails to even realize that the bug\nhas two components, \"-s\" failing to clear the bits previously set,\nand other options not clearing the bit set by \"-s\".\n\nThis version may still be rough, but at least the full test suite\nhas been run with it, so I have a bit more confidence than the\nearlier one (which may not mean much).\n\n------- >8 ------------- >8 ------------- >8 -------------\nSergey Organov noticed and reported \"--patch --no-patch --raw\"\nbehaves differently from \"--raw\".  It turns out there are a few\ninteresting bugs in the implementation and documentation.\n\n * First, the documentation for \"--no-patch\" was unclear that it\n   could be read to mean \"--no-patch\" countermands an earlier\n   \"--patch\" but not other things.  The intention of \"--no-patch\"\n   ever since it was introduced at d09cd15d (diff: allow --no-patch\n   as synonym for -s, 2013-07-16) was to serve as a synonym for\n   \"-s\", so \"--raw --patch --no-patch\" should have produced no\n   output, but it can be (mis)read to allow showing only \"--raw\"\n   output.\n\n * Then the interaction between \"-s\" and other format options were\n   poorly implemented.  Modern versions of Git uses one bit each to\n   represent formatting options like \"--patch\", \"--stat\" in a single\n   output_format word, but for historical reasons, \"-s\" also is\n   represented as another bit in the same word.  This allows two\n   interesting bugs to happen, and we have both.\n\n   (1) After setting a format bit, then setting NO_OUTPUT with \"-s\",\n       the code to process another \"--<format>\" option drops the\n       NO_OUTPUT bit to allow output to be shown again.  However,\n       the code to handle \"-s\" only set NO_OUTPUT without unsetting\n       format bits set earlier, so the earlier format bit got\n       revealed upon seeing the second \"--<format>\" option.  THis is\n       the problem Sergey observed.\n\n   (2) After setting NO_OUTPUT with \"-s\", code to process\n       \"--<format>\" option can forget to unset NO_OUTPUT, leaving\n       the command still silent.\n\nIt is tempting to change the meaning of \"--no-patch\" to mean\n\"disable only the patch format output\" and reimplement \"-s\" as \"not\nshowing anything\", but it would be an end-user visible change in\nbehaviour.  Let's fix the interactions of these bits to first make\n\"-s\" work as intended.\n\nThe fix is conceptually very simple.\n\n * Whenever we set DIFF_FORMAT_FOO becasuse we saw the \"--foo\"\n   option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n   given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n   do so in some of the options and caused (2) above.\n\n * When processing \"-s\" option, we should not just set\n   DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n   We didn't do so and retained format bits set by options\n   previously seen, causing (1) above.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/diff-options.txt |  7 +++++--\n diff.c                         | 24 +++++++++++++-----------\n t/t4000-diff-format.sh         | 32 +++++++++++++++++++++++++++++++-\n 3 files changed, 49 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e9..7d5bb65a49 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -29,8 +29,11 @@ endif::git-diff[]\n \n -s::\n --no-patch::\n-\tSuppress diff output. Useful for commands like `git show` that\n-\tshow the patch by default, or to cancel the effect of `--patch`.\n+\tSuppress all output from the diff machinery.  Useful for\n+\tcommands like `git show` that show the patch by default to\n+\tsquelch their output, or to cancel the effect of options like\n+\t`--patch`, `--stat` earlier on the command line in an alias.\n+\n endif::git-format-patch[]\n \n ifdef::git-log[]\ndiff --git a/diff.c b/diff.c\nindex 648f6717a5..5a2f096683 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)\n \t} else\n \t\tBUG(\"%s should not get here\", opt->long_name);\n \n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n \toptions->stat_name_width = name_width;\n \toptions->stat_graph_width = graph_width;\n@@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \t * The caller knows a dirstat-related option is given from the command\n \t * line; allow it to say \"return this_function();\"\n \t */\n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->output_format |= DIFF_FORMAT_DIRSTAT;\n \treturn 1;\n }\n@@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt,\n \t\toptions->flags.stat_with_summary = 0;\n \t} else {\n \t\toptions->flags.stat_with_summary = 1;\n+\t\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n \t}\n \treturn 0;\n@@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BITOP('p', \"patch\", &options->output_format,\n \t\t\t  N_(\"generate patch\"),\n \t\t\t  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n-\t\tOPT_BIT_F('s', \"no-patch\", &options->output_format,\n-\t\t\t  N_(\"suppress diff output\"),\n-\t\t\t  DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT('s', \"no-patch\", &options->output_format,\n+\t\t\t    N_(\"suppress diff output\"), DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BITOP('u', NULL, &options->output_format,\n \t\t\t  N_(\"generate patch\"),\n \t\t\t  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n@@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),\n \t\tOPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n \t\t\t N_(\"generate diffs with <n> lines context\")),\n-\t\tOPT_BIT_F(0, \"raw\", &options->output_format,\n+\t\tOPT_BITOP(0, \"raw\", &options->output_format,\n \t\t\t  N_(\"generate the diff in raw format\"),\n-\t\t\t  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BITOP(0, \"patch-with-raw\", &options->output_format,\n \t\t\t  N_(\"synonym for '-p --raw'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,\n@@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t  N_(\"synonym for '-p --stat'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,\n \t\t\t  DIFF_FORMAT_NO_OUTPUT),\n-\t\tOPT_BIT_F(0, \"numstat\", &options->output_format,\n+\t\tOPT_BITOP(0, \"numstat\", &options->output_format,\n \t\t\t  N_(\"machine friendly --stat\"),\n-\t\t\t  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),\n-\t\tOPT_BIT_F(0, \"shortstat\", &options->output_format,\n+\t\t\t  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),\n+\t\tOPT_BITOP(0, \"shortstat\", &options->output_format,\n \t\t\t  N_(\"output only the last line of --stat\"),\n-\t\t\t  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_CALLBACK_F('X', \"dirstat\", options, N_(\"<param1,param2>...\"),\n \t\t\t       N_(\"output the distribution of relative amount of changes for each sub-directory\"),\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,\n@@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"check\", &options->output_format,\n \t\t\t  N_(\"warn if changes introduce conflict markers or whitespace errors\"),\n \t\t\t  DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG),\n-\t\tOPT_BIT_F(0, \"summary\", &options->output_format,\n+\t\tOPT_BITOP(0, \"summary\", &options->output_format,\n \t\t\t  N_(\"condensed summary such as creations, renames and mode changes\"),\n-\t\t\t  DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BIT_F(0, \"name-only\", &options->output_format,\n \t\t\t  N_(\"show only names of changed files\"),\n \t\t\t  DIFF_FORMAT_NAME, PARSE_OPT_NONEG),\ndiff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh\nindex bfcaae390f..762b9d4c60 100755\n--- a/t/t4000-diff-format.sh\n+++ b/t/t4000-diff-format.sh\n@@ -5,6 +5,9 @@\n \n test_description='Test built-in diff output engine.\n \n+We happen to know that all diff plumbing and diff Porcelain share the\n+same command line parser, so testing one should be sufficient; pick\n+diff-files as a representative.\n '\n \n TEST_PASSES_SANITIZE_LEAK=true\n@@ -16,9 +19,11 @@ Line 2\n line 3'\n cat path0 >path1\n chmod +x path1\n+mkdir path2\n+>path2/path3\n \n test_expect_success 'update-index --add two files with and without +x.' '\n-\tgit update-index --add path0 path1\n+\tgit update-index --add path0 path1 path2/path3\n '\n \n mv path0 path0-\n@@ -91,4 +96,29 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n \ttest_must_be_empty err\n '\n \n+\n+# Smudge path2/path3 so that dirstat has something to show\n+date >path2/path3\n+\n+for format in stat raw numstat shortstat dirstat\n+do\n+\ttest_expect_success \"--no-patch in 'git diff-files --no-patch --$format' is a no-op\" '\n+\t\tgit diff-files --no-patch \"--$format\" >actual &&\n+\t\tgit diff-files \"--$format\" >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"--no-patch clears all previous ones\" '\n+\t\tgit diff-files --$format -s -p >actual &&\n+\t\tgit diff-files -p >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"--no-patch in 'git diff --no-patch --$format' is a no-op\" '\n+\t\tgit diff --no-patch \"--$format\" >actual &&\n+\t\tgit diff \"--$format\" >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_done\n-- \n2.40.1-476-g69c786637d\n\n\n"},{"id":"476615","messageId":"xmqq4joribyv.fsf@gitster.g","threadId":"59688","inReplyTo":"xmqqfs8bith1.fsf_-_@gitster.g","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T05:28:08Z","receivedAt":"2023-05-05T05:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>  * Then the interaction between \"-s\" and other format options were\n>    poorly implemented.  Modern versions of Git uses one bit each to\n>    represent formatting options like \"--patch\", \"--stat\" in a single\n>    output_format word, but for historical reasons, \"-s\" also is\n>    represented as another bit in the same word.\n\nAn obvious improvement strategy is to stop using the NO_OUTPUT bit\nand instead make \"-s\" to clear the \"output_format\" word, and make\n\"--[no-]raw\", \"--[no-]stat\", \"--[no-]patch\", etc. to flip their own\nbit in the same \"output_format\" word.  I think the \"historical\nreasons\" why we did not do that was because we wanted to be able to\ndo a flexible defaulting.  We may want to say \"if no output-format\noption is given from the command line, default to \"--patch\", but\notherwise do not set the \"--patch\" bit on\", for example.\nInitializing the \"output_format\" word with \"--patch\" bit on would\nnot work---when \"--raw\" is given from the command line, we want to\nclear that \"--patch\" bit we set for default and set \"--raw\" bit on.\nWe can initialize the \"output_format\" word to 0, and OR in the bits\nfor each format option as we process them, and then flip the\n\"--patch\" bit on if \"output_format\" word is still 0 after command\nline parsing is done.  This would almost work, except that it would\nmake it hard to tell \"no command line options\" case and \"'-s' cleared\nall bits\" case apart (the former wants to default to \"--patch\",\nwhile the latter wants to stay \"no output\"), and it probably was the\nreason why we gave an extra NO_OUTPUT bit to the \"-s\" option.  In\nhindsight, the arrangement certainly made other things harder and\nprone to unnecessary bugs.\n\nAnyway...\n\n"},{"id":"476619","messageId":"87sfcbyy8c.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqqfs8bith1.fsf_-_@gitster.g","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-05T08:32:51Z","receivedAt":"2023-05-05T08:33:35Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I haven't run any tests (not just your new one, but existing ones)\n>> but ...\n>\n> And of course, not writing tests fails to even realize that the bug\n> has two components, \"-s\" failing to clear the bits previously set,\n> and other options not clearing the bit set by \"-s\".\n>\n> This version may still be rough, but at least the full test suite\n> has been run with it, so I have a bit more confidence than the\n> earlier one (which may not mean much).\n>\n> ------- >8 ------------- >8 ------------- >8 -------------\n> Sergey Organov noticed and reported \"--patch --no-patch --raw\"\n> behaves differently from \"--raw\".  It turns out there are a few\n> interesting bugs in the implementation and documentation.\n>\n>  * First, the documentation for \"--no-patch\" was unclear that it\n>    could be read to mean \"--no-patch\" countermands an earlier\n>    \"--patch\" but not other things.  The intention of \"--no-patch\"\n>    ever since it was introduced at d09cd15d (diff: allow --no-patch\n>    as synonym for -s, 2013-07-16) was to serve as a synonym for\n>    \"-s\", so \"--raw --patch --no-patch\" should have produced no\n>    output, but it can be (mis)read to allow showing only \"--raw\"\n>    output.\n>\n>  * Then the interaction between \"-s\" and other format options were\n>    poorly implemented.  Modern versions of Git uses one bit each to\n>    represent formatting options like \"--patch\", \"--stat\" in a single\n>    output_format word, but for historical reasons, \"-s\" also is\n>    represented as another bit in the same word.  This allows two\n>    interesting bugs to happen, and we have both.\n>\n>    (1) After setting a format bit, then setting NO_OUTPUT with \"-s\",\n>        the code to process another \"--<format>\" option drops the\n>        NO_OUTPUT bit to allow output to be shown again.  However,\n>        the code to handle \"-s\" only set NO_OUTPUT without unsetting\n>        format bits set earlier, so the earlier format bit got\n>        revealed upon seeing the second \"--<format>\" option.  THis is\n>        the problem Sergey observed.\n>\n>    (2) After setting NO_OUTPUT with \"-s\", code to process\n>        \"--<format>\" option can forget to unset NO_OUTPUT, leaving\n>        the command still silent.\n>\n> It is tempting to change the meaning of \"--no-patch\" to mean\n> \"disable only the patch format output\" and reimplement \"-s\" as \"not\n> showing anything\", but it would be an end-user visible change in\n> behaviour.  Let's fix the interactions of these bits to first make\n> \"-s\" work as intended.\n>\n> The fix is conceptually very simple.\n>\n>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the \"--foo\"\n>    option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n>    do so in some of the options and caused (2) above.\n>\n>  * When processing \"-s\" option, we should not just set\n>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n>    We didn't do so and retained format bits set by options\n>    previously seen, causing (1) above.\n\nSounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as\nwell, I wonder, as absence of any output bits effectively means \"no\noutput\"?\n\nThanks,\n-- Sergey Organov\n"},{"id":"476625","messageId":"xmqq8re2g2pj.fsf@gitster.g","threadId":"59688","inReplyTo":"87sfcbyy8c.fsf@osv.gnss.ru","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T16:31:04Z","receivedAt":"2023-05-05T16:31:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n>>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the \"--foo\"\n>>    option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n>>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n>>    do so in some of the options and caused (2) above.\n>>\n>>  * When processing \"-s\" option, we should not just set\n>>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n>>    We didn't do so and retained format bits set by options\n>>    previously seen, causing (1) above.\n>\n> Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as\n> well, I wonder, as absence of any output bits effectively means \"no\n> output\"?\n\nNot quite.  The latter is not \"set 0 to output_format word\", but\n\"set 0 to output_format word and then flip only NO_OUTPUT bit on\".\nI've written a bit more on it in a follow-up message to the patch.\n\nThanks.\n"},{"id":"476626","messageId":"xmqqlei2en6t.fsf@gitster.g","threadId":"59688","inReplyTo":"xmqq4joribyv.fsf@gitster.g","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T16:51:38Z","receivedAt":"2023-05-05T16:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  I think the \"historical\n> reasons\" why we did not do that was because we wanted to be able to\n> do a flexible defaulting. ...\n> This would almost work, except that it would\n> make it hard to tell \"no command line options\" case and \"'-s' cleared\n> all bits\" case apart (the former wants to default to \"--patch\",\n> while the latter wants to stay \"no output\"), and it probably was the\n> reason why we gave an extra NO_OUTPUT bit to the \"-s\" option.  In\n> hindsight, the arrangement certainly made other things harder and\n> prone to unnecessary bugs.\n>\n> Anyway...\n\nThe distinction between the presense of NO_OUTPUT bit and absolutely\nempty output_format word indeed is used by \"git show\", in the\nbuiltin/log.c::show_setup_revisions_tweak() function.\n\nWe could lose DIFF_FORMAT_NO_OUTPUT bit, but then we need to replace\nit with something else (i.e. DIFF_FORMAT_OPTION_GIVEN bit), and\n\n * \"--patch\", \"--raw\", etc. will set DIFF_FORMAT_$format bit and\n   DIFF_FORMAT_OPTION_GIVEN bit on for each format.  \"--no-raw\", \n   etc. will set off DIFF_FORMAT_$format bit but still record the\n   fact that we saw an option from the command line by setting\n   DIFF_FORMAT_OPTION_GIVEN bit.\n\n * \"-s\" (and its synonym \"--no-patch\") will set the\n   DIFF_FORMAT_OPTION_GIVEN bit on, and clear all other bits.\n\nwhich I suspect would make the code much cleaner without breaking\nany end-user expectations.\n\nOnce that is in place, transitioning \"--no-patch\" to mean the\ncounterpart of \"--patch\", just like \"--no-raw\" only defeats an\nearlier \"--raw\", would be quite simple at the code level.  The\nsocial cost of migrating the end-user expectations might be too\ngreat for it to be worth, but at least the \"GIVEN\" bit clean-up\nalone may be worth it.\n\nNot that I would be starting the process right away...\n\n"},{"id":"476627","messageId":"20230505165952.335256-1-gitster@pobox.com","threadId":"59688","inReplyTo":"xmqqfs8bith1.fsf_-_@gitster.g","subject":"[PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T16:59:52Z","receivedAt":"2023-05-05T16:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov noticed and reported \"--patch --no-patch --raw\"\nbehaves differently from just \"--raw\".  It turns out that there are\na few interesting bugs in the implementation and documentation.\n\n * First, the documentation for \"--no-patch\" was unclear that it\n   could be read to mean \"--no-patch\" countermands an earlier\n   \"--patch\" but not other things.  The intention of \"--no-patch\"\n   ever since it was introduced at d09cd15d (diff: allow --no-patch\n   as synonym for -s, 2013-07-16) was to serve as a synonym for\n   \"-s\", so \"--raw --patch --no-patch\" should have produced no\n   output, but it can be (mis)read to allow showing only \"--raw\"\n   output.\n\n * Then the interaction between \"-s\" and other format options were\n   poorly implemented.  Modern versions of Git uses one bit each to\n   represent formatting options like \"--patch\", \"--stat\" in a single\n   output_format word, but for historical reasons, \"-s\" also is\n   represented as another bit in the same word.  This allows two\n   interesting bugs to happen, and we have both X-<.\n\n   (1) After setting a format bit, then setting NO_OUTPUT with \"-s\",\n       the code to process another \"--<format>\" option drops the\n       NO_OUTPUT bit to allow output to be shown again.  However,\n       the code to handle \"-s\" only set NO_OUTPUT without unsetting\n       format bits set earlier, so the earlier format bit got\n       revealed upon seeing the second \"--<format>\" option.  This is\n       the problem Sergey observed.\n\n   (2) After setting NO_OUTPUT with \"-s\", code to process\n       \"--<format>\" option can forget to unset NO_OUTPUT, leaving\n       the command still silent.\n\nIt is tempting to change the meaning of \"--no-patch\" to mean\n\"disable only the patch format output\" and reimplement \"-s\" as \"not\nshowing anything\", but it would be an end-user visible change in\nbehavior.  Let's fix the interactions of these bits to first make\n\"-s\" work as intended.\n\nThe fix is conceptually very simple.\n\n * Whenever we set DIFF_FORMAT_FOO because we saw the \"--foo\"\n   option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n   given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n   do so in some of the options and caused (2) above.\n\n * When processing \"-s\" option, we should not just set\n   DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n   We didn't do so and retained format bits set by options\n   previously seen, causing (1) above.\n\nIt is even more tempting to lose NO_OUTPUT bit and instead take\noutput_format word being 0 as its replacement, but that would break\nthe mechanism \"git show\" uses to default to \"--patch\" output, where\nthe distinction between telling the command to be silent with \"-s\"\nand having no output format specified on the command line matters,\nand an explicit output format given on the command line should not\nbe \"combined\" with the default \"--patch\" format.\n\nSo, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we\nmay want to replace it with OPTION_GIVEN bit, and\n\n * make \"--patch\", \"--raw\", etc. set DIFF_FORMAT_$format bit and\n   DIFF_FORMAT_OPTION_GIVEN bit on for each format.  \"--no-raw\",\n   etc. will set off DIFF_FORMAT_$format bit but still record the\n   fact that we saw an option from the command line by setting\n   DIFF_FORMAT_OPTION_GIVEN bit.\n\n * make \"-s\" (and its synonym \"--no-patch\") clear all other bits\n   and set only the DIFF_FORMAT_OPTION_GIVEN bit on.\n\nwhich I suspect would make the code much cleaner without breaking\nany end-user expectations.\n\nOnce that is in place, transitioning \"--no-patch\" to mean the\ncounterpart of \"--patch\", just like \"--no-raw\" only defeats an\nearlier \"--raw\", would be quite simple at the code level.  The\nsocial cost of migrating the end-user expectations might be too\ngreat for it to be worth, but at least the \"GIVEN\" bit clean-up\nalone may be worth it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/diff-options.txt |  7 +++++--\n diff.c                         | 24 +++++++++++++-----------\n t/t4000-diff-format.sh         | 34 +++++++++++++++++++++++++++++++++-\n 3 files changed, 51 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e9..7d5bb65a49 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -29,8 +29,11 @@ endif::git-diff[]\n \n -s::\n --no-patch::\n-\tSuppress diff output. Useful for commands like `git show` that\n-\tshow the patch by default, or to cancel the effect of `--patch`.\n+\tSuppress all output from the diff machinery.  Useful for\n+\tcommands like `git show` that show the patch by default to\n+\tsquelch their output, or to cancel the effect of options like\n+\t`--patch`, `--stat` earlier on the command line in an alias.\n+\n endif::git-format-patch[]\n \n ifdef::git-log[]\ndiff --git a/diff.c b/diff.c\nindex 648f6717a5..5a2f096683 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)\n \t} else\n \t\tBUG(\"%s should not get here\", opt->long_name);\n \n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n \toptions->stat_name_width = name_width;\n \toptions->stat_graph_width = graph_width;\n@@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \t * The caller knows a dirstat-related option is given from the command\n \t * line; allow it to say \"return this_function();\"\n \t */\n+\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \toptions->output_format |= DIFF_FORMAT_DIRSTAT;\n \treturn 1;\n }\n@@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt,\n \t\toptions->flags.stat_with_summary = 0;\n \t} else {\n \t\toptions->flags.stat_with_summary = 1;\n+\t\toptions->output_format &= ~DIFF_FORMAT_NO_OUTPUT;\n \t\toptions->output_format |= DIFF_FORMAT_DIFFSTAT;\n \t}\n \treturn 0;\n@@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BITOP('p', \"patch\", &options->output_format,\n \t\t\t  N_(\"generate patch\"),\n \t\t\t  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n-\t\tOPT_BIT_F('s', \"no-patch\", &options->output_format,\n-\t\t\t  N_(\"suppress diff output\"),\n-\t\t\t  DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT('s', \"no-patch\", &options->output_format,\n+\t\t\t    N_(\"suppress diff output\"), DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BITOP('u', NULL, &options->output_format,\n \t\t\t  N_(\"generate patch\"),\n \t\t\t  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),\n@@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),\n \t\tOPT_BOOL('W', \"function-context\", &options->flags.funccontext,\n \t\t\t N_(\"generate diffs with <n> lines context\")),\n-\t\tOPT_BIT_F(0, \"raw\", &options->output_format,\n+\t\tOPT_BITOP(0, \"raw\", &options->output_format,\n \t\t\t  N_(\"generate the diff in raw format\"),\n-\t\t\t  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BITOP(0, \"patch-with-raw\", &options->output_format,\n \t\t\t  N_(\"synonym for '-p --raw'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,\n@@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t  N_(\"synonym for '-p --stat'\"),\n \t\t\t  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,\n \t\t\t  DIFF_FORMAT_NO_OUTPUT),\n-\t\tOPT_BIT_F(0, \"numstat\", &options->output_format,\n+\t\tOPT_BITOP(0, \"numstat\", &options->output_format,\n \t\t\t  N_(\"machine friendly --stat\"),\n-\t\t\t  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),\n-\t\tOPT_BIT_F(0, \"shortstat\", &options->output_format,\n+\t\t\t  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),\n+\t\tOPT_BITOP(0, \"shortstat\", &options->output_format,\n \t\t\t  N_(\"output only the last line of --stat\"),\n-\t\t\t  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_CALLBACK_F('X', \"dirstat\", options, N_(\"<param1,param2>...\"),\n \t\t\t       N_(\"output the distribution of relative amount of changes for each sub-directory\"),\n \t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,\n@@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"check\", &options->output_format,\n \t\t\t  N_(\"warn if changes introduce conflict markers or whitespace errors\"),\n \t\t\t  DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG),\n-\t\tOPT_BIT_F(0, \"summary\", &options->output_format,\n+\t\tOPT_BITOP(0, \"summary\", &options->output_format,\n \t\t\t  N_(\"condensed summary such as creations, renames and mode changes\"),\n-\t\t\t  DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG),\n+\t\t\t  DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT),\n \t\tOPT_BIT_F(0, \"name-only\", &options->output_format,\n \t\t\t  N_(\"show only names of changed files\"),\n \t\t\t  DIFF_FORMAT_NAME, PARSE_OPT_NONEG),\ndiff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh\nindex bfcaae390f..8d50331b8c 100755\n--- a/t/t4000-diff-format.sh\n+++ b/t/t4000-diff-format.sh\n@@ -5,6 +5,9 @@\n \n test_description='Test built-in diff output engine.\n \n+We happen to know that all diff plumbing and diff Porcelain share the\n+same command line parser, so testing one should be sufficient; pick\n+diff-files as a representative.\n '\n \n TEST_PASSES_SANITIZE_LEAK=true\n@@ -16,9 +19,11 @@ Line 2\n line 3'\n cat path0 >path1\n chmod +x path1\n+mkdir path2\n+>path2/path3\n \n test_expect_success 'update-index --add two files with and without +x.' '\n-\tgit update-index --add path0 path1\n+\tgit update-index --add path0 path1 path2/path3\n '\n \n mv path0 path0-\n@@ -91,4 +96,31 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n \ttest_must_be_empty err\n '\n \n+\n+# Smudge path2/path3 so that dirstat has something to show\n+date >path2/path3\n+\n+for format in stat raw numstat shortstat summary \\\n+\tdirstat cumulative dirstat-by-file \\\n+\tpatch-with-raw patch-with-stat compact-summary\n+do\n+\ttest_expect_success \"--no-patch in 'git diff-files --no-patch --$format' is a no-op\" '\n+\t\tgit diff-files --no-patch \"--$format\" >actual &&\n+\t\tgit diff-files \"--$format\" >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"--no-patch clears all previous ones\" '\n+\t\tgit diff-files --$format -s -p >actual &&\n+\t\tgit diff-files -p >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"--no-patch in 'git diff --no-patch --$format' is a no-op\" '\n+\t\tgit diff --no-patch \"--$format\" >actual &&\n+\t\tgit diff \"--$format\" >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_done\n-- \n2.40.1-476-g69c786637d\n\n"},{"id":"476628","messageId":"87h6sqzoyt.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"xmqq8re2g2pj.fsf@gitster.g","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-05T17:07:38Z","receivedAt":"2023-05-05T17:07:45Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>>>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the \"--foo\"\n>>> option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n>>> given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to do\n>>> so in some of the options and caused (2) above.\n>>> * When processing \"-s\" option, we should not just set\n>>> DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. We\n>>> didn't do so and retained format bits set by options previously\n>>> seen, causing (1) above.\n>> Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete\n>> as well, I wonder, as absence of any output bits effectively means\n>> \"no output\"?\n>\n> Not quite. The latter is not \"set 0 to output_format word\", but \"set 0\n> to output_format word and then flip only NO_OUTPUT bit on\". I've\n> written a bit more on it in a follow-up message to the patch.\n\nYep, I've noticed that post after I sent the question.\n\nThanks,\n-- Sergey Organov\n"},{"id":"476636","messageId":"CAPig+cT=3dmtEEApiPUvB9+5ZHx+uwc1NXhYsf4peYiSwPYPsQ@mail.gmail.com","threadId":"59688","inReplyTo":"20230505165952.335256-1-gitster@pobox.com","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-05T17:41:16Z","receivedAt":"2023-05-05T17:41:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 5, 2023 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Sergey Organov noticed and reported \"--patch --no-patch --raw\"\n> behaves differently from just \"--raw\".  It turns out that there are\n> a few interesting bugs in the implementation and documentation.\n>\n>  * First, the documentation for \"--no-patch\" was unclear that it\n>    could be read to mean \"--no-patch\" countermands an earlier\n>    \"--patch\" but not other things.  The intention of \"--no-patch\"\n>    ever since it was introduced at d09cd15d (diff: allow --no-patch\n>    as synonym for -s, 2013-07-16) was to serve as a synonym for\n>    \"-s\", so \"--raw --patch --no-patch\" should have produced no\n>    output, but it can be (mis)read to allow showing only \"--raw\"\n>    output.\n>\n>  * Then the interaction between \"-s\" and other format options were\n>    poorly implemented.  Modern versions of Git uses one bit each to\n>    represent formatting options like \"--patch\", \"--stat\" in a single\n>    output_format word, but for historical reasons, \"-s\" also is\n>    represented as another bit in the same word.  This allows two\n>    interesting bugs to happen, and we have both X-<.\n>\n>    (1) After setting a format bit, then setting NO_OUTPUT with \"-s\",\n>        the code to process another \"--<format>\" option drops the\n>        NO_OUTPUT bit to allow output to be shown again.  However,\n>        the code to handle \"-s\" only set NO_OUTPUT without unsetting\n>        format bits set earlier, so the earlier format bit got\n>        revealed upon seeing the second \"--<format>\" option.  This is\n\nGlad to see \"THis\" from v1 fixed.\n\n>        the problem Sergey observed.\n>\n>    (2) After setting NO_OUTPUT with \"-s\", code to process\n>        \"--<format>\" option can forget to unset NO_OUTPUT, leaving\n>        the command still silent.\n>\n> It is tempting to change the meaning of \"--no-patch\" to mean\n> \"disable only the patch format output\" and reimplement \"-s\" as \"not\n> showing anything\", but it would be an end-user visible change in\n> behavior.  Let's fix the interactions of these bits to first make\n> \"-s\" work as intended.\n>\n> The fix is conceptually very simple.\n>\n>  * Whenever we set DIFF_FORMAT_FOO because we saw the \"--foo\"\n>    option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n>    do so in some of the options and caused (2) above.\n>\n>  * When processing \"-s\" option, we should not just set\n>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n>    We didn't do so and retained format bits set by options\n>    previously seen, causing (1) above.\n\nThe above description is very clear and well stated, even to someone\nlike me who didn't follow the discussion which culminated in this\npatch.\n\n> It is even more tempting to lose NO_OUTPUT bit and instead take\n> output_format word being 0 as its replacement, but that would break\n> the mechanism \"git show\" uses to default to \"--patch\" output, where\n> the distinction between telling the command to be silent with \"-s\"\n> and having no output format specified on the command line matters,\n> and an explicit output format given on the command line should not\n> be \"combined\" with the default \"--patch\" format.\n>\n> So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we\n> may want to replace it with OPTION_GIVEN bit, and\n>\n>  * make \"--patch\", \"--raw\", etc. set DIFF_FORMAT_$format bit and\n>    DIFF_FORMAT_OPTION_GIVEN bit on for each format.  \"--no-raw\",\n>    etc. will set off DIFF_FORMAT_$format bit but still record the\n>    fact that we saw an option from the command line by setting\n>    DIFF_FORMAT_OPTION_GIVEN bit.\n>\n>  * make \"-s\" (and its synonym \"--no-patch\") clear all other bits\n>    and set only the DIFF_FORMAT_OPTION_GIVEN bit on.\n>\n> which I suspect would make the code much cleaner without breaking\n> any end-user expectations.\n>\n> Once that is in place, transitioning \"--no-patch\" to mean the\n> counterpart of \"--patch\", just like \"--no-raw\" only defeats an\n> earlier \"--raw\", would be quite simple at the code level.  The\n> social cost of migrating the end-user expectations might be too\n> great for it to be worth, but at least the \"GIVEN\" bit clean-up\n\ns/worth/worthwhile/\n\n> alone may be worth it.\n\nAnd this final part addresses the big question which v1 left dangling\n(specifically, \"why the proposed patch doesn't eliminate NO_OUTPUT\naltogether). Good.\n\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"476642","messageId":"xmqqwn1md2m5.fsf@gitster.g","threadId":"59688","inReplyTo":"CAPig+cT=3dmtEEApiPUvB9+5ZHx+uwc1NXhYsf4peYiSwPYPsQ@mail.gmail.com","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T19:01:22Z","receivedAt":"2023-05-05T19:01:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, May 5, 2023 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n> Glad to see \"THis\" from v1 fixed.\n> ...\n> And this final part addresses the big question which v1 left dangling\n> (specifically, \"why the proposed patch doesn't eliminate NO_OUTPUT\n> altogether). Good.\n\nThanks.\n"},{"id":"476649","messageId":"20230505211917.2746751-1-gitster@pobox.com","threadId":"59688","inReplyTo":"20230505165952.335256-1-gitster@pobox.com","subject":"[PATCH 0/2] dirstat: leakfix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T21:19:15Z","receivedAt":"2023-05-05T21:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>  t/t4000-diff-format.sh         | 34 +++++++++++++++++++++++++++++++++-\n> ...\n> +for format in stat raw numstat shortstat summary \\\n> +\tdirstat cumulative dirstat-by-file \\\n> +\tpatch-with-raw patch-with-stat compact-summary\n\nUnfortunately, because t4000 is marked as passing with leak\nsanitizer on, even though this series does not introduce any new\nleaks (in fact, there is nothing in the series that allocates pieces\nof memory at all), the CI will fail with the sanitizer job.\n\nNeedless to say, I hate the current arrangement of these tests.\nThose who happen to use tools or features that have nothing to do\nwith the topic being developed that introduces no new leaks are\npunished by a test failure.\n\nHere are a pair of patches that plug leaks in dirstat code.  This\nallows the \"fix interaction between -s and others\" patch that adds\na test that exercises --dirstat in t4000 to be queued without\nbreaking the leak sanitizer.\n\nAlso t4047 that is about dirstat can now be marked as leak free.\n\nJunio C Hamano (2):\n  diff: refactor common tail part of dirstat computation\n  diff: plug leaks in dirstat\n\n diff.c                  | 34 ++++++++++++++++++++--------------\n t/t4047-diff-dirstat.sh |  2 ++\n 2 files changed, 22 insertions(+), 14 deletions(-)\n\n-- \n2.40.1-476-g69c786637d\n\n"},{"id":"476650","messageId":"20230505211917.2746751-3-gitster@pobox.com","threadId":"59688","inReplyTo":"20230505211917.2746751-1-gitster@pobox.com","subject":"[PATCH 2/2] diff: plug leaks in dirstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T21:19:17Z","receivedAt":"2023-05-05T21:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The array of dirstat_file contained in the dirstat_dir structure is\nnot freed after the processing ends.  Unfortunately, the member that\npoints at the array, .files, is incremented as the gather_dirstat()\nfunction recursively walks it, and this needs to be plugged by\nremembering the beginning of the array before gather_dirstat() mucks\nwith it and freeing it after we are done.\n\nWe can mark t4047 as leak-free.  t4000, which is marked as\nleak-free, now can exercise dirstat in it, which will happen next.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c                  | 17 +++++++++++------\n t/t4047-diff-dirstat.sh |  2 ++\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e13d0f8b67..d52db685f7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2975,13 +2975,18 @@ static void conclude_dirstat(struct diff_options *options,\n \t\t\t     struct dirstat_dir *dir,\n \t\t\t     unsigned long changed)\n {\n-\t/* This can happen even with many files, if everything was renames */\n-\tif (!changed)\n-\t\treturn;\n+\tstruct dirstat_file *to_free = dir->files;\n+\n+\tif (!changed) {\n+\t\t/* This can happen even with many files, if everything was renames */\n+\t\t;\n+\t} else {\n+\t\t/* Show all directories with more than x% of the changes */\n+\t\tQSORT(dir->files, dir->nr, dirstat_compare);\n+\t\tgather_dirstat(options, dir, changed, \"\", 0);\n+\t}\n \n-\t/* Show all directories with more than x% of the changes */\n-\tQSORT(dir->files, dir->nr, dirstat_compare);\n-\tgather_dirstat(options, dir, changed, \"\", 0);\n+\tfree(to_free);\n }\n \n static void show_dirstat(struct diff_options *options)\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex 7fec2cb9cd..70224c3da1 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='diff --dirstat tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # set up two commits where the second commit has these files\n-- \n2.40.1-476-g69c786637d\n\n"},{"id":"476651","messageId":"20230505211917.2746751-2-gitster@pobox.com","threadId":"59688","inReplyTo":"20230505211917.2746751-1-gitster@pobox.com","subject":"[PATCH 1/2] diff: refactor common tail part of dirstat computation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-05T21:19:16Z","receivedAt":"2023-05-05T21:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This will become useful when we plug leaks in these two functions.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a5..e13d0f8b67 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2971,6 +2971,19 @@ static int dirstat_compare(const void *_a, const void *_b)\n \treturn strcmp(a->name, b->name);\n }\n \n+static void conclude_dirstat(struct diff_options *options,\n+\t\t\t     struct dirstat_dir *dir,\n+\t\t\t     unsigned long changed)\n+{\n+\t/* This can happen even with many files, if everything was renames */\n+\tif (!changed)\n+\t\treturn;\n+\n+\t/* Show all directories with more than x% of the changes */\n+\tQSORT(dir->files, dir->nr, dirstat_compare);\n+\tgather_dirstat(options, dir, changed, \"\", 0);\n+}\n+\n static void show_dirstat(struct diff_options *options)\n {\n \tint i;\n@@ -3060,13 +3073,7 @@ static void show_dirstat(struct diff_options *options)\n \t\tdir.nr++;\n \t}\n \n-\t/* This can happen even with many files, if everything was renames */\n-\tif (!changed)\n-\t\treturn;\n-\n-\t/* Show all directories with more than x% of the changes */\n-\tQSORT(dir.files, dir.nr, dirstat_compare);\n-\tgather_dirstat(options, &dir, changed, \"\", 0);\n+\tconclude_dirstat(options, &dir, changed);\n }\n \n static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options)\n@@ -3104,13 +3111,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o\n \t\tdir.nr++;\n \t}\n \n-\t/* This can happen even with many files, if everything was renames */\n-\tif (!changed)\n-\t\treturn;\n-\n-\t/* Show all directories with more than x% of the changes */\n-\tQSORT(dir.files, dir.nr, dirstat_compare);\n-\tgather_dirstat(options, &dir, changed, \"\", 0);\n+\tconclude_dirstat(options, &dir, changed);\n }\n \n static void free_diffstat_file(struct diffstat_file *f)\n-- \n2.40.1-476-g69c786637d\n\n"},{"id":"476851","messageId":"645995f53dd75_7c6829483@chronos.notmuch","threadId":"59688","inReplyTo":"20230505165952.335256-1-gitster@pobox.com","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T00:38:13Z","receivedAt":"2023-05-09T00:38:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Sergey Organov noticed and reported \"--patch --no-patch --raw\"\n> behaves differently from just \"--raw\".  It turns out that there are\n> a few interesting bugs in the implementation and documentation.\n> \n>  * First, the documentation for \"--no-patch\" was unclear that it\n>    could be read to mean \"--no-patch\" countermands an earlier\n>    \"--patch\" but not other things.  The intention of \"--no-patch\"\n>    ever since it was introduced at d09cd15d (diff: allow --no-patch\n>    as synonym for -s, 2013-07-16) was to serve as a synonym for\n>    \"-s\", so \"--raw --patch --no-patch\" should have produced no\n>    output, but it can be (mis)read to allow showing only \"--raw\"\n>    output.\n\nI would say that is orthogonal.\n\n>  * Then the interaction between \"-s\" and other format options were\n>    poorly implemented.  Modern versions of Git uses one bit each to\n>    represent formatting options like \"--patch\", \"--stat\" in a single\n>    output_format word, but for historical reasons, \"-s\" also is\n>    represented as another bit in the same word.  This allows two\n>    interesting bugs to happen, and we have both X-<.\n> \n>    (1) After setting a format bit, then setting NO_OUTPUT with \"-s\",\n>        the code to process another \"--<format>\" option drops the\n>        NO_OUTPUT bit to allow output to be shown again.  However,\n>        the code to handle \"-s\" only set NO_OUTPUT without unsetting\n\ns/only set/only sets/\n\n>        format bits set earlier, so the earlier format bit got\n>        revealed upon seeing the second \"--<format>\" option.  This is\n>        the problem Sergey observed.\n> \n>    (2) After setting NO_OUTPUT with \"-s\", code to process\n\ns/code/the code/\n\n>        \"--<format>\" option can forget to unset NO_OUTPUT, leaving\n>        the command still silent.\n\n> It is tempting to change the meaning of \"--no-patch\" to mean\n> \"disable only the patch format output\" and reimplement \"-s\" as \"not\n> showing anything\", but it would be an end-user visible change in\n> behavior.\n\nYes, it would be a change in behavior from what no reasonable user would\nexpect, to what most reasonable users would expct.\n\nThese are synonyms:\n\n 1.a. git diff --patch-with-raw\n 1.b. git diff --patch --raw\n\nAnd so should these:\n\n 2.a. git diff --raw\n 2.b. git diff --no-patch --raw\n\nBut who on Earth would then think these are different?\n\n 2.b. git diff --no-patch --raw\n 2.c. git diff --raw --no-patch\n\nYour patch is *already* an end-user visible change in behavior, so why\nnot do the end-user visible change in behavior that reasonable users\nwould expect?\n\n> Let's fix the interactions of these bits to first make \"-s\" work as\n> intended.\n\nIs it though?\n\n> The fix is conceptually very simple.\n> \n>  * Whenever we set DIFF_FORMAT_FOO because we saw the \"--foo\"\n>    option (e.g. DIFF_FORMAT_RAW is set when the \"--raw\" option is\n>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to\n>    do so in some of the options and caused (2) above.\n> \n>  * When processing \"-s\" option, we should not just set\n>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.\n>    We didn't do so and retained format bits set by options\n>    previously seen, causing (1) above.\n> \n> It is even more tempting to lose NO_OUTPUT bit and instead take\n> output_format word being 0 as its replacement, but that would break\n> the mechanism \"git show\" uses to default to \"--patch\" output, where\n> the distinction between telling the command to be silent with \"-s\"\n> and having no output format specified on the command line matters,\n> and an explicit output format given on the command line should not\n> be \"combined\" with the default \"--patch\" format.\n\nThat's because the logic is not correct, the default should not be 0,\nthe default should be a different value, for example\nDIFF_FORMAT_DEFAULT, that way each tool can update DIFF_FORMAT_DEFAULT\nto whatever default is desired.\n\nThen 0 doesn't mean default, it means NO_OUTPUT, and then removing all\nthe formats--including DIFF_FORMAT_DEFAULT--makes it clear what the user\nintends to do.\n\n> So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we\n> may want to replace it with OPTION_GIVEN bit, and\n> \n>  * make \"--patch\", \"--raw\", etc. set DIFF_FORMAT_$format bit and\n>    DIFF_FORMAT_OPTION_GIVEN bit on for each format.  \"--no-raw\",\n>    etc. will set off DIFF_FORMAT_$format bit but still record the\n>    fact that we saw an option from the command line by setting\n>    DIFF_FORMAT_OPTION_GIVEN bit.\n> \n>  * make \"-s\" (and its synonym \"--no-patch\") clear all other bits\n>    and set only the DIFF_FORMAT_OPTION_GIVEN bit on.\n> \n> which I suspect would make the code much cleaner without breaking\n> any end-user expectations.\n\nWhy DIFF_FORMAT_OPTION_GIVEN? DIFF_FORMAT_DEFAULT as the opposite is\nmuch more understandable.\n\n> Once that is in place, transitioning \"--no-patch\" to mean the\n> counterpart of \"--patch\", just like \"--no-raw\" only defeats an\n> earlier \"--raw\", would be quite simple at the code level.\n\nIt's not only simple, it's a no-op, as (~DIFF_FORMAT_PATCH |\nDIFF_FORMAT_OPTION_GIVEN) becomes indistinguishible from\nDIFF_FORMAT_NO_OUTPUT unless another optin like DIFF_FORMAT_RAW is\nspecified.\n\nI'm sending a patch series that shows that to be the case.\n\n-- \nFelipe Contreras\n"},{"id":"476857","messageId":"64599bdee22b4_7c6829422@chronos.notmuch","threadId":"59688","inReplyTo":"xmqqttwskse5.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T01:03:26Z","receivedAt":"2023-05-09T01:03:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> > Sergey Organov <sorganov@gmail.com> writes:\n> >\n> >> No problem from my side, but are you sure?\n> >\n> > Absolutely.\n> >\n> > I've seen people just say \"we document a failed one\" and leave it at\n> > that, without attempting to fix.  I am trying to see if pushing back\n> > at first would serve as a good way to encourage these known failure\n> > to be fixed, without accumulating too many expect_failure in our\n> > test suite, which will waste cycles at CI runs (which do not need to\n> > be reminded something is known to be broken).  I will try not to do\n> > this when I do not positively know the author of such a patch is\n> > capable enough to provide a fix, though, and you are unlucky enough\n> > to have shown your abilities in the past ;-)\n> \n> I ended up spending some time digging history and remembered that\n> \"--no-patch\" was added as a synonym to \"-s\" by d09cd15d (diff: allow\n> --no-patch as synonym for -s, 2013-07-16).  These\n> \n>     git diff -p --stat --no-patch HEAD^ HEAD\n>     git diff -p --raw --no-patch HEAD^ HEAD\n> \n> would show no output from the diff machinery, patches, diffstats,\n> raw object names, etc.\n> \n> And this turns out to be a prime example why the approach to ask\n> contributors do more, would help the project overall.\n\nIt would also help the project to reward the contributors who actually\ndo more.\n\nOtherwise why would a contributor feel incentivized to do more, if that\nwork is simply going to land flat on the ground?\n\n> It hopefully would have been \"ah, the intent is not documented\n> correctly, and here is a documentation patch to fix it.\"\n\nThat would be assuming that the intent of a developer is all that\nmatters.\n\nI disagree.\n\nWhat a reasonable user would expect also matters.\n\n> When a command does not behave the way one thinks it should, being\n> curious is good.  Reporting it as a potential bug is also good.  But\n> it would help the project more if it was triaged before reporting it\n> as a potential bug, if the reporter is capable of doing so.\n\nThis entirely depends on one's definition of \"bug\".\n\nTo me a bug is unexpected behavior. Some people think documenting\nunexpected behavior makes it not a bug, but to me it's just a documented\nbug.\n\n\"It's not a bug, it's a feature!\"\n\n> Those who encounter behaviour unexpected to them are more numerous\n> than those who can report it as a potential bug (many people are not\n> equipped to write a good bug report),\n\nIs it just unexpected to them? Or is it unexpected to most users?\n\nSo what would a reasonable user expect `--no-patch` to do? I think a\nreasonable user would expect it to negate the effect of `--patch`, and\nnothing more.\n\nThe fact that a minority of users expect `--no-patch` to disable all\noutput--not just the one of `--patch`--would not make it not a bug in my\nbook.\n\n> Those who can come up with a solution is even more scarse.\n\nAnd those who can come up with a solution that the maintainer deems\nworthy of merging are way, way scarcer.\n\n-- \nFelipe Contreras\n"},{"id":"476858","messageId":"64599cc234708_7c6829426@chronos.notmuch","threadId":"59688","inReplyTo":"xmqq1qjwj7go.fsf@gitster.g","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T01:07:14Z","receivedAt":"2023-05-09T01:07:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> > Sergey Organov <sorganov@gmail.com> writes:\n> >\n> >> --patch followed by --no-patch is to be a no-op according to the \"git\n> >> log\" manual page.\n> >\n> > I briefly wondered if it is a bug in the documentation.\n> > ... when \"git log -p --raw\" shows both patch and raw, I do not\n> > think of a reason why \"git log -p --raw --no-patch\" should not\n> > behave similarly.\n> \n> So, to tie the loose ends, \"log -p --raw --no-patch\" and \"log -p\n> --stat --no-patch\" do behave similarly.  Where my reaction was\n> mistaken was that I did not read the manual page myself that clearly\n> said it is the same as \"-s\" that suppresses diff output (where \"diff\n> output\" is not limited to \"patch\"---diffstat is also output of \"diff\"),\n> and incorrectly thought that \"--no-patch\" would countermand only\n> \"--patch\" and nothing else.\n\nIf Sergey, you, and me all agreed on what `--no-patch` should do\n(without reading the manpage), isn't that an indication that that is the\nexpected behavior?\n\nThe fact that the documentation documents some unexpected behavior,\ndoesn't mean it isn't a bug.\n\nI would say it's a documented bug.\n\n-- \nFelipe Contreras\n"},{"id":"476859","messageId":"64599ee988190_7c68294b4@chronos.notmuch","threadId":"59688","inReplyTo":"xmqq4joribyv.fsf@gitster.g","subject":"Re: [PATCH] diff: fix behaviour of the \"-s\" option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T01:16:25Z","receivedAt":"2023-05-09T01:16:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >  * Then the interaction between \"-s\" and other format options were\n> >    poorly implemented.  Modern versions of Git uses one bit each to\n> >    represent formatting options like \"--patch\", \"--stat\" in a single\n> >    output_format word, but for historical reasons, \"-s\" also is\n> >    represented as another bit in the same word.\n> \n> An obvious improvement strategy is to stop using the NO_OUTPUT bit\n> and instead make \"-s\" to clear the \"output_format\" word, and make\n> \"--[no-]raw\", \"--[no-]stat\", \"--[no-]patch\", etc. to flip their own\n> bit in the same \"output_format\" word.  I think the \"historical\n> reasons\" why we did not do that was because we wanted to be able to\n> do a flexible defaulting.  We may want to say \"if no output-format\n> option is given from the command line, default to \"--patch\", but\n> otherwise do not set the \"--patch\" bit on\", for example.\n> Initializing the \"output_format\" word with \"--patch\" bit on would\n> not work---when \"--raw\" is given from the command line, we want to\n> clear that \"--patch\" bit we set for default and set \"--raw\" bit on.\n> We can initialize the \"output_format\" word to 0, and OR in the bits\n> for each format option as we process them, and then flip the\n> \"--patch\" bit on if \"output_format\" word is still 0 after command\n> line parsing is done.  This would almost work, except that it would\n> make it hard to tell \"no command line options\" case and \"'-s' cleared\n> all bits\" case apart (the former wants to default to \"--patch\",\n> while the latter wants to stay \"no output\"), and it probably was the\n> reason why we gave an extra NO_OUTPUT bit to the \"-s\" option.  In\n> hindsight, the arrangement certainly made other things harder and\n> prone to unnecessary bugs.\n\nThat's easy to solve by introducing a DIFF_FORMAT_DEFAULT item, which\nwould be different from 0.\n\nThen every command can update DIFF_FORMAT_DEFAULT to the desired\ndefault, and if the default is cleared (e.g. `--no-patch`) that would\nnot happen.\n\n-- \nFelipe Contreras\n"},{"id":"476860","messageId":"xmqqsfc62t8y.fsf@gitster.g","threadId":"59688","inReplyTo":"645995f53dd75_7c6829483@chronos.notmuch","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-09T01:22:53Z","receivedAt":"2023-05-09T01:23:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Let's fix the interactions of these bits to first make \"-s\" work as\n>> intended.\n>\n> Is it though?\n\nYes.\n\nIf the proposed log message says \"as intended\", the author thinks it\nis.  Throwing a rhetorical question and stopping at that is not\nuseful; you'd need to explain yourself if you think differently.\nUnless the only effect you want is to be argumentative and annoy\nothers, that is.\n\nI've dug the history and as I explained elsewhere in the earlier\ndiscussion, I know that the \"--no-patch\" originally was added as a\nsynonym for \"-s\" that makes the output from the diff machinery\nsilent---I have a good reason to believe that it is making \"-s\" and\n\"--no-patch\" both work as intended.\n\nI would not say that we should *not* move further with a follow up\ntopic, but I think we should consider doing so only after the dust\nsettles from this round.\n\n"},{"id":"476862","messageId":"6459a33b14bd6_7c682947d@chronos.notmuch","threadId":"59688","inReplyTo":"87o7n03qgq.fsf@osv.gnss.ru","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T01:34:51Z","receivedAt":"2023-05-09T01:34:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n\n> > When a command does not behave the way one thinks it should, being\n> > curious is good.  Reporting it as a potential bug is also good.  But\n> > it would help the project more if it was triaged before reporting it\n> > as a potential bug, if the reporter is capable of doing so.  Those\n> > who encounter behaviour unexpected to them are more numerous than\n> > those who can report it as a potential bug (many people are not\n> > equipped to write a good bug report), and those who can triage and\n> > diagnose a bug report are fewer.  Those who can come up with a\n> > solution is even more scarse.\n> \n> I'm afraid the solution I'd come up with won't be welcomed.\n\nMy solutions are often not welcomed, and yet I still implement them.\n\nIt might be a waste of time, but often I've found out that very quickly\nafter attempting to come up with a solution I realize there's a lot of\ndetail I was missing initially, so even if the solution is not welcomed,\nit helps me to understand the problem space and be more helpful in the\ndiscussion of potential solutions.\n\nSo if I were you, I would still attempt to do it, just to gather some\nunderstanding.\n\nVery often I myself realize the solution I initially thought was the\ncorrect one turns out the be completely undoable, and often I need to\nattempt more than one.\n\nIf I'm content with a solution, I send it to the mailing list,\nregardless of the probability of it being merged, because in my view an\nunmerged patch still provides value, as it creates a record that might\nbe referenced in the future.\n\nIn fact, quite recently somebody resent a patch of mine that fixes an\nobvious regression [1]. So even if the maintainer has not merged my\npatch--and thus it could be said my patch was not \"welcomed\"--the fact\nis that it was not welcomed by the maintainer, but it was welcomed by\nthe community.\n\nI for one welcome any and all attempts to fix git's awful user\ninterface, regardless of the reception of the maintainer, and the \"core\nclub\".\n\nCheers.\n\n[1] https://lore.kernel.org/git/pull.1499.git.git.1682573243090.gitgitgadget@gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"476868","messageId":"6459c31038e81_7c68294ee@chronos.notmuch","threadId":"59688","inReplyTo":"xmqqsfc62t8y.fsf@gitster.g","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-09T03:50:40Z","receivedAt":"2023-05-09T03:50:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> >> Let's fix the interactions of these bits to first make \"-s\" work as\n> >> intended.\n> >\n> > Is it though?\n> \n> Yes.\n> \n> If the proposed log message says \"as intended\", the author thinks it\n> is.\n\nThe question is not if the author of the patch thinks this is the way\n`-s` is intended to work, the question is if this is the way `-s` is\nintended to work.\n\nThe way `-s` is intended to work is completely independent of what the\nauthor of the patch thinks, as `-s` existed well before this patch.\n\nA cursory search for `-s` in diff-tree.c shows:\n\n  Author: Linus Torvalds <torvalds@ppc970.osdl.org>\n  Date:   Fri May 6 10:56:35 2005 -0700\n\n      git-diff-tree: clean up output\n      \n      This only shows the tree headers when something actually changed. Also,\n      add a \"silent\" mode, which doesn't actually show the changes at all,\n      just the commit information.\n\nSo presumably the original author of `-s` intended for it to not show\nany changes at all, but that was before any of the non-patch options\nwere introduced.\n\nSo, 18 years later: what is the intention behind `-s`?\n\n> Throwing a rhetorical question and stopping at that is not\n> useful;\n\nWho says this is a rhetorical question?\n\n`-s` was introduced 18 years ago, before any of the non-patch options\nwere introduced.\n\nI do not think the intention behind `-s` in 2023 is clear at all, and\nthe patch does not attempt to answer that.\n\n> Unless the only effect you want is to be argumentative and annoy\n> others, that is.\n\nAssume good faith:\nhttps://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith\n\n> I've dug the history and as I explained elsewhere in the earlier\n> discussion, I know that the \"--no-patch\" originally was added as a\n> synonym for \"-s\" that makes the output from the diff machinery\n> silent---I have a good reason to believe that it is making \"-s\" and\n> \"--no-patch\" both work as intended.\n\nI don't think so.\n\n`-s` might have been added to make all the diff machinery silent, but\n`--no-patch` is a different question, as the commit message of d09cd15d\nmakes abundantly clear:\n\n  diff: allow --no-patch as synonym for -s\n  \n  This follows the usual convention of having a --no-foo option to negate\n  --foo.\n\nNow we know `-s` is not an antonym of `--patch`, so the commit message\nof d09cd15d cannot possibly be correct.\n\nThere's only three options now:\n\n 1. `-s` doesn't turn all the diff machinery silent, only --patch\n 2. `--no-patch` is decoupled from `--patch`\n 3. `--no-patch` is decoupled from `-s`\n\nI don't think think there's any other reasonable option, including the\nstatus quo.\n\n> I would not say that we should *not* move further with a follow up\n> topic, but I think we should consider doing so only after the dust\n> settles from this round.\n\nBut what is that dust?\n\nDo you agree with the following?\n\n 1. No reasonable user would consider the status quo to be expected.\n 2. Any change to the status quo would incur in backwards-incompatible\n    changes for end-users.\n\nIf so, the only question remaining is what backwards-incompatible\nchanges shall be implemented.\n\n-- \nFelipe Contreras\n"},{"id":"476945","messageId":"xmqqjzxgzua0.fsf@gitster.g","threadId":"59688","inReplyTo":"6459c31038e81_7c68294ee@chronos.notmuch","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T04:26:31Z","receivedAt":"2023-05-10T04:29:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> > Is it though?\n>> \n>> Yes.\n>> \n>> If the proposed log message says \"as intended\", the author thinks it\n>> is.\n>\n> The question is not if the author of the patch thinks this is the way\n> `-s` is intended to work, the question is if this is the way `-s` is\n> intended to work.\n\nThe \"author\" refers to the author of the \"proposed log message\" of\nthe patch in question, i.e. me in this case.  The author of the\npatch under discussion thinks it is, so asking \"Is it?\", implying\nyou do not agree, is nothing but a rhetorical question, and doing\nso, without explaining why, wastes time on both sides.\n\nI am not interested in getting involved in unproductive arguments\nwith you (or with anybody else for that matter).  I've been giving\nyou benefit of doubt, but I'll go back to refrain from responding to\nyour message, unless it is a patch that I can say \"I agree 100% with\nwhat the proposed log message says and what the patch text does,\nlooking great, thanks. Will queue.\" to, which has been my default\nstance.\n\nPast experience tells me that to any review other than \"100% good\",\nI would see responses in an unpleasant and hostile manner.  Anything\nthat asks clarification for something unclear in your patch, or\nsuggests alternatives or improvements.  And it led to unproductive\nand irritating waste of time number of times, and eventually you\nwere asked to leave the development community for at least a few\ntimes.\n\n"},{"id":"476973","messageId":"87wn1ggv8s.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"64599cc234708_7c6829426@chronos.notmuch","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-10T13:40:35Z","receivedAt":"2023-05-10T13:40:42Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> > Sergey Organov <sorganov@gmail.com> writes:\n>> >\n>> >> --patch followed by --no-patch is to be a no-op according to the \"git\n>> >> log\" manual page.\n>> >\n>> > I briefly wondered if it is a bug in the documentation.\n>> > ... when \"git log -p --raw\" shows both patch and raw, I do not\n>> > think of a reason why \"git log -p --raw --no-patch\" should not\n>> > behave similarly.\n>> \n>> So, to tie the loose ends, \"log -p --raw --no-patch\" and \"log -p\n>> --stat --no-patch\" do behave similarly.  Where my reaction was\n>> mistaken was that I did not read the manual page myself that clearly\n>> said it is the same as \"-s\" that suppresses diff output (where \"diff\n>> output\" is not limited to \"patch\"---diffstat is also output of \"diff\"),\n>> and incorrectly thought that \"--no-patch\" would countermand only\n>> \"--patch\" and nothing else.\n>\n> If Sergey, you, and me all agreed on what `--no-patch` should do\n> (without reading the manpage), isn't that an indication that that is the\n> expected behavior?\n>\n> The fact that the documentation documents some unexpected behavior,\n> doesn't mean it isn't a bug.\n>\n> I would say it's a documented bug.\n\nYep, it is. Chances are this will end-up in the \"won't fix\" category\nthough, similar to unfortunate '-m'. In which case I think it's better\nto explicitly mark it in the documentation as such: won't fix.\n\nThanks,\n-- Sergey Organov\n"},{"id":"476974","messageId":"87v8h0guks.fsf@osv.gnss.ru","threadId":"59688","inReplyTo":"6459a33b14bd6_7c682947d@chronos.notmuch","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2023-05-10T13:54:59Z","receivedAt":"2023-05-10T13:55:44Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Sergey Organov wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> > When a command does not behave the way one thinks it should, being\n>> > curious is good.  Reporting it as a potential bug is also good.  But\n>> > it would help the project more if it was triaged before reporting it\n>> > as a potential bug, if the reporter is capable of doing so.  Those\n>> > who encounter behaviour unexpected to them are more numerous than\n>> > those who can report it as a potential bug (many people are not\n>> > equipped to write a good bug report), and those who can triage and\n>> > diagnose a bug report are fewer.  Those who can come up with a\n>> > solution is even more scarse.\n>>\n>> I'm afraid the solution I'd come up with won't be welcomed.\n>\n> My solutions are often not welcomed, and yet I still implement them.\n>\n> It might be a waste of time, but often I've found out that very quickly\n> after attempting to come up with a solution I realize there's a lot of\n> detail I was missing initially, so even if the solution is not welcomed,\n> it helps me to understand the problem space and be more helpful in the\n> discussion of potential solutions.\n>\n> So if I were you, I would still attempt to do it, just to gather some\n> understanding.\n\nI sympathize, and I did recently. However, I figure I'd rather spend my\ntime elsewhere, say, in the Linux kernel, where my experience is\nsomewhat different, and allows me to enjoy my work.\n\n[...]\n\n>\n> I for one welcome any and all attempts to fix git's awful user\n> interface, regardless of the reception of the maintainer, and the \"core\n> club\".\n\nFor UI, the problem is that there is no core model defined, nor any\nguidelines are given, so every discussion ends-up being what \"makes\nsense\" and what doesn't for a user, everyone involved having his own\npreference, that often even changes over time.\n\nIn this situation attempting to fix the UI sounds like waste of efforts,\nas nobody can actually point at the state of the UI to which we are\nwilling to converge, so there are no objective criteria for accepting of\nfixup patches.\n\nThanks,\n-- Sergey Organov\n"},{"id":"476997","messageId":"645c0f0b9ba51_7b63e2943@chronos.notmuch","threadId":"59688","inReplyTo":"87wn1ggv8s.fsf@osv.gnss.ru","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T21:39:23Z","receivedAt":"2023-05-10T21:39:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > Junio C Hamano wrote:\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >> > Sergey Organov <sorganov@gmail.com> writes:\n> >> >\n> >> >> --patch followed by --no-patch is to be a no-op according to the \"git\n> >> >> log\" manual page.\n> >> >\n> >> > I briefly wondered if it is a bug in the documentation.\n> >> > ... when \"git log -p --raw\" shows both patch and raw, I do not\n> >> > think of a reason why \"git log -p --raw --no-patch\" should not\n> >> > behave similarly.\n> >> \n> >> So, to tie the loose ends, \"log -p --raw --no-patch\" and \"log -p\n> >> --stat --no-patch\" do behave similarly.  Where my reaction was\n> >> mistaken was that I did not read the manual page myself that clearly\n> >> said it is the same as \"-s\" that suppresses diff output (where \"diff\n> >> output\" is not limited to \"patch\"---diffstat is also output of \"diff\"),\n> >> and incorrectly thought that \"--no-patch\" would countermand only\n> >> \"--patch\" and nothing else.\n> >\n> > If Sergey, you, and me all agreed on what `--no-patch` should do\n> > (without reading the manpage), isn't that an indication that that is the\n> > expected behavior?\n> >\n> > The fact that the documentation documents some unexpected behavior,\n> > doesn't mean it isn't a bug.\n> >\n> > I would say it's a documented bug.\n> \n> Yep, it is. Chances are this will end-up in the \"won't fix\" category\n> though, similar to unfortunate '-m'.\n\nProbably.\n\n> In which case I think it's better to explicitly mark it in the documentation\n> as such: won't fix.\n\nAgreed.\n\n-- \nFelipe Contreras\n"},{"id":"476998","messageId":"645c128cd8e0_7b63e2941a@chronos.notmuch","threadId":"59688","inReplyTo":"87v8h0guks.fsf@osv.gnss.ru","subject":"Re: [PATCH] t4013: add expected failure for \"log --patch --no-patch\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T21:54:20Z","receivedAt":"2023-05-10T21:54:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> > Sergey Organov wrote:\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >> > When a command does not behave the way one thinks it should, being\n> >> > curious is good.  Reporting it as a potential bug is also good.  But\n> >> > it would help the project more if it was triaged before reporting it\n> >> > as a potential bug, if the reporter is capable of doing so.  Those\n> >> > who encounter behaviour unexpected to them are more numerous than\n> >> > those who can report it as a potential bug (many people are not\n> >> > equipped to write a good bug report), and those who can triage and\n> >> > diagnose a bug report are fewer.  Those who can come up with a\n> >> > solution is even more scarse.\n> >>\n> >> I'm afraid the solution I'd come up with won't be welcomed.\n> >\n> > My solutions are often not welcomed, and yet I still implement them.\n> >\n> > It might be a waste of time, but often I've found out that very quickly\n> > after attempting to come up with a solution I realize there's a lot of\n> > detail I was missing initially, so even if the solution is not welcomed,\n> > it helps me to understand the problem space and be more helpful in the\n> > discussion of potential solutions.\n> >\n> > So if I were you, I would still attempt to do it, just to gather some\n> > understanding.\n> \n> I sympathize, and I did recently. However, I figure I'd rather spend my\n> time elsewhere, say, in the Linux kernel, where my experience is\n> somewhat different, and allows me to enjoy my work.\n\nCompletely agree.\n\nMy experience in the Linux project is that of a true meritocracy: Linus\nTorvalds doesn't have to like me, if the patch is good, it gets merged. Period.\n\n> > I for one welcome any and all attempts to fix git's awful user\n> > interface, regardless of the reception of the maintainer, and the \"core\n> > club\".\n> \n> For UI, the problem is that there is no core model defined, nor any\n> guidelines are given, so every discussion ends-up being what \"makes\n> sense\" and what doesn't for a user, everyone involved having his own\n> preference, that often even changes over time.\n> \n> In this situation attempting to fix the UI sounds like waste of efforts,\n> as nobody can actually point at the state of the UI to which we are\n> willing to converge, so there are no objective criteria for accepting of\n> fixup patches.\n\nIt's even worse than that. There used to be objective criteria like the old Git\nUser's Surveys [1], but it turned out Git developers did not care about the\nfeedback from users, which is why there wasn't any point in continuing them.\n\nAnd worse: even when all Git developers agree on a UI change, except one, it\ndoesn't matter, because that one has absolute veto power.\n\nNot very hopeful prospects for Git's UI.\n\nCheers.\n\n[1] https://archive.kernel.org/oldwiki/git.wiki.kernel.org/index.php/GitSurvey2016.html\n\n-- \nFelipe Contreras\n"},{"id":"477007","messageId":"645c25dcb590b_7b63e294ea@chronos.notmuch","threadId":"59688","inReplyTo":"xmqqjzxgzua0.fsf@gitster.g","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T23:16:44Z","receivedAt":"2023-05-10T23:16:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> >> > Is it though?\n> >> \n> >> Yes.\n> >> \n> >> If the proposed log message says \"as intended\", the author thinks it\n> >> is.\n> >\n> > The question is not if the author of the patch thinks this is the way\n> > `-s` is intended to work, the question is if this is the way `-s` is\n> > intended to work.\n> \n> The \"author\" refers to the author of the \"proposed log message\" of\n> the patch in question, i.e. me in this case.  The author of the\n> patch under discussion thinks it is, so asking \"Is it?\",\n\nThis is the full quote:\n\n====\nLet's fix the interactions of these bits to first make \"-s\" work as intended.\n====\n\nIf instead you meant this:\n\n====\nLet's fix the interactions of these bits to first make \"-s\" work as I intend.\n====\n\nThen that's not a rationale, you are essentially saying \"let's do X because I\nwant\".\n\n> I am not interested in getting involved in unproductive arguments with you\n> (or with anybody else for that matter).\n\nThis is the way the review process works and all git developers have to go\ntrough it.\n\nWe all have to convince others our proposed change is desirable.\n\nYour patch is implementing a backwards-incompatible change:\n\n  git diff -s --raw master\n\nThat command used not produce any output and after your patch it now produces\noutput.\n\nYour commit message does not provide a rationale as to why *we* want to\nimplement this backwards-incompatible change.\n\n\"This is the way *I* intend `-s` to work\" is not a rationale.\n\n> And it led to unproductive and irritating waste of time number of times, and\n> eventually you were asked to leave the development community for at least a\n> few times.\n\nThat is blatantly false. As a member of Git's Project Leadership Committee, you\nshould know precisely how many times the committee has excercised this power,\nand it hasn't been \"a few times\", it has been one time.\n\nAnd this is a smoke screen: your commit message still doesn't provide any\nrationale as to why `-s` should work the way *you* intend.\n\nThrowing personal attacks at a reviewer for merely pointing out an issue in the\ncommit message is far from productive.\n\n-- \nFelipe Contreras\n"},{"id":"477013","messageId":"645c2bc57fdd0_7c6152945e@chronos.notmuch","threadId":"59688","inReplyTo":"645c25dcb590b_7b63e294ea@chronos.notmuch","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T23:41:57Z","receivedAt":"2023-05-10T23:42:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras wrote:\n> Junio C Hamano wrote:\n\n> > And it led to unproductive and irritating waste of time number of times, and\n> > eventually you were asked to leave the development community for at least a\n> > few times.\n> \n> That is blatantly false. As a member of Git's Project Leadership Committee, you\n> should know precisely how many times the committee has excercised this power,\n> and it hasn't been \"a few times\", it has been one time.\n\nAnd for the record: that one time I was asked by the committee to not interact\nwith certain members of the community for a few months.\n\nThe amount of times I was asked to \"leave the development community\" is *zero*.\n\n-- \nFelipe Contreras\n"},{"id":"477014","messageId":"20230511012558.GA1464167@coredump.intra.peff.net","threadId":"59688","inReplyTo":"645c2bc57fdd0_7c6152945e@chronos.notmuch","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-11T01:25:58Z","receivedAt":"2023-05-11T01:26:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 10, 2023 at 05:41:57PM -0600, Felipe Contreras wrote:\n\n> Felipe Contreras wrote:\n> > Junio C Hamano wrote:\n> \n> > > And it led to unproductive and irritating waste of time number of times, and\n> > > eventually you were asked to leave the development community for at least a\n> > > few times.\n> > \n> > That is blatantly false. As a member of Git's Project Leadership Committee, you\n> > should know precisely how many times the committee has excercised this power,\n> > and it hasn't been \"a few times\", it has been one time.\n> \n> And for the record: that one time I was asked by the committee to not interact\n> with certain members of the community for a few months.\n> \n> The amount of times I was asked to \"leave the development community\" is *zero*.\n\nYou're right, in the sense that the first time you were asked to leave\nwe did not have a CoC, and nor was the PLC expected to be part of such\nconversations at that time. Likewise, many times during which your\nbehavior has been a problem on the list, people did not ask you to\nleave, but simply said \"I am not going to read your messages anymore\".\n\nFor example, here's Junio asking you to leave in 2013:\n\n  https://lore.kernel.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/\n\nHere's him explaining a few months later why your patches aren't getting\nreviewed:\n\n  https://lore.kernel.org/git/xmqqtxgjg35a.fsf@gitster.dls.corp.google.com/\n\nHere's me addressing complaints about your behavior half a year after\nthat:\n\n https://lore.kernel.org/git/20140514202646.GE2715@sigill.intra.peff.net/\n\n    That last one has some gmane links; if anyone truly wants to follow\n    them (and I don't recommend that as being worth your time), the lore\n    equivalents are:\n\n      https://lore.kernel.org/git/20140425191236.GA31637@sigill.intra.peff.net/\n\n      https://lore.kernel.org/git/480ACEB0-7629-44DF-805F-E9543E66241B@quendi.de/\n\n      https://lore.kernel.org/git/7vfvl0htys.fsf@alter.siamese.dyndns.org/\n\n      https://lore.kernel.org/git/20140502223612.GA11374@sigill.intra.peff.net/\n\nI'm sure you will find reason to argue with all of that. But I think the\nspirit of \"it led to an unproductive and irritating waste of time a\nnumber of times\" is accurate. And this thread is one more example.\n\nYou can feel free to respond if you want to; I'm not planning to\nparticipate further in this thread (and in case you were not aware, I'm\nnot on the PLC any more). I just didn't want Junio to think he was alone\nin his view of the situation.\n\n-Peff\n"},{"id":"477015","messageId":"xmqqpm77zlec.fsf@gitster.g","threadId":"59688","inReplyTo":"645c25dcb590b_7b63e294ea@chronos.notmuch","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T01:50:35Z","receivedAt":"2023-05-11T01:50:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> The \"author\" refers to the author of the \"proposed log message\" of\n>> the patch in question, i.e. me in this case.  The author of the\n>> patch under discussion thinks it is, so asking \"Is it?\",\n>\n> This is the full quote:\n>\n> ====\n> Let's fix the interactions of these bits to first make \"-s\" work as intended.\n> ====\n>\n> If instead you meant this:\n>\n> ====\n> Let's fix the interactions of these bits to first make \"-s\" work as I intend.\n> ====\n>\n> Then that's not a rationale, you are essentially saying \"let's do X because I\n> want\".\n\nThis will be the last message from me on this.  I wouldn't have even\nseen the message I am responding to, as I've already done my \"once\nevery few days sweep the spam folder to find things to salvage\", but\nsomebody notified me of it, so...\n\nI didn't say and I didn't mean \"as I intend\", and you know that.\n\nI, the author of the patch under discussion, know that it is the\nintention of the author of the earlier commit that introduced\n\"--no-patch\" to make it work identically as \"-s\".\n\nI even had a quote from that earlier commit in the proposed log\nmessage of the patch (look for d09cd15d) to substantiate the fact\nthat it was the intended way for the option \"--no-patch\" to work.\nSo, either you are arguing against the patch you didn't even read,\nor you are playing your usual word twisting game just for the sake\nof arguing.\n\n>> And it led to unproductive and irritating waste of time number of times, and\n>> eventually you were asked to leave the development community for at least a\n>> few times.\n>\n> That is blatantly false. As a member of Git's Project Leadership Committee, you\n> should know precisely how many times the committee has excercised this power,\n> and it hasn't been \"a few times\", it has been one time.\n\nYou were asked to leave in May 2014, and according to that message\nfrom May 2014 [*1*], apparently you were asked to leave after a big\n\"Felipe eruption\" in the summer of 2013 [*2*].  These happened long\nbefore the project adopted a formal CoC at 5cdf2301 (add a Code of\nConduct document, 2019-09-24).\n\nBut apparently the \"fact\" does not matter to you.  I know that your\nnext excuse will be \"I said the committee never exercised this power\nmore than once, which is a FACT\", which may let you keep arguing\nfurther.\n\n\n[References]\n\n*1* https://lore.kernel.org/git/53709788.2050201@alum.mit.edu/\n*2* https://public-inbox.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/\n\n"},{"id":"477243","messageId":"645eff0d77c74_21b4f8294d5@chronos.notmuch","threadId":"59688","inReplyTo":"20230511012558.GA1464167@coredump.intra.peff.net","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-13T03:07:57Z","receivedAt":"2023-05-13T03:09:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Wed, May 10, 2023 at 05:41:57PM -0600, Felipe Contreras wrote:\n> > Felipe Contreras wrote:\n> > > Junio C Hamano wrote:\n\n> > > > And it led to unproductive and irritating waste of time number of times, and\n> > > > eventually you were asked to leave the development community for at least a\n> > > > few times.\n> > > \n> > > That is blatantly false. As a member of Git's Project Leadership Committee, you\n> > > should know precisely how many times the committee has excercised this power,\n> > > and it hasn't been \"a few times\", it has been one time.\n> > \n> > And for the record: that one time I was asked by the committee to not interact\n> > with certain members of the community for a few months.\n> > \n> > The amount of times I was asked to \"leave the development community\" is *zero*.\n> \n> You're right, in the sense that the first time you were asked to leave\n> we did not have a CoC,\n\nThat is is once again: *false*.\n\nThe git community has *never* asked me to leave.\n\n> Likewise, many times during which your behavior has been a problem on the\n> list,\n\nFalse: it was not a problem on the list, it was a problem *for some people* on\nthe list.\n\n> people did not ask you to leave, but simply said \"I am not going to read your\n> messages anymore\".\n\nYes, and for every person who has said \"I am not going to read your messages\"\npublicly, I received a response saying \"thank you for saying what we are all\nthinking but cannot say aloud for fear of reprisals\" privately.\n\nYou do understand that people have different opinions? Some people hate Donald\nTrump, some people don't. And some people cannot express their honest opinion\nat $dayjob.\n\nHaving a different opinion is OK. And the foundation of a functioning civilized\ndemocracy is to tolerate the opinions of others.\n\n> For example, here's Junio asking you to leave in 2013:\n> \n>   https://lore.kernel.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/\n\nRead the thread.\n\nMy objective was to show that the code organization was wrong, and libgit.a was\nnot an actual library. If there ever was any hope of having an actual libgit\nlibrary, the code needed to be reorganized:\n\n====\n  The plan is simple; make libgit.a a proper library, starting by\n  clarifying what goes into libgit.a, and what doesn't. If there's any\n  hopes of ever having a public library, it's clear what code doesn't\n  belong in libgit.a; code that is meant for builtins, that code belongs\n  in builtins/lib.a, or similar.\n====\nFelipe Contreras: [1]\n\nThe whole libification project of Google proves I was right: git was not (and\nis not) ready to be a library. libgit.a is not nearly close to be an actual\nstandalone library. Pretty far from it.\n\nJust today Elijah Newren sent a 27-patch series [2] attempting to move in the\nright direction, but doesn't even begin to tip the scales to make libgit.so\npossible.\n\nMy proposal did receive positive feedback:\n\n====\n  Nice joke patch to illustrate your point ;)\n====\nRamkumar Ramachandra: [3]\n\n====\n  This is a good example: yes, I'm convinced that the code does need to\n  be reorganized.\n====\nRamkumar Ramachandra: [4]\n\nEven you yourself provided useful positive feedback based on my proposal:\n\n====\n  If we want to start caring, then we probably need to create a separate\n  \"kitchen sink\"-like library, with the rule that things in libgit.a\n  cannot depend on it. In other words, a support library for Git's\n  commands, for the parts that are not appropriate to expose as part of a\n  library API.\n====\nJeff King: [5]\n\nJunio also provided good feedback initially:\n\n====\n  Another thing to think about is looking at pieces of data and\n  functions defined in each *.o files and moving things around within\n  them.  For example, looking at the dependency chain I quoted earlier\n  for sequencer.o to build upload-pack, which is about responding to\n  \"git fetch\" on the sending side:\n\n  ...\n\n  It is already crazy. There is no reason for the pack sender to be\n  linking with the sequencer interpreter machinery. If the function\n  definition (and possibly other ones) are split into separate source\n  files (still in libgit.a), git-upload-pack binary does not have to\n  pull in the whole sequencer.c at all.\n====\nJunio C Hamano: [6]\n\nThings started to turn south when I expressed the following opinion:\n\n====\n  But init_copy_notes_for_rewrite() can *not* be used by anything other\n  than git builtins. Standalone binaries will never use such a function,\n  therefore it doesn't belong in libgit.a. Another example is\n  alias_lookup(). They belong in builtin/lib.a.\n====\nFelipe Contreras: [7]\n\nJunio asked me for an example of a function that would not belong to libgit.so,\nand I said `init_copy_notes_for_rewrite()` is an example of a function that\nnothing outside the `git` binary would need.\n\n====\n  But that is not a good justification for closing door to others that\n  come later who may want to have a standalone that would want to use\n  it.  Think about rewriting filter-branch.sh in C but not as a\n  built-in, for example.\n====\nJunio C Hamano: [8]\n\nI argued nobody would actually do that, and I was right, as eventually\nfilter-branch.sh was rewritten in C, but as a builtin, as I said it would.\n\nJunio then argued that there was no justification for my claim that certain\nfunctions would only be used by git builtins, and therefore they should not\nbelong in a libgit.so library:\n\n====\n  >> You still haven't justified why we have to _forbid_ any outside\n  >> callers from calling copy_notes_for_rewrite().\n  >\n  > Because only builtins _should_ use it.\n\n  And there is no justification behind that \"_should_\" claim; you are\n  not making any technical argument to explain it.\n====\nJunio C Hamano: [9]\n\nGoogle's libification project proves I was right: some functions should not\nbelong in libgit.a.\n\nIf Junio had listened to me back in 2013, the changes Google developers are\nworking on now to make libgit.a something that remotely resembles an actual\nlibrary would not be as monumental as they are in 2023.\n\nInstead of considering my argument, Junio chose to attack me personally:\n\n====\n  I do not see a point in continuing to discuss this (or any design\n  level issues) with you.  You seem to go into a wrong direction to\n  break the design of the overall system, not in a direction to\n  improve anything.  I do not know, and at this point I do not care,\n  if you are doing so deliberately to sabotage Git.  Just stop.\n====\nJunio C Hamano: [9]\n\nEven if Junio's opinion was the correct one (it's not: as Google's libification\nproject proves), it's not OK to personally attack a contributor merely for\nexpressing an opinion that happens to differ from that of the maintainer.\n\nI am entitled to have my own opinion.\n\nI already know what you are going to argue back: you are going to argue that\nGoogle's libification project is different from my argument, but it's not:\nEmily Shaffer's introductory mail explained the same thing:\n\n====\n  In other words, for some modules which\n  already have clear boundaries inside of Git - like config.[ch],\n  strbuf.[ch], etc. - we want to remove some implicit dependencies, like\n  references to globals, and make explicit other dependencies, like\n  references to other modules within Git.\n====\nEmily Shaffer: [10]\n\nGoogle developers clearly believe the boundaries between \"modules\" are not\nclear, and they should be. Which is *exactly* what I argued back in 2013.\n\nYou say Junio asked me to leave, but you conveniently avoid explaining *why*:\nbecause he didn't like my opinion.\n\nJunio was not content with simply saying \"let's agree to disagree\", he threw yet\nanother personal attack:\n\n====\n  So I do not think this is not even a bikeshedding.  Just one side\n  being right, and the other side continuing to repeat nonsense\n  without listening.\n====\nJunio C Hamano: [11]\n\nAnd then:\n\n====\n  But what followed was a nonsense, which ended up wastign everybody's\n  time:\n====\nJunio C Hamano: [12]\n\nThis breaks the current code of conduct, as it clearly is a behavior that is\nnot:\n\n * Demonstrating empathy and kindness toward other people\n * Being respectful of differing opinions, viewpoints, and experiences\n\nThis is what I objectively did *not* do in that thread:\n\n * Denigrate the opinions of others\n * Personally attack anybody\n\nIt was Junio the one who did that, not me.\n\nJunio asked me to leave because I expressed an *opinion* he did not like.\n\nJunio asked me to leave because I said in my opinion `copy_notes_for_rewrite()`\ndoes not belong in libgit.a, because only git builtins should use it.\n\nThat's it.\n\nI think it's incredibly deceitful of you to claim \"Junio asked you to leave\"\nand provide a link, without explaining *why*.\n\nFast-forward to 2023, and Google developers are using the same language as I\ndid in 2013:\n\n====\n  Strbuf is a widely used basic structure that should only interact with other\n  primitives in strbuf.[ch].\n====\nCalvin Wan: [13]\n\nIs Junio asking them to leave the project for merely daring to express an\nopinion about what *should* be the direction the Git project takes?\n\nOf course not.\n\nIronically, the link you shared is a perfect example the double standards of\nthe Git project, in which a normative statement from a Google employee is par\nfor the course, but a normative statement from an unaffiliated contributor\n(i.e. me) is complete heresy.\n\nAll of this is of course, nothing more than a smoke screen from the topic at hand.\n\n---\n\nThis is the topic:\n\nSubject: Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options\n\nAll that matters here is this:\n\n 1. Apply Junio's patch\n 2. Run this command `git diff -s --raw @~`\n\nDoes the command produce the same output before and after the patch? Yes or no.\n\nThat is it.\n\nStop dragging personal drama between Junio and me from 2013 in which nobody\nelse participated--including you--and answer the *only* relevant question in\nthis thread.\n\nDoes Junio's patch change the current behavior?\n\n a. Yes\n b. No\n\nCheers.\n\n[1] https://lore.kernel.org/git/CAMP44s0cozMsTo7KQAjnqkqmvMwMw9D3SZrVxg48MOXkH9UQJQ@mail.gmail.com/\n[2] https://lore.kernel.org/git/pull.1525.v2.git.1683875068.gitgitgadget@gmail.com/\n[3] https://lore.kernel.org/git/CALkWK0mA7MXQv1k5bFpZLARDOHxU5kzKFXzcyUfb6NLZZY-=FA@mail.gmail.com/\n[4] https://lore.kernel.org/git/CALkWK0=7PRndNc7XQ-PCPbVCp9vck909bA561JhQG6uXXj1n4g@mail.gmail.com/\n[5] https://lore.kernel.org/git/20130610220627.GB28345@sigill.intra.peff.net/\n[6] https://lore.kernel.org/git/7vwqq1ct0g.fsf@alter.siamese.dyndns.org/\n[7] https://lore.kernel.org/git/CAMP44s03iXPVnunBdFT8etvZ-ew-D15A7mCV3wAAFXMNCpRAgA@mail.gmail.com/\n[8] https://lore.kernel.org/git/7vppvsbkc3.fsf@alter.siamese.dyndns.org/\n[9] https://lore.kernel.org/git/7vobbca1sr.fsf@alter.siamese.dyndns.org/\n[10] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/\n[11] https://lore.kernel.org/git/7vehc8a05n.fsf@alter.siamese.dyndns.org/\n[12] https://lore.kernel.org/git/7vzjuv14ir.fsf@alter.siamese.dyndns.org/\n[13] https://lore.kernel.org/git/20230503184849.1809304-1-calvinwan@google.com/\n\n-- \nFelipe Contreras\n"},{"id":"477244","messageId":"645f20f332476_21e719294d8@chronos.notmuch","threadId":"59688","inReplyTo":"xmqqpm77zlec.fsf@gitster.g","subject":"Re: [PATCH v2] diff: fix interaction between the \"-s\" option and other options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-13T05:32:35Z","receivedAt":"2023-05-13T05:40:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> >> The \"author\" refers to the author of the \"proposed log message\" of\n> >> the patch in question, i.e. me in this case.  The author of the\n> >> patch under discussion thinks it is, so asking \"Is it?\",\n> >\n> > This is the full quote:\n> >\n> > ====\n> > Let's fix the interactions of these bits to first make \"-s\" work as intended.\n> > ====\n> >\n> > If instead you meant this:\n> >\n> > ====\n> > Let's fix the interactions of these bits to first make \"-s\" work as I intend.\n> > ====\n> >\n> > Then that's not a rationale, you are essentially saying \"let's do X because I\n> > want\".\n> \n> This will be the last message from me on this.  I wouldn't have even\n> seen the message I am responding to, as I've already done my \"once\n> every few days sweep the spam folder to find things to salvage\",\n\nComment that breaks the code of conduct:\n\n * Demonstrating empathy and kindness toward other people\n * Being respectful of differing opinions, viewpoints, and experiences\n\nIs the maintainer exempt from following the code of conduct?\n\n> I didn't say and I didn't mean \"as I intend\", and you know that.\n\nNo, I don't know that, because I don't make assumptions.\n\nYou said this:\n\n====\n  >> Let's fix the interactions of these bits to first make \"-s\" work as\n  >> intended.\n  >\n  > Is it though?\n\n  Yes.\n\n  If the proposed log message says \"as intended\", the author thinks it\n  is.\n====\n[1]\n\nSince you are \"the author\", the above directly translates to \"I think it is as\nintended\", but I responded directly with:\n\n====\n  The question is not if the author of the patch thinks this is the way\n  `-s` is intended to work, the question is if this is the way `-s` is\n  intended to work.\n====\n[2]\n\nWhich is a perfectly valid response, to which you replied:\n\n====\n  The \"author\" refers to the author of the \"proposed log message\" of\n  the patch in question, i.e. me in this case.  The author of the\n  patch under discussion thinks it is, so asking \"Is it?\", implying\n  you do not agree, is nothing but a rhetorical question, and doing\n  so, without explaining why, wastes time on both sides.\n====\n[3]\n\nThis is not a valid response, because the question was never \"does the author\nof the patch think this behavior is intended\", the question was \"is this\nbehavior intended\", and I made that abundantly clear in [2].\n\nSo there's only two options:\n\n a. This is the behavior you intend, and you meant to say this is the\n    behaviour you intend.\n b. This is the behavior you think is intended, in which case if you think so\n    or not is irrelevant, instead you need to provide a rationale for why\n    you think that is the case, which you never did.\n\nIf it is `a`: that's not a rationale. If it is `b`: you still need a rationale.\nEither way no rationale was provided in the commit message (or anywhere else).\n\nYou chose to avoid this question and instead throw personal attacks in [3],\nwhich is not productive.\n\nFortunately for the project I decided to investigate the whole history behind\nthe true intention behind `-s` in [4].\n\nIn that investigation it became exceedingly clear that the intention behind\n`-s` is different from the intention behind `--no-patch`. And it also became\nclear that after making `output_format` a bit field: the intention of `-s`\nbecame unclear.\n\nThe culmination of that investigation is the thread in which `--no-patch` was\nintroduced [5]. In that thread Matthieu Moy explained the true purpose was to\nmake it more accessible to silence the output of `git show`.\n\nFurthermore, Matthieu Moy happened to respond today, and make it even more\nclear [6]:\n\n====\n  Looking more closely, it's rather clear to me \n  they are not, and that\n\n     git show --raw --patch --no-patch\n\n  should be equivalent to\n\n     git show --raw\n====\n\nWhich is *exactly* what I and Sergey argued, and to repeat and make it\nunquestionably clear:\n\n  `--raw --patch --no-patch` should be equivalent to `--raw`.\n\nPeriod.\n\nYou can throw all the personal attacks you want, but what you think is the\nintended behavior of `-s` is irrelevant, the fact is that the intended behavior\nof `--no-patch` is independent from the intended behavior of `-s`.\n\nHistory--and the explicit explanation of the original author--proves that.\n\nSo, when I asked \"is it though?\", that wasn't a rhetorical question intended to\nwaste time: the answer is clearly: NO.\n\nThis is not the way `-s` is intended to work.\n\n> >> And it led to unproductive and irritating waste of time number of times, and\n> >> eventually you were asked to leave the development community for at least a\n> >> few times.\n> >\n> > That is blatantly false. As a member of Git's Project Leadership Committee, you\n> > should know precisely how many times the committee has excercised this power,\n> > and it hasn't been \"a few times\", it has been one time.\n> \n> You were asked to leave in May 2014, and according to that message\n> from May 2014 [*1*],\n\nThis is the worst kind of misrepresentation.\n\nThe fact that *one person* said something, doesn't mean *the community* said that.\n\nAnybody who is the leader of any organization should understand that the\nopinion of *one person* is not the same as the opinion of a whole community.\n\nAnd this is--once again--a smoke screen.\n\nWhatever one person said in 2014 is totally and completely irrelevant to the\ntopic at hand.\n\n---\n\nThe commit message of the patch does not explain why the behavior of `-s`\nshould be changed in a backwards-incompatible way.\n\n[1] https://lore.kernel.org/git/xmqqsfc62t8y.fsf@gitster.g/\n[2] https://lore.kernel.org/git/6459c31038e81_7c68294ee@chronos.notmuch/\n[3] https://lore.kernel.org/git/xmqqjzxgzua0.fsf@gitster.g/\n[4] https://lore.kernel.org/git/645c5da0981c1_16961a29455@chronos.notmuch/\n[5] https://lore.kernel.org/git/1373893639-13413-1-git-send-email-Matthieu.Moy@imag.fr/\n[6] https://lore.kernel.org/git/4f713a29-1a34-2f71-ee54-c01020be903a@univ-lyon1.fr/\n\n-- \nFelipe Contreras\n"}]}