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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 9, 2023, 03:50 UTC
Message-ID: <6459c31038e81_7c68294ee@chronos.notmuch>
In-Reply-To: <xmqqsfc62t8y.fsf@gitster.g>

Junio C Hamano wrote:

Show 11 quoted lines

> 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

Show 5 quoted lines

> 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