[v1,0/7] diff: fix -s and --no-patch

Message

Felipe Contreras May 12, 2023, 8:03 a.m. UTC

The diff code assumes 0 means the default, and --no-patch means
NO_OUTPUT.

The problem with this approach is that it doesn't allow distinguishing
`git diff --no-patch`, `git diff --patch --no-patch`, and `git diff`.

By introducing a DIFF_FORMAT_DEFAULT (which is not 0) it's now possible
to properly distinguish these arguments, and get rid of
DIFF_FORMAT_NO_OUTPUT which should have never been considered a format,
but the absence of a format.

This fixes an issue Sergey Organov reported.

Up to patch #2 (diff: introduce DIFF_FORMAT_DEFAULT) there are no
functional changes, patch #3 (diff: make DIFF_FORMAT_NO_OUTPUT 0)
achieves the same as a series from Junio Hamano [1] except more
properly. Patch #4 adds a simplified version of Junio's test cases.

Then in patch #5 --no-patch is split from -s, making it work as
intended: negates --patch, but not other formats.

The rest are some niceties.

Now all these work correctly:

 1. git diff --raw
 2. git diff -s --raw
 3. git diff --no-patch
 4. git diff --no-patch --raw
 5. git diff --patch --no-patch --raw
 6. git diff --raw --patch --no-patch

[1] https://lore.kernel.org/git/[e-mail]/

Felipe Contreras (7):
  line-log: set patch format explicitly by default
  diff: introduce DIFF_FORMAT_DEFAULT
  diff: make DIFF_FORMAT_NO_OUTPUT 0
  test: add various tests for diff formats with -s
  diff: split --no-patch from -s
  diff: add --silent as alias of -s
  diff: remove DIFF_FORMAT_NO_OUTPUT

 Documentation/diff-options.txt |  6 ++--
 blame.c                        |  6 ++--
 builtin/diff-files.c           |  2 +-
 builtin/diff-index.c           |  2 +-
 builtin/diff-tree.c            |  2 +-
 builtin/diff.c                 |  2 +-
 builtin/log.c                  | 16 +++++++---
 builtin/stash.c                |  4 +--
 builtin/submodule--helper.c    |  2 +-
 combine-diff.c                 | 10 +++---
 diff-merges.c                  |  2 +-
 diff-no-index.c                |  2 +-
 diff.c                         | 56 ++++++++++++++++++----------------
 diff.h                         |  6 +---
 line-log.c                     |  2 +-
 log-tree.c                     |  4 +--
 merge-ort.c                    |  4 +--
 merge-recursive.c              |  4 +--
 notes-merge.c                  |  4 +--
 range-diff.c                   |  4 +--
 revision.c                     |  6 ++--
 t/t4000-diff-format.sh         | 26 ++++++++++++++--
 tree-diff.c                    |  2 +-
 23 files changed, 99 insertions(+), 75 deletions(-)

Comments

Felipe Contreras May 12, 2023, 8:20 a.m. UTC | #1

Felipe Contreras wrote:
> This fixes an issue Sergey Organov reported.

Sergey, as you can see this series fixes the issue you reported.

First, I think these should remain working the same, simply for convenience:

 * git diff         # default output
 * git diff --patch # patch output
 * git diff --raw   # raw output
 * git diff --stat  # stat output

I don't think there's a way I can be convinced otherwise.

But there's many changes:

 1. git diff -s --raw                 # before: nil, after: raw
 2. git diff --no-patch --raw         # before: nil, after: raw
 3. git diff --patch --no-patch --raw # before: nil, after: raw
 4. git diff --raw --patch --no-patch # before: nil, after: raw

I don't think there's any way you can say my 174 changes make the code work
"exactly the same".

And this is better than Junio's solution, because #4 outputs a raw format,
while in Junio's solution it doesn't output anything.

Even if you don't agree with everything, this solution is better than the
status quo, and it's better than Junio's solution as it fixes --no-patch
immediately.

Cheers.

Sergey Organov May 12, 2023, 9:32 a.m. UTC | #2

Felipe Contreras <[e-mail]> writes:

> Felipe Contreras wrote:
>> This fixes an issue Sergey Organov reported.
>
> Sergey, as you can see this series fixes the issue you reported.
>
> First, I think these should remain working the same, simply for convenience:
>
>  * git diff         # default output
>  * git diff --patch # patch output
>  * git diff --raw   # raw output
>  * git diff --stat  # stat output
>
> I don't think there's a way I can be convinced otherwise.

Fine with me.

>
> But there's many changes:
>
>  1. git diff -s --raw                 # before: nil, after: raw
>  2. git diff --no-patch --raw         # before: nil, after: raw
>  3. git diff --patch --no-patch --raw # before: nil, after: raw
>  4. git diff --raw --patch --no-patch # before: nil, after: raw

Fine as well.

>
> I don't think there's any way you can say my 174 changes make the code work
> "exactly the same".

I said that in the context where we discussed entirely separate issue
"handling of defaults by Git commands". Irrelevant to these series as
they don't touch this aspect as visible from outside, even though you
do change the implementation for better.

>
> And this is better than Junio's solution, because #4 outputs a raw format,
> while in Junio's solution it doesn't output anything.

Yes, and that's where I agreed from the very beginning.

> Even if you don't agree with everything, this solution is better than the
> status quo, and it's better than Junio's solution as it fixes --no-patch
> immediately.

Yep, it fixes "--no-patch" semantics indeed, and as I already said, I do
vote in favor of this change, for what it's worth.

Thanks,
-- Sergey Organov