Re: [PATCH 2/5] Rework diff options

[Timo Hirvonen <tihirvon@gmail.com>]

Junio C Hamano <junkio@cox.net> wrote:

> > 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.

> > @@ -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.

> > 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/