* [PATCH] t4013: add expected failure for "log --patch --no-patch" @ 2023-05-03 13:41 Sergey Organov 2023-05-03 16:57 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Sergey Organov @ 2023-05-03 13:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Sergey Organov --patch followed by --no-patch is to be a no-op according to the "git log" manual page. In reality this sequence breaks --raw output though (and who knows what else?) Add a test_expected_failure case for the issue. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- t/t4013-diff-various.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 5de1d190759f..f876b0cc8ec3 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode diff-tree -R --stat --compact-summary initial mode EOF +# This should succeed as --patch followed by --no-patch sequence is to +# be a no-op according to the manual page. In reality it breaks --raw +# though. Needs to be fixed. +test_expect_failure '--no-patch cancels --patch only' ' + git log --raw master >result && + process_diffs result >expected && + git log --patch --no-patch --raw >result && + process_diffs result >actual && + test_cmp expected actual +' + test_expect_success 'log -m matches pure log' ' git log master >result && process_diffs result >expected && -- 2.25.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 13:41 [PATCH] t4013: add expected failure for "log --patch --no-patch" Sergey Organov @ 2023-05-03 16:57 ` Junio C Hamano 2023-05-03 17:31 ` Sergey Organov 2023-05-04 18:07 ` Junio C Hamano 0 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-03 16:57 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > --patch followed by --no-patch is to be a no-op according to the "git > log" manual page. I briefly wondered if it is a bug in the documentation. But it is clear (at least to me) that "git log -p --stat --no-patch" wants to show only "--stat", and when "git log -p --raw" shows both patch and raw, I do not think of a reason why "git log -p --raw --no-patch" should not behave similarly. > Add a test_expected_failure case for the issue. That is unsatisfactory, though. Can you back-burner it and send in a fix with the same test flipping expect_failure to expect_success instead? Thanks. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > t/t4013-diff-various.sh | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 5de1d190759f..f876b0cc8ec3 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode > diff-tree -R --stat --compact-summary initial mode > EOF > > +# This should succeed as --patch followed by --no-patch sequence is to > +# be a no-op according to the manual page. In reality it breaks --raw > +# though. Needs to be fixed. > +test_expect_failure '--no-patch cancels --patch only' ' > + git log --raw master >result && > + process_diffs result >expected && > + git log --patch --no-patch --raw >result && > + process_diffs result >actual && > + test_cmp expected actual > +' > + > test_expect_success 'log -m matches pure log' ' > git log master >result && > process_diffs result >expected && ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 16:57 ` Junio C Hamano @ 2023-05-03 17:31 ` Sergey Organov 2023-05-03 18:07 ` Junio C Hamano 2023-05-04 18:07 ` Junio C Hamano 1 sibling, 1 reply; 42+ messages in thread From: Sergey Organov @ 2023-05-03 17:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> --patch followed by --no-patch is to be a no-op according to the "git >> log" manual page. > > I briefly wondered if it is a bug in the documentation. But it is > clear (at least to me) that "git log -p --stat --no-patch" wants to > show only "--stat", and when "git log -p --raw" shows both patch and > raw, I do not think of a reason why "git log -p --raw --no-patch" > should not behave similarly. > >> Add a test_expected_failure case for the issue. > > That is unsatisfactory, though. Can you back-burner it and send in > a fix with the same test flipping expect_failure to expect_success > instead? No problem from my side, but are you sure? - test_expect_failure [<prereq>] <message> <script> This is NOT the opposite of test_expect_success, but is used to mark a test that demonstrates a known breakage. Don't we need exactly this in this particular case? Demonstrate a known breakage? I'm confused. > > Thanks. > >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> t/t4013-diff-various.sh | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >> index 5de1d190759f..f876b0cc8ec3 100755 >> --- a/t/t4013-diff-various.sh >> +++ b/t/t4013-diff-various.sh >> @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode >> diff-tree -R --stat --compact-summary initial mode >> EOF >> >> +# This should succeed as --patch followed by --no-patch sequence is to >> +# be a no-op according to the manual page. In reality it breaks --raw >> +# though. Needs to be fixed. >> +test_expect_failure '--no-patch cancels --patch only' ' >> + git log --raw master >result && >> + process_diffs result >expected && >> + git log --patch --no-patch --raw >result && >> + process_diffs result >actual && >> + test_cmp expected actual >> +' >> + >> test_expect_success 'log -m matches pure log' ' >> git log master >result && >> process_diffs result >expected && Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 17:31 ` Sergey Organov @ 2023-05-03 18:07 ` Junio C Hamano 2023-05-03 18:32 ` Felipe Contreras ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-03 18:07 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > No problem from my side, but are you sure? Absolutely. I've seen people just say "we document a failed one" and leave it at that, without attempting to fix. I am trying to see if pushing back at first would serve as a good way to encourage these known failure to be fixed, without accumulating too many expect_failure in our test suite, which will waste cycles at CI runs (which do not need to be reminded something is known to be broken). I will try not to do this when I do not positively know the author of such a patch is capable enough to provide a fix, though, and you are unlucky enough to have shown your abilities in the past ;-) Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 18:07 ` Junio C Hamano @ 2023-05-03 18:32 ` Felipe Contreras 2023-05-03 19:49 ` Sergey Organov 2023-05-04 15:50 ` Junio C Hamano 2 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-03 18:32 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: git Junio C Hamano wrote: > I am trying to see if pushing back at first would serve as a good way > to encourage these known failure to be fixed, without accumulating too > many expect_failure in our test suite, which will waste cycles at CI > runs > (which do not need to be reminded something is known to be broken) We don't? There's plenty of things that are broken in git that people have forgotten. If wasting cycles on CI runs is your concern, I would gladly write a patch to skipp all the test_expect_failure tests. I would rather have documented all the things are known to be broken today than not, because in a month that might not be the case. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 18:07 ` Junio C Hamano 2023-05-03 18:32 ` Felipe Contreras @ 2023-05-03 19:49 ` Sergey Organov 2023-05-04 15:50 ` Junio C Hamano 2 siblings, 0 replies; 42+ messages in thread From: Sergey Organov @ 2023-05-03 19:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> No problem from my side, but are you sure? > > Absolutely. > > I've seen people just say "we document a failed one" and leave it at > that, without attempting to fix. I am trying to see if pushing back > at first would serve as a good way to encourage these known failure > to be fixed, without accumulating too many expect_failure in our > test suite, which will waste cycles at CI runs (which do not need to > be reminded something is known to be broken). I will try not to do > this when I do not positively know the author of such a patch is > capable enough to provide a fix, though, and you are unlucky enough > to have shown your abilities in the past ;-) Thanks for the credit, but as my recent attempts to fix 2 obvious deficiencies in Git CI (one of them being my own) failed quite miserably, I figure I have no idea how these things in CI are to be treated, so I prefer to leave a fix to somebody else, who actually groks what makes sense in the Git UI, and what doesn't. That said, in case you still need the test with expect_success, below is one rerolled. Thanks, -- Sergey Organov --- >8 --- Subject: [PATCH] t4013: add test for "log --patch --no-patch" --patch followed by --no-patch is to be a no-op according to the "git log" manual page. In reality this sequence breaks --raw output though (and who knows what else?) Add test case for the issue. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- t/t4013-diff-various.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 5de1d190759f..32907bf142fc 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode diff-tree -R --stat --compact-summary initial mode EOF +# This should succeed as --patch followed by --no-patch sequence is to +# be a no-op according to the manual page. In reality it breaks --raw +# though. Needs to be fixed. +test_expect_success '--no-patch cancels --patch only' ' + git log --raw master >result && + process_diffs result >expected && + git log --patch --no-patch --raw >result && + process_diffs result >actual && + test_cmp expected actual +' + test_expect_success 'log -m matches pure log' ' git log master >result && process_diffs result >expected && -- 2.25.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 18:07 ` Junio C Hamano 2023-05-03 18:32 ` Felipe Contreras 2023-05-03 19:49 ` Sergey Organov @ 2023-05-04 15:50 ` Junio C Hamano 2023-05-04 18:24 ` Sergey Organov 2023-05-09 1:03 ` Felipe Contreras 2 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-04 15:50 UTC (permalink / raw) To: Sergey Organov; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> No problem from my side, but are you sure? > > Absolutely. > > I've seen people just say "we document a failed one" and leave it at > that, without attempting to fix. I am trying to see if pushing back > at first would serve as a good way to encourage these known failure > to be fixed, without accumulating too many expect_failure in our > test suite, which will waste cycles at CI runs (which do not need to > be reminded something is known to be broken). I will try not to do > this when I do not positively know the author of such a patch is > capable enough to provide a fix, though, and you are unlucky enough > to have shown your abilities in the past ;-) I ended up spending some time digging history and remembered that "--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow --no-patch as synonym for -s, 2013-07-16). These git diff -p --stat --no-patch HEAD^ HEAD git diff -p --raw --no-patch HEAD^ HEAD would show no output from the diff machinery, patches, diffstats, raw object names, etc. And this turns out to be a prime example why the approach to ask contributors do more, would help the project overall. What I should have done, instead of asking for the test with its expect_failure turned into expect_success *and* a fix to the code to make the new test work, was to ask to see if it is really a bug in the behaviour or if the documentation is wrong. Then your reaction wouldn't have been "are you sure?". It hopefully would have been "ah, the intent is not documented correctly, and here is a documentation patch to fix it." When a command does not behave the way one thinks it should, being curious is good. Reporting it as a potential bug is also good. But it would help the project more if it was triaged before reporting it as a potential bug, if the reporter is capable of doing so. Those who encounter behaviour unexpected to them are more numerous than those who can report it as a potential bug (many people are not equipped to write a good bug report), and those who can triage and diagnose a bug report are fewer. Those who can come up with a solution is even more scarse. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 15:50 ` Junio C Hamano @ 2023-05-04 18:24 ` Sergey Organov 2023-05-04 20:53 ` Junio C Hamano 2023-05-09 1:34 ` [PATCH] t4013: add expected failure for "log --patch --no-patch" Felipe Contreras 2023-05-09 1:03 ` Felipe Contreras 1 sibling, 2 replies; 42+ messages in thread From: Sergey Organov @ 2023-05-04 18:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> No problem from my side, but are you sure? >> >> Absolutely. >> >> I've seen people just say "we document a failed one" and leave it at >> that, without attempting to fix. I am trying to see if pushing back >> at first would serve as a good way to encourage these known failure >> to be fixed, without accumulating too many expect_failure in our >> test suite, which will waste cycles at CI runs (which do not need to >> be reminded something is known to be broken). I will try not to do >> this when I do not positively know the author of such a patch is >> capable enough to provide a fix, though, and you are unlucky enough >> to have shown your abilities in the past ;-) > > I ended up spending some time digging history and remembered that > "--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow > --no-patch as synonym for -s, 2013-07-16). These > > git diff -p --stat --no-patch HEAD^ HEAD > git diff -p --raw --no-patch HEAD^ HEAD > > would show no output from the diff machinery, patches, diffstats, > raw object names, etc. [-s meaning "silent" at that time? If so, making --no-patch a synonym for "silent", and then documenting -s a synonym for --no-patch sounds like quite a twitch.] Anyway, this seems pretty irrelevant to the test case. Even if we spell --no-patch as -s, git diff -s --raw HEAD^ HEAD should produce what? To me it should be the same as git diff --raw HEAD^ HEAD as -s turns off everything, and then --raw is turned on. In reality this is not the case though, and that's what the test case is about. Notice that git diff -s --patch does produce the patch output, whereas git diff -s --raw git diff -s --stat produce none. Sounds like nonsense. > And this turns out to be a prime example why the approach to ask > contributors do more, would help the project overall. What I should > have done, instead of asking for the test with its expect_failure > turned into expect_success *and* a fix to the code to make the new > test work, was to ask to see if it is really a bug in the behaviour or > if the documentation is wrong. Then your reaction wouldn't have been > "are you sure?". It hopefully would have been "ah, the intent is not > documented correctly, and here is a documentation patch to fix it. Yep, documentation then needs to be fixed as well to match the intention, but this is unrelated to the test-case, see above. > When a command does not behave the way one thinks it should, being > curious is good. Reporting it as a potential bug is also good. But > it would help the project more if it was triaged before reporting it > as a potential bug, if the reporter is capable of doing so. Those > who encounter behaviour unexpected to them are more numerous than > those who can report it as a potential bug (many people are not > equipped to write a good bug report), and those who can triage and > diagnose a bug report are fewer. Those who can come up with a > solution is even more scarse. I'm afraid the solution I'd come up with won't be welcomed. If I'd start to "fix" it, it'd be likely set of independent options: --patch --no-patch --raw --no-raw --stat --no-stat and then -s being just a shortcut for "--no-raw --no-patch --no-stat" Easy to understand, simple to implement, straightforward to document, all intentions are perfectly obvious. But then these are to be new options to keep backward compatibility, and... No, thanks. Overall, as I neither able to make sense of the current set of intentions, nor even figure out what they are in the first place, I'm not the right person to fix implementation of these intentions, or even figure out for sure if a fix is needed. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 18:24 ` Sergey Organov @ 2023-05-04 20:53 ` Junio C Hamano 2023-05-04 21:37 ` Re* " Junio C Hamano 2023-05-09 1:34 ` [PATCH] t4013: add expected failure for "log --patch --no-patch" Felipe Contreras 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-04 20:53 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > [-s meaning "silent" at that time? If so, making --no-patch a synonym for > "silent", and then documenting -s a synonym for --no-patch sounds like > quite a twitch.] I thought it stood for "squelch", but it turns out that it was introduced by f4f21ce3 (git-diff-tree: clean up output, 2005-05-06), as a "silent" mode, way before anything else in the thread has happened. In Git v1.8.4, "--no-patch" was added as a synonym for "-s", and ever since we documented that they are equivalents. > Anyway, this seems pretty irrelevant to the test case. Even > if we spell --no-patch as -s, > > git diff -s --raw HEAD^ HEAD > > should produce what? To me it should be the same as > > git diff --raw HEAD^ HEAD > > as -s turns off everything, and then --raw is turned on. Ah, yeah, I missed that part of your sample command. It does seem quite puzzling, and the bad (or good?) part of the story is that my fresh build of v1.8.4 behaves exactly the same way. And with "-s" we can try versions before v1.8.4 (the only change that version made was to introduce "--no-patch" as its synonym). It seems it behaved that way at least back to v1.5.3 (which happens to be the oldest version I consider worth comparing with more modern versions). > Notice that > > git diff -s --patch > > does produce the patch output, whereas > > git diff -s --raw > git diff -s --stat > > produce none. Yes, you're right. > Sounds like nonsense. Sounds like a bug to me. I wonder how it came about. Did we forget to add support of the equivalent of what "-s --patch" does, when we added "--raw" and "--stat", perhaps? > I'm afraid the solution I'd come up with won't be welcomed. If I'd start > to "fix" it, it'd be likely set of independent options: > > --patch --no-patch > --raw --no-raw > --stat --no-stat > > and then > > -s being just a shortcut for "--no-raw --no-patch --no-stat" If I were writing Git from scratch without any existing users, that would be how I would design it (modulo that I would make sure we have some mechanism to make it easier for developers who may add a new output <format> to ensure that "-s" also implies "--no-<format>" for the new <format> they are adding to the mix). The fact that this wasn't brought up until now may mean that nobody would notice if we redefined the definition of "--no-patch" to behave that way, as long as "-s" keeps its original meaning. I dunno. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re* [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 20:53 ` Junio C Hamano @ 2023-05-04 21:37 ` Junio C Hamano 2023-05-04 23:10 ` [PATCH] diff: fix behaviour of the "-s" option Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-04 21:37 UTC (permalink / raw) To: Sergey Organov; +Cc: git Junio C Hamano <gitster@pobox.com> writes: >> ... it'd be likely set of independent options: >> --patch --no-patch >> --raw --no-raw >> --stat --no-stat >> >> and then >> >> -s being just a shortcut for "--no-raw --no-patch --no-stat" > > If I were writing Git from scratch without any existing users, that > would be how I would design it (modulo that I would make sure we > have some mechanism to make it easier for developers who may add > a new output <format> to ensure that "-s" also implies "--no-<format>" > for the new <format> they are adding to the mix). > > The fact that this wasn't brought up until now may mean that nobody > would notice if we redefined the definition of "--no-patch" to > behave that way, as long as "-s" keeps its original meaning. > > I dunno. I haven't run any tests (not just your new one, but existing ones) but at least "git diff -s --stat" and "git diff -s --raw" do countermand the earlier "-s" with this patch. I am not signing it off because I started from the options[] array in add_diff_options() and tweaked those I happened to notice, and haven't checked if we need to adjust other entries in the array. diff.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git c/diff.c w/diff.c index 1e83aaee6b..2d8025a9f7 100644 --- c/diff.c +++ w/diff.c @@ -4929,6 +4929,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset) BUG("%s should not get here", opt->long_name); options->output_format |= DIFF_FORMAT_DIFFSTAT; + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->stat_name_width = name_width; options->stat_graph_width = graph_width; options->stat_width = width; @@ -4947,6 +4948,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params) * The caller knows a dirstat-related option is given from the command * line; allow it to say "return this_function();" */ + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIRSTAT; return 1; } @@ -5502,9 +5504,9 @@ struct option *add_diff_options(const struct option *opts, PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified), OPT_BOOL('W', "function-context", &options->flags.funccontext, N_("generate diffs with <n> lines context")), - OPT_BIT_F(0, "raw", &options->output_format, + OPT_BITOP(0, "raw", &options->output_format, N_("generate the diff in raw format"), - DIFF_FORMAT_RAW, PARSE_OPT_NONEG), + DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT), OPT_BITOP(0, "patch-with-raw", &options->output_format, N_("synonym for '-p --raw'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW, @@ -5513,12 +5515,12 @@ struct option *add_diff_options(const struct option *opts, N_("synonym for '-p --stat'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT, DIFF_FORMAT_NO_OUTPUT), - OPT_BIT_F(0, "numstat", &options->output_format, + OPT_BITOP(0, "numstat", &options->output_format, N_("machine friendly --stat"), - DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG), - OPT_BIT_F(0, "shortstat", &options->output_format, + DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT), + OPT_BITOP(0, "shortstat", &options->output_format, N_("output only the last line of --stat"), - DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG), + DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT), OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."), N_("output the distribution of relative amount of changes for each sub-directory"), PARSE_OPT_NONEG | PARSE_OPT_OPTARG, ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] diff: fix behaviour of the "-s" option 2023-05-04 21:37 ` Re* " Junio C Hamano @ 2023-05-04 23:10 ` Junio C Hamano 2023-05-05 5:28 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-04 23:10 UTC (permalink / raw) To: git; +Cc: Sergey Organov Junio C Hamano <gitster@pobox.com> writes: > I haven't run any tests (not just your new one, but existing ones) > but ... And of course, not writing tests fails to even realize that the bug has two components, "-s" failing to clear the bits previously set, and other options not clearing the bit set by "-s". This version may still be rough, but at least the full test suite has been run with it, so I have a bit more confidence than the earlier one (which may not mean much). ------- >8 ------------- >8 ------------- >8 ------------- Sergey Organov noticed and reported "--patch --no-patch --raw" behaves differently from "--raw". It turns out there are a few interesting bugs in the implementation and documentation. * First, the documentation for "--no-patch" was unclear that it could be read to mean "--no-patch" countermands an earlier "--patch" but not other things. The intention of "--no-patch" ever since it was introduced at d09cd15d (diff: allow --no-patch as synonym for -s, 2013-07-16) was to serve as a synonym for "-s", so "--raw --patch --no-patch" should have produced no output, but it can be (mis)read to allow showing only "--raw" output. * Then the interaction between "-s" and other format options were poorly implemented. Modern versions of Git uses one bit each to represent formatting options like "--patch", "--stat" in a single output_format word, but for historical reasons, "-s" also is represented as another bit in the same word. This allows two interesting bugs to happen, and we have both. (1) After setting a format bit, then setting NO_OUTPUT with "-s", the code to process another "--<format>" option drops the NO_OUTPUT bit to allow output to be shown again. However, the code to handle "-s" only set NO_OUTPUT without unsetting format bits set earlier, so the earlier format bit got revealed upon seeing the second "--<format>" option. THis is the problem Sergey observed. (2) After setting NO_OUTPUT with "-s", code to process "--<format>" option can forget to unset NO_OUTPUT, leaving the command still silent. It is tempting to change the meaning of "--no-patch" to mean "disable only the patch format output" and reimplement "-s" as "not showing anything", but it would be an end-user visible change in behaviour. Let's fix the interactions of these bits to first make "-s" work as intended. The fix is conceptually very simple. * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo" option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to do so in some of the options and caused (2) above. * When processing "-s" option, we should not just set DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. We didn't do so and retained format bits set by options previously seen, causing (1) above. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/diff-options.txt | 7 +++++-- diff.c | 24 +++++++++++++----------- t/t4000-diff-format.sh | 32 +++++++++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 3674ac48e9..7d5bb65a49 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -29,8 +29,11 @@ endif::git-diff[] -s:: --no-patch:: - Suppress diff output. Useful for commands like `git show` that - show the patch by default, or to cancel the effect of `--patch`. + Suppress all output from the diff machinery. Useful for + commands like `git show` that show the patch by default to + squelch their output, or to cancel the effect of options like + `--patch`, `--stat` earlier on the command line in an alias. + endif::git-format-patch[] ifdef::git-log[] diff --git a/diff.c b/diff.c index 648f6717a5..5a2f096683 100644 --- a/diff.c +++ b/diff.c @@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset) } else BUG("%s should not get here", opt->long_name); + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIFFSTAT; options->stat_name_width = name_width; options->stat_graph_width = graph_width; @@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params) * The caller knows a dirstat-related option is given from the command * line; allow it to say "return this_function();" */ + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIRSTAT; return 1; } @@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt, options->flags.stat_with_summary = 0; } else { options->flags.stat_with_summary = 1; + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIFFSTAT; } return 0; @@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options) OPT_BITOP('p', "patch", &options->output_format, N_("generate patch"), DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT), - OPT_BIT_F('s', "no-patch", &options->output_format, - N_("suppress diff output"), - DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG), + OPT_SET_INT('s', "no-patch", &options->output_format, + N_("suppress diff output"), DIFF_FORMAT_NO_OUTPUT), OPT_BITOP('u', NULL, &options->output_format, N_("generate patch"), DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT), @@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options) PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified), OPT_BOOL('W', "function-context", &options->flags.funccontext, N_("generate diffs with <n> lines context")), - OPT_BIT_F(0, "raw", &options->output_format, + OPT_BITOP(0, "raw", &options->output_format, N_("generate the diff in raw format"), - DIFF_FORMAT_RAW, PARSE_OPT_NONEG), + DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT), OPT_BITOP(0, "patch-with-raw", &options->output_format, N_("synonym for '-p --raw'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW, @@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options) N_("synonym for '-p --stat'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT, DIFF_FORMAT_NO_OUTPUT), - OPT_BIT_F(0, "numstat", &options->output_format, + OPT_BITOP(0, "numstat", &options->output_format, N_("machine friendly --stat"), - DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG), - OPT_BIT_F(0, "shortstat", &options->output_format, + DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT), + OPT_BITOP(0, "shortstat", &options->output_format, N_("output only the last line of --stat"), - DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG), + DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT), OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."), N_("output the distribution of relative amount of changes for each sub-directory"), PARSE_OPT_NONEG | PARSE_OPT_OPTARG, @@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options) OPT_BIT_F(0, "check", &options->output_format, N_("warn if changes introduce conflict markers or whitespace errors"), DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG), - OPT_BIT_F(0, "summary", &options->output_format, + OPT_BITOP(0, "summary", &options->output_format, N_("condensed summary such as creations, renames and mode changes"), - DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG), + DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT), OPT_BIT_F(0, "name-only", &options->output_format, N_("show only names of changed files"), DIFF_FORMAT_NAME, PARSE_OPT_NONEG), diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh index bfcaae390f..762b9d4c60 100755 --- a/t/t4000-diff-format.sh +++ b/t/t4000-diff-format.sh @@ -5,6 +5,9 @@ test_description='Test built-in diff output engine. +We happen to know that all diff plumbing and diff Porcelain share the +same command line parser, so testing one should be sufficient; pick +diff-files as a representative. ' TEST_PASSES_SANITIZE_LEAK=true @@ -16,9 +19,11 @@ Line 2 line 3' cat path0 >path1 chmod +x path1 +mkdir path2 +>path2/path3 test_expect_success 'update-index --add two files with and without +x.' ' - git update-index --add path0 path1 + git update-index --add path0 path1 path2/path3 ' mv path0 path0- @@ -91,4 +96,29 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch' test_must_be_empty err ' + +# Smudge path2/path3 so that dirstat has something to show +date >path2/path3 + +for format in stat raw numstat shortstat dirstat +do + test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" ' + git diff-files --no-patch "--$format" >actual && + git diff-files "--$format" >expect && + test_cmp expect actual + ' + + test_expect_success "--no-patch clears all previous ones" ' + git diff-files --$format -s -p >actual && + git diff-files -p >expect && + test_cmp expect actual + ' + + test_expect_success "--no-patch in 'git diff --no-patch --$format' is a no-op" ' + git diff --no-patch "--$format" >actual && + git diff "--$format" >expect && + test_cmp expect actual + ' +done + test_done -- 2.40.1-476-g69c786637d ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-04 23:10 ` [PATCH] diff: fix behaviour of the "-s" option Junio C Hamano @ 2023-05-05 5:28 ` Junio C Hamano 2023-05-05 16:51 ` Junio C Hamano 2023-05-09 1:16 ` Felipe Contreras 2023-05-05 8:32 ` Sergey Organov 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano 2 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 5:28 UTC (permalink / raw) To: git Junio C Hamano <gitster@pobox.com> writes: > * Then the interaction between "-s" and other format options were > poorly implemented. Modern versions of Git uses one bit each to > represent formatting options like "--patch", "--stat" in a single > output_format word, but for historical reasons, "-s" also is > represented as another bit in the same word. An obvious improvement strategy is to stop using the NO_OUTPUT bit and instead make "-s" to clear the "output_format" word, and make "--[no-]raw", "--[no-]stat", "--[no-]patch", etc. to flip their own bit in the same "output_format" word. I think the "historical reasons" why we did not do that was because we wanted to be able to do a flexible defaulting. We may want to say "if no output-format option is given from the command line, default to "--patch", but otherwise do not set the "--patch" bit on", for example. Initializing the "output_format" word with "--patch" bit on would not work---when "--raw" is given from the command line, we want to clear that "--patch" bit we set for default and set "--raw" bit on. We can initialize the "output_format" word to 0, and OR in the bits for each format option as we process them, and then flip the "--patch" bit on if "output_format" word is still 0 after command line parsing is done. This would almost work, except that it would make it hard to tell "no command line options" case and "'-s' cleared all bits" case apart (the former wants to default to "--patch", while the latter wants to stay "no output"), and it probably was the reason why we gave an extra NO_OUTPUT bit to the "-s" option. In hindsight, the arrangement certainly made other things harder and prone to unnecessary bugs. Anyway... ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-05 5:28 ` Junio C Hamano @ 2023-05-05 16:51 ` Junio C Hamano 2023-05-09 1:16 ` Felipe Contreras 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 16:51 UTC (permalink / raw) To: git Junio C Hamano <gitster@pobox.com> writes: > ... I think the "historical > reasons" why we did not do that was because we wanted to be able to > do a flexible defaulting. ... > This would almost work, except that it would > make it hard to tell "no command line options" case and "'-s' cleared > all bits" case apart (the former wants to default to "--patch", > while the latter wants to stay "no output"), and it probably was the > reason why we gave an extra NO_OUTPUT bit to the "-s" option. In > hindsight, the arrangement certainly made other things harder and > prone to unnecessary bugs. > > Anyway... The distinction between the presense of NO_OUTPUT bit and absolutely empty output_format word indeed is used by "git show", in the builtin/log.c::show_setup_revisions_tweak() function. We could lose DIFF_FORMAT_NO_OUTPUT bit, but then we need to replace it with something else (i.e. DIFF_FORMAT_OPTION_GIVEN bit), and * "--patch", "--raw", etc. will set DIFF_FORMAT_$format bit and DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw", etc. will set off DIFF_FORMAT_$format bit but still record the fact that we saw an option from the command line by setting DIFF_FORMAT_OPTION_GIVEN bit. * "-s" (and its synonym "--no-patch") will set the DIFF_FORMAT_OPTION_GIVEN bit on, and clear all other bits. which I suspect would make the code much cleaner without breaking any end-user expectations. Once that is in place, transitioning "--no-patch" to mean the counterpart of "--patch", just like "--no-raw" only defeats an earlier "--raw", would be quite simple at the code level. The social cost of migrating the end-user expectations might be too great for it to be worth, but at least the "GIVEN" bit clean-up alone may be worth it. Not that I would be starting the process right away... ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-05 5:28 ` Junio C Hamano 2023-05-05 16:51 ` Junio C Hamano @ 2023-05-09 1:16 ` Felipe Contreras 1 sibling, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 1:16 UTC (permalink / raw) To: Junio C Hamano, git Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > * Then the interaction between "-s" and other format options were > > poorly implemented. Modern versions of Git uses one bit each to > > represent formatting options like "--patch", "--stat" in a single > > output_format word, but for historical reasons, "-s" also is > > represented as another bit in the same word. > > An obvious improvement strategy is to stop using the NO_OUTPUT bit > and instead make "-s" to clear the "output_format" word, and make > "--[no-]raw", "--[no-]stat", "--[no-]patch", etc. to flip their own > bit in the same "output_format" word. I think the "historical > reasons" why we did not do that was because we wanted to be able to > do a flexible defaulting. We may want to say "if no output-format > option is given from the command line, default to "--patch", but > otherwise do not set the "--patch" bit on", for example. > Initializing the "output_format" word with "--patch" bit on would > not work---when "--raw" is given from the command line, we want to > clear that "--patch" bit we set for default and set "--raw" bit on. > We can initialize the "output_format" word to 0, and OR in the bits > for each format option as we process them, and then flip the > "--patch" bit on if "output_format" word is still 0 after command > line parsing is done. This would almost work, except that it would > make it hard to tell "no command line options" case and "'-s' cleared > all bits" case apart (the former wants to default to "--patch", > while the latter wants to stay "no output"), and it probably was the > reason why we gave an extra NO_OUTPUT bit to the "-s" option. In > hindsight, the arrangement certainly made other things harder and > prone to unnecessary bugs. That's easy to solve by introducing a DIFF_FORMAT_DEFAULT item, which would be different from 0. Then every command can update DIFF_FORMAT_DEFAULT to the desired default, and if the default is cleared (e.g. `--no-patch`) that would not happen. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-04 23:10 ` [PATCH] diff: fix behaviour of the "-s" option Junio C Hamano 2023-05-05 5:28 ` Junio C Hamano @ 2023-05-05 8:32 ` Sergey Organov 2023-05-05 16:31 ` Junio C Hamano 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano 2 siblings, 1 reply; 42+ messages in thread From: Sergey Organov @ 2023-05-05 8:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> I haven't run any tests (not just your new one, but existing ones) >> but ... > > And of course, not writing tests fails to even realize that the bug > has two components, "-s" failing to clear the bits previously set, > and other options not clearing the bit set by "-s". > > This version may still be rough, but at least the full test suite > has been run with it, so I have a bit more confidence than the > earlier one (which may not mean much). > > ------- >8 ------------- >8 ------------- >8 ------------- > Sergey Organov noticed and reported "--patch --no-patch --raw" > behaves differently from "--raw". It turns out there are a few > interesting bugs in the implementation and documentation. > > * First, the documentation for "--no-patch" was unclear that it > could be read to mean "--no-patch" countermands an earlier > "--patch" but not other things. The intention of "--no-patch" > ever since it was introduced at d09cd15d (diff: allow --no-patch > as synonym for -s, 2013-07-16) was to serve as a synonym for > "-s", so "--raw --patch --no-patch" should have produced no > output, but it can be (mis)read to allow showing only "--raw" > output. > > * Then the interaction between "-s" and other format options were > poorly implemented. Modern versions of Git uses one bit each to > represent formatting options like "--patch", "--stat" in a single > output_format word, but for historical reasons, "-s" also is > represented as another bit in the same word. This allows two > interesting bugs to happen, and we have both. > > (1) After setting a format bit, then setting NO_OUTPUT with "-s", > the code to process another "--<format>" option drops the > NO_OUTPUT bit to allow output to be shown again. However, > the code to handle "-s" only set NO_OUTPUT without unsetting > format bits set earlier, so the earlier format bit got > revealed upon seeing the second "--<format>" option. THis is > the problem Sergey observed. > > (2) After setting NO_OUTPUT with "-s", code to process > "--<format>" option can forget to unset NO_OUTPUT, leaving > the command still silent. > > It is tempting to change the meaning of "--no-patch" to mean > "disable only the patch format output" and reimplement "-s" as "not > showing anything", but it would be an end-user visible change in > behaviour. Let's fix the interactions of these bits to first make > "-s" work as intended. > > The fix is conceptually very simple. > > * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo" > option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is > given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to > do so in some of the options and caused (2) above. > > * When processing "-s" option, we should not just set > DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. > We didn't do so and retained format bits set by options > previously seen, causing (1) above. Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as well, I wonder, as absence of any output bits effectively means "no output"? Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-05 8:32 ` Sergey Organov @ 2023-05-05 16:31 ` Junio C Hamano 2023-05-05 17:07 ` Sergey Organov 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 16:31 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: >> * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo" >> option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is >> given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to >> do so in some of the options and caused (2) above. >> >> * When processing "-s" option, we should not just set >> DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. >> We didn't do so and retained format bits set by options >> previously seen, causing (1) above. > > Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as > well, I wonder, as absence of any output bits effectively means "no > output"? Not quite. The latter is not "set 0 to output_format word", but "set 0 to output_format word and then flip only NO_OUTPUT bit on". I've written a bit more on it in a follow-up message to the patch. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] diff: fix behaviour of the "-s" option 2023-05-05 16:31 ` Junio C Hamano @ 2023-05-05 17:07 ` Sergey Organov 0 siblings, 0 replies; 42+ messages in thread From: Sergey Organov @ 2023-05-05 17:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo" >>> option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is >>> given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to do >>> so in some of the options and caused (2) above. >>> * When processing "-s" option, we should not just set >>> DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. We >>> didn't do so and retained format bits set by options previously >>> seen, causing (1) above. >> Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete >> as well, I wonder, as absence of any output bits effectively means >> "no output"? > > Not quite. The latter is not "set 0 to output_format word", but "set 0 > to output_format word and then flip only NO_OUTPUT bit on". I've > written a bit more on it in a follow-up message to the patch. Yep, I've noticed that post after I sent the question. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-04 23:10 ` [PATCH] diff: fix behaviour of the "-s" option Junio C Hamano 2023-05-05 5:28 ` Junio C Hamano 2023-05-05 8:32 ` Sergey Organov @ 2023-05-05 16:59 ` Junio C Hamano 2023-05-05 17:41 ` Eric Sunshine ` (2 more replies) 2 siblings, 3 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 16:59 UTC (permalink / raw) To: git Sergey Organov noticed and reported "--patch --no-patch --raw" behaves differently from just "--raw". It turns out that there are a few interesting bugs in the implementation and documentation. * First, the documentation for "--no-patch" was unclear that it could be read to mean "--no-patch" countermands an earlier "--patch" but not other things. The intention of "--no-patch" ever since it was introduced at d09cd15d (diff: allow --no-patch as synonym for -s, 2013-07-16) was to serve as a synonym for "-s", so "--raw --patch --no-patch" should have produced no output, but it can be (mis)read to allow showing only "--raw" output. * Then the interaction between "-s" and other format options were poorly implemented. Modern versions of Git uses one bit each to represent formatting options like "--patch", "--stat" in a single output_format word, but for historical reasons, "-s" also is represented as another bit in the same word. This allows two interesting bugs to happen, and we have both X-<. (1) After setting a format bit, then setting NO_OUTPUT with "-s", the code to process another "--<format>" option drops the NO_OUTPUT bit to allow output to be shown again. However, the code to handle "-s" only set NO_OUTPUT without unsetting format bits set earlier, so the earlier format bit got revealed upon seeing the second "--<format>" option. This is the problem Sergey observed. (2) After setting NO_OUTPUT with "-s", code to process "--<format>" option can forget to unset NO_OUTPUT, leaving the command still silent. It is tempting to change the meaning of "--no-patch" to mean "disable only the patch format output" and reimplement "-s" as "not showing anything", but it would be an end-user visible change in behavior. Let's fix the interactions of these bits to first make "-s" work as intended. The fix is conceptually very simple. * Whenever we set DIFF_FORMAT_FOO because we saw the "--foo" option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to do so in some of the options and caused (2) above. * When processing "-s" option, we should not just set DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. We didn't do so and retained format bits set by options previously seen, causing (1) above. It is even more tempting to lose NO_OUTPUT bit and instead take output_format word being 0 as its replacement, but that would break the mechanism "git show" uses to default to "--patch" output, where the distinction between telling the command to be silent with "-s" and having no output format specified on the command line matters, and an explicit output format given on the command line should not be "combined" with the default "--patch" format. So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we may want to replace it with OPTION_GIVEN bit, and * make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw", etc. will set off DIFF_FORMAT_$format bit but still record the fact that we saw an option from the command line by setting DIFF_FORMAT_OPTION_GIVEN bit. * make "-s" (and its synonym "--no-patch") clear all other bits and set only the DIFF_FORMAT_OPTION_GIVEN bit on. which I suspect would make the code much cleaner without breaking any end-user expectations. Once that is in place, transitioning "--no-patch" to mean the counterpart of "--patch", just like "--no-raw" only defeats an earlier "--raw", would be quite simple at the code level. The social cost of migrating the end-user expectations might be too great for it to be worth, but at least the "GIVEN" bit clean-up alone may be worth it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/diff-options.txt | 7 +++++-- diff.c | 24 +++++++++++++----------- t/t4000-diff-format.sh | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 3674ac48e9..7d5bb65a49 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -29,8 +29,11 @@ endif::git-diff[] -s:: --no-patch:: - Suppress diff output. Useful for commands like `git show` that - show the patch by default, or to cancel the effect of `--patch`. + Suppress all output from the diff machinery. Useful for + commands like `git show` that show the patch by default to + squelch their output, or to cancel the effect of options like + `--patch`, `--stat` earlier on the command line in an alias. + endif::git-format-patch[] ifdef::git-log[] diff --git a/diff.c b/diff.c index 648f6717a5..5a2f096683 100644 --- a/diff.c +++ b/diff.c @@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset) } else BUG("%s should not get here", opt->long_name); + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIFFSTAT; options->stat_name_width = name_width; options->stat_graph_width = graph_width; @@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params) * The caller knows a dirstat-related option is given from the command * line; allow it to say "return this_function();" */ + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIRSTAT; return 1; } @@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt, options->flags.stat_with_summary = 0; } else { options->flags.stat_with_summary = 1; + options->output_format &= ~DIFF_FORMAT_NO_OUTPUT; options->output_format |= DIFF_FORMAT_DIFFSTAT; } return 0; @@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options) OPT_BITOP('p', "patch", &options->output_format, N_("generate patch"), DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT), - OPT_BIT_F('s', "no-patch", &options->output_format, - N_("suppress diff output"), - DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG), + OPT_SET_INT('s', "no-patch", &options->output_format, + N_("suppress diff output"), DIFF_FORMAT_NO_OUTPUT), OPT_BITOP('u', NULL, &options->output_format, N_("generate patch"), DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT), @@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options) PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified), OPT_BOOL('W', "function-context", &options->flags.funccontext, N_("generate diffs with <n> lines context")), - OPT_BIT_F(0, "raw", &options->output_format, + OPT_BITOP(0, "raw", &options->output_format, N_("generate the diff in raw format"), - DIFF_FORMAT_RAW, PARSE_OPT_NONEG), + DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT), OPT_BITOP(0, "patch-with-raw", &options->output_format, N_("synonym for '-p --raw'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW, @@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options) N_("synonym for '-p --stat'"), DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT, DIFF_FORMAT_NO_OUTPUT), - OPT_BIT_F(0, "numstat", &options->output_format, + OPT_BITOP(0, "numstat", &options->output_format, N_("machine friendly --stat"), - DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG), - OPT_BIT_F(0, "shortstat", &options->output_format, + DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT), + OPT_BITOP(0, "shortstat", &options->output_format, N_("output only the last line of --stat"), - DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG), + DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT), OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."), N_("output the distribution of relative amount of changes for each sub-directory"), PARSE_OPT_NONEG | PARSE_OPT_OPTARG, @@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options) OPT_BIT_F(0, "check", &options->output_format, N_("warn if changes introduce conflict markers or whitespace errors"), DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG), - OPT_BIT_F(0, "summary", &options->output_format, + OPT_BITOP(0, "summary", &options->output_format, N_("condensed summary such as creations, renames and mode changes"), - DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG), + DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT), OPT_BIT_F(0, "name-only", &options->output_format, N_("show only names of changed files"), DIFF_FORMAT_NAME, PARSE_OPT_NONEG), diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh index bfcaae390f..8d50331b8c 100755 --- a/t/t4000-diff-format.sh +++ b/t/t4000-diff-format.sh @@ -5,6 +5,9 @@ test_description='Test built-in diff output engine. +We happen to know that all diff plumbing and diff Porcelain share the +same command line parser, so testing one should be sufficient; pick +diff-files as a representative. ' TEST_PASSES_SANITIZE_LEAK=true @@ -16,9 +19,11 @@ Line 2 line 3' cat path0 >path1 chmod +x path1 +mkdir path2 +>path2/path3 test_expect_success 'update-index --add two files with and without +x.' ' - git update-index --add path0 path1 + git update-index --add path0 path1 path2/path3 ' mv path0 path0- @@ -91,4 +96,31 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch' test_must_be_empty err ' + +# Smudge path2/path3 so that dirstat has something to show +date >path2/path3 + +for format in stat raw numstat shortstat summary \ + dirstat cumulative dirstat-by-file \ + patch-with-raw patch-with-stat compact-summary +do + test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" ' + git diff-files --no-patch "--$format" >actual && + git diff-files "--$format" >expect && + test_cmp expect actual + ' + + test_expect_success "--no-patch clears all previous ones" ' + git diff-files --$format -s -p >actual && + git diff-files -p >expect && + test_cmp expect actual + ' + + test_expect_success "--no-patch in 'git diff --no-patch --$format' is a no-op" ' + git diff --no-patch "--$format" >actual && + git diff "--$format" >expect && + test_cmp expect actual + ' +done + test_done -- 2.40.1-476-g69c786637d ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano @ 2023-05-05 17:41 ` Eric Sunshine 2023-05-05 19:01 ` Junio C Hamano 2023-05-05 21:19 ` [PATCH 0/2] dirstat: leakfix Junio C Hamano 2023-05-09 0:38 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Felipe Contreras 2 siblings, 1 reply; 42+ messages in thread From: Eric Sunshine @ 2023-05-05 17:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, May 5, 2023 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote: > Sergey Organov noticed and reported "--patch --no-patch --raw" > behaves differently from just "--raw". It turns out that there are > a few interesting bugs in the implementation and documentation. > > * First, the documentation for "--no-patch" was unclear that it > could be read to mean "--no-patch" countermands an earlier > "--patch" but not other things. The intention of "--no-patch" > ever since it was introduced at d09cd15d (diff: allow --no-patch > as synonym for -s, 2013-07-16) was to serve as a synonym for > "-s", so "--raw --patch --no-patch" should have produced no > output, but it can be (mis)read to allow showing only "--raw" > output. > > * Then the interaction between "-s" and other format options were > poorly implemented. Modern versions of Git uses one bit each to > represent formatting options like "--patch", "--stat" in a single > output_format word, but for historical reasons, "-s" also is > represented as another bit in the same word. This allows two > interesting bugs to happen, and we have both X-<. > > (1) After setting a format bit, then setting NO_OUTPUT with "-s", > the code to process another "--<format>" option drops the > NO_OUTPUT bit to allow output to be shown again. However, > the code to handle "-s" only set NO_OUTPUT without unsetting > format bits set earlier, so the earlier format bit got > revealed upon seeing the second "--<format>" option. This is Glad to see "THis" from v1 fixed. > the problem Sergey observed. > > (2) After setting NO_OUTPUT with "-s", code to process > "--<format>" option can forget to unset NO_OUTPUT, leaving > the command still silent. > > It is tempting to change the meaning of "--no-patch" to mean > "disable only the patch format output" and reimplement "-s" as "not > showing anything", but it would be an end-user visible change in > behavior. Let's fix the interactions of these bits to first make > "-s" work as intended. > > The fix is conceptually very simple. > > * Whenever we set DIFF_FORMAT_FOO because we saw the "--foo" > option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is > given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to > do so in some of the options and caused (2) above. > > * When processing "-s" option, we should not just set > DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. > We didn't do so and retained format bits set by options > previously seen, causing (1) above. The above description is very clear and well stated, even to someone like me who didn't follow the discussion which culminated in this patch. > It is even more tempting to lose NO_OUTPUT bit and instead take > output_format word being 0 as its replacement, but that would break > the mechanism "git show" uses to default to "--patch" output, where > the distinction between telling the command to be silent with "-s" > and having no output format specified on the command line matters, > and an explicit output format given on the command line should not > be "combined" with the default "--patch" format. > > So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we > may want to replace it with OPTION_GIVEN bit, and > > * make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and > DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw", > etc. will set off DIFF_FORMAT_$format bit but still record the > fact that we saw an option from the command line by setting > DIFF_FORMAT_OPTION_GIVEN bit. > > * make "-s" (and its synonym "--no-patch") clear all other bits > and set only the DIFF_FORMAT_OPTION_GIVEN bit on. > > which I suspect would make the code much cleaner without breaking > any end-user expectations. > > Once that is in place, transitioning "--no-patch" to mean the > counterpart of "--patch", just like "--no-raw" only defeats an > earlier "--raw", would be quite simple at the code level. The > social cost of migrating the end-user expectations might be too > great for it to be worth, but at least the "GIVEN" bit clean-up s/worth/worthwhile/ > alone may be worth it. And this final part addresses the big question which v1 left dangling (specifically, "why the proposed patch doesn't eliminate NO_OUTPUT altogether). Good. > Signed-off-by: Junio C Hamano <gitster@pobox.com> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-05 17:41 ` Eric Sunshine @ 2023-05-05 19:01 ` Junio C Hamano 0 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 19:01 UTC (permalink / raw) To: Eric Sunshine; +Cc: git Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, May 5, 2023 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote: > ... > Glad to see "THis" from v1 fixed. > ... > And this final part addresses the big question which v1 left dangling > (specifically, "why the proposed patch doesn't eliminate NO_OUTPUT > altogether). Good. Thanks. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] dirstat: leakfix 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano 2023-05-05 17:41 ` Eric Sunshine @ 2023-05-05 21:19 ` Junio C Hamano 2023-05-05 21:19 ` [PATCH 1/2] diff: refactor common tail part of dirstat computation Junio C Hamano 2023-05-05 21:19 ` [PATCH 2/2] diff: plug leaks in dirstat Junio C Hamano 2023-05-09 0:38 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Felipe Contreras 2 siblings, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 21:19 UTC (permalink / raw) To: git Junio C Hamano <gitster@pobox.com> writes: > t/t4000-diff-format.sh | 34 +++++++++++++++++++++++++++++++++- > ... > +for format in stat raw numstat shortstat summary \ > + dirstat cumulative dirstat-by-file \ > + patch-with-raw patch-with-stat compact-summary Unfortunately, because t4000 is marked as passing with leak sanitizer on, even though this series does not introduce any new leaks (in fact, there is nothing in the series that allocates pieces of memory at all), the CI will fail with the sanitizer job. Needless to say, I hate the current arrangement of these tests. Those who happen to use tools or features that have nothing to do with the topic being developed that introduces no new leaks are punished by a test failure. Here are a pair of patches that plug leaks in dirstat code. This allows the "fix interaction between -s and others" patch that adds a test that exercises --dirstat in t4000 to be queued without breaking the leak sanitizer. Also t4047 that is about dirstat can now be marked as leak free. Junio C Hamano (2): diff: refactor common tail part of dirstat computation diff: plug leaks in dirstat diff.c | 34 ++++++++++++++++++++-------------- t/t4047-diff-dirstat.sh | 2 ++ 2 files changed, 22 insertions(+), 14 deletions(-) -- 2.40.1-476-g69c786637d ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] diff: refactor common tail part of dirstat computation 2023-05-05 21:19 ` [PATCH 0/2] dirstat: leakfix Junio C Hamano @ 2023-05-05 21:19 ` Junio C Hamano 2023-05-05 21:19 ` [PATCH 2/2] diff: plug leaks in dirstat Junio C Hamano 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 21:19 UTC (permalink / raw) To: git This will become useful when we plug leaks in these two functions. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/diff.c b/diff.c index 648f6717a5..e13d0f8b67 100644 --- a/diff.c +++ b/diff.c @@ -2971,6 +2971,19 @@ static int dirstat_compare(const void *_a, const void *_b) return strcmp(a->name, b->name); } +static void conclude_dirstat(struct diff_options *options, + struct dirstat_dir *dir, + unsigned long changed) +{ + /* This can happen even with many files, if everything was renames */ + if (!changed) + return; + + /* Show all directories with more than x% of the changes */ + QSORT(dir->files, dir->nr, dirstat_compare); + gather_dirstat(options, dir, changed, "", 0); +} + static void show_dirstat(struct diff_options *options) { int i; @@ -3060,13 +3073,7 @@ static void show_dirstat(struct diff_options *options) dir.nr++; } - /* This can happen even with many files, if everything was renames */ - if (!changed) - return; - - /* Show all directories with more than x% of the changes */ - QSORT(dir.files, dir.nr, dirstat_compare); - gather_dirstat(options, &dir, changed, "", 0); + conclude_dirstat(options, &dir, changed); } static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options) @@ -3104,13 +3111,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o dir.nr++; } - /* This can happen even with many files, if everything was renames */ - if (!changed) - return; - - /* Show all directories with more than x% of the changes */ - QSORT(dir.files, dir.nr, dirstat_compare); - gather_dirstat(options, &dir, changed, "", 0); + conclude_dirstat(options, &dir, changed); } static void free_diffstat_file(struct diffstat_file *f) -- 2.40.1-476-g69c786637d ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] diff: plug leaks in dirstat 2023-05-05 21:19 ` [PATCH 0/2] dirstat: leakfix Junio C Hamano 2023-05-05 21:19 ` [PATCH 1/2] diff: refactor common tail part of dirstat computation Junio C Hamano @ 2023-05-05 21:19 ` Junio C Hamano 1 sibling, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-05 21:19 UTC (permalink / raw) To: git The array of dirstat_file contained in the dirstat_dir structure is not freed after the processing ends. Unfortunately, the member that points at the array, .files, is incremented as the gather_dirstat() function recursively walks it, and this needs to be plugged by remembering the beginning of the array before gather_dirstat() mucks with it and freeing it after we are done. We can mark t4047 as leak-free. t4000, which is marked as leak-free, now can exercise dirstat in it, which will happen next. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 17 +++++++++++------ t/t4047-diff-dirstat.sh | 2 ++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index e13d0f8b67..d52db685f7 100644 --- a/diff.c +++ b/diff.c @@ -2975,13 +2975,18 @@ static void conclude_dirstat(struct diff_options *options, struct dirstat_dir *dir, unsigned long changed) { - /* This can happen even with many files, if everything was renames */ - if (!changed) - return; + struct dirstat_file *to_free = dir->files; + + if (!changed) { + /* This can happen even with many files, if everything was renames */ + ; + } else { + /* Show all directories with more than x% of the changes */ + QSORT(dir->files, dir->nr, dirstat_compare); + gather_dirstat(options, dir, changed, "", 0); + } - /* Show all directories with more than x% of the changes */ - QSORT(dir->files, dir->nr, dirstat_compare); - gather_dirstat(options, dir, changed, "", 0); + free(to_free); } static void show_dirstat(struct diff_options *options) diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh index 7fec2cb9cd..70224c3da1 100755 --- a/t/t4047-diff-dirstat.sh +++ b/t/t4047-diff-dirstat.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='diff --dirstat tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # set up two commits where the second commit has these files -- 2.40.1-476-g69c786637d ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano 2023-05-05 17:41 ` Eric Sunshine 2023-05-05 21:19 ` [PATCH 0/2] dirstat: leakfix Junio C Hamano @ 2023-05-09 0:38 ` Felipe Contreras 2023-05-09 1:22 ` Junio C Hamano 2 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 0:38 UTC (permalink / raw) To: Junio C Hamano, git Junio C Hamano wrote: > Sergey Organov noticed and reported "--patch --no-patch --raw" > behaves differently from just "--raw". It turns out that there are > a few interesting bugs in the implementation and documentation. > > * First, the documentation for "--no-patch" was unclear that it > could be read to mean "--no-patch" countermands an earlier > "--patch" but not other things. The intention of "--no-patch" > ever since it was introduced at d09cd15d (diff: allow --no-patch > as synonym for -s, 2013-07-16) was to serve as a synonym for > "-s", so "--raw --patch --no-patch" should have produced no > output, but it can be (mis)read to allow showing only "--raw" > output. I would say that is orthogonal. > * Then the interaction between "-s" and other format options were > poorly implemented. Modern versions of Git uses one bit each to > represent formatting options like "--patch", "--stat" in a single > output_format word, but for historical reasons, "-s" also is > represented as another bit in the same word. This allows two > interesting bugs to happen, and we have both X-<. > > (1) After setting a format bit, then setting NO_OUTPUT with "-s", > the code to process another "--<format>" option drops the > NO_OUTPUT bit to allow output to be shown again. However, > the code to handle "-s" only set NO_OUTPUT without unsetting s/only set/only sets/ > format bits set earlier, so the earlier format bit got > revealed upon seeing the second "--<format>" option. This is > the problem Sergey observed. > > (2) After setting NO_OUTPUT with "-s", code to process s/code/the code/ > "--<format>" option can forget to unset NO_OUTPUT, leaving > the command still silent. > It is tempting to change the meaning of "--no-patch" to mean > "disable only the patch format output" and reimplement "-s" as "not > showing anything", but it would be an end-user visible change in > behavior. Yes, it would be a change in behavior from what no reasonable user would expect, to what most reasonable users would expct. These are synonyms: 1.a. git diff --patch-with-raw 1.b. git diff --patch --raw And so should these: 2.a. git diff --raw 2.b. git diff --no-patch --raw But who on Earth would then think these are different? 2.b. git diff --no-patch --raw 2.c. git diff --raw --no-patch Your patch is *already* an end-user visible change in behavior, so why not do the end-user visible change in behavior that reasonable users would expect? > Let's fix the interactions of these bits to first make "-s" work as > intended. Is it though? > The fix is conceptually very simple. > > * Whenever we set DIFF_FORMAT_FOO because we saw the "--foo" > option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is > given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to > do so in some of the options and caused (2) above. > > * When processing "-s" option, we should not just set > DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. > We didn't do so and retained format bits set by options > previously seen, causing (1) above. > > It is even more tempting to lose NO_OUTPUT bit and instead take > output_format word being 0 as its replacement, but that would break > the mechanism "git show" uses to default to "--patch" output, where > the distinction between telling the command to be silent with "-s" > and having no output format specified on the command line matters, > and an explicit output format given on the command line should not > be "combined" with the default "--patch" format. That's because the logic is not correct, the default should not be 0, the default should be a different value, for example DIFF_FORMAT_DEFAULT, that way each tool can update DIFF_FORMAT_DEFAULT to whatever default is desired. Then 0 doesn't mean default, it means NO_OUTPUT, and then removing all the formats--including DIFF_FORMAT_DEFAULT--makes it clear what the user intends to do. > So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we > may want to replace it with OPTION_GIVEN bit, and > > * make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and > DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw", > etc. will set off DIFF_FORMAT_$format bit but still record the > fact that we saw an option from the command line by setting > DIFF_FORMAT_OPTION_GIVEN bit. > > * make "-s" (and its synonym "--no-patch") clear all other bits > and set only the DIFF_FORMAT_OPTION_GIVEN bit on. > > which I suspect would make the code much cleaner without breaking > any end-user expectations. Why DIFF_FORMAT_OPTION_GIVEN? DIFF_FORMAT_DEFAULT as the opposite is much more understandable. > Once that is in place, transitioning "--no-patch" to mean the > counterpart of "--patch", just like "--no-raw" only defeats an > earlier "--raw", would be quite simple at the code level. It's not only simple, it's a no-op, as (~DIFF_FORMAT_PATCH | DIFF_FORMAT_OPTION_GIVEN) becomes indistinguishible from DIFF_FORMAT_NO_OUTPUT unless another optin like DIFF_FORMAT_RAW is specified. I'm sending a patch series that shows that to be the case. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-09 0:38 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Felipe Contreras @ 2023-05-09 1:22 ` Junio C Hamano 2023-05-09 3:50 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-09 1:22 UTC (permalink / raw) To: Felipe Contreras; +Cc: git Felipe Contreras <felipe.contreras@gmail.com> writes: >> Let's fix the interactions of these bits to first make "-s" work as >> intended. > > Is it though? Yes. If the proposed log message says "as intended", the author thinks it is. Throwing a rhetorical question and stopping at that is not useful; you'd need to explain yourself if you think differently. Unless the only effect you want is to be argumentative and annoy others, that is. I've dug the history and as I explained elsewhere in the earlier discussion, I know that the "--no-patch" originally was added as a synonym for "-s" that makes the output from the diff machinery silent---I have a good reason to believe that it is making "-s" and "--no-patch" both work as intended. I would not say that we should *not* move further with a follow up topic, but I think we should consider doing so only after the dust settles from this round. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-09 1:22 ` Junio C Hamano @ 2023-05-09 3:50 ` Felipe Contreras 2023-05-10 4:26 ` Junio C Hamano 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 3:50 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras; +Cc: git Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> Let's fix the interactions of these bits to first make "-s" work as > >> intended. > > > > Is it though? > > Yes. > > If the proposed log message says "as intended", the author thinks it > is. The question is not if the author of the patch thinks this is the way `-s` is intended to work, the question is if this is the way `-s` is intended to work. The way `-s` is intended to work is completely independent of what the author of the patch thinks, as `-s` existed well before this patch. A cursory search for `-s` in diff-tree.c shows: Author: Linus Torvalds <torvalds@ppc970.osdl.org> Date: Fri May 6 10:56:35 2005 -0700 git-diff-tree: clean up output This only shows the tree headers when something actually changed. Also, add a "silent" mode, which doesn't actually show the changes at all, just the commit information. So presumably the original author of `-s` intended for it to not show any changes at all, but that was before any of the non-patch options were introduced. So, 18 years later: what is the intention behind `-s`? > Throwing a rhetorical question and stopping at that is not > useful; Who says this is a rhetorical question? `-s` was introduced 18 years ago, before any of the non-patch options were introduced. I do not think the intention behind `-s` in 2023 is clear at all, and the patch does not attempt to answer that. > Unless the only effect you want is to be argumentative and annoy > others, that is. Assume good faith: https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith > I've dug the history and as I explained elsewhere in the earlier > discussion, I know that the "--no-patch" originally was added as a > synonym for "-s" that makes the output from the diff machinery > silent---I have a good reason to believe that it is making "-s" and > "--no-patch" both work as intended. I don't think so. `-s` might have been added to make all the diff machinery silent, but `--no-patch` is a different question, as the commit message of d09cd15d makes abundantly clear: diff: allow --no-patch as synonym for -s This follows the usual convention of having a --no-foo option to negate --foo. Now we know `-s` is not an antonym of `--patch`, so the commit message of d09cd15d cannot possibly be correct. There's only three options now: 1. `-s` doesn't turn all the diff machinery silent, only --patch 2. `--no-patch` is decoupled from `--patch` 3. `--no-patch` is decoupled from `-s` I don't think think there's any other reasonable option, including the status quo. > I would not say that we should *not* move further with a follow up > topic, but I think we should consider doing so only after the dust > settles from this round. But what is that dust? Do you agree with the following? 1. No reasonable user would consider the status quo to be expected. 2. Any change to the status quo would incur in backwards-incompatible changes for end-users. If so, the only question remaining is what backwards-incompatible changes shall be implemented. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-09 3:50 ` Felipe Contreras @ 2023-05-10 4:26 ` Junio C Hamano 2023-05-10 23:16 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-10 4:26 UTC (permalink / raw) To: Felipe Contreras; +Cc: git Felipe Contreras <felipe.contreras@gmail.com> writes: >> > Is it though? >> >> Yes. >> >> If the proposed log message says "as intended", the author thinks it >> is. > > The question is not if the author of the patch thinks this is the way > `-s` is intended to work, the question is if this is the way `-s` is > intended to work. The "author" refers to the author of the "proposed log message" of the patch in question, i.e. me in this case. The author of the patch under discussion thinks it is, so asking "Is it?", implying you do not agree, is nothing but a rhetorical question, and doing so, without explaining why, wastes time on both sides. I am not interested in getting involved in unproductive arguments with you (or with anybody else for that matter). I've been giving you benefit of doubt, but I'll go back to refrain from responding to your message, unless it is a patch that I can say "I agree 100% with what the proposed log message says and what the patch text does, looking great, thanks. Will queue." to, which has been my default stance. Past experience tells me that to any review other than "100% good", I would see responses in an unpleasant and hostile manner. Anything that asks clarification for something unclear in your patch, or suggests alternatives or improvements. And it led to unproductive and irritating waste of time number of times, and eventually you were asked to leave the development community for at least a few times. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-10 4:26 ` Junio C Hamano @ 2023-05-10 23:16 ` Felipe Contreras 2023-05-10 23:41 ` Felipe Contreras 2023-05-11 1:50 ` Junio C Hamano 0 siblings, 2 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-10 23:16 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras; +Cc: git Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> > Is it though? > >> > >> Yes. > >> > >> If the proposed log message says "as intended", the author thinks it > >> is. > > > > The question is not if the author of the patch thinks this is the way > > `-s` is intended to work, the question is if this is the way `-s` is > > intended to work. > > The "author" refers to the author of the "proposed log message" of > the patch in question, i.e. me in this case. The author of the > patch under discussion thinks it is, so asking "Is it?", This is the full quote: ==== Let's fix the interactions of these bits to first make "-s" work as intended. ==== If instead you meant this: ==== Let's fix the interactions of these bits to first make "-s" work as I intend. ==== Then that's not a rationale, you are essentially saying "let's do X because I want". > I am not interested in getting involved in unproductive arguments with you > (or with anybody else for that matter). This is the way the review process works and all git developers have to go trough it. We all have to convince others our proposed change is desirable. Your patch is implementing a backwards-incompatible change: git diff -s --raw master That command used not produce any output and after your patch it now produces output. Your commit message does not provide a rationale as to why *we* want to implement this backwards-incompatible change. "This is the way *I* intend `-s` to work" is not a rationale. > And it led to unproductive and irritating waste of time number of times, and > eventually you were asked to leave the development community for at least a > few times. That is blatantly false. As a member of Git's Project Leadership Committee, you should know precisely how many times the committee has excercised this power, and it hasn't been "a few times", it has been one time. And this is a smoke screen: your commit message still doesn't provide any rationale as to why `-s` should work the way *you* intend. Throwing personal attacks at a reviewer for merely pointing out an issue in the commit message is far from productive. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-10 23:16 ` Felipe Contreras @ 2023-05-10 23:41 ` Felipe Contreras 2023-05-11 1:25 ` Jeff King 2023-05-11 1:50 ` Junio C Hamano 1 sibling, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2023-05-10 23:41 UTC (permalink / raw) To: Felipe Contreras, Junio C Hamano, Felipe Contreras; +Cc: git Felipe Contreras wrote: > Junio C Hamano wrote: > > And it led to unproductive and irritating waste of time number of times, and > > eventually you were asked to leave the development community for at least a > > few times. > > That is blatantly false. As a member of Git's Project Leadership Committee, you > should know precisely how many times the committee has excercised this power, > and it hasn't been "a few times", it has been one time. And for the record: that one time I was asked by the committee to not interact with certain members of the community for a few months. The amount of times I was asked to "leave the development community" is *zero*. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-10 23:41 ` Felipe Contreras @ 2023-05-11 1:25 ` Jeff King 2023-05-13 3:07 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Jeff King @ 2023-05-11 1:25 UTC (permalink / raw) To: Felipe Contreras; +Cc: Junio C Hamano, git On Wed, May 10, 2023 at 05:41:57PM -0600, Felipe Contreras wrote: > Felipe Contreras wrote: > > Junio C Hamano wrote: > > > > And it led to unproductive and irritating waste of time number of times, and > > > eventually you were asked to leave the development community for at least a > > > few times. > > > > That is blatantly false. As a member of Git's Project Leadership Committee, you > > should know precisely how many times the committee has excercised this power, > > and it hasn't been "a few times", it has been one time. > > And for the record: that one time I was asked by the committee to not interact > with certain members of the community for a few months. > > The amount of times I was asked to "leave the development community" is *zero*. You're right, in the sense that the first time you were asked to leave we did not have a CoC, and nor was the PLC expected to be part of such conversations at that time. Likewise, many times during which your behavior has been a problem on the list, people did not ask you to leave, but simply said "I am not going to read your messages anymore". For example, here's Junio asking you to leave in 2013: https://lore.kernel.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/ Here's him explaining a few months later why your patches aren't getting reviewed: https://lore.kernel.org/git/xmqqtxgjg35a.fsf@gitster.dls.corp.google.com/ Here's me addressing complaints about your behavior half a year after that: https://lore.kernel.org/git/20140514202646.GE2715@sigill.intra.peff.net/ That last one has some gmane links; if anyone truly wants to follow them (and I don't recommend that as being worth your time), the lore equivalents are: https://lore.kernel.org/git/20140425191236.GA31637@sigill.intra.peff.net/ https://lore.kernel.org/git/480ACEB0-7629-44DF-805F-E9543E66241B@quendi.de/ https://lore.kernel.org/git/7vfvl0htys.fsf@alter.siamese.dyndns.org/ https://lore.kernel.org/git/20140502223612.GA11374@sigill.intra.peff.net/ I'm sure you will find reason to argue with all of that. But I think the spirit of "it led to an unproductive and irritating waste of time a number of times" is accurate. And this thread is one more example. You can feel free to respond if you want to; I'm not planning to participate further in this thread (and in case you were not aware, I'm not on the PLC any more). I just didn't want Junio to think he was alone in his view of the situation. -Peff ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-11 1:25 ` Jeff King @ 2023-05-13 3:07 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-13 3:07 UTC (permalink / raw) To: Jeff King, Felipe Contreras; +Cc: Junio C Hamano, git Jeff King wrote: > On Wed, May 10, 2023 at 05:41:57PM -0600, Felipe Contreras wrote: > > Felipe Contreras wrote: > > > Junio C Hamano wrote: > > > > And it led to unproductive and irritating waste of time number of times, and > > > > eventually you were asked to leave the development community for at least a > > > > few times. > > > > > > That is blatantly false. As a member of Git's Project Leadership Committee, you > > > should know precisely how many times the committee has excercised this power, > > > and it hasn't been "a few times", it has been one time. > > > > And for the record: that one time I was asked by the committee to not interact > > with certain members of the community for a few months. > > > > The amount of times I was asked to "leave the development community" is *zero*. > > You're right, in the sense that the first time you were asked to leave > we did not have a CoC, That is is once again: *false*. The git community has *never* asked me to leave. > Likewise, many times during which your behavior has been a problem on the > list, False: it was not a problem on the list, it was a problem *for some people* on the list. > people did not ask you to leave, but simply said "I am not going to read your > messages anymore". Yes, and for every person who has said "I am not going to read your messages" publicly, I received a response saying "thank you for saying what we are all thinking but cannot say aloud for fear of reprisals" privately. You do understand that people have different opinions? Some people hate Donald Trump, some people don't. And some people cannot express their honest opinion at $dayjob. Having a different opinion is OK. And the foundation of a functioning civilized democracy is to tolerate the opinions of others. > For example, here's Junio asking you to leave in 2013: > > https://lore.kernel.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/ Read the thread. My objective was to show that the code organization was wrong, and libgit.a was not an actual library. If there ever was any hope of having an actual libgit library, the code needed to be reorganized: ==== The plan is simple; make libgit.a a proper library, starting by clarifying what goes into libgit.a, and what doesn't. If there's any hopes of ever having a public library, it's clear what code doesn't belong in libgit.a; code that is meant for builtins, that code belongs in builtins/lib.a, or similar. ==== Felipe Contreras: [1] The whole libification project of Google proves I was right: git was not (and is not) ready to be a library. libgit.a is not nearly close to be an actual standalone library. Pretty far from it. Just today Elijah Newren sent a 27-patch series [2] attempting to move in the right direction, but doesn't even begin to tip the scales to make libgit.so possible. My proposal did receive positive feedback: ==== Nice joke patch to illustrate your point ;) ==== Ramkumar Ramachandra: [3] ==== This is a good example: yes, I'm convinced that the code does need to be reorganized. ==== Ramkumar Ramachandra: [4] Even you yourself provided useful positive feedback based on my proposal: ==== If we want to start caring, then we probably need to create a separate "kitchen sink"-like library, with the rule that things in libgit.a cannot depend on it. In other words, a support library for Git's commands, for the parts that are not appropriate to expose as part of a library API. ==== Jeff King: [5] Junio also provided good feedback initially: ==== Another thing to think about is looking at pieces of data and functions defined in each *.o files and moving things around within them. For example, looking at the dependency chain I quoted earlier for sequencer.o to build upload-pack, which is about responding to "git fetch" on the sending side: ... It is already crazy. There is no reason for the pack sender to be linking with the sequencer interpreter machinery. If the function definition (and possibly other ones) are split into separate source files (still in libgit.a), git-upload-pack binary does not have to pull in the whole sequencer.c at all. ==== Junio C Hamano: [6] Things started to turn south when I expressed the following opinion: ==== But init_copy_notes_for_rewrite() can *not* be used by anything other than git builtins. Standalone binaries will never use such a function, therefore it doesn't belong in libgit.a. Another example is alias_lookup(). They belong in builtin/lib.a. ==== Felipe Contreras: [7] Junio asked me for an example of a function that would not belong to libgit.so, and I said `init_copy_notes_for_rewrite()` is an example of a function that nothing outside the `git` binary would need. ==== But that is not a good justification for closing door to others that come later who may want to have a standalone that would want to use it. Think about rewriting filter-branch.sh in C but not as a built-in, for example. ==== Junio C Hamano: [8] I argued nobody would actually do that, and I was right, as eventually filter-branch.sh was rewritten in C, but as a builtin, as I said it would. Junio then argued that there was no justification for my claim that certain functions would only be used by git builtins, and therefore they should not belong in a libgit.so library: ==== >> You still haven't justified why we have to _forbid_ any outside >> callers from calling copy_notes_for_rewrite(). > > Because only builtins _should_ use it. And there is no justification behind that "_should_" claim; you are not making any technical argument to explain it. ==== Junio C Hamano: [9] Google's libification project proves I was right: some functions should not belong in libgit.a. If Junio had listened to me back in 2013, the changes Google developers are working on now to make libgit.a something that remotely resembles an actual library would not be as monumental as they are in 2023. Instead of considering my argument, Junio chose to attack me personally: ==== I do not see a point in continuing to discuss this (or any design level issues) with you. You seem to go into a wrong direction to break the design of the overall system, not in a direction to improve anything. I do not know, and at this point I do not care, if you are doing so deliberately to sabotage Git. Just stop. ==== Junio C Hamano: [9] Even if Junio's opinion was the correct one (it's not: as Google's libification project proves), it's not OK to personally attack a contributor merely for expressing an opinion that happens to differ from that of the maintainer. I am entitled to have my own opinion. I already know what you are going to argue back: you are going to argue that Google's libification project is different from my argument, but it's not: Emily Shaffer's introductory mail explained the same thing: ==== In other words, for some modules which already have clear boundaries inside of Git - like config.[ch], strbuf.[ch], etc. - we want to remove some implicit dependencies, like references to globals, and make explicit other dependencies, like references to other modules within Git. ==== Emily Shaffer: [10] Google developers clearly believe the boundaries between "modules" are not clear, and they should be. Which is *exactly* what I argued back in 2013. You say Junio asked me to leave, but you conveniently avoid explaining *why*: because he didn't like my opinion. Junio was not content with simply saying "let's agree to disagree", he threw yet another personal attack: ==== So I do not think this is not even a bikeshedding. Just one side being right, and the other side continuing to repeat nonsense without listening. ==== Junio C Hamano: [11] And then: ==== But what followed was a nonsense, which ended up wastign everybody's time: ==== Junio C Hamano: [12] This breaks the current code of conduct, as it clearly is a behavior that is not: * Demonstrating empathy and kindness toward other people * Being respectful of differing opinions, viewpoints, and experiences This is what I objectively did *not* do in that thread: * Denigrate the opinions of others * Personally attack anybody It was Junio the one who did that, not me. Junio asked me to leave because I expressed an *opinion* he did not like. Junio asked me to leave because I said in my opinion `copy_notes_for_rewrite()` does not belong in libgit.a, because only git builtins should use it. That's it. I think it's incredibly deceitful of you to claim "Junio asked you to leave" and provide a link, without explaining *why*. Fast-forward to 2023, and Google developers are using the same language as I did in 2013: ==== Strbuf is a widely used basic structure that should only interact with other primitives in strbuf.[ch]. ==== Calvin Wan: [13] Is Junio asking them to leave the project for merely daring to express an opinion about what *should* be the direction the Git project takes? Of course not. Ironically, the link you shared is a perfect example the double standards of the Git project, in which a normative statement from a Google employee is par for the course, but a normative statement from an unaffiliated contributor (i.e. me) is complete heresy. All of this is of course, nothing more than a smoke screen from the topic at hand. --- This is the topic: Subject: Re: [PATCH v2] diff: fix interaction between the "-s" option and other options All that matters here is this: 1. Apply Junio's patch 2. Run this command `git diff -s --raw @~` Does the command produce the same output before and after the patch? Yes or no. That is it. Stop dragging personal drama between Junio and me from 2013 in which nobody else participated--including you--and answer the *only* relevant question in this thread. Does Junio's patch change the current behavior? a. Yes b. No Cheers. [1] https://lore.kernel.org/git/CAMP44s0cozMsTo7KQAjnqkqmvMwMw9D3SZrVxg48MOXkH9UQJQ@mail.gmail.com/ [2] https://lore.kernel.org/git/pull.1525.v2.git.1683875068.gitgitgadget@gmail.com/ [3] https://lore.kernel.org/git/CALkWK0mA7MXQv1k5bFpZLARDOHxU5kzKFXzcyUfb6NLZZY-=FA@mail.gmail.com/ [4] https://lore.kernel.org/git/CALkWK0=7PRndNc7XQ-PCPbVCp9vck909bA561JhQG6uXXj1n4g@mail.gmail.com/ [5] https://lore.kernel.org/git/20130610220627.GB28345@sigill.intra.peff.net/ [6] https://lore.kernel.org/git/7vwqq1ct0g.fsf@alter.siamese.dyndns.org/ [7] https://lore.kernel.org/git/CAMP44s03iXPVnunBdFT8etvZ-ew-D15A7mCV3wAAFXMNCpRAgA@mail.gmail.com/ [8] https://lore.kernel.org/git/7vppvsbkc3.fsf@alter.siamese.dyndns.org/ [9] https://lore.kernel.org/git/7vobbca1sr.fsf@alter.siamese.dyndns.org/ [10] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/ [11] https://lore.kernel.org/git/7vehc8a05n.fsf@alter.siamese.dyndns.org/ [12] https://lore.kernel.org/git/7vzjuv14ir.fsf@alter.siamese.dyndns.org/ [13] https://lore.kernel.org/git/20230503184849.1809304-1-calvinwan@google.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-10 23:16 ` Felipe Contreras 2023-05-10 23:41 ` Felipe Contreras @ 2023-05-11 1:50 ` Junio C Hamano 2023-05-13 5:32 ` Felipe Contreras 1 sibling, 1 reply; 42+ messages in thread From: Junio C Hamano @ 2023-05-11 1:50 UTC (permalink / raw) To: Felipe Contreras; +Cc: git Felipe Contreras <felipe.contreras@gmail.com> writes: >> The "author" refers to the author of the "proposed log message" of >> the patch in question, i.e. me in this case. The author of the >> patch under discussion thinks it is, so asking "Is it?", > > This is the full quote: > > ==== > Let's fix the interactions of these bits to first make "-s" work as intended. > ==== > > If instead you meant this: > > ==== > Let's fix the interactions of these bits to first make "-s" work as I intend. > ==== > > Then that's not a rationale, you are essentially saying "let's do X because I > want". This will be the last message from me on this. I wouldn't have even seen the message I am responding to, as I've already done my "once every few days sweep the spam folder to find things to salvage", but somebody notified me of it, so... I didn't say and I didn't mean "as I intend", and you know that. I, the author of the patch under discussion, know that it is the intention of the author of the earlier commit that introduced "--no-patch" to make it work identically as "-s". I even had a quote from that earlier commit in the proposed log message of the patch (look for d09cd15d) to substantiate the fact that it was the intended way for the option "--no-patch" to work. So, either you are arguing against the patch you didn't even read, or you are playing your usual word twisting game just for the sake of arguing. >> And it led to unproductive and irritating waste of time number of times, and >> eventually you were asked to leave the development community for at least a >> few times. > > That is blatantly false. As a member of Git's Project Leadership Committee, you > should know precisely how many times the committee has excercised this power, > and it hasn't been "a few times", it has been one time. You were asked to leave in May 2014, and according to that message from May 2014 [*1*], apparently you were asked to leave after a big "Felipe eruption" in the summer of 2013 [*2*]. These happened long before the project adopted a formal CoC at 5cdf2301 (add a Code of Conduct document, 2019-09-24). But apparently the "fact" does not matter to you. I know that your next excuse will be "I said the committee never exercised this power more than once, which is a FACT", which may let you keep arguing further. [References] *1* https://lore.kernel.org/git/53709788.2050201@alum.mit.edu/ *2* https://public-inbox.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] diff: fix interaction between the "-s" option and other options 2023-05-11 1:50 ` Junio C Hamano @ 2023-05-13 5:32 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-13 5:32 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras; +Cc: git Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > >> The "author" refers to the author of the "proposed log message" of > >> the patch in question, i.e. me in this case. The author of the > >> patch under discussion thinks it is, so asking "Is it?", > > > > This is the full quote: > > > > ==== > > Let's fix the interactions of these bits to first make "-s" work as intended. > > ==== > > > > If instead you meant this: > > > > ==== > > Let's fix the interactions of these bits to first make "-s" work as I intend. > > ==== > > > > Then that's not a rationale, you are essentially saying "let's do X because I > > want". > > This will be the last message from me on this. I wouldn't have even > seen the message I am responding to, as I've already done my "once > every few days sweep the spam folder to find things to salvage", Comment that breaks the code of conduct: * Demonstrating empathy and kindness toward other people * Being respectful of differing opinions, viewpoints, and experiences Is the maintainer exempt from following the code of conduct? > I didn't say and I didn't mean "as I intend", and you know that. No, I don't know that, because I don't make assumptions. You said this: ==== >> Let's fix the interactions of these bits to first make "-s" work as >> intended. > > Is it though? Yes. If the proposed log message says "as intended", the author thinks it is. ==== [1] Since you are "the author", the above directly translates to "I think it is as intended", but I responded directly with: ==== The question is not if the author of the patch thinks this is the way `-s` is intended to work, the question is if this is the way `-s` is intended to work. ==== [2] Which is a perfectly valid response, to which you replied: ==== The "author" refers to the author of the "proposed log message" of the patch in question, i.e. me in this case. The author of the patch under discussion thinks it is, so asking "Is it?", implying you do not agree, is nothing but a rhetorical question, and doing so, without explaining why, wastes time on both sides. ==== [3] This is not a valid response, because the question was never "does the author of the patch think this behavior is intended", the question was "is this behavior intended", and I made that abundantly clear in [2]. So there's only two options: a. This is the behavior you intend, and you meant to say this is the behaviour you intend. b. This is the behavior you think is intended, in which case if you think so or not is irrelevant, instead you need to provide a rationale for why you think that is the case, which you never did. If it is `a`: that's not a rationale. If it is `b`: you still need a rationale. Either way no rationale was provided in the commit message (or anywhere else). You chose to avoid this question and instead throw personal attacks in [3], which is not productive. Fortunately for the project I decided to investigate the whole history behind the true intention behind `-s` in [4]. In that investigation it became exceedingly clear that the intention behind `-s` is different from the intention behind `--no-patch`. And it also became clear that after making `output_format` a bit field: the intention of `-s` became unclear. The culmination of that investigation is the thread in which `--no-patch` was introduced [5]. In that thread Matthieu Moy explained the true purpose was to make it more accessible to silence the output of `git show`. Furthermore, Matthieu Moy happened to respond today, and make it even more clear [6]: ==== Looking more closely, it's rather clear to me they are not, and that git show --raw --patch --no-patch should be equivalent to git show --raw ==== Which is *exactly* what I and Sergey argued, and to repeat and make it unquestionably clear: `--raw --patch --no-patch` should be equivalent to `--raw`. Period. You can throw all the personal attacks you want, but what you think is the intended behavior of `-s` is irrelevant, the fact is that the intended behavior of `--no-patch` is independent from the intended behavior of `-s`. History--and the explicit explanation of the original author--proves that. So, when I asked "is it though?", that wasn't a rhetorical question intended to waste time: the answer is clearly: NO. This is not the way `-s` is intended to work. > >> And it led to unproductive and irritating waste of time number of times, and > >> eventually you were asked to leave the development community for at least a > >> few times. > > > > That is blatantly false. As a member of Git's Project Leadership Committee, you > > should know precisely how many times the committee has excercised this power, > > and it hasn't been "a few times", it has been one time. > > You were asked to leave in May 2014, and according to that message > from May 2014 [*1*], This is the worst kind of misrepresentation. The fact that *one person* said something, doesn't mean *the community* said that. Anybody who is the leader of any organization should understand that the opinion of *one person* is not the same as the opinion of a whole community. And this is--once again--a smoke screen. Whatever one person said in 2014 is totally and completely irrelevant to the topic at hand. --- The commit message of the patch does not explain why the behavior of `-s` should be changed in a backwards-incompatible way. [1] https://lore.kernel.org/git/xmqqsfc62t8y.fsf@gitster.g/ [2] https://lore.kernel.org/git/6459c31038e81_7c68294ee@chronos.notmuch/ [3] https://lore.kernel.org/git/xmqqjzxgzua0.fsf@gitster.g/ [4] https://lore.kernel.org/git/645c5da0981c1_16961a29455@chronos.notmuch/ [5] https://lore.kernel.org/git/1373893639-13413-1-git-send-email-Matthieu.Moy@imag.fr/ [6] https://lore.kernel.org/git/4f713a29-1a34-2f71-ee54-c01020be903a@univ-lyon1.fr/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 18:24 ` Sergey Organov 2023-05-04 20:53 ` Junio C Hamano @ 2023-05-09 1:34 ` Felipe Contreras 2023-05-10 13:54 ` Sergey Organov 1 sibling, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 1:34 UTC (permalink / raw) To: Sergey Organov, Junio C Hamano; +Cc: git Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > > When a command does not behave the way one thinks it should, being > > curious is good. Reporting it as a potential bug is also good. But > > it would help the project more if it was triaged before reporting it > > as a potential bug, if the reporter is capable of doing so. Those > > who encounter behaviour unexpected to them are more numerous than > > those who can report it as a potential bug (many people are not > > equipped to write a good bug report), and those who can triage and > > diagnose a bug report are fewer. Those who can come up with a > > solution is even more scarse. > > I'm afraid the solution I'd come up with won't be welcomed. My solutions are often not welcomed, and yet I still implement them. It might be a waste of time, but often I've found out that very quickly after attempting to come up with a solution I realize there's a lot of detail I was missing initially, so even if the solution is not welcomed, it helps me to understand the problem space and be more helpful in the discussion of potential solutions. So if I were you, I would still attempt to do it, just to gather some understanding. Very often I myself realize the solution I initially thought was the correct one turns out the be completely undoable, and often I need to attempt more than one. If I'm content with a solution, I send it to the mailing list, regardless of the probability of it being merged, because in my view an unmerged patch still provides value, as it creates a record that might be referenced in the future. In fact, quite recently somebody resent a patch of mine that fixes an obvious regression [1]. So even if the maintainer has not merged my patch--and thus it could be said my patch was not "welcomed"--the fact is that it was not welcomed by the maintainer, but it was welcomed by the community. I for one welcome any and all attempts to fix git's awful user interface, regardless of the reception of the maintainer, and the "core club". Cheers. [1] https://lore.kernel.org/git/pull.1499.git.git.1682573243090.gitgitgadget@gmail.com/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-09 1:34 ` [PATCH] t4013: add expected failure for "log --patch --no-patch" Felipe Contreras @ 2023-05-10 13:54 ` Sergey Organov 2023-05-10 21:54 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Sergey Organov @ 2023-05-10 13:54 UTC (permalink / raw) To: Felipe Contreras; +Cc: Junio C Hamano, git Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Junio C Hamano <gitster@pobox.com> writes: > >> > When a command does not behave the way one thinks it should, being >> > curious is good. Reporting it as a potential bug is also good. But >> > it would help the project more if it was triaged before reporting it >> > as a potential bug, if the reporter is capable of doing so. Those >> > who encounter behaviour unexpected to them are more numerous than >> > those who can report it as a potential bug (many people are not >> > equipped to write a good bug report), and those who can triage and >> > diagnose a bug report are fewer. Those who can come up with a >> > solution is even more scarse. >> >> I'm afraid the solution I'd come up with won't be welcomed. > > My solutions are often not welcomed, and yet I still implement them. > > It might be a waste of time, but often I've found out that very quickly > after attempting to come up with a solution I realize there's a lot of > detail I was missing initially, so even if the solution is not welcomed, > it helps me to understand the problem space and be more helpful in the > discussion of potential solutions. > > So if I were you, I would still attempt to do it, just to gather some > understanding. I sympathize, and I did recently. However, I figure I'd rather spend my time elsewhere, say, in the Linux kernel, where my experience is somewhat different, and allows me to enjoy my work. [...] > > I for one welcome any and all attempts to fix git's awful user > interface, regardless of the reception of the maintainer, and the "core > club". For UI, the problem is that there is no core model defined, nor any guidelines are given, so every discussion ends-up being what "makes sense" and what doesn't for a user, everyone involved having his own preference, that often even changes over time. In this situation attempting to fix the UI sounds like waste of efforts, as nobody can actually point at the state of the UI to which we are willing to converge, so there are no objective criteria for accepting of fixup patches. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-10 13:54 ` Sergey Organov @ 2023-05-10 21:54 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-10 21:54 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: Junio C Hamano, git Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > Sergey Organov wrote: > >> Junio C Hamano <gitster@pobox.com> writes: > > > >> > When a command does not behave the way one thinks it should, being > >> > curious is good. Reporting it as a potential bug is also good. But > >> > it would help the project more if it was triaged before reporting it > >> > as a potential bug, if the reporter is capable of doing so. Those > >> > who encounter behaviour unexpected to them are more numerous than > >> > those who can report it as a potential bug (many people are not > >> > equipped to write a good bug report), and those who can triage and > >> > diagnose a bug report are fewer. Those who can come up with a > >> > solution is even more scarse. > >> > >> I'm afraid the solution I'd come up with won't be welcomed. > > > > My solutions are often not welcomed, and yet I still implement them. > > > > It might be a waste of time, but often I've found out that very quickly > > after attempting to come up with a solution I realize there's a lot of > > detail I was missing initially, so even if the solution is not welcomed, > > it helps me to understand the problem space and be more helpful in the > > discussion of potential solutions. > > > > So if I were you, I would still attempt to do it, just to gather some > > understanding. > > I sympathize, and I did recently. However, I figure I'd rather spend my > time elsewhere, say, in the Linux kernel, where my experience is > somewhat different, and allows me to enjoy my work. Completely agree. My experience in the Linux project is that of a true meritocracy: Linus Torvalds doesn't have to like me, if the patch is good, it gets merged. Period. > > I for one welcome any and all attempts to fix git's awful user > > interface, regardless of the reception of the maintainer, and the "core > > club". > > For UI, the problem is that there is no core model defined, nor any > guidelines are given, so every discussion ends-up being what "makes > sense" and what doesn't for a user, everyone involved having his own > preference, that often even changes over time. > > In this situation attempting to fix the UI sounds like waste of efforts, > as nobody can actually point at the state of the UI to which we are > willing to converge, so there are no objective criteria for accepting of > fixup patches. It's even worse than that. There used to be objective criteria like the old Git User's Surveys [1], but it turned out Git developers did not care about the feedback from users, which is why there wasn't any point in continuing them. And worse: even when all Git developers agree on a UI change, except one, it doesn't matter, because that one has absolute veto power. Not very hopeful prospects for Git's UI. Cheers. [1] https://archive.kernel.org/oldwiki/git.wiki.kernel.org/index.php/GitSurvey2016.html -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 15:50 ` Junio C Hamano 2023-05-04 18:24 ` Sergey Organov @ 2023-05-09 1:03 ` Felipe Contreras 1 sibling, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 1:03 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: git Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Sergey Organov <sorganov@gmail.com> writes: > > > >> No problem from my side, but are you sure? > > > > Absolutely. > > > > I've seen people just say "we document a failed one" and leave it at > > that, without attempting to fix. I am trying to see if pushing back > > at first would serve as a good way to encourage these known failure > > to be fixed, without accumulating too many expect_failure in our > > test suite, which will waste cycles at CI runs (which do not need to > > be reminded something is known to be broken). I will try not to do > > this when I do not positively know the author of such a patch is > > capable enough to provide a fix, though, and you are unlucky enough > > to have shown your abilities in the past ;-) > > I ended up spending some time digging history and remembered that > "--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow > --no-patch as synonym for -s, 2013-07-16). These > > git diff -p --stat --no-patch HEAD^ HEAD > git diff -p --raw --no-patch HEAD^ HEAD > > would show no output from the diff machinery, patches, diffstats, > raw object names, etc. > > And this turns out to be a prime example why the approach to ask > contributors do more, would help the project overall. It would also help the project to reward the contributors who actually do more. Otherwise why would a contributor feel incentivized to do more, if that work is simply going to land flat on the ground? > It hopefully would have been "ah, the intent is not documented > correctly, and here is a documentation patch to fix it." That would be assuming that the intent of a developer is all that matters. I disagree. What a reasonable user would expect also matters. > When a command does not behave the way one thinks it should, being > curious is good. Reporting it as a potential bug is also good. But > it would help the project more if it was triaged before reporting it > as a potential bug, if the reporter is capable of doing so. This entirely depends on one's definition of "bug". To me a bug is unexpected behavior. Some people think documenting unexpected behavior makes it not a bug, but to me it's just a documented bug. "It's not a bug, it's a feature!" > Those who encounter behaviour unexpected to them are more numerous > than those who can report it as a potential bug (many people are not > equipped to write a good bug report), Is it just unexpected to them? Or is it unexpected to most users? So what would a reasonable user expect `--no-patch` to do? I think a reasonable user would expect it to negate the effect of `--patch`, and nothing more. The fact that a minority of users expect `--no-patch` to disable all output--not just the one of `--patch`--would not make it not a bug in my book. > Those who can come up with a solution is even more scarse. And those who can come up with a solution that the maintainer deems worthy of merging are way, way scarcer. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-03 16:57 ` Junio C Hamano 2023-05-03 17:31 ` Sergey Organov @ 2023-05-04 18:07 ` Junio C Hamano 2023-05-04 18:26 ` Sergey Organov 2023-05-09 1:07 ` Felipe Contreras 1 sibling, 2 replies; 42+ messages in thread From: Junio C Hamano @ 2023-05-04 18:07 UTC (permalink / raw) To: Sergey Organov; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> --patch followed by --no-patch is to be a no-op according to the "git >> log" manual page. > > I briefly wondered if it is a bug in the documentation. > ... when "git log -p --raw" shows both patch and raw, I do not > think of a reason why "git log -p --raw --no-patch" should not > behave similarly. So, to tie the loose ends, "log -p --raw --no-patch" and "log -p --stat --no-patch" do behave similarly. Where my reaction was mistaken was that I did not read the manual page myself that clearly said it is the same as "-s" that suppresses diff output (where "diff output" is not limited to "patch"---diffstat is also output of "diff"), and incorrectly thought that "--no-patch" would countermand only "--patch" and nothing else. In Documentation/diff-options.txt we have this snippet: -s:: --no-patch:: Suppress diff output. Useful for commands like `git show` that show the patch by default, or to cancel the effect of `--patch`. I imagine that argument could be made that the last half-sentence can be read to say that the option is usable ONLY to cancel the effect of `--patch` without cancelling the effect of anything else. But that smells like a bit of stretch, as "like" in "commands like" is a sign, at least to me, that it gives a few examples without attempting to be exhaustive (meaning that it is too much to read "ONLY" that is not written in "or to cancel the effect of").. Here is my attempt to make it tighter to avoid getting mis-read: Suppress all output from the diff machinery. Useful for commands like `git show` that show the patch by default to squelch their output, or to cancel the effect of options like `--patch`, `--stat` earlier on the command line in an alias. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 18:07 ` Junio C Hamano @ 2023-05-04 18:26 ` Sergey Organov 2023-05-09 1:07 ` Felipe Contreras 1 sibling, 0 replies; 42+ messages in thread From: Sergey Organov @ 2023-05-04 18:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> --patch followed by --no-patch is to be a no-op according to the "git >>> log" manual page. >> >> I briefly wondered if it is a bug in the documentation. >> ... when "git log -p --raw" shows both patch and raw, I do not >> think of a reason why "git log -p --raw --no-patch" should not >> behave similarly. > > So, to tie the loose ends, "log -p --raw --no-patch" and "log -p > --stat --no-patch" do behave similarly. Where my reaction was > mistaken was that I did not read the manual page myself that clearly > said it is the same as "-s" that suppresses diff output (where "diff > output" is not limited to "patch"---diffstat is also output of "diff"), > and incorrectly thought that "--no-patch" would countermand only > "--patch" and nothing else. > > In Documentation/diff-options.txt we have this snippet: > > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > > I imagine that argument could be made that the last half-sentence > can be read to say that the option is usable ONLY to cancel the > effect of `--patch` without cancelling the effect of anything else. > > But that smells like a bit of stretch, as "like" in "commands like" > is a sign, at least to me, that it gives a few examples without > attempting to be exhaustive (meaning that it is too much to read > "ONLY" that is not written in "or to cancel the effect of").. > > Here is my attempt to make it tighter to avoid getting mis-read: > > Suppress all output from the diff machinery. Useful for > commands like `git show` that show the patch by default to > squelch their output, or to cancel the effect of options like > `--patch`, `--stat` earlier on the command line in an alias. This is fine, but is irrelevant to the test-case. Please refer to my answer to your previous reply on the issue for details. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-04 18:07 ` Junio C Hamano 2023-05-04 18:26 ` Sergey Organov @ 2023-05-09 1:07 ` Felipe Contreras 2023-05-10 13:40 ` Sergey Organov 1 sibling, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2023-05-09 1:07 UTC (permalink / raw) To: Junio C Hamano, Sergey Organov; +Cc: git Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Sergey Organov <sorganov@gmail.com> writes: > > > >> --patch followed by --no-patch is to be a no-op according to the "git > >> log" manual page. > > > > I briefly wondered if it is a bug in the documentation. > > ... when "git log -p --raw" shows both patch and raw, I do not > > think of a reason why "git log -p --raw --no-patch" should not > > behave similarly. > > So, to tie the loose ends, "log -p --raw --no-patch" and "log -p > --stat --no-patch" do behave similarly. Where my reaction was > mistaken was that I did not read the manual page myself that clearly > said it is the same as "-s" that suppresses diff output (where "diff > output" is not limited to "patch"---diffstat is also output of "diff"), > and incorrectly thought that "--no-patch" would countermand only > "--patch" and nothing else. If Sergey, you, and me all agreed on what `--no-patch` should do (without reading the manpage), isn't that an indication that that is the expected behavior? The fact that the documentation documents some unexpected behavior, doesn't mean it isn't a bug. I would say it's a documented bug. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-09 1:07 ` Felipe Contreras @ 2023-05-10 13:40 ` Sergey Organov 2023-05-10 21:39 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Sergey Organov @ 2023-05-10 13:40 UTC (permalink / raw) To: Felipe Contreras; +Cc: Junio C Hamano, git Felipe Contreras <felipe.contreras@gmail.com> writes: > Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> > Sergey Organov <sorganov@gmail.com> writes: >> > >> >> --patch followed by --no-patch is to be a no-op according to the "git >> >> log" manual page. >> > >> > I briefly wondered if it is a bug in the documentation. >> > ... when "git log -p --raw" shows both patch and raw, I do not >> > think of a reason why "git log -p --raw --no-patch" should not >> > behave similarly. >> >> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p >> --stat --no-patch" do behave similarly. Where my reaction was >> mistaken was that I did not read the manual page myself that clearly >> said it is the same as "-s" that suppresses diff output (where "diff >> output" is not limited to "patch"---diffstat is also output of "diff"), >> and incorrectly thought that "--no-patch" would countermand only >> "--patch" and nothing else. > > If Sergey, you, and me all agreed on what `--no-patch` should do > (without reading the manpage), isn't that an indication that that is the > expected behavior? > > The fact that the documentation documents some unexpected behavior, > doesn't mean it isn't a bug. > > I would say it's a documented bug. Yep, it is. Chances are this will end-up in the "won't fix" category though, similar to unfortunate '-m'. In which case I think it's better to explicitly mark it in the documentation as such: won't fix. Thanks, -- Sergey Organov ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] t4013: add expected failure for "log --patch --no-patch" 2023-05-10 13:40 ` Sergey Organov @ 2023-05-10 21:39 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2023-05-10 21:39 UTC (permalink / raw) To: Sergey Organov, Felipe Contreras; +Cc: Junio C Hamano, git Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Junio C Hamano wrote: > >> Junio C Hamano <gitster@pobox.com> writes: > >> > Sergey Organov <sorganov@gmail.com> writes: > >> > > >> >> --patch followed by --no-patch is to be a no-op according to the "git > >> >> log" manual page. > >> > > >> > I briefly wondered if it is a bug in the documentation. > >> > ... when "git log -p --raw" shows both patch and raw, I do not > >> > think of a reason why "git log -p --raw --no-patch" should not > >> > behave similarly. > >> > >> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p > >> --stat --no-patch" do behave similarly. Where my reaction was > >> mistaken was that I did not read the manual page myself that clearly > >> said it is the same as "-s" that suppresses diff output (where "diff > >> output" is not limited to "patch"---diffstat is also output of "diff"), > >> and incorrectly thought that "--no-patch" would countermand only > >> "--patch" and nothing else. > > > > If Sergey, you, and me all agreed on what `--no-patch` should do > > (without reading the manpage), isn't that an indication that that is the > > expected behavior? > > > > The fact that the documentation documents some unexpected behavior, > > doesn't mean it isn't a bug. > > > > I would say it's a documented bug. > > Yep, it is. Chances are this will end-up in the "won't fix" category > though, similar to unfortunate '-m'. Probably. > In which case I think it's better to explicitly mark it in the documentation > as such: won't fix. Agreed. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2023-05-13 5:40 UTC | newest] Thread overview: 42+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-03 13:41 [PATCH] t4013: add expected failure for "log --patch --no-patch" Sergey Organov 2023-05-03 16:57 ` Junio C Hamano 2023-05-03 17:31 ` Sergey Organov 2023-05-03 18:07 ` Junio C Hamano 2023-05-03 18:32 ` Felipe Contreras 2023-05-03 19:49 ` Sergey Organov 2023-05-04 15:50 ` Junio C Hamano 2023-05-04 18:24 ` Sergey Organov 2023-05-04 20:53 ` Junio C Hamano 2023-05-04 21:37 ` Re* " Junio C Hamano 2023-05-04 23:10 ` [PATCH] diff: fix behaviour of the "-s" option Junio C Hamano 2023-05-05 5:28 ` Junio C Hamano 2023-05-05 16:51 ` Junio C Hamano 2023-05-09 1:16 ` Felipe Contreras 2023-05-05 8:32 ` Sergey Organov 2023-05-05 16:31 ` Junio C Hamano 2023-05-05 17:07 ` Sergey Organov 2023-05-05 16:59 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Junio C Hamano 2023-05-05 17:41 ` Eric Sunshine 2023-05-05 19:01 ` Junio C Hamano 2023-05-05 21:19 ` [PATCH 0/2] dirstat: leakfix Junio C Hamano 2023-05-05 21:19 ` [PATCH 1/2] diff: refactor common tail part of dirstat computation Junio C Hamano 2023-05-05 21:19 ` [PATCH 2/2] diff: plug leaks in dirstat Junio C Hamano 2023-05-09 0:38 ` [PATCH v2] diff: fix interaction between the "-s" option and other options Felipe Contreras 2023-05-09 1:22 ` Junio C Hamano 2023-05-09 3:50 ` Felipe Contreras 2023-05-10 4:26 ` Junio C Hamano 2023-05-10 23:16 ` Felipe Contreras 2023-05-10 23:41 ` Felipe Contreras 2023-05-11 1:25 ` Jeff King 2023-05-13 3:07 ` Felipe Contreras 2023-05-11 1:50 ` Junio C Hamano 2023-05-13 5:32 ` Felipe Contreras 2023-05-09 1:34 ` [PATCH] t4013: add expected failure for "log --patch --no-patch" Felipe Contreras 2023-05-10 13:54 ` Sergey Organov 2023-05-10 21:54 ` Felipe Contreras 2023-05-09 1:03 ` Felipe Contreras 2023-05-04 18:07 ` Junio C Hamano 2023-05-04 18:26 ` Sergey Organov 2023-05-09 1:07 ` Felipe Contreras 2023-05-10 13:40 ` Sergey Organov 2023-05-10 21:39 ` Felipe Contreras
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).