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

Commit Message

Junio C Hamano May 4, 2023, 9:37 p.m. UTC

Junio C Hamano <[e-mail]> 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(-)

Patch

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,