[RFC PATCH 0/4] diff: fix default/no-output

[RFC PATCH 0/4] diff: fix default/no-output

[Felipe Contreras]

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 of a
format, but the absense of the DIFF_FORMAT_PATCH format.

This fixes an issue Sergey Organov reported.

Now all these 3 work correctly:

 1. git diff --raw
 2. git diff --patch --no-patch --raw
 3. git diff --raw --patch --no-patch

Felipe Contreras (3):
  line-log: set patch format explicitly by default
  diff: introduce DIFF_FORMAT_DEFAULT
  diff: remove DIFF_FORMAT_NO_OUTPUT

Sergey Organov (1):
  t4013: add expected failure for "log --patch --no-patch"

 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                      | 53 ++++++++++++++++++-------------------
 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/t4013-diff-various.sh     |  8 ++++++
 tree-diff.c                 |  2 +-
 22 files changed, 78 insertions(+), 69 deletions(-)

-- 
2.40.0+fc1

[RFC PATCH 1/4] t4013: add expected failure for "log --patch --no-patch"

[Felipe Contreras]

From: Sergey Organov <sorganov@gmail.com>

--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>
Signed-off-by: Felipe Contreras <felipe.contreras@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 5de1d19075..f876b0cc8e 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.40.0+fc1

[RFC PATCH 2/4] line-log: set patch format explicitly by default

[Felipe Contreras]

Will help further changes.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 5 +++++
 line-log.c    | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4f162ff4d0..568207916b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -889,6 +889,11 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	opt.revarg_opt = REVARG_COMMITTISH;
 	opt.tweak = log_setup_revisions_tweak;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+
+	if (!rev.diffopt.output_format)
+		if (rev.line_level_traverse)
+			rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+
 	return cmd_log_deinit(cmd_log_walk(&rev), &rev);
 }
 
diff --git a/line-log.c b/line-log.c
index 10c19daec4..4499fd0c59 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1140,7 +1140,7 @@ int line_log_print(struct rev_info *rev, struct commit *commit)
 {
 
 	show_log(rev);
-	if (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {
+	if (rev->diffopt.output_format & DIFF_FORMAT_PATCH) {
 		struct line_log_data *range = lookup_line_range(rev, commit);
 		dump_diff_hacky(rev, range);
 	}
-- 
2.40.0+fc1

[RFC PATCH 3/4] diff: introduce DIFF_FORMAT_DEFAULT

[Felipe Contreras]

As the name suggests this is the default format, which means no format
was specified.

This is not the same as DIFF_FORMAT_PATCH, as some commands like `git
diff-files` use a different default.

This makes it possible to distinguish `git diff` (DEFAULT)
from  `git diff --no-patch` (0).

Will help further changes.

There should be no functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/diff-files.c |  2 +-
 builtin/diff-index.c |  2 +-
 builtin/diff-tree.c  |  2 +-
 builtin/diff.c       |  2 +-
 builtin/log.c        | 13 ++++++++-----
 builtin/stash.c      |  2 +-
 diff-merges.c        |  2 +-
 diff-no-index.c      |  2 +-
 diff.c               | 39 ++++++++++++++++++++-------------------
 diff.h               |  1 +
 range-diff.c         |  2 +-
 revision.c           |  4 ++--
 12 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..b831b89236 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -52,7 +52,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 			usage(diff_files_usage);
 		argv++; argc--;
 	}
-	if (!rev.diffopt.output_format)
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	rev.diffopt.rotate_to_strict = 1;
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index b9a19bb7d3..863c51c9b5 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -48,7 +48,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		else
 			usage(diff_cache_usage);
 	}
-	if (!rev.diffopt.output_format)
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 
 	rev.diffopt.rotate_to_strict = 1;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 385c2d0230..75e5e96d05 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -99,7 +99,7 @@ COMMON_DIFF_OPTIONS_HELP;
 
 static void diff_tree_tweak_rev(struct rev_info *rev, struct setup_revision_opt *opt)
 {
-	if (!rev->diffopt.output_format) {
+	if (rev->diffopt.output_format == DIFF_FORMAT_DEFAULT) {
 		if (rev->dense_combined_merges)
 			rev->diffopt.output_format = DIFF_FORMAT_PATCH;
 		else
diff --git a/builtin/diff.c b/builtin/diff.c
index 5a6a5d7f4b..a01de13194 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -504,7 +504,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
-	if (!rev.diffopt.output_format) {
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 		diff_setup_done(&rev.diffopt);
 	}
diff --git a/builtin/log.c b/builtin/log.c
index 568207916b..13c196c391 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -276,7 +276,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			     PARSE_OPT_KEEP_DASHDASH);
 
 	if (quiet)
-		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
+		rev->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
 	argc = setup_revisions(argc, argv, rev, opt);
 
 	/* Any arguments at this point are not recognized */
@@ -632,7 +632,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	opt.def = "HEAD";
 	opt.revarg_opt = REVARG_COMMITTISH;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
-	if (!rev.diffopt.output_format)
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	return cmd_log_deinit(cmd_log_walk(&rev), &rev);
 }
@@ -724,7 +724,7 @@ static void show_setup_revisions_tweak(struct rev_info *rev,
 		diff_merges_default_to_first_parent(rev);
 	else
 		diff_merges_default_to_dense_combined(rev);
-	if (!rev->diffopt.output_format)
+	if (rev->diffopt.output_format == DIFF_FORMAT_DEFAULT)
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
 }
 
@@ -890,9 +890,12 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	opt.tweak = log_setup_revisions_tweak;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
 
-	if (!rev.diffopt.output_format)
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT) {
 		if (rev.line_level_traverse)
 			rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+		else
+			rev.diffopt.output_format = 0;
+	}
 
 	return cmd_log_deinit(cmd_log_walk(&rev), &rev);
 }
@@ -2125,7 +2128,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die(_("--remerge-diff does not make sense"));
 
 	if (!use_patch_format &&
-		(!rev.diffopt.output_format ||
+		(rev.diffopt.output_format == DIFF_FORMAT_DEFAULT ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
 	if (!rev.diffopt.stat_width)
diff --git a/builtin/stash.c b/builtin/stash.c
index a7e17ffe38..398e3c9f61 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -944,7 +944,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 	argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
 	if (argc > 1)
 		goto usage;
-	if (!rev.diffopt.output_format) {
+	if (rev.diffopt.output_format == DIFF_FORMAT_DEFAULT) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 		diff_setup_done(&rev.diffopt);
 	}
diff --git a/diff-merges.c b/diff-merges.c
index ec97616db1..9960d7cc36 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -183,7 +183,7 @@ void diff_merges_setup_revs(struct rev_info *revs)
 	if (revs->merges_imply_patch)
 		revs->diff = 1;
 	if (revs->merges_imply_patch || revs->merges_need_diff) {
-		if (!revs->diffopt.output_format)
+		if (revs->diffopt.output_format == DIFF_FORMAT_DEFAULT)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
 }
diff --git a/diff-no-index.c b/diff-no-index.c
index 4296940f90..45596cb1be 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -282,7 +282,7 @@ int diff_no_index(struct rev_info *revs,
 	fixup_paths(paths, &replacement);
 
 	revs->diffopt.skip_stat_unmatch = 1;
-	if (!revs->diffopt.output_format)
+	if (revs->diffopt.output_format == DIFF_FORMAT_DEFAULT)
 		revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 
 	revs->diffopt.flags.no_index = 1;
diff --git a/diff.c b/diff.c
index 1e83aaee6b..c9077bfed6 100644
--- a/diff.c
+++ b/diff.c
@@ -4667,6 +4667,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	options->file = stdout;
 	options->repo = r;
 
+	options->output_format = DIFF_FORMAT_DEFAULT;
 	options->output_indicators[OUTPUT_INDICATOR_NEW] = '+';
 	options->output_indicators[OUTPUT_INDICATOR_OLD] = '-';
 	options->output_indicators[OUTPUT_INDICATOR_CONTEXT] = ' ';
@@ -4985,7 +4986,7 @@ static int diff_opt_diff_filter(const struct option *option,
 
 static void enable_patch_output(int *fmt)
 {
-	*fmt &= ~DIFF_FORMAT_NO_OUTPUT;
+	*fmt &= ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT);
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
@@ -5490,13 +5491,13 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_GROUP(N_("Diff output format 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,
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+		OPT_BITOP('s', "no-patch", &options->output_format,
 			  N_("suppress diff output"),
-			  DIFF_FORMAT_NO_OUTPUT, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_NO_OUTPUT, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH),
 		OPT_BITOP('u', NULL, &options->output_format,
 			  N_("generate patch"),
-			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
 		OPT_CALLBACK_F('U', "unified", options, N_("<n>"),
 			       N_("generate diffs with <n> lines context"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
@@ -5508,17 +5509,17 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_BITOP(0, "patch-with-raw", &options->output_format,
 			  N_("synonym for '-p --raw'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,
-			  DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
 		OPT_BITOP(0, "patch-with-stat", &options->output_format,
 			  N_("synonym for '-p --stat'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,
-			  DIFF_FORMAT_NO_OUTPUT),
-		OPT_BIT_F(0, "numstat", &options->output_format,
+			  DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+		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_DEFAULT),
+		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_DEFAULT),
 		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,
@@ -5531,18 +5532,18 @@ struct option *add_diff_options(const struct option *opts,
 			       N_("synonym for --dirstat=files,param1,param2..."),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
 			       diff_opt_dirstat),
-		OPT_BIT_F(0, "check", &options->output_format,
+		OPT_BITOP(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,
+			  DIFF_FORMAT_CHECKDIFF, DIFF_FORMAT_DEFAULT),
+		OPT_BITOP(0, "summary", &options->output_format,
 			  N_("condensed summary such as creations, renames and mode changes"),
-			  DIFF_FORMAT_SUMMARY, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "name-only", &options->output_format,
+			  DIFF_FORMAT_SUMMARY, DIFF_FORMAT_DEFAULT),
+		OPT_BITOP(0, "name-only", &options->output_format,
 			  N_("show only names of changed files"),
-			  DIFF_FORMAT_NAME, PARSE_OPT_NONEG),
-		OPT_BIT_F(0, "name-status", &options->output_format,
+			  DIFF_FORMAT_NAME, DIFF_FORMAT_DEFAULT),
+		OPT_BITOP(0, "name-status", &options->output_format,
 			  N_("show only names and status of changed files"),
-			  DIFF_FORMAT_NAME_STATUS, PARSE_OPT_NONEG),
+			  DIFF_FORMAT_NAME_STATUS, DIFF_FORMAT_DEFAULT),
 		OPT_CALLBACK_F(0, "stat", options, N_("<width>[,<name-width>[,<count>]]"),
 			       N_("generate diffstat"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_stat),
diff --git a/diff.h b/diff.h
index 6a0737b9c3..35bdbc40fa 100644
--- a/diff.h
+++ b/diff.h
@@ -102,6 +102,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_FORMAT_PATCH	0x0010
 #define DIFF_FORMAT_SHORTSTAT	0x0020
 #define DIFF_FORMAT_DIRSTAT	0x0040
+#define DIFF_FORMAT_DEFAULT	0x0080
 
 /* These override all above */
 #define DIFF_FORMAT_NAME	0x0100
diff --git a/range-diff.c b/range-diff.c
index a1e0cffb9f..240f13d8dd 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -492,7 +492,7 @@ static void output(struct string_list *a, struct string_list *b,
 		repo_diff_setup(the_repository, &opts);
 
 	opts.no_free = 1;
-	if (!opts.output_format)
+	if (opts.output_format == DIFF_FORMAT_DEFAULT)
 		opts.output_format = DIFF_FORMAT_PATCH;
 	opts.flags.suppress_diff_headers = 1;
 	opts.flags.dual_color_diffed_diffs =
diff --git a/revision.c b/revision.c
index b33cc1d106..cf68b533fd 100644
--- a/revision.c
+++ b/revision.c
@@ -2966,7 +2966,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
-	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
+	if (revs->diffopt.output_format & ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT))
 		revs->diff = 1;
 
 	/* Pickaxe, diff-filter and rename following need diffs */
@@ -3030,7 +3030,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
 
 	if (revs->line_level_traverse &&
-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
+	    (revs->diffopt.output_format & ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
 
 	if (revs->expand_tabs_in_log < 0)
-- 
2.40.0+fc1

[RFC PATCH 4/4] diff: remove DIFF_FORMAT_NO_OUTPUT

[Felipe Contreras]

Instead use an empty output_format (0) as NO_OUTPUT.

This makes it so `--no-patch` only negates the effect of `--patch`, not
shut down all output.

So now `git diff --no-patch --raw` works correctly, as it's the opposite
of `git diff --patch --raw`.

Semantically `--no-patch` used to be considered a format, now it's the
opposite of the `--patch` format.

Cc: Sergey Organov <sorganov@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 blame.c                     |  6 +++---
 builtin/log.c               |  2 +-
 builtin/stash.c             |  2 +-
 builtin/submodule--helper.c |  2 +-
 combine-diff.c              | 10 ++++------
 diff.c                      | 26 ++++++++++++--------------
 diff.h                      |  5 -----
 log-tree.c                  |  4 ++--
 merge-ort.c                 |  4 ++--
 merge-recursive.c           |  4 ++--
 notes-merge.c               |  4 ++--
 range-diff.c                |  2 +-
 revision.c                  |  6 +++---
 t/t4013-diff-various.sh     |  5 +----
 tree-diff.c                 |  2 +-
 15 files changed, 36 insertions(+), 48 deletions(-)

diff --git a/blame.c b/blame.c
index b830654062..d382af6798 100644
--- a/blame.c
+++ b/blame.c
@@ -1337,7 +1337,7 @@ static struct blame_origin *find_origin(struct repository *r,
 	repo_diff_setup(r, &diff_opts);
 	diff_opts.flags.recursive = 1;
 	diff_opts.detect_rename = 0;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	paths[0] = origin->path;
 	paths[1] = NULL;
 
@@ -1420,7 +1420,7 @@ static struct blame_origin *find_rename(struct repository *r,
 	repo_diff_setup(r, &diff_opts);
 	diff_opts.flags.recursive = 1;
 	diff_opts.detect_rename = DIFF_DETECT_RENAME;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	diff_opts.single_follow = origin->path;
 	diff_setup_done(&diff_opts);
 
@@ -2242,7 +2242,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,
 
 	repo_diff_setup(sb->repo, &diff_opts);
 	diff_opts.flags.recursive = 1;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 
 	diff_setup_done(&diff_opts);
 
diff --git a/builtin/log.c b/builtin/log.c
index 13c196c391..b3e6ee8782 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -276,7 +276,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			     PARSE_OPT_KEEP_DASHDASH);
 
 	if (quiet)
-		rev->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
+		rev->diffopt.output_format = 0;
 	argc = setup_revisions(argc, argv, rev, opt);
 
 	/* Any arguments at this point are not recognized */
diff --git a/builtin/stash.c b/builtin/stash.c
index 398e3c9f61..d7a4ade8d4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -437,7 +437,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 	repo_diff_setup(the_repository, &diff_opts);
 	diff_opts.flags.recursive = 1;
 	diff_opts.detect_rename = 0;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	diff_setup_done(&diff_opts);
 
 	do_diff_cache(orig_tree, &diff_opts);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bf8d666ce..885cd57e90 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1120,7 +1120,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
 	rev.abbrev = 0;
 	precompose_argv_prefix(diff_args.nr, diff_args.v, NULL);
 	setup_revisions(diff_args.nr, diff_args.v, &rev, &opt);
-	rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;
+	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = submodule_summary_callback;
 	rev.diffopt.format_callback_data = &list;
 
diff --git a/combine-diff.c b/combine-diff.c
index f7e9fb5747..86e01b2634 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1393,7 +1393,7 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,
 	int output_format = opt->output_format;
 	const char *orderfile = opt->orderfile;
 
-	opt->output_format = DIFF_FORMAT_NO_OUTPUT;
+	opt->output_format = 0;
 	/* tell diff_tree to emit paths in sorted (=tree) order */
 	opt->orderfile = NULL;
 
@@ -1407,17 +1407,15 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,
 		if (i == 0 && stat_opt)
 			opt->output_format = stat_opt;
 		else
-			opt->output_format = DIFF_FORMAT_NO_OUTPUT;
+			opt->output_format = 0;
 		diff_tree_oid(&parents->oid[i], oid, "", opt);
 		diffcore_std(opt);
 		paths = intersect_paths(paths, i, num_parent,
 					combined_all_paths);
 
 		/* if showing diff, show it in requested order */
-		if (opt->output_format != DIFF_FORMAT_NO_OUTPUT &&
-		    orderfile) {
+		if (opt->output_format && orderfile)
 			diffcore_order(orderfile);
-		}
 
 		diff_flush(opt);
 	}
@@ -1520,7 +1518,7 @@ void diff_tree_combined(const struct object_id *oid,
 		show_log(rev);
 
 		if (rev->verbose_header && opt->output_format &&
-		    opt->output_format != DIFF_FORMAT_NO_OUTPUT &&
+		    opt->output_format &&
 		    !commit_format_is_empty(rev->commit_format))
 			printf("%s%c", diff_line_prefix(opt),
 			       opt->line_termination);
diff --git a/diff.c b/diff.c
index c9077bfed6..afe2f849b0 100644
--- a/diff.c
+++ b/diff.c
@@ -4748,8 +4748,7 @@ void diff_setup_done(struct diff_options *options)
 {
 	unsigned check_mask = DIFF_FORMAT_NAME |
 			      DIFF_FORMAT_NAME_STATUS |
-			      DIFF_FORMAT_CHECKDIFF |
-			      DIFF_FORMAT_NO_OUTPUT;
+			      DIFF_FORMAT_CHECKDIFF;
 	/*
 	 * This must be signed because we're comparing against a potentially
 	 * negative value.
@@ -4760,8 +4759,8 @@ void diff_setup_done(struct diff_options *options)
 		options->set_default(options);
 
 	if (HAS_MULTI_BITS(options->output_format & check_mask))
-		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
-			"--name-only", "--name-status", "--check", "-s");
+		die(_("options '%s', '%s', and '%s' cannot be used together"),
+			"--name-only", "--name-status", "--check");
 
 	if (HAS_MULTI_BITS(options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
 		die(_("options '%s', '%s', and '%s' cannot be used together"),
@@ -4800,8 +4799,7 @@ void diff_setup_done(struct diff_options *options)
 
 	if (options->output_format & (DIFF_FORMAT_NAME |
 				      DIFF_FORMAT_NAME_STATUS |
-				      DIFF_FORMAT_CHECKDIFF |
-				      DIFF_FORMAT_NO_OUTPUT))
+				      DIFF_FORMAT_CHECKDIFF))
 		options->output_format &= ~(DIFF_FORMAT_RAW |
 					    DIFF_FORMAT_NUMSTAT |
 					    DIFF_FORMAT_DIFFSTAT |
@@ -4845,7 +4843,7 @@ void diff_setup_done(struct diff_options *options)
 	 * exit code in such a case either.
 	 */
 	if (options->flags.quick) {
-		options->output_format = DIFF_FORMAT_NO_OUTPUT;
+		options->output_format = 0;
 		options->flags.exit_with_status = 1;
 	}
 
@@ -4986,7 +4984,7 @@ static int diff_opt_diff_filter(const struct option *option,
 
 static void enable_patch_output(int *fmt)
 {
-	*fmt &= ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT);
+	*fmt &= ~DIFF_FORMAT_DEFAULT;
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
@@ -5491,13 +5489,13 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_GROUP(N_("Diff output format options")),
 		OPT_BITOP('p', "patch", &options->output_format,
 			  N_("generate patch"),
-			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT),
 		OPT_BITOP('s', "no-patch", &options->output_format,
 			  N_("suppress diff output"),
-			  DIFF_FORMAT_NO_OUTPUT, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH),
+			  0, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH),
 		OPT_BITOP('u', NULL, &options->output_format,
 			  N_("generate patch"),
-			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_DEFAULT),
 		OPT_CALLBACK_F('U', "unified", options, N_("<n>"),
 			       N_("generate diffs with <n> lines context"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_unified),
@@ -5509,11 +5507,11 @@ struct option *add_diff_options(const struct option *opts,
 		OPT_BITOP(0, "patch-with-raw", &options->output_format,
 			  N_("synonym for '-p --raw'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW,
-			  DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_DEFAULT),
 		OPT_BITOP(0, "patch-with-stat", &options->output_format,
 			  N_("synonym for '-p --stat'"),
 			  DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT,
-			  DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT),
+			  DIFF_FORMAT_DEFAULT),
 		OPT_BITOP(0, "numstat", &options->output_format,
 			  N_("machine friendly --stat"),
 			  DIFF_FORMAT_NUMSTAT, DIFF_FORMAT_DEFAULT),
@@ -6644,7 +6642,7 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
-	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	if (!output_format &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
 		/*
diff --git a/diff.h b/diff.h
index 35bdbc40fa..31375b7e8a 100644
--- a/diff.h
+++ b/diff.h
@@ -109,11 +109,6 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_FORMAT_NAME_STATUS	0x0200
 #define DIFF_FORMAT_CHECKDIFF	0x0400
 
-/* Same as output_format = 0 but we know that -s flag was given
- * and we should not give default value to output_format.
- */
-#define DIFF_FORMAT_NO_OUTPUT	0x0800
-
 #define DIFF_FORMAT_CALLBACK	0x1000
 
 #define DIFF_FLAGS_INIT { 0 }
diff --git a/log-tree.c b/log-tree.c
index 143b86fecf..0b1097c0bf 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -873,7 +873,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 
 	if (diff_queue_is_empty(&opt->diffopt)) {
 		int saved_fmt = opt->diffopt.output_format;
-		opt->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
+		opt->diffopt.output_format = 0;
 		diff_flush(&opt->diffopt);
 		opt->diffopt.output_format = saved_fmt;
 		return 0;
@@ -881,7 +881,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 
 	if (opt->loginfo && !opt->no_commit_id) {
 		show_log(opt);
-		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
+		if (opt->diffopt.output_format &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE &&
 		    !commit_format_is_empty(opt->commit_format)) {
diff --git a/merge-ort.c b/merge-ort.c
index 34ec2675a2..f1d1db4970 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3247,7 +3247,7 @@ static int detect_regular_renames(struct merge_options *opt,
 		diff_opts.rename_limit = 7000;
 	diff_opts.rename_score = opt->rename_score;
 	diff_opts.show_rename_progress = opt->show_rename_progress;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	diff_setup_done(&diff_opts);
 
 	diff_queued_diff = renames->pairs[side_index];
@@ -3268,7 +3268,7 @@ static int detect_regular_renames(struct merge_options *opt,
 
 	renames->pairs[side_index] = diff_queued_diff;
 
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	diff_queued_diff.nr = 0;
 	diff_queued_diff.queue = NULL;
 	diff_flush(&diff_opts);
diff --git a/merge-recursive.c b/merge-recursive.c
index 9875bdb11c..3cc8c956eb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1902,7 +1902,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
 	opts.rename_limit = (opt->rename_limit >= 0) ? opt->rename_limit : 7000;
 	opts.rename_score = opt->rename_score;
 	opts.show_rename_progress = opt->show_rename_progress;
-	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	opts.output_format = 0;
 	diff_setup_done(&opts);
 	diff_tree_oid(&o_tree->object.oid, &tree->object.oid, "", &opts);
 	diffcore_std(&opts);
@@ -1912,7 +1912,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
 	ret = xmalloc(sizeof(*ret));
 	*ret = diff_queued_diff;
 
-	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	opts.output_format = 0;
 	diff_queued_diff.nr = 0;
 	diff_queued_diff.queue = NULL;
 	diff_flush(&opts);
diff --git a/notes-merge.c b/notes-merge.c
index 233e49e319..9a8bac2579 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -139,7 +139,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 
 	repo_diff_setup(o->repo, &opt);
 	opt.flags.recursive = 1;
-	opt.output_format = DIFF_FORMAT_NO_OUTPUT;
+	opt.output_format = 0;
 	diff_setup_done(&opt);
 	diff_tree_oid(base, remote, "", &opt);
 	diffcore_std(&opt);
@@ -201,7 +201,7 @@ static void diff_tree_local(struct notes_merge_options *o,
 
 	repo_diff_setup(o->repo, &opt);
 	opt.flags.recursive = 1;
-	opt.output_format = DIFF_FORMAT_NO_OUTPUT;
+	opt.output_format = 0;
 	diff_setup_done(&opt);
 	diff_tree_oid(base, local, "", &opt);
 	diffcore_std(&opt);
diff --git a/range-diff.c b/range-diff.c
index 240f13d8dd..db3f2a4958 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -542,7 +542,7 @@ static void output(struct string_list *a, struct string_list *b,
 			a_util = a->items[b_util->matching].util;
 			output_pair_header(&opts, patch_no_width,
 					   &buf, &dashes, a_util, b_util);
-			if (!(opts.output_format & DIFF_FORMAT_NO_OUTPUT))
+			if (opts.output_format)
 				patch_diff(a->items[b_util->matching].string,
 					   b->items[j].string, &opts);
 			a_util->shown = 1;
diff --git a/revision.c b/revision.c
index cf68b533fd..52c2f415c7 100644
--- a/revision.c
+++ b/revision.c
@@ -2966,7 +2966,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
-	if (revs->diffopt.output_format & ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_NO_OUTPUT))
+	if (revs->diffopt.output_format & ~DIFF_FORMAT_DEFAULT)
 		revs->diff = 1;
 
 	/* Pickaxe, diff-filter and rename following need diffs */
@@ -3030,8 +3030,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
 
 	if (revs->line_level_traverse &&
-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-		die(_("-L does not yet support diff formats besides -p and -s"));
+	    (revs->diffopt.output_format & ~(DIFF_FORMAT_DEFAULT | DIFF_FORMAT_PATCH)))
+		die(_("-L does not yet support diff formats besides -p"));
 
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index f876b0cc8e..3a43edb8d8 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -457,10 +457,7 @@ 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' '
+test_expect_success '--no-patch cancels --patch only' '
 	git log --raw master >result &&
 	process_diffs result >expected &&
 	git log --patch --no-patch --raw >result &&
diff --git a/tree-diff.c b/tree-diff.c
index 69031d7cba..a2e872fbe7 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -613,7 +613,7 @@ static void try_to_follow_renames(const struct object_id *old_oid,
 	repo_diff_setup(opt->repo, &diff_opts);
 	diff_opts.flags.recursive = 1;
 	diff_opts.flags.find_copies_harder = 1;
-	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_opts.output_format = 0;
 	diff_opts.single_follow = opt->pathspec.items[0].match;
 	diff_opts.break_opt = opt->break_opt;
 	diff_opts.rename_score = opt->rename_score;
-- 
2.40.0+fc1