| Message ID | 20230503134118.73504-1-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t4013: add expected failure for "log --patch --no-patch" | expand |
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 &&
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
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.
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.
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 &&
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.
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.
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
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
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.
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.
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.
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 <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
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
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.
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
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 &&
--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(+)