[PATCH] t4013: add expected failure for "log --patch --no-patch"

[PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

--patch followed by --no-patch is to be a no-op according to the "git
log" manual page. In reality this sequence breaks --raw output
though (and who knows what else?)

Add a test_expected_failure case for the issue.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5de1d190759f..f876b0cc8ec3 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+# This should succeed as --patch followed by --no-patch sequence is to
+# be a no-op according to the manual page. In reality it breaks --raw
+# though. Needs to be fixed.
+test_expect_failure '--no-patch cancels --patch only' '
+	git log --raw master >result &&
+	process_diffs result >expected &&
+	git log --patch --no-patch --raw >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -m matches pure log' '
 	git log master >result &&
 	process_diffs result >expected &&
-- 
2.25.1

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> --patch followed by --no-patch is to be a no-op according to the "git
> log" manual page.

I briefly wondered if it is a bug in the documentation.  But it is
clear (at least to me) that "git log -p --stat --no-patch" wants to
show only "--stat", and when "git log -p --raw" shows both patch and
raw, I do not think of a reason why "git log -p --raw --no-patch"
should not behave similarly.

> Add a test_expected_failure case for the issue.

That is unsatisfactory, though.  Can you back-burner it and send in
a fix with the same test flipping expect_failure to expect_success
instead?

Thanks.

>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  t/t4013-diff-various.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 5de1d190759f..f876b0cc8ec3 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode
>  diff-tree -R --stat --compact-summary initial mode
>  EOF
>  
> +# This should succeed as --patch followed by --no-patch sequence is to
> +# be a no-op according to the manual page. In reality it breaks --raw
> +# though. Needs to be fixed.
> +test_expect_failure '--no-patch cancels --patch only' '
> +	git log --raw master >result &&
> +	process_diffs result >expected &&
> +	git log --patch --no-patch --raw >result &&
> +	process_diffs result >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'log -m matches pure log' '
>  	git log master >result &&
>  	process_diffs result >expected &&

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> --patch followed by --no-patch is to be a no-op according to the "git
>> log" manual page.
>
> I briefly wondered if it is a bug in the documentation.  But it is
> clear (at least to me) that "git log -p --stat --no-patch" wants to
> show only "--stat", and when "git log -p --raw" shows both patch and
> raw, I do not think of a reason why "git log -p --raw --no-patch"
> should not behave similarly.
>
>> Add a test_expected_failure case for the issue.
>
> That is unsatisfactory, though.  Can you back-burner it and send in
> a fix with the same test flipping expect_failure to expect_success
> instead?

No problem from my side, but are you sure?

 - test_expect_failure [<prereq>] <message> <script>

   This is NOT the opposite of test_expect_success, but is used
   to mark a test that demonstrates a known breakage.

Don't we need exactly this in this particular case? Demonstrate a known
breakage?

I'm confused.

>
> Thanks.
>
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  t/t4013-diff-various.sh | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
>> index 5de1d190759f..f876b0cc8ec3 100755
>> --- a/t/t4013-diff-various.sh
>> +++ b/t/t4013-diff-various.sh
>> @@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode
>>  diff-tree -R --stat --compact-summary initial mode
>>  EOF
>>  
>> +# This should succeed as --patch followed by --no-patch sequence is to
>> +# be a no-op according to the manual page. In reality it breaks --raw
>> +# though. Needs to be fixed.
>> +test_expect_failure '--no-patch cancels --patch only' '
>> +	git log --raw master >result &&
>> +	process_diffs result >expected &&
>> +	git log --patch --no-patch --raw >result &&
>> +	process_diffs result >actual &&
>> +	test_cmp expected actual
>> +'
>> +
>>  test_expect_success 'log -m matches pure log' '
>>  	git log master >result &&
>>  	process_diffs result >expected &&

Thanks,
-- Sergey Organov

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> No problem from my side, but are you sure?

Absolutely.

I've seen people just say "we document a failed one" and leave it at
that, without attempting to fix.  I am trying to see if pushing back
at first would serve as a good way to encourage these known failure
to be fixed, without accumulating too many expect_failure in our
test suite, which will waste cycles at CI runs (which do not need to
be reminded something is known to be broken).  I will try not to do
this when I do not positively know the author of such a patch is
capable enough to provide a fix, though, and you are unlucky enough
to have shown your abilities in the past ;-)

Thanks.

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

Junio C Hamano wrote:
> I am trying to see if pushing back at first would serve as a good way
> to encourage these known failure to be fixed, without accumulating too
> many expect_failure in our test suite, which will waste cycles at CI
> runs

> (which do not need to be reminded something is known to be broken)

We don't?

There's plenty of things that are broken in git that people have
forgotten.

If wasting cycles on CI runs is your concern, I would gladly write a
patch to skipp all the test_expect_failure tests.

I would rather have documented all the things are known to be broken
today than not, because in a month that might not be the case.

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> No problem from my side, but are you sure?
>
> Absolutely.
>
> I've seen people just say "we document a failed one" and leave it at
> that, without attempting to fix.  I am trying to see if pushing back
> at first would serve as a good way to encourage these known failure
> to be fixed, without accumulating too many expect_failure in our
> test suite, which will waste cycles at CI runs (which do not need to
> be reminded something is known to be broken).  I will try not to do
> this when I do not positively know the author of such a patch is
> capable enough to provide a fix, though, and you are unlucky enough
> to have shown your abilities in the past ;-)

Thanks for the credit, but as my recent attempts to fix 2 obvious
deficiencies in Git CI (one of them being my own) failed quite
miserably, I figure I have no idea how these things in CI are to be
treated, so I prefer to leave a fix to somebody else, who actually groks
what makes sense in the Git UI, and what doesn't.

That said, in case you still need the test with expect_success, below is
one rerolled.

Thanks,
-- Sergey Organov

--- >8 ---

Subject: [PATCH] t4013: add test for "log --patch --no-patch"

--patch followed by --no-patch is to be a no-op according to the "git
log" manual page. In reality this sequence breaks --raw output
though (and who knows what else?)

Add test case for the issue.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5de1d190759f..32907bf142fc 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -457,6 +457,17 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+# This should succeed as --patch followed by --no-patch sequence is to
+# be a no-op according to the manual page. In reality it breaks --raw
+# though. Needs to be fixed.
+test_expect_success '--no-patch cancels --patch only' '
+	git log --raw master >result &&
+	process_diffs result >expected &&
+	git log --patch --no-patch --raw >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -m matches pure log' '
 	git log master >result &&
 	process_diffs result >expected &&
-- 
2.25.1

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> No problem from my side, but are you sure?
>
> Absolutely.
>
> I've seen people just say "we document a failed one" and leave it at
> that, without attempting to fix.  I am trying to see if pushing back
> at first would serve as a good way to encourage these known failure
> to be fixed, without accumulating too many expect_failure in our
> test suite, which will waste cycles at CI runs (which do not need to
> be reminded something is known to be broken).  I will try not to do
> this when I do not positively know the author of such a patch is
> capable enough to provide a fix, though, and you are unlucky enough
> to have shown your abilities in the past ;-)

I ended up spending some time digging history and remembered that
"--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow
--no-patch as synonym for -s, 2013-07-16).  These

    git diff -p --stat --no-patch HEAD^ HEAD
    git diff -p --raw --no-patch HEAD^ HEAD

would show no output from the diff machinery, patches, diffstats,
raw object names, etc.

And this turns out to be a prime example why the approach to ask
contributors do more, would help the project overall.  What I should
have done, instead of asking for the test with its expect_failure
turned into expect_success *and* a fix to the code to make the new
test work, was to ask to see if it is really a bug in the behaviour
or if the documentation is wrong.  Then your reaction wouldn't have
been "are you sure?".  It hopefully would have been "ah, the intent
is not documented correctly, and here is a documentation patch to
fix it."

When a command does not behave the way one thinks it should, being
curious is good.  Reporting it as a potential bug is also good.  But
it would help the project more if it was triaged before reporting it
as a potential bug, if the reporter is capable of doing so.  Those
who encounter behaviour unexpected to them are more numerous than
those who can report it as a potential bug (many people are not
equipped to write a good bug report), and those who can triage and
diagnose a bug report are fewer.  Those who can come up with a
solution is even more scarse.

Thanks.

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> No problem from my side, but are you sure?
>>
>> Absolutely.
>>
>> I've seen people just say "we document a failed one" and leave it at
>> that, without attempting to fix.  I am trying to see if pushing back
>> at first would serve as a good way to encourage these known failure
>> to be fixed, without accumulating too many expect_failure in our
>> test suite, which will waste cycles at CI runs (which do not need to
>> be reminded something is known to be broken).  I will try not to do
>> this when I do not positively know the author of such a patch is
>> capable enough to provide a fix, though, and you are unlucky enough
>> to have shown your abilities in the past ;-)
>
> I ended up spending some time digging history and remembered that
> "--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow
> --no-patch as synonym for -s, 2013-07-16).  These
>
>     git diff -p --stat --no-patch HEAD^ HEAD
>     git diff -p --raw --no-patch HEAD^ HEAD
>
> would show no output from the diff machinery, patches, diffstats,
> raw object names, etc.

[-s meaning "silent" at that time? If so, making --no-patch a synonym for
"silent", and then documenting -s a synonym for --no-patch sounds like
quite a twitch.]

Anyway, this seems pretty irrelevant to the test case. Even
if we spell --no-patch as -s,

 git diff -s --raw HEAD^ HEAD

should produce what? To me it should be the same as

 git diff --raw HEAD^ HEAD

as -s turns off everything, and then --raw is turned on. In reality this
is not the case though, and that's what the test case is about.

Notice that

 git diff -s --patch

does produce the patch output, whereas

 git diff -s --raw
 git diff -s --stat

produce none. Sounds like nonsense.

> And this turns out to be a prime example why the approach to ask
> contributors do more, would help the project overall. What I should
> have done, instead of asking for the test with its expect_failure
> turned into expect_success *and* a fix to the code to make the new
> test work, was to ask to see if it is really a bug in the behaviour or
> if the documentation is wrong. Then your reaction wouldn't have been
> "are you sure?". It hopefully would have been "ah, the intent is not
> documented correctly, and here is a documentation patch to fix it.

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

> When a command does not behave the way one thinks it should, being
> curious is good.  Reporting it as a potential bug is also good.  But
> it would help the project more if it was triaged before reporting it
> as a potential bug, if the reporter is capable of doing so.  Those
> who encounter behaviour unexpected to them are more numerous than
> those who can report it as a potential bug (many people are not
> equipped to write a good bug report), and those who can triage and
> diagnose a bug report are fewer.  Those who can come up with a
> solution is even more scarse.

I'm afraid the solution I'd come up with won't be welcomed. If I'd start
to "fix" it, it'd be likely set of independent options:

 --patch --no-patch
 --raw   --no-raw
 --stat  --no-stat

and then

 -s being just a shortcut for "--no-raw --no-patch --no-stat"

Easy to understand, simple to implement, straightforward to document,
all intentions are perfectly obvious. But then these are to be new
options to keep backward compatibility, and... No, thanks.

Overall, as I neither able to make sense of the current set of
intentions, nor even figure out what they are in the first place, I'm
not the right person to fix implementation of these intentions, or even
figure out for sure if a fix is needed.

Thanks,
-- Sergey Organov

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> [-s meaning "silent" at that time? If so, making --no-patch a synonym for
> "silent", and then documenting -s a synonym for --no-patch sounds like
> quite a twitch.]

I thought it stood for "squelch", but it turns out that it was
introduced by f4f21ce3 (git-diff-tree: clean up output, 2005-05-06),
as a "silent" mode, way before anything else in the thread has
happened.

In Git v1.8.4, "--no-patch" was added as a synonym for "-s", and
ever since we documented that they are equivalents.

> Anyway, this seems pretty irrelevant to the test case. Even
> if we spell --no-patch as -s,
>
>  git diff -s --raw HEAD^ HEAD
>
> should produce what? To me it should be the same as
>
>  git diff --raw HEAD^ HEAD
>
> as -s turns off everything, and then --raw is turned on.

Ah, yeah, I missed that part of your sample command.  It does seem
quite puzzling, and the bad (or good?) part of the story is that my
fresh build of v1.8.4 behaves exactly the same way.  And with "-s"
we can try versions before v1.8.4 (the only change that version made
was to introduce "--no-patch" as its synonym).  It seems it behaved
that way at least back to v1.5.3 (which happens to be the oldest
version I consider worth comparing with more modern versions).

> Notice that
>
>  git diff -s --patch
>
> does produce the patch output, whereas
>
>  git diff -s --raw
>  git diff -s --stat
>
> produce none.

Yes, you're right.

> Sounds like nonsense.

Sounds like a bug to me.  I wonder how it came about.  Did we forget
to add support of the equivalent of what "-s --patch" does, when we
added "--raw" and "--stat", perhaps?

> I'm afraid the solution I'd come up with won't be welcomed. If I'd start
> to "fix" it, it'd be likely set of independent options:
>
>  --patch --no-patch
>  --raw   --no-raw
>  --stat  --no-stat
>
> and then
>
>  -s being just a shortcut for "--no-raw --no-patch --no-stat"

If I were writing Git from scratch without any existing users, that
would be how I would design it (modulo that I would make sure we
have some mechanism to make it easier for developers who may add
a new output <format> to ensure that "-s" also implies "--no-<format>"
for the new <format> they are adding to the mix).

The fact that this wasn't brought up until now may mean that nobody
would notice if we redefined the definition of "--no-patch" to
behave that way, as long as "-s" keeps its original meaning.  

I dunno.

Thanks.

Re* [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

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

>> ... it'd be likely set of independent options:

>>  --patch --no-patch
>>  --raw   --no-raw
>>  --stat  --no-stat
>>
>> and then
>>
>>  -s being just a shortcut for "--no-raw --no-patch --no-stat"
>
> If I were writing Git from scratch without any existing users, that
> would be how I would design it (modulo that I would make sure we
> have some mechanism to make it easier for developers who may add
> a new output <format> to ensure that "-s" also implies "--no-<format>"
> for the new <format> they are adding to the mix).
>
> The fact that this wasn't brought up until now may mean that nobody
> would notice if we redefined the definition of "--no-patch" to
> behave that way, as long as "-s" keeps its original meaning.  
>
> I dunno.

I haven't run any tests (not just your new one, but existing ones)
but at least "git diff -s --stat" and "git diff -s --raw" do countermand
the earlier "-s" with this patch.  I am not signing it off because I
started from the options[] array in add_diff_options() and tweaked
those I happened to notice, and haven't checked if we need to adjust
other entries in the array.

 diff.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git c/diff.c w/diff.c
index 1e83aaee6b..2d8025a9f7 100644
--- c/diff.c
+++ w/diff.c
@@ -4929,6 +4929,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)
 		BUG("%s should not get here", opt->long_name);
 
 	options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->stat_name_width = name_width;
 	options->stat_graph_width = graph_width;
 	options->stat_width = width;
@@ -4947,6 +4948,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	 * The caller knows a dirstat-related option is given from the command
 	 * line; allow it to say "return this_function();"
 	 */
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->output_format |= DIFF_FORMAT_DIRSTAT;
 	return 1;
 }
@@ -5502,9 +5504,9 @@ struct option *add_diff_options(const struct option *opts,
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
 		OPT_BOOL('W', "function-context", &options->flags.funccontext,
 			 N_("generate diffs with <n> lines context")),
-		OPT_BIT_F(0, "raw", &options->output_format,
+		OPT_BITOP(0, "raw", &options->output_format,
 			  N_("generate the diff in raw format"),
-			  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP(0, "patch-with-raw", &options->output_format,
 			  N_("synonym for '-p --raw'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,
@@ -5513,12 +5515,12 @@ struct option *add_diff_options(const struct option *opts,
 			  N_("synonym for '-p --stat'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,
 			  DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F(0, "numstat", &options->output_format,
+		OPT_BITOP(0, "numstat", &options->output_format,
 			  N_("machine friendly --stat"),
-			  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "shortstat", &options->output_format,
+			  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),
+		OPT_BITOP(0, "shortstat", &options->output_format,
 			  N_("output only the last line of --stat"),
-			  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),
 		OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."),
 			       N_("output the distribution of relative amount of changes for each sub-directory"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,

[PATCH] diff: fix behaviour of the "-s" option

[Junio C Hamano]

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

> I haven't run any tests (not just your new one, but existing ones)
> but ...

And of course, not writing tests fails to even realize that the bug
has two components, "-s" failing to clear the bits previously set,
and other options not clearing the bit set by "-s".

This version may still be rough, but at least the full test suite
has been run with it, so I have a bit more confidence than the
earlier one (which may not mean much).

------- >8 ------------- >8 ------------- >8 -------------
Sergey Organov noticed and reported "--patch --no-patch --raw"
behaves differently from "--raw".  It turns out there are a few
interesting bugs in the implementation and documentation.

 * First, the documentation for "--no-patch" was unclear that it
   could be read to mean "--no-patch" countermands an earlier
   "--patch" but not other things.  The intention of "--no-patch"
   ever since it was introduced at d09cd15d (diff: allow --no-patch
   as synonym for -s, 2013-07-16) was to serve as a synonym for
   "-s", so "--raw --patch --no-patch" should have produced no
   output, but it can be (mis)read to allow showing only "--raw"
   output.

 * Then the interaction between "-s" and other format options were
   poorly implemented.  Modern versions of Git uses one bit each to
   represent formatting options like "--patch", "--stat" in a single
   output_format word, but for historical reasons, "-s" also is
   represented as another bit in the same word.  This allows two
   interesting bugs to happen, and we have both.

   (1) After setting a format bit, then setting NO_OUTPUT with "-s",
       the code to process another "--<format>" option drops the
       NO_OUTPUT bit to allow output to be shown again.  However,
       the code to handle "-s" only set NO_OUTPUT without unsetting
       format bits set earlier, so the earlier format bit got
       revealed upon seeing the second "--<format>" option.  THis is
       the problem Sergey observed.

   (2) After setting NO_OUTPUT with "-s", code to process
       "--<format>" option can forget to unset NO_OUTPUT, leaving
       the command still silent.

It is tempting to change the meaning of "--no-patch" to mean
"disable only the patch format output" and reimplement "-s" as "not
showing anything", but it would be an end-user visible change in
behaviour.  Let's fix the interactions of these bits to first make
"-s" work as intended.

The fix is conceptually very simple.

 * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo"
   option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
   given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to
   do so in some of the options and caused (2) above.

 * When processing "-s" option, we should not just set
   DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
   We didn't do so and retained format bits set by options
   previously seen, causing (1) above.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |  7 +++++--
 diff.c                         | 24 +++++++++++++-----------
 t/t4000-diff-format.sh         | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3674ac48e9..7d5bb65a49 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -29,8 +29,11 @@ endif::git-diff[]
 
 -s::
 --no-patch::
-	Suppress diff output. Useful for commands like `git show` that
-	show the patch by default, or to cancel the effect of `--patch`.
+	Suppress all output from the diff machinery.  Useful for
+	commands like `git show` that show the patch by default to
+	squelch their output, or to cancel the effect of options like
+	`--patch`, `--stat` earlier on the command line in an alias.
+
 endif::git-format-patch[]
 
 ifdef::git-log[]
diff --git a/diff.c b/diff.c
index 648f6717a5..5a2f096683 100644
--- a/diff.c
+++ b/diff.c
@@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)
 	} else
 		BUG("%s should not get here", opt->long_name);
 
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	options->stat_name_width = name_width;
 	options->stat_graph_width = graph_width;
@@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	 * The caller knows a dirstat-related option is given from the command
 	 * line; allow it to say "return this_function();"
 	 */
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->output_format |= DIFF_FORMAT_DIRSTAT;
 	return 1;
 }
@@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt,
 		options->flags.stat_with_summary = 0;
 	} else {
 		options->flags.stat_with_summary = 1;
+		options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	}
 	return 0;
@@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BITOP('p', "patch", &options->output_format,
 			  N_("generate patch"),
 			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F('s', "no-patch", &options->output_format,
-			  N_("suppress diff output"),
-			  DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG),
+		OPT_SET_INT('s', "no-patch", &options->output_format,
+			    N_("suppress diff output"), DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP('u', NULL, &options->output_format,
 			  N_("generate patch"),
 			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
@@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options)
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
 		OPT_BOOL('W', "function-context", &options->flags.funccontext,
 			 N_("generate diffs with <n> lines context")),
-		OPT_BIT_F(0, "raw", &options->output_format,
+		OPT_BITOP(0, "raw", &options->output_format,
 			  N_("generate the diff in raw format"),
-			  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP(0, "patch-with-raw", &options->output_format,
 			  N_("synonym for '-p --raw'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,
@@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options)
 			  N_("synonym for '-p --stat'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,
 			  DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F(0, "numstat", &options->output_format,
+		OPT_BITOP(0, "numstat", &options->output_format,
 			  N_("machine friendly --stat"),
-			  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "shortstat", &options->output_format,
+			  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),
+		OPT_BITOP(0, "shortstat", &options->output_format,
 			  N_("output only the last line of --stat"),
-			  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),
 		OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."),
 			       N_("output the distribution of relative amount of changes for each sub-directory"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
@@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BIT_F(0, "check", &options->output_format,
 			  N_("warn if changes introduce conflict markers or whitespace errors"),
 			  DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "summary", &options->output_format,
+		OPT_BITOP(0, "summary", &options->output_format,
 			  N_("condensed summary such as creations, renames and mode changes"),
-			  DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT),
 		OPT_BIT_F(0, "name-only", &options->output_format,
 			  N_("show only names of changed files"),
 			  DIFF_FORMAT_NAME, PARSE_OPT_NONEG),
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index bfcaae390f..762b9d4c60 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -5,6 +5,9 @@
 
 test_description='Test built-in diff output engine.
 
+We happen to know that all diff plumbing and diff Porcelain share the
+same command line parser, so testing one should be sufficient; pick
+diff-files as a representative.
 '
 
 TEST_PASSES_SANITIZE_LEAK=true
@@ -16,9 +19,11 @@ Line 2
 line 3'
 cat path0 >path1
 chmod +x path1
+mkdir path2
+>path2/path3
 
 test_expect_success 'update-index --add two files with and without +x.' '
-	git update-index --add path0 path1
+	git update-index --add path0 path1 path2/path3
 '
 
 mv path0 path0-
@@ -91,4 +96,29 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 	test_must_be_empty err
 '
 
+
+# Smudge path2/path3 so that dirstat has something to show
+date >path2/path3
+
+for format in stat raw numstat shortstat dirstat
+do
+	test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" '
+		git diff-files --no-patch "--$format" >actual &&
+		git diff-files "--$format" >expect &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "--no-patch clears all previous ones" '
+		git diff-files --$format -s -p >actual &&
+		git diff-files -p >expect &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "--no-patch in 'git diff --no-patch --$format' is a no-op" '
+		git diff --no-patch "--$format" >actual &&
+		git diff "--$format" >expect &&
+		test_cmp expect actual
+	'
+done
+
 test_done
-- 
2.40.1-476-g69c786637d

Re: [PATCH] diff: fix behaviour of the "-s" option

[Junio C Hamano]

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

>  * Then the interaction between "-s" and other format options were
>    poorly implemented.  Modern versions of Git uses one bit each to
>    represent formatting options like "--patch", "--stat" in a single
>    output_format word, but for historical reasons, "-s" also is
>    represented as another bit in the same word.

An obvious improvement strategy is to stop using the NO_OUTPUT bit
and instead make "-s" to clear the "output_format" word, and make
"--[no-]raw", "--[no-]stat", "--[no-]patch", etc. to flip their own
bit in the same "output_format" word.  I think the "historical
reasons" why we did not do that was because we wanted to be able to
do a flexible defaulting.  We may want to say "if no output-format
option is given from the command line, default to "--patch", but
otherwise do not set the "--patch" bit on", for example.
Initializing the "output_format" word with "--patch" bit on would
not work---when "--raw" is given from the command line, we want to
clear that "--patch" bit we set for default and set "--raw" bit on.
We can initialize the "output_format" word to 0, and OR in the bits
for each format option as we process them, and then flip the
"--patch" bit on if "output_format" word is still 0 after command
line parsing is done.  This would almost work, except that it would
make it hard to tell "no command line options" case and "'-s' cleared
all bits" case apart (the former wants to default to "--patch",
while the latter wants to stay "no output"), and it probably was the
reason why we gave an extra NO_OUTPUT bit to the "-s" option.  In
hindsight, the arrangement certainly made other things harder and
prone to unnecessary bugs.

Anyway...

Re: [PATCH] diff: fix behaviour of the "-s" option

[Junio C Hamano]

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

> ...  I think the "historical
> reasons" why we did not do that was because we wanted to be able to
> do a flexible defaulting. ...
> This would almost work, except that it would
> make it hard to tell "no command line options" case and "'-s' cleared
> all bits" case apart (the former wants to default to "--patch",
> while the latter wants to stay "no output"), and it probably was the
> reason why we gave an extra NO_OUTPUT bit to the "-s" option.  In
> hindsight, the arrangement certainly made other things harder and
> prone to unnecessary bugs.
>
> Anyway...

The distinction between the presense of NO_OUTPUT bit and absolutely
empty output_format word indeed is used by "git show", in the
builtin/log.c::show_setup_revisions_tweak() function.

We could lose DIFF_FORMAT_NO_OUTPUT bit, but then we need to replace
it with something else (i.e. DIFF_FORMAT_OPTION_GIVEN bit), and

 * "--patch", "--raw", etc. will set DIFF_FORMAT_$format bit and
   DIFF_FORMAT_OPTION_GIVEN bit on for each format.  "--no-raw", 
   etc. will set off DIFF_FORMAT_$format bit but still record the
   fact that we saw an option from the command line by setting
   DIFF_FORMAT_OPTION_GIVEN bit.

 * "-s" (and its synonym "--no-patch") will set the
   DIFF_FORMAT_OPTION_GIVEN bit on, and clear all other bits.

which I suspect would make the code much cleaner without breaking
any end-user expectations.

Once that is in place, transitioning "--no-patch" to mean the
counterpart of "--patch", just like "--no-raw" only defeats an
earlier "--raw", would be quite simple at the code level.  The
social cost of migrating the end-user expectations might be too
great for it to be worth, but at least the "GIVEN" bit clean-up
alone may be worth it.

Not that I would be starting the process right away...

Re: [PATCH] diff: fix behaviour of the "-s" option

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >  * Then the interaction between "-s" and other format options were
> >    poorly implemented.  Modern versions of Git uses one bit each to
> >    represent formatting options like "--patch", "--stat" in a single
> >    output_format word, but for historical reasons, "-s" also is
> >    represented as another bit in the same word.
> 
> An obvious improvement strategy is to stop using the NO_OUTPUT bit
> and instead make "-s" to clear the "output_format" word, and make
> "--[no-]raw", "--[no-]stat", "--[no-]patch", etc. to flip their own
> bit in the same "output_format" word.  I think the "historical
> reasons" why we did not do that was because we wanted to be able to
> do a flexible defaulting.  We may want to say "if no output-format
> option is given from the command line, default to "--patch", but
> otherwise do not set the "--patch" bit on", for example.
> Initializing the "output_format" word with "--patch" bit on would
> not work---when "--raw" is given from the command line, we want to
> clear that "--patch" bit we set for default and set "--raw" bit on.
> We can initialize the "output_format" word to 0, and OR in the bits
> for each format option as we process them, and then flip the
> "--patch" bit on if "output_format" word is still 0 after command
> line parsing is done.  This would almost work, except that it would
> make it hard to tell "no command line options" case and "'-s' cleared
> all bits" case apart (the former wants to default to "--patch",
> while the latter wants to stay "no output"), and it probably was the
> reason why we gave an extra NO_OUTPUT bit to the "-s" option.  In
> hindsight, the arrangement certainly made other things harder and
> prone to unnecessary bugs.

That's easy to solve by introducing a DIFF_FORMAT_DEFAULT item, which
would be different from 0.

Then every command can update DIFF_FORMAT_DEFAULT to the desired
default, and if the default is cleared (e.g. `--no-patch`) that would
not happen.

-- 
Felipe Contreras

Re: [PATCH] diff: fix behaviour of the "-s" option

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I haven't run any tests (not just your new one, but existing ones)
>> but ...
>
> And of course, not writing tests fails to even realize that the bug
> has two components, "-s" failing to clear the bits previously set,
> and other options not clearing the bit set by "-s".
>
> This version may still be rough, but at least the full test suite
> has been run with it, so I have a bit more confidence than the
> earlier one (which may not mean much).
>
> ------- >8 ------------- >8 ------------- >8 -------------
> Sergey Organov noticed and reported "--patch --no-patch --raw"
> behaves differently from "--raw".  It turns out there are a few
> interesting bugs in the implementation and documentation.
>
>  * First, the documentation for "--no-patch" was unclear that it
>    could be read to mean "--no-patch" countermands an earlier
>    "--patch" but not other things.  The intention of "--no-patch"
>    ever since it was introduced at d09cd15d (diff: allow --no-patch
>    as synonym for -s, 2013-07-16) was to serve as a synonym for
>    "-s", so "--raw --patch --no-patch" should have produced no
>    output, but it can be (mis)read to allow showing only "--raw"
>    output.
>
>  * Then the interaction between "-s" and other format options were
>    poorly implemented.  Modern versions of Git uses one bit each to
>    represent formatting options like "--patch", "--stat" in a single
>    output_format word, but for historical reasons, "-s" also is
>    represented as another bit in the same word.  This allows two
>    interesting bugs to happen, and we have both.
>
>    (1) After setting a format bit, then setting NO_OUTPUT with "-s",
>        the code to process another "--<format>" option drops the
>        NO_OUTPUT bit to allow output to be shown again.  However,
>        the code to handle "-s" only set NO_OUTPUT without unsetting
>        format bits set earlier, so the earlier format bit got
>        revealed upon seeing the second "--<format>" option.  THis is
>        the problem Sergey observed.
>
>    (2) After setting NO_OUTPUT with "-s", code to process
>        "--<format>" option can forget to unset NO_OUTPUT, leaving
>        the command still silent.
>
> It is tempting to change the meaning of "--no-patch" to mean
> "disable only the patch format output" and reimplement "-s" as "not
> showing anything", but it would be an end-user visible change in
> behaviour.  Let's fix the interactions of these bits to first make
> "-s" work as intended.
>
> The fix is conceptually very simple.
>
>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo"
>    option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to
>    do so in some of the options and caused (2) above.
>
>  * When processing "-s" option, we should not just set
>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
>    We didn't do so and retained format bits set by options
>    previously seen, causing (1) above.

Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as
well, I wonder, as absence of any output bits effectively means "no
output"?

Thanks,
-- Sergey Organov

Re: [PATCH] diff: fix behaviour of the "-s" option

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo"
>>    option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
>>    given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to
>>    do so in some of the options and caused (2) above.
>>
>>  * When processing "-s" option, we should not just set
>>    DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
>>    We didn't do so and retained format bits set by options
>>    previously seen, causing (1) above.
>
> Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete as
> well, I wonder, as absence of any output bits effectively means "no
> output"?

Not quite.  The latter is not "set 0 to output_format word", but
"set 0 to output_format word and then flip only NO_OUTPUT bit on".
I've written a bit more on it in a follow-up message to the patch.

Thanks.

Re: [PATCH] diff: fix behaviour of the "-s" option

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>>  * Whenever we set DIFF_FORMAT_FOO becasuse we saw the "--foo"
>>> option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
>>> given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to do
>>> so in some of the options and caused (2) above.
>>> * When processing "-s" option, we should not just set
>>> DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits. We
>>> didn't do so and retained format bits set by options previously
>>> seen, causing (1) above.
>> Sounds good to me. Doesn't this makes DIFF_FORMAT_NO_OUTPUT obsolete
>> as well, I wonder, as absence of any output bits effectively means
>> "no output"?
>
> Not quite. The latter is not "set 0 to output_format word", but "set 0
> to output_format word and then flip only NO_OUTPUT bit on". I've
> written a bit more on it in a follow-up message to the patch.

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

Thanks,
-- Sergey Organov

[PATCH v2] diff: fix interaction between the "-s" option and other options

[Junio C Hamano]

Sergey Organov noticed and reported "--patch --no-patch --raw"
behaves differently from just "--raw".  It turns out that there are
a few interesting bugs in the implementation and documentation.

 * First, the documentation for "--no-patch" was unclear that it
   could be read to mean "--no-patch" countermands an earlier
   "--patch" but not other things.  The intention of "--no-patch"
   ever since it was introduced at d09cd15d (diff: allow --no-patch
   as synonym for -s, 2013-07-16) was to serve as a synonym for
   "-s", so "--raw --patch --no-patch" should have produced no
   output, but it can be (mis)read to allow showing only "--raw"
   output.

 * Then the interaction between "-s" and other format options were
   poorly implemented.  Modern versions of Git uses one bit each to
   represent formatting options like "--patch", "--stat" in a single
   output_format word, but for historical reasons, "-s" also is
   represented as another bit in the same word.  This allows two
   interesting bugs to happen, and we have both X-<.

   (1) After setting a format bit, then setting NO_OUTPUT with "-s",
       the code to process another "--<format>" option drops the
       NO_OUTPUT bit to allow output to be shown again.  However,
       the code to handle "-s" only set NO_OUTPUT without unsetting
       format bits set earlier, so the earlier format bit got
       revealed upon seeing the second "--<format>" option.  This is
       the problem Sergey observed.

   (2) After setting NO_OUTPUT with "-s", code to process
       "--<format>" option can forget to unset NO_OUTPUT, leaving
       the command still silent.

It is tempting to change the meaning of "--no-patch" to mean
"disable only the patch format output" and reimplement "-s" as "not
showing anything", but it would be an end-user visible change in
behavior.  Let's fix the interactions of these bits to first make
"-s" work as intended.

The fix is conceptually very simple.

 * Whenever we set DIFF_FORMAT_FOO because we saw the "--foo"
   option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
   given), we make sure we drop DIFF_FORMAT_NO_OUTPUT.  We forgot to
   do so in some of the options and caused (2) above.

 * When processing "-s" option, we should not just set
   DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
   We didn't do so and retained format bits set by options
   previously seen, causing (1) above.

It is even more tempting to lose NO_OUTPUT bit and instead take
output_format word being 0 as its replacement, but that would break
the mechanism "git show" uses to default to "--patch" output, where
the distinction between telling the command to be silent with "-s"
and having no output format specified on the command line matters,
and an explicit output format given on the command line should not
be "combined" with the default "--patch" format.

So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we
may want to replace it with OPTION_GIVEN bit, and

 * make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and
   DIFF_FORMAT_OPTION_GIVEN bit on for each format.  "--no-raw",
   etc. will set off DIFF_FORMAT_$format bit but still record the
   fact that we saw an option from the command line by setting
   DIFF_FORMAT_OPTION_GIVEN bit.

 * make "-s" (and its synonym "--no-patch") clear all other bits
   and set only the DIFF_FORMAT_OPTION_GIVEN bit on.

which I suspect would make the code much cleaner without breaking
any end-user expectations.

Once that is in place, transitioning "--no-patch" to mean the
counterpart of "--patch", just like "--no-raw" only defeats an
earlier "--raw", would be quite simple at the code level.  The
social cost of migrating the end-user expectations might be too
great for it to be worth, but at least the "GIVEN" bit clean-up
alone may be worth it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |  7 +++++--
 diff.c                         | 24 +++++++++++++-----------
 t/t4000-diff-format.sh         | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3674ac48e9..7d5bb65a49 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -29,8 +29,11 @@ endif::git-diff[]
 
 -s::
 --no-patch::
-	Suppress diff output. Useful for commands like `git show` that
-	show the patch by default, or to cancel the effect of `--patch`.
+	Suppress all output from the diff machinery.  Useful for
+	commands like `git show` that show the patch by default to
+	squelch their output, or to cancel the effect of options like
+	`--patch`, `--stat` earlier on the command line in an alias.
+
 endif::git-format-patch[]
 
 ifdef::git-log[]
diff --git a/diff.c b/diff.c
index 648f6717a5..5a2f096683 100644
--- a/diff.c
+++ b/diff.c
@@ -4868,6 +4868,7 @@ static int diff_opt_stat(const struct option *opt, const char *value, int unset)
 	} else
 		BUG("%s should not get here", opt->long_name);
 
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	options->stat_name_width = name_width;
 	options->stat_graph_width = graph_width;
@@ -4887,6 +4888,7 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	 * The caller knows a dirstat-related option is given from the command
 	 * line; allow it to say "return this_function();"
 	 */
+	options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 	options->output_format |= DIFF_FORMAT_DIRSTAT;
 	return 1;
 }
@@ -5086,6 +5088,7 @@ static int diff_opt_compact_summary(const struct option *opt,
 		options->flags.stat_with_summary = 0;
 	} else {
 		options->flags.stat_with_summary = 1;
+		options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	}
 	return 0;
@@ -5404,9 +5407,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BITOP('p', "patch", &options->output_format,
 			  N_("generate patch"),
 			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F('s', "no-patch", &options->output_format,
-			  N_("suppress diff output"),
-			  DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG),
+		OPT_SET_INT('s', "no-patch", &options->output_format,
+			    N_("suppress diff output"), DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP('u', NULL, &options->output_format,
 			  N_("generate patch"),
 			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
@@ -5415,9 +5417,9 @@ static void prep_parse_options(struct diff_options *options)
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
 		OPT_BOOL('W', "function-context", &options->flags.funccontext,
 			 N_("generate diffs with <n> lines context")),
-		OPT_BIT_F(0, "raw", &options->output_format,
+		OPT_BITOP(0, "raw", &options->output_format,
 			  N_("generate the diff in raw format"),
-			  DIFF_FORMAT_RAW, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_RAW, DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP(0, "patch-with-raw", &options->output_format,
 			  N_("synonym for '-p --raw'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,
@@ -5426,12 +5428,12 @@ static void prep_parse_options(struct diff_options *options)
 			  N_("synonym for '-p --stat'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,
 			  DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F(0, "numstat", &options->output_format,
+		OPT_BITOP(0, "numstat", &options->output_format,
 			  N_("machine friendly --stat"),
-			  DIFF_FORMAT_NUMSTAT, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "shortstat", &options->output_format,
+			  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_NO_OUTPUT),
+		OPT_BITOP(0, "shortstat", &options->output_format,
 			  N_("output only the last line of --stat"),
-			  DIFF_FORMAT_SHORTSTAT, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_SHORTSTAT, DIFF_FORMAT_NO_OUTPUT),
 		OPT_CALLBACK_F('X', "dirstat", options, N_("<param1,param2>..."),
 			       N_("output the distribution of relative amount of changes for each sub-directory"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
@@ -5447,9 +5449,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BIT_F(0, "check", &options->output_format,
 			  N_("warn if changes introduce conflict markers or whitespace errors"),
 			  DIFF_FORMAT_CHECKDIFF, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "summary", &options->output_format,
+		OPT_BITOP(0, "summary", &options->output_format,
 			  N_("condensed summary such as creations, renames and mode changes"),
-			  DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_SUMMARY, DIFF_FORMAT_NO_OUTPUT),
 		OPT_BIT_F(0, "name-only", &options->output_format,
 			  N_("show only names of changed files"),
 			  DIFF_FORMAT_NAME, PARSE_OPT_NONEG),
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index bfcaae390f..8d50331b8c 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -5,6 +5,9 @@
 
 test_description='Test built-in diff output engine.
 
+We happen to know that all diff plumbing and diff Porcelain share the
+same command line parser, so testing one should be sufficient; pick
+diff-files as a representative.
 '
 
 TEST_PASSES_SANITIZE_LEAK=true
@@ -16,9 +19,11 @@ Line 2
 line 3'
 cat path0 >path1
 chmod +x path1
+mkdir path2
+>path2/path3
 
 test_expect_success 'update-index --add two files with and without +x.' '
-	git update-index --add path0 path1
+	git update-index --add path0 path1 path2/path3
 '
 
 mv path0 path0-
@@ -91,4 +96,31 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'
 	test_must_be_empty err
 '
 
+
+# Smudge path2/path3 so that dirstat has something to show
+date >path2/path3
+
+for format in stat raw numstat shortstat summary \
+	dirstat cumulative dirstat-by-file \
+	patch-with-raw patch-with-stat compact-summary
+do
+	test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" '
+		git diff-files --no-patch "--$format" >actual &&
+		git diff-files "--$format" >expect &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "--no-patch clears all previous ones" '
+		git diff-files --$format -s -p >actual &&
+		git diff-files -p >expect &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "--no-patch in 'git diff --no-patch --$format' is a no-op" '
+		git diff --no-patch "--$format" >actual &&
+		git diff "--$format" >expect &&
+		test_cmp expect actual
+	'
+done
+
 test_done
-- 
2.40.1-476-g69c786637d

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Eric Sunshine]

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>

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Junio C Hamano]

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.

[PATCH 0/2] dirstat: leakfix

[Junio C Hamano]

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

>  t/t4000-diff-format.sh         | 34 +++++++++++++++++++++++++++++++++-
> ...
> +for format in stat raw numstat shortstat summary \
> +	dirstat cumulative dirstat-by-file \
> +	patch-with-raw patch-with-stat compact-summary

Unfortunately, because t4000 is marked as passing with leak
sanitizer on, even though this series does not introduce any new
leaks (in fact, there is nothing in the series that allocates pieces
of memory at all), the CI will fail with the sanitizer job.

Needless to say, I hate the current arrangement of these tests.
Those who happen to use tools or features that have nothing to do
with the topic being developed that introduces no new leaks are
punished by a test failure.

Here are a pair of patches that plug leaks in dirstat code.  This
allows the "fix interaction between -s and others" patch that adds
a test that exercises --dirstat in t4000 to be queued without
breaking the leak sanitizer.

Also t4047 that is about dirstat can now be marked as leak free.

Junio C Hamano (2):
  diff: refactor common tail part of dirstat computation
  diff: plug leaks in dirstat

 diff.c                  | 34 ++++++++++++++++++++--------------
 t/t4047-diff-dirstat.sh |  2 ++
 2 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.40.1-476-g69c786637d

[PATCH 1/2] diff: refactor common tail part of dirstat computation

[Junio C Hamano]

This will become useful when we plug leaks in these two functions.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 648f6717a5..e13d0f8b67 100644
--- a/diff.c
+++ b/diff.c
@@ -2971,6 +2971,19 @@ static int dirstat_compare(const void *_a, const void *_b)
 	return strcmp(a->name, b->name);
 }
 
+static void conclude_dirstat(struct diff_options *options,
+			     struct dirstat_dir *dir,
+			     unsigned long changed)
+{
+	/* This can happen even with many files, if everything was renames */
+	if (!changed)
+		return;
+
+	/* Show all directories with more than x% of the changes */
+	QSORT(dir->files, dir->nr, dirstat_compare);
+	gather_dirstat(options, dir, changed, "", 0);
+}
+
 static void show_dirstat(struct diff_options *options)
 {
 	int i;
@@ -3060,13 +3073,7 @@ static void show_dirstat(struct diff_options *options)
 		dir.nr++;
 	}
 
-	/* This can happen even with many files, if everything was renames */
-	if (!changed)
-		return;
-
-	/* Show all directories with more than x% of the changes */
-	QSORT(dir.files, dir.nr, dirstat_compare);
-	gather_dirstat(options, &dir, changed, "", 0);
+	conclude_dirstat(options, &dir, changed);
 }
 
 static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options)
@@ -3104,13 +3111,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
 		dir.nr++;
 	}
 
-	/* This can happen even with many files, if everything was renames */
-	if (!changed)
-		return;
-
-	/* Show all directories with more than x% of the changes */
-	QSORT(dir.files, dir.nr, dirstat_compare);
-	gather_dirstat(options, &dir, changed, "", 0);
+	conclude_dirstat(options, &dir, changed);
 }
 
 static void free_diffstat_file(struct diffstat_file *f)
-- 
2.40.1-476-g69c786637d

[PATCH 2/2] diff: plug leaks in dirstat

[Junio C Hamano]

The array of dirstat_file contained in the dirstat_dir structure is
not freed after the processing ends.  Unfortunately, the member that
points at the array, .files, is incremented as the gather_dirstat()
function recursively walks it, and this needs to be plugged by
remembering the beginning of the array before gather_dirstat() mucks
with it and freeing it after we are done.

We can mark t4047 as leak-free.  t4000, which is marked as
leak-free, now can exercise dirstat in it, which will happen next.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                  | 17 +++++++++++------
 t/t4047-diff-dirstat.sh |  2 ++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index e13d0f8b67..d52db685f7 100644
--- a/diff.c
+++ b/diff.c
@@ -2975,13 +2975,18 @@ static void conclude_dirstat(struct diff_options *options,
 			     struct dirstat_dir *dir,
 			     unsigned long changed)
 {
-	/* This can happen even with many files, if everything was renames */
-	if (!changed)
-		return;
+	struct dirstat_file *to_free = dir->files;
+
+	if (!changed) {
+		/* This can happen even with many files, if everything was renames */
+		;
+	} else {
+		/* Show all directories with more than x% of the changes */
+		QSORT(dir->files, dir->nr, dirstat_compare);
+		gather_dirstat(options, dir, changed, "", 0);
+	}
 
-	/* Show all directories with more than x% of the changes */
-	QSORT(dir->files, dir->nr, dirstat_compare);
-	gather_dirstat(options, dir, changed, "", 0);
+	free(to_free);
 }
 
 static void show_dirstat(struct diff_options *options)
diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index 7fec2cb9cd..70224c3da1 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='diff --dirstat tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # set up two commits where the second commit has these files
-- 
2.40.1-476-g69c786637d

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

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

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Junio C Hamano]

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.

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

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

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Junio C Hamano]

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.

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

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

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

Felipe Contreras wrote:
> Junio C Hamano wrote:

> > And it led to unproductive and irritating waste of time number of times, and
> > eventually you were asked to leave the development community for at least a
> > few times.
> 
> That is blatantly false. As a member of Git's Project Leadership Committee, you
> should know precisely how many times the committee has excercised this power,
> and it hasn't been "a few times", it has been one time.

And for the record: that one time I was asked by the committee to not interact
with certain members of the community for a few months.

The amount of times I was asked to "leave the development community" is *zero*.

-- 
Felipe Contreras

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Jeff King]

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

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

Jeff King wrote:
> On Wed, May 10, 2023 at 05:41:57PM -0600, Felipe Contreras wrote:
> > Felipe Contreras wrote:
> > > Junio C Hamano wrote:

> > > > And it led to unproductive and irritating waste of time number of times, and
> > > > eventually you were asked to leave the development community for at least a
> > > > few times.
> > > 
> > > That is blatantly false. As a member of Git's Project Leadership Committee, you
> > > should know precisely how many times the committee has excercised this power,
> > > and it hasn't been "a few times", it has been one time.
> > 
> > And for the record: that one time I was asked by the committee to not interact
> > with certain members of the community for a few months.
> > 
> > The amount of times I was asked to "leave the development community" is *zero*.
> 
> You're right, in the sense that the first time you were asked to leave
> we did not have a CoC,

That is is once again: *false*.

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

> Likewise, many times during which your behavior has been a problem on the
> list,

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

> people did not ask you to leave, but simply said "I am not going to read your
> messages anymore".

Yes, and for every person who has said "I am not going to read your messages"
publicly, I received a response saying "thank you for saying what we are all
thinking but cannot say aloud for fear of reprisals" privately.

You do understand that people have different opinions? Some people hate Donald
Trump, some people don't. And some people cannot express their honest opinion
at $dayjob.

Having a different opinion is OK. And the foundation of a functioning civilized
democracy is to tolerate the opinions of others.

> For example, here's Junio asking you to leave in 2013:
> 
>   https://lore.kernel.org/git/7vsj0lvs8f.fsf@alter.siamese.dyndns.org/

Read the thread.

My objective was to show that the code organization was wrong, and libgit.a was
not an actual library. If there ever was any hope of having an actual libgit
library, the code needed to be reorganized:

====
  The plan is simple; make libgit.a a proper library, starting by
  clarifying what goes into libgit.a, and what doesn't. If there's any
  hopes of ever having a public library, it's clear what code doesn't
  belong in libgit.a; code that is meant for builtins, that code belongs
  in builtins/lib.a, or similar.
====
Felipe Contreras: [1]

The whole libification project of Google proves I was right: git was not (and
is not) ready to be a library. libgit.a is not nearly close to be an actual
standalone library. Pretty far from it.

Just today Elijah Newren sent a 27-patch series [2] attempting to move in the
right direction, but doesn't even begin to tip the scales to make libgit.so
possible.

My proposal did receive positive feedback:

====
  Nice joke patch to illustrate your point ;)
====
Ramkumar Ramachandra: [3]

====
  This is a good example: yes, I'm convinced that the code does need to
  be reorganized.
====
Ramkumar Ramachandra: [4]

Even you yourself provided useful positive feedback based on my proposal:

====
  If we want to start caring, then we probably need to create a separate
  "kitchen sink"-like library, with the rule that things in libgit.a
  cannot depend on it. In other words, a support library for Git's
  commands, for the parts that are not appropriate to expose as part of a
  library API.
====
Jeff King: [5]

Junio also provided good feedback initially:

====
  Another thing to think about is looking at pieces of data and
  functions defined in each *.o files and moving things around within
  them.  For example, looking at the dependency chain I quoted earlier
  for sequencer.o to build upload-pack, which is about responding to
  "git fetch" on the sending side:

  ...

  It is already crazy. There is no reason for the pack sender to be
  linking with the sequencer interpreter machinery. If the function
  definition (and possibly other ones) are split into separate source
  files (still in libgit.a), git-upload-pack binary does not have to
  pull in the whole sequencer.c at all.
====
Junio C Hamano: [6]

Things started to turn south when I expressed the following opinion:

====
  But init_copy_notes_for_rewrite() can *not* be used by anything other
  than git builtins. Standalone binaries will never use such a function,
  therefore it doesn't belong in libgit.a. Another example is
  alias_lookup(). They belong in builtin/lib.a.
====
Felipe Contreras: [7]

Junio asked me for an example of a function that would not belong to libgit.so,
and I said `init_copy_notes_for_rewrite()` is an example of a function that
nothing outside the `git` binary would need.

====
  But that is not a good justification for closing door to others that
  come later who may want to have a standalone that would want to use
  it.  Think about rewriting filter-branch.sh in C but not as a
  built-in, for example.
====
Junio C Hamano: [8]

I argued nobody would actually do that, and I was right, as eventually
filter-branch.sh was rewritten in C, but as a builtin, as I said it would.

Junio then argued that there was no justification for my claim that certain
functions would only be used by git builtins, and therefore they should not
belong in a libgit.so library:

====
  >> You still haven't justified why we have to _forbid_ any outside
  >> callers from calling copy_notes_for_rewrite().
  >
  > Because only builtins _should_ use it.

  And there is no justification behind that "_should_" claim; you are
  not making any technical argument to explain it.
====
Junio C Hamano: [9]

Google's libification project proves I was right: some functions should not
belong in libgit.a.

If Junio had listened to me back in 2013, the changes Google developers are
working on now to make libgit.a something that remotely resembles an actual
library would not be as monumental as they are in 2023.

Instead of considering my argument, Junio chose to attack me personally:

====
  I do not see a point in continuing to discuss this (or any design
  level issues) with you.  You seem to go into a wrong direction to
  break the design of the overall system, not in a direction to
  improve anything.  I do not know, and at this point I do not care,
  if you are doing so deliberately to sabotage Git.  Just stop.
====
Junio C Hamano: [9]

Even if Junio's opinion was the correct one (it's not: as Google's libification
project proves), it's not OK to personally attack a contributor merely for
expressing an opinion that happens to differ from that of the maintainer.

I am entitled to have my own opinion.

I already know what you are going to argue back: you are going to argue that
Google's libification project is different from my argument, but it's not:
Emily Shaffer's introductory mail explained the same thing:

====
  In other words, for some modules which
  already have clear boundaries inside of Git - like config.[ch],
  strbuf.[ch], etc. - we want to remove some implicit dependencies, like
  references to globals, and make explicit other dependencies, like
  references to other modules within Git.
====
Emily Shaffer: [10]

Google developers clearly believe the boundaries between "modules" are not
clear, and they should be. Which is *exactly* what I argued back in 2013.

You say Junio asked me to leave, but you conveniently avoid explaining *why*:
because he didn't like my opinion.

Junio was not content with simply saying "let's agree to disagree", he threw yet
another personal attack:

====
  So I do not think this is not even a bikeshedding.  Just one side
  being right, and the other side continuing to repeat nonsense
  without listening.
====
Junio C Hamano: [11]

And then:

====
  But what followed was a nonsense, which ended up wastign everybody's
  time:
====
Junio C Hamano: [12]

This breaks the current code of conduct, as it clearly is a behavior that is
not:

 * Demonstrating empathy and kindness toward other people
 * Being respectful of differing opinions, viewpoints, and experiences

This is what I objectively did *not* do in that thread:

 * Denigrate the opinions of others
 * Personally attack anybody

It was Junio the one who did that, not me.

Junio asked me to leave because I expressed an *opinion* he did not like.

Junio asked me to leave because I said in my opinion `copy_notes_for_rewrite()`
does not belong in libgit.a, because only git builtins should use it.

That's it.

I think it's incredibly deceitful of you to claim "Junio asked you to leave"
and provide a link, without explaining *why*.

Fast-forward to 2023, and Google developers are using the same language as I
did in 2013:

====
  Strbuf is a widely used basic structure that should only interact with other
  primitives in strbuf.[ch].
====
Calvin Wan: [13]

Is Junio asking them to leave the project for merely daring to express an
opinion about what *should* be the direction the Git project takes?

Of course not.

Ironically, the link you shared is a perfect example the double standards of
the Git project, in which a normative statement from a Google employee is par
for the course, but a normative statement from an unaffiliated contributor
(i.e. me) is complete heresy.

All of this is of course, nothing more than a smoke screen from the topic at hand.

---

This is the topic:

Subject: Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

All that matters here is this:

 1. Apply Junio's patch
 2. Run this command `git diff -s --raw @~`

Does the command produce the same output before and after the patch? Yes or no.

That is it.

Stop dragging personal drama between Junio and me from 2013 in which nobody
else participated--including you--and answer the *only* relevant question in
this thread.

Does Junio's patch change the current behavior?

 a. Yes
 b. No

Cheers.

[1] https://lore.kernel.org/git/CAMP44s0cozMsTo7KQAjnqkqmvMwMw9D3SZrVxg48MOXkH9UQJQ@mail.gmail.com/
[2] https://lore.kernel.org/git/pull.1525.v2.git.1683875068.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/CALkWK0mA7MXQv1k5bFpZLARDOHxU5kzKFXzcyUfb6NLZZY-=FA@mail.gmail.com/
[4] https://lore.kernel.org/git/CALkWK0=7PRndNc7XQ-PCPbVCp9vck909bA561JhQG6uXXj1n4g@mail.gmail.com/
[5] https://lore.kernel.org/git/20130610220627.GB28345@sigill.intra.peff.net/
[6] https://lore.kernel.org/git/7vwqq1ct0g.fsf@alter.siamese.dyndns.org/
[7] https://lore.kernel.org/git/CAMP44s03iXPVnunBdFT8etvZ-ew-D15A7mCV3wAAFXMNCpRAgA@mail.gmail.com/
[8] https://lore.kernel.org/git/7vppvsbkc3.fsf@alter.siamese.dyndns.org/
[9] https://lore.kernel.org/git/7vobbca1sr.fsf@alter.siamese.dyndns.org/
[10] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/
[11] https://lore.kernel.org/git/7vehc8a05n.fsf@alter.siamese.dyndns.org/
[12] https://lore.kernel.org/git/7vzjuv14ir.fsf@alter.siamese.dyndns.org/
[13] https://lore.kernel.org/git/20230503184849.1809304-1-calvinwan@google.com/

-- 
Felipe Contreras

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Junio C Hamano]

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/

Re: [PATCH v2] diff: fix interaction between the "-s" option and other options

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

> >> The "author" refers to the author of the "proposed log message" of
> >> the patch in question, i.e. me in this case.  The author of the
> >> patch under discussion thinks it is, so asking "Is it?",
> >
> > This is the full quote:
> >
> > ====
> > Let's fix the interactions of these bits to first make "-s" work as intended.
> > ====
> >
> > If instead you meant this:
> >
> > ====
> > Let's fix the interactions of these bits to first make "-s" work as I intend.
> > ====
> >
> > Then that's not a rationale, you are essentially saying "let's do X because I
> > want".
> 
> This will be the last message from me on this.  I wouldn't have even
> seen the message I am responding to, as I've already done my "once
> every few days sweep the spam folder to find things to salvage",

Comment that breaks the code of conduct:

 * Demonstrating empathy and kindness toward other people
 * Being respectful of differing opinions, viewpoints, and experiences

Is the maintainer exempt from following the code of conduct?

> I didn't say and I didn't mean "as I intend", and you know that.

No, I don't know that, because I don't make assumptions.

You said this:

====
  >> Let's fix the interactions of these bits to first make "-s" work as
  >> intended.
  >
  > Is it though?

  Yes.

  If the proposed log message says "as intended", the author thinks it
  is.
====
[1]

Since you are "the author", the above directly translates to "I think it is as
intended", but I responded directly with:

====
  The question is not if the author of the patch thinks this is the way
  `-s` is intended to work, the question is if this is the way `-s` is
  intended to work.
====
[2]

Which is a perfectly valid response, to which you replied:

====
  The "author" refers to the author of the "proposed log message" of
  the patch in question, i.e. me in this case.  The author of the
  patch under discussion thinks it is, so asking "Is it?", implying
  you do not agree, is nothing but a rhetorical question, and doing
  so, without explaining why, wastes time on both sides.
====
[3]

This is not a valid response, because the question was never "does the author
of the patch think this behavior is intended", the question was "is this
behavior intended", and I made that abundantly clear in [2].

So there's only two options:

 a. This is the behavior you intend, and you meant to say this is the
    behaviour you intend.
 b. This is the behavior you think is intended, in which case if you think so
    or not is irrelevant, instead you need to provide a rationale for why
    you think that is the case, which you never did.

If it is `a`: that's not a rationale. If it is `b`: you still need a rationale.
Either way no rationale was provided in the commit message (or anywhere else).

You chose to avoid this question and instead throw personal attacks in [3],
which is not productive.

Fortunately for the project I decided to investigate the whole history behind
the true intention behind `-s` in [4].

In that investigation it became exceedingly clear that the intention behind
`-s` is different from the intention behind `--no-patch`. And it also became
clear that after making `output_format` a bit field: the intention of `-s`
became unclear.

The culmination of that investigation is the thread in which `--no-patch` was
introduced [5]. In that thread Matthieu Moy explained the true purpose was to
make it more accessible to silence the output of `git show`.

Furthermore, Matthieu Moy happened to respond today, and make it even more
clear [6]:

====
  Looking more closely, it's rather clear to me 
  they are not, and that

     git show --raw --patch --no-patch

  should be equivalent to

     git show --raw
====

Which is *exactly* what I and Sergey argued, and to repeat and make it
unquestionably clear:

  `--raw --patch --no-patch` should be equivalent to `--raw`.

Period.

You can throw all the personal attacks you want, but what you think is the
intended behavior of `-s` is irrelevant, the fact is that the intended behavior
of `--no-patch` is independent from the intended behavior of `-s`.

History--and the explicit explanation of the original author--proves that.

So, when I asked "is it though?", that wasn't a rhetorical question intended to
waste time: the answer is clearly: NO.

This is not the way `-s` is intended to work.

> >> And it led to unproductive and irritating waste of time number of times, and
> >> eventually you were asked to leave the development community for at least a
> >> few times.
> >
> > That is blatantly false. As a member of Git's Project Leadership Committee, you
> > should know precisely how many times the committee has excercised this power,
> > and it hasn't been "a few times", it has been one time.
> 
> You were asked to leave in May 2014, and according to that message
> from May 2014 [*1*],

This is the worst kind of misrepresentation.

The fact that *one person* said something, doesn't mean *the community* said that.

Anybody who is the leader of any organization should understand that the
opinion of *one person* is not the same as the opinion of a whole community.

And this is--once again--a smoke screen.

Whatever one person said in 2014 is totally and completely irrelevant to the
topic at hand.

---

The commit message of the patch does not explain why the behavior of `-s`
should be changed in a backwards-incompatible way.

[1] https://lore.kernel.org/git/xmqqsfc62t8y.fsf@gitster.g/
[2] https://lore.kernel.org/git/6459c31038e81_7c68294ee@chronos.notmuch/
[3] https://lore.kernel.org/git/xmqqjzxgzua0.fsf@gitster.g/
[4] https://lore.kernel.org/git/645c5da0981c1_16961a29455@chronos.notmuch/
[5] https://lore.kernel.org/git/1373893639-13413-1-git-send-email-Matthieu.Moy@imag.fr/
[6] https://lore.kernel.org/git/4f713a29-1a34-2f71-ee54-c01020be903a@univ-lyon1.fr/

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

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

> > When a command does not behave the way one thinks it should, being
> > curious is good.  Reporting it as a potential bug is also good.  But
> > it would help the project more if it was triaged before reporting it
> > as a potential bug, if the reporter is capable of doing so.  Those
> > who encounter behaviour unexpected to them are more numerous than
> > those who can report it as a potential bug (many people are not
> > equipped to write a good bug report), and those who can triage and
> > diagnose a bug report are fewer.  Those who can come up with a
> > solution is even more scarse.
> 
> I'm afraid the solution I'd come up with won't be welcomed.

My solutions are often not welcomed, and yet I still implement them.

It might be a waste of time, but often I've found out that very quickly
after attempting to come up with a solution I realize there's a lot of
detail I was missing initially, so even if the solution is not welcomed,
it helps me to understand the problem space and be more helpful in the
discussion of potential solutions.

So if I were you, I would still attempt to do it, just to gather some
understanding.

Very often I myself realize the solution I initially thought was the
correct one turns out the be completely undoable, and often I need to
attempt more than one.

If I'm content with a solution, I send it to the mailing list,
regardless of the probability of it being merged, because in my view an
unmerged patch still provides value, as it creates a record that might
be referenced in the future.

In fact, quite recently somebody resent a patch of mine that fixes an
obvious regression [1]. So even if the maintainer has not merged my
patch--and thus it could be said my patch was not "welcomed"--the fact
is that it was not welcomed by the maintainer, but it was welcomed by
the community.

I for one welcome any and all attempts to fix git's awful user
interface, regardless of the reception of the maintainer, and the "core
club".

Cheers.

[1] https://lore.kernel.org/git/pull.1499.git.git.1682573243090.gitgitgadget@gmail.com/

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Sergey Organov wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>
>> > When a command does not behave the way one thinks it should, being
>> > curious is good.  Reporting it as a potential bug is also good.  But
>> > it would help the project more if it was triaged before reporting it
>> > as a potential bug, if the reporter is capable of doing so.  Those
>> > who encounter behaviour unexpected to them are more numerous than
>> > those who can report it as a potential bug (many people are not
>> > equipped to write a good bug report), and those who can triage and
>> > diagnose a bug report are fewer.  Those who can come up with a
>> > solution is even more scarse.
>>
>> I'm afraid the solution I'd come up with won't be welcomed.
>
> My solutions are often not welcomed, and yet I still implement them.
>
> It might be a waste of time, but often I've found out that very quickly
> after attempting to come up with a solution I realize there's a lot of
> detail I was missing initially, so even if the solution is not welcomed,
> it helps me to understand the problem space and be more helpful in the
> discussion of potential solutions.
>
> So if I were you, I would still attempt to do it, just to gather some
> understanding.

I sympathize, and I did recently. However, I figure I'd rather spend my
time elsewhere, say, in the Linux kernel, where my experience is
somewhat different, and allows me to enjoy my work.

[...]

>
> I for one welcome any and all attempts to fix git's awful user
> interface, regardless of the reception of the maintainer, and the "core
> club".

For UI, the problem is that there is no core model defined, nor any
guidelines are given, so every discussion ends-up being what "makes
sense" and what doesn't for a user, everyone involved having his own
preference, that often even changes over time.

In this situation attempting to fix the UI sounds like waste of efforts,
as nobody can actually point at the state of the UI to which we are
willing to converge, so there are no objective criteria for accepting of
fixup patches.

Thanks,
-- Sergey Organov

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

Sergey Organov wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > Sergey Organov wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >
> >> > When a command does not behave the way one thinks it should, being
> >> > curious is good.  Reporting it as a potential bug is also good.  But
> >> > it would help the project more if it was triaged before reporting it
> >> > as a potential bug, if the reporter is capable of doing so.  Those
> >> > who encounter behaviour unexpected to them are more numerous than
> >> > those who can report it as a potential bug (many people are not
> >> > equipped to write a good bug report), and those who can triage and
> >> > diagnose a bug report are fewer.  Those who can come up with a
> >> > solution is even more scarse.
> >>
> >> I'm afraid the solution I'd come up with won't be welcomed.
> >
> > My solutions are often not welcomed, and yet I still implement them.
> >
> > It might be a waste of time, but often I've found out that very quickly
> > after attempting to come up with a solution I realize there's a lot of
> > detail I was missing initially, so even if the solution is not welcomed,
> > it helps me to understand the problem space and be more helpful in the
> > discussion of potential solutions.
> >
> > So if I were you, I would still attempt to do it, just to gather some
> > understanding.
> 
> I sympathize, and I did recently. However, I figure I'd rather spend my
> time elsewhere, say, in the Linux kernel, where my experience is
> somewhat different, and allows me to enjoy my work.

Completely agree.

My experience in the Linux project is that of a true meritocracy: Linus
Torvalds doesn't have to like me, if the patch is good, it gets merged. Period.

> > I for one welcome any and all attempts to fix git's awful user
> > interface, regardless of the reception of the maintainer, and the "core
> > club".
> 
> For UI, the problem is that there is no core model defined, nor any
> guidelines are given, so every discussion ends-up being what "makes
> sense" and what doesn't for a user, everyone involved having his own
> preference, that often even changes over time.
> 
> In this situation attempting to fix the UI sounds like waste of efforts,
> as nobody can actually point at the state of the UI to which we are
> willing to converge, so there are no objective criteria for accepting of
> fixup patches.

It's even worse than that. There used to be objective criteria like the old Git
User's Surveys [1], but it turned out Git developers did not care about the
feedback from users, which is why there wasn't any point in continuing them.

And worse: even when all Git developers agree on a UI change, except one, it
doesn't matter, because that one has absolute veto power.

Not very hopeful prospects for Git's UI.

Cheers.

[1] https://archive.kernel.org/oldwiki/git.wiki.kernel.org/index.php/GitSurvey2016.html

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Sergey Organov <sorganov@gmail.com> writes:
> >
> >> No problem from my side, but are you sure?
> >
> > Absolutely.
> >
> > I've seen people just say "we document a failed one" and leave it at
> > that, without attempting to fix.  I am trying to see if pushing back
> > at first would serve as a good way to encourage these known failure
> > to be fixed, without accumulating too many expect_failure in our
> > test suite, which will waste cycles at CI runs (which do not need to
> > be reminded something is known to be broken).  I will try not to do
> > this when I do not positively know the author of such a patch is
> > capable enough to provide a fix, though, and you are unlucky enough
> > to have shown your abilities in the past ;-)
> 
> I ended up spending some time digging history and remembered that
> "--no-patch" was added as a synonym to "-s" by d09cd15d (diff: allow
> --no-patch as synonym for -s, 2013-07-16).  These
> 
>     git diff -p --stat --no-patch HEAD^ HEAD
>     git diff -p --raw --no-patch HEAD^ HEAD
> 
> would show no output from the diff machinery, patches, diffstats,
> raw object names, etc.
> 
> And this turns out to be a prime example why the approach to ask
> contributors do more, would help the project overall.

It would also help the project to reward the contributors who actually
do more.

Otherwise why would a contributor feel incentivized to do more, if that
work is simply going to land flat on the ground?

> It hopefully would have been "ah, the intent is not documented
> correctly, and here is a documentation patch to fix it."

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

I disagree.

What a reasonable user would expect also matters.

> When a command does not behave the way one thinks it should, being
> curious is good.  Reporting it as a potential bug is also good.  But
> it would help the project more if it was triaged before reporting it
> as a potential bug, if the reporter is capable of doing so.

This entirely depends on one's definition of "bug".

To me a bug is unexpected behavior. Some people think documenting
unexpected behavior makes it not a bug, but to me it's just a documented
bug.

"It's not a bug, it's a feature!"

> Those who encounter behaviour unexpected to them are more numerous
> than those who can report it as a potential bug (many people are not
> equipped to write a good bug report),

Is it just unexpected to them? Or is it unexpected to most users?

So what would a reasonable user expect `--no-patch` to do? I think a
reasonable user would expect it to negate the effect of `--patch`, and
nothing more.

The fact that a minority of users expect `--no-patch` to disable all
output--not just the one of `--patch`--would not make it not a bug in my
book.

> Those who can come up with a solution is even more scarse.

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

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Junio C Hamano]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> --patch followed by --no-patch is to be a no-op according to the "git
>> log" manual page.
>
> I briefly wondered if it is a bug in the documentation.
> ... when "git log -p --raw" shows both patch and raw, I do not
> think of a reason why "git log -p --raw --no-patch" should not
> behave similarly.

So, to tie the loose ends, "log -p --raw --no-patch" and "log -p
--stat --no-patch" do behave similarly.  Where my reaction was
mistaken was that I did not read the manual page myself that clearly
said it is the same as "-s" that suppresses diff output (where "diff
output" is not limited to "patch"---diffstat is also output of "diff"),
and incorrectly thought that "--no-patch" would countermand only
"--patch" and nothing else.

In Documentation/diff-options.txt we have this snippet:

    -s::
    --no-patch::
            Suppress diff output. Useful for commands like `git show` that
            show the patch by default, or to cancel the effect of `--patch`.

I imagine that argument could be made that the last half-sentence
can be read to say that the option is usable ONLY to cancel the
effect of `--patch` without cancelling the effect of anything else.

But that smells like a bit of stretch, as "like" in "commands like"
is a sign, at least to me, that it gives a few examples without
attempting to be exhaustive (meaning that it is too much to read
"ONLY" that is not written in "or to cancel the effect of")..

Here is my attempt to make it tighter to avoid getting mis-read:

    Suppress all output from the diff machinery.  Useful for
    commands like `git show` that show the patch by default to
    squelch their output, or to cancel the effect of options like
    `--patch`, `--stat` earlier on the command line in an alias.

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> --patch followed by --no-patch is to be a no-op according to the "git
>>> log" manual page.
>>
>> I briefly wondered if it is a bug in the documentation.
>> ... when "git log -p --raw" shows both patch and raw, I do not
>> think of a reason why "git log -p --raw --no-patch" should not
>> behave similarly.
>
> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p
> --stat --no-patch" do behave similarly.  Where my reaction was
> mistaken was that I did not read the manual page myself that clearly
> said it is the same as "-s" that suppresses diff output (where "diff
> output" is not limited to "patch"---diffstat is also output of "diff"),
> and incorrectly thought that "--no-patch" would countermand only
> "--patch" and nothing else.
>
> In Documentation/diff-options.txt we have this snippet:
>
>     -s::
>     --no-patch::
>             Suppress diff output. Useful for commands like `git show` that
>             show the patch by default, or to cancel the effect of `--patch`.
>
> I imagine that argument could be made that the last half-sentence
> can be read to say that the option is usable ONLY to cancel the
> effect of `--patch` without cancelling the effect of anything else.
>
> But that smells like a bit of stretch, as "like" in "commands like"
> is a sign, at least to me, that it gives a few examples without
> attempting to be exhaustive (meaning that it is too much to read
> "ONLY" that is not written in "or to cancel the effect of")..
>
> Here is my attempt to make it tighter to avoid getting mis-read:
>
>     Suppress all output from the diff machinery.  Useful for
>     commands like `git show` that show the patch by default to
>     squelch their output, or to cancel the effect of options like
>     `--patch`, `--stat` earlier on the command line in an alias.

This is fine, but is irrelevant to the test-case. Please refer to my
answer to your previous reply on the issue for details.

Thanks,
-- Sergey Organov

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Sergey Organov <sorganov@gmail.com> writes:
> >
> >> --patch followed by --no-patch is to be a no-op according to the "git
> >> log" manual page.
> >
> > I briefly wondered if it is a bug in the documentation.
> > ... when "git log -p --raw" shows both patch and raw, I do not
> > think of a reason why "git log -p --raw --no-patch" should not
> > behave similarly.
> 
> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p
> --stat --no-patch" do behave similarly.  Where my reaction was
> mistaken was that I did not read the manual page myself that clearly
> said it is the same as "-s" that suppresses diff output (where "diff
> output" is not limited to "patch"---diffstat is also output of "diff"),
> and incorrectly thought that "--no-patch" would countermand only
> "--patch" and nothing else.

If Sergey, you, and me all agreed on what `--no-patch` should do
(without reading the manpage), isn't that an indication that that is the
expected behavior?

The fact that the documentation documents some unexpected behavior,
doesn't mean it isn't a bug.

I would say it's a documented bug.

-- 
Felipe Contreras

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Sergey Organov]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> > Sergey Organov <sorganov@gmail.com> writes:
>> >
>> >> --patch followed by --no-patch is to be a no-op according to the "git
>> >> log" manual page.
>> >
>> > I briefly wondered if it is a bug in the documentation.
>> > ... when "git log -p --raw" shows both patch and raw, I do not
>> > think of a reason why "git log -p --raw --no-patch" should not
>> > behave similarly.
>> 
>> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p
>> --stat --no-patch" do behave similarly.  Where my reaction was
>> mistaken was that I did not read the manual page myself that clearly
>> said it is the same as "-s" that suppresses diff output (where "diff
>> output" is not limited to "patch"---diffstat is also output of "diff"),
>> and incorrectly thought that "--no-patch" would countermand only
>> "--patch" and nothing else.
>
> If Sergey, you, and me all agreed on what `--no-patch` should do
> (without reading the manpage), isn't that an indication that that is the
> expected behavior?
>
> The fact that the documentation documents some unexpected behavior,
> doesn't mean it isn't a bug.
>
> I would say it's a documented bug.

Yep, it is. Chances are this will end-up in the "won't fix" category
though, similar to unfortunate '-m'. In which case I think it's better
to explicitly mark it in the documentation as such: won't fix.

Thanks,
-- Sergey Organov

Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

Sergey Organov wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >> > Sergey Organov <sorganov@gmail.com> writes:
> >> >
> >> >> --patch followed by --no-patch is to be a no-op according to the "git
> >> >> log" manual page.
> >> >
> >> > I briefly wondered if it is a bug in the documentation.
> >> > ... when "git log -p --raw" shows both patch and raw, I do not
> >> > think of a reason why "git log -p --raw --no-patch" should not
> >> > behave similarly.
> >> 
> >> So, to tie the loose ends, "log -p --raw --no-patch" and "log -p
> >> --stat --no-patch" do behave similarly.  Where my reaction was
> >> mistaken was that I did not read the manual page myself that clearly
> >> said it is the same as "-s" that suppresses diff output (where "diff
> >> output" is not limited to "patch"---diffstat is also output of "diff"),
> >> and incorrectly thought that "--no-patch" would countermand only
> >> "--patch" and nothing else.
> >
> > If Sergey, you, and me all agreed on what `--no-patch` should do
> > (without reading the manpage), isn't that an indication that that is the
> > expected behavior?
> >
> > The fact that the documentation documents some unexpected behavior,
> > doesn't mean it isn't a bug.
> >
> > I would say it's a documented bug.
> 
> Yep, it is. Chances are this will end-up in the "won't fix" category
> though, similar to unfortunate '-m'.

Probably.

> In which case I think it's better to explicitly mark it in the documentation
> as such: won't fix.

Agreed.

-- 
Felipe Contreras