| Message ID | 20230505165952.335256-1-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9d484b92ed38c2222b6880e0bc2b572fdc837dbd |
| Headers | show |
| Series | [v2] diff: fix interaction between the "-s" option and other options | expand |
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>
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.
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 <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.
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 <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.
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 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*.
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
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/
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/
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/
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
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(-)