Re* [PATCH] Add --filedirstat diff option

[Junio C Hamano <gitster@pobox.com>]

Heikki Orsila <shd@modeemi.fi> writes:

> I wouldn't add new semantics for --dirstat=x since it's easy to add 
> new options. We already have --cumulative, so adding options is not 
> new. How about "--dirstat --filemode"? It's long, but it is obvious. I 
> don't think length is relevant here. A shorter but obscure syntax would 
> be "--dirstat2" :-) 

I wouldn't actually buy "easy to add" argument.  Following the
easiest-to-implement path down often result in inconsistent and messy UI.

IOW, I may use that expression from time to time like: "while it may be
easy to add another option, that would lead to many options that depend on
other options, with a messy end result", in a negative sense.

But you are right to point out that --cumulative is an option that affects
how --dirstat operates.  And I think the option parsing of that code is
buggy.  It should imply --dirstat, shouldn't it?

So I do not mind --filemode or whatever option that is expressed as a
word on the command line separate from --dirstat, but we really should
consider this and --cumulative conceptually a sub-option to --dirstat.

Here is a sample patch to fix --cumulative (but of course it is untested);
I think your --filemode (or whatever its final name is) should hook into
the same place as this patch touches, as far as command line parsing is
concerned.

-- >8 --
diff --cumulative is a sub-option of --dirstat

The option used to be implemented as if it is a totally independent one,
but "git diff --cumulative" would not mean anything without "--dirstat".

This makes --cumulative imply --dirstat.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c |    9 ++++++---
 diff.h |    2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git c/diff.c w/diff.c
index 135dec4..cbd151b 100644
--- c/diff.c
+++ w/diff.c
@@ -1078,7 +1078,7 @@ static void show_dirstat(struct diff_options *options)
 	dir.alloc = 0;
 	dir.nr = 0;
 	dir.percent = options->dirstat_percent;
-	dir.cumulative = options->output_format & DIFF_FORMAT_CUMULATIVE;
+	dir.cumulative = DIFF_OPT_TST(options, DIRSTAT_CUMULATIVE);
 
 	changed = 0;
 	for (i = 0; i < q->nr; i++) {
@@ -2300,6 +2300,7 @@ void diff_setup(struct diff_options *options)
 	options->break_opt = -1;
 	options->rename_limit = -1;
 	options->dirstat_percent = 3;
+	DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
 	options->context = 3;
 
 	options->change = diff_change;
@@ -2472,8 +2473,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;
 	else if (opt_arg(arg, 'X', "dirstat", &options->dirstat_percent))
 		options->output_format |= DIFF_FORMAT_DIRSTAT;
-	else if (!strcmp(arg, "--cumulative"))
-		options->output_format |= DIFF_FORMAT_CUMULATIVE;
+	else if (!strcmp(arg, "--cumulative")) {
+		options->output_format |= DIFF_FORMAT_DIRSTAT;
+		DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
+	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
diff --git c/diff.h w/diff.h
index 50fb5dd..7f53beb 100644
--- c/diff.h
+++ w/diff.h
@@ -31,7 +31,6 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_FORMAT_PATCH	0x0010
 #define DIFF_FORMAT_SHORTSTAT	0x0020
 #define DIFF_FORMAT_DIRSTAT	0x0040
-#define DIFF_FORMAT_CUMULATIVE	0x0080
 
 /* These override all above */
 #define DIFF_FORMAT_NAME	0x0100
@@ -64,6 +63,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_CHECK_FAILED        (1 << 16)
 #define DIFF_OPT_RELATIVE_NAME       (1 << 17)
 #define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)
+#define DIFF_OPT_DIRSTAT_CUMULATIVE  (1 << 19)
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)