Re: [PATCH 2/7] Merge with_raw, with_stat and summary variables to output_format

[Timo Hirvonen <tihirvon@gmail.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Hi,
> 
> thank you very much for doing the extra step and using the original 
> constant names. I appreciate that.
> 
> On Sat, 24 Jun 2006, Timo Hirvonen wrote:
> 
> > @@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
> >  	struct diff_options *opt = &rev->diffopt;
> >  	if (!p->len)
> >  		return;
> > -	switch (opt->output_format) {
> > -	case DIFF_FORMAT_RAW:
> > -	case DIFF_FORMAT_NAME_STATUS:
> > -	case DIFF_FORMAT_NAME:
> > +	if (opt->output_format & (DIFF_FORMAT_RAW |
> > +				  DIFF_FORMAT_NAME |
> > +				  DIFF_FORMAT_NAME_STATUS)) {
> >  		show_raw_diff(p, num_parent, rev);
> > -		return;
> > -	case DIFF_FORMAT_PATCH:
> > +	} else if (opt->output_format & DIFF_FORMAT_PATCH) {
> 
> Not that it matters, but this "else" could go. (Otherwise,  "--raw -p" 
> would be the same as "--raw", right?)

Just tested, ./git log -p --raw displays both raw and patch.  I think it
works because I changed diff_tree_combined() to use show_raw_diff() and
show_patch_diff() directly.

It feels 'wrong' to check flags and then call a function which checks
the flags again.  This combined diff stuff is confusing.

> > @@ -856,19 +846,18 @@ void diff_tree_combined(const unsigned c
> > [...]
> >  
> > -		if (do_diffstat && rev->loginfo)
> > -			show_log(rev, rev->loginfo,
> > -				 opt->with_stat ? "---\n" : "\n");
> > +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
> > +			show_log(rev, rev->loginfo, "---\n");
> >  		diff_flush(&diffopts);
> > -		if (opt->with_stat)
> > +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
> >  			putchar('\n');
> >  	}
> 
> Just a remark: this hunk actually changes behaviour. "with_stat" meant 
> that the stat was prepended before something like a patch, and therefore a 
> separator was needed. If you pass only "--stat", the separator will be 
> printed anyway now.

You are right, it now prints --- when it should print empty line.

> > +	int inter_name_termination = '\t';
> > +	int line_termination = options->line_termination;
> > +
> > +	if (!line_termination)
> > +		inter_name_termination = 0;
> 
> <nit type=minor>
> 	This should be part of patch 1/7.
> </nit>

That clean up was possible only after I made other changes to the code,
I think.  At least it wasn't obvious when I wrote 1/7.

> >  		show_stats(diffstat);
> >  		free(diffstat);
> 
> Why not go the full nine yards, and make diffstat not a pointer, but the 
> struct itself? You would avoid calloc()ing and free()ing. (Of course, 
> instead of calloc()ing you have to memset() it to 0.)

I was blind :)

> > +	if (output_format & DIFF_FORMAT_PATCH) {
> > +		if (output_format & (DIFF_FORMAT_DIFFSTAT |
> > +				     DIFF_FORMAT_SUMMARY)) {
> > +			if (options->stat_sep)
> > +				fputs(options->stat_sep, stdout);
> > +			else
> > +				putchar(options->line_termination);
> 
> Are we sure we do not want something like
> 
> 	if (output_format / DIFF_FORMAT_DIFFSTAT > 1)
> 		/* output separator */
> 
> after each format (this example being after the diffstat), the condition 
> being: if there is still an output format to come, add the separator?

I'm not sure what you mean.

It outputs separator between (diffstat and/or summary) and patch.
There's no separator between diffstat and summary or raw and diffstat.
Should there be one?


Thanks for your comments.  Should I patch the patch or send a fixed one?
I'm currently too tired to write any code.

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