patcht4013: add expected failure 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 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(+)

Sergey Organov <sorganov@gmail.com> writes:

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.

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.

Junio C Hamano <gitster@pobox.com> writes:

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, -- Sergey Organov

Sergey Organov <sorganov@gmail.com> writes:

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:

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:

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(+)

Junio C Hamano <gitster@pobox.com> writes:

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:

[-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.

Yep, documentation then needs to be fixed as well to match the intention, but this is unrelated to the test-case, see above.

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

Sergey Organov <sorganov@gmail.com> writes:

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.

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).

Yes, you're right.

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?

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 <gitster@pobox.com> writes:

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(-)

Junio C Hamano <gitster@pobox.com> writes:

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(-)

Junio C Hamano <gitster@pobox.com> writes:

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...

Junio C Hamano <gitster@pobox.com> writes:

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...

Junio C Hamano wrote:

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.

Junio C Hamano <gitster@pobox.com> writes:

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

Sergey Organov <sorganov@gmail.com> writes:

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.

Junio C Hamano <gitster@pobox.com> writes:

Yep, I've noticed that post after I sent the question.

Thanks, -- Sergey Organov

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(-)

On Fri, May 5, 2023 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:

Glad to see "THis" from v1 fixed.

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.

s/worth/worthwhile/

And this final part addresses the big question which v1 left dangling (specifically, "why the proposed patch doesn't eliminate NO_OUTPUT altogether). Good.

Eric Sunshine <sunshine@sunshineco.com> writes:

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

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(-)

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(-)

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(-)

Junio C Hamano wrote:

I would say that is orthogonal.

s/only set/only sets/

s/code/the code/

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?

Is it though?

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.

Why DIFF_FORMAT_OPTION_GIVEN? DIFF_FORMAT_DEFAULT as the opposite is much more understandable.

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:

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:

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`?

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.

Assume good faith: https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith

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.

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:

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:

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 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.

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:

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:

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

Jeff King wrote:

That is is once again: *false*.

The git community has *never* asked me to leave.

False: it was not a problem on the list, it was a problem *for some people* on
the list.

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.

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 <felipe.contreras@gmail.com> writes:

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.

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/

Junio C Hamano wrote:

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?

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.

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/

Sergey Organov wrote:

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:

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.

[...]

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:

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.

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

Junio C Hamano wrote:

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?

That would be assuming that the intent of a developer is all that matters.

I disagree.

What a reasonable user would expect also matters.

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!"

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.

And those who can come up with a solution that the maintainer deems worthy of merging are way, way scarcer.

Junio C Hamano <gitster@pobox.com> writes:

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:

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

Junio C Hamano wrote:

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 <felipe.contreras@gmail.com> writes:

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

Sergey Organov wrote:

Probably.

Agreed.