Re: [PATCH 2/5] Rework diff options

Junio C Hamano [off-list ref] wrote:

quoted

diff --git a/builtin-log.c b/builtin-log.c
index 5a8a50b..e4a6385 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -26,8 +26,8 @@ static int cmd_log_wc(int argc, const ch
 	if (rev->always_show_header) {
 		if (rev->diffopt.pickaxe || rev->diffopt.filter) {
 			rev->always_show_header = 0;
-			if (rev->diffopt.output_format == DIFF_FORMAT_RAW)
-				rev->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
+			if (rev->diffopt.output_fmt & OUTPUT_FMT_RAW)
+				rev->diffopt.output_fmt |= OUTPUT_FMT_NONE;
 		}
 	}

The original code is saying "For git-log command (i.e. when
always-show-header is on), if the command line did not override
but ended up asking for diff only because it wanted to do -S or
--diff-filter, do not show any diff" which is quite an opaque
logic.

I'll just remove this change from the fixed patch 2/5.  New version of
the patch 3/5 should then fix this logic.

quoted

@@ -1371,23 +1371,26 @@ int diff_setup_done(struct diff_options 
 	    (0 <= options->rename_limit && !options->detect_rename))
 		return -1;
 
+	if (options->output_fmt & OUTPUT_FMT_NONE)
+		options->output_fmt = 0;
+
+	if (options->output_fmt & (OUTPUT_FMT_NAME |
+				   OUTPUT_FMT_CHECKDIFF |
+				   OUTPUT_FMT_NONE))
+		options->output_fmt &= ~(OUTPUT_FMT_RAW |
+					 OUTPUT_FMT_DIFFSTAT |
+					 OUTPUT_FMT_SUMMARY |
+					 OUTPUT_FMT_PATCH);
+

Maybe doing the same for --name-status?

Will fix.  Originally I made --name-status imply --name-only but changed
it and forgot to fix this.

I wonder if the --name,
--name-status and --check should be mutually exclusive.  What
happens when you specify more than one of them?

I'll just make it die() then. If it breaks something then the code is
really dumb anyway.

quoted

diff --git a/revision.c b/revision.c
index b963f2a..4ad2272 100644
--- a/revision.c
+++ b/revision.c

@@ -852,8 +852,8 @@ int setup_revisions(int argc, const char
 	if (revs->combine_merges) {
 		revs->ignore_merges = 0;
 		if (revs->dense_combined_merges &&
-		    (revs->diffopt.output_format != DIFF_FORMAT_DIFFSTAT))
-			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
+		   !(revs->diffopt.output_fmt & OUTPUT_FMT_DIFFSTAT))
+			revs->diffopt.output_fmt |= OUTPUT_FMT_PATCH;
 	}
 	revs->diffopt.abbrev = revs->abbrev;
 	diff_setup_done(&revs->diffopt);

This tells it to default to patch format unless we are asked to
do diffstat only, in which case we just show stat without patch.
The new logic seems to be fishy.

If we first initialize it to 0 instead of DIFF_FORMAT_RAW and after
command line flags have been parsed, if it still is 0, then default to
DIFF_FORMAT_PATCH.

 - could I ask you to redo a patch to do only the clean-up part
   first, so that I can accept it for either "next" or "master".

 - Then after I take the clean-up, could you rebase four
   remainder patches ("Rework diff options" to "Add --patch
   option for diff-*") on the result?  The patches this round
   are already split quite well in that the first one does the
   enum to bit conversion and the latter three cleans things up
   (all of which I like a lot).  As Johannes suggested, it might
   be easier to review if they reused the same preprocessor
   symbols instead of renaming them.  I'd take them for "next".

Yes, all this makes sense.

-- 
http://onion.dynserv.net/~timo/