git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jun 24, 2006, 20:52 UTC
Message-ID
<Pine.LNX.4.63.0606242219320.29667@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To
<20060624202153.1001a66c.tihirvon@gmail.com>
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:
Show 15 quoted lines
> @@ -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?)

Show 6 quoted lines
>  		show_patch_diff(p, num_parent, dense, rev);
> -		return;
> -	default:
> -		return;
>  	}
>  }
Show 13 quoted lines
> @@ -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.

Show 15 quoted lines
> diff --git a/diff.c b/diff.c
> index f358546..bfed79c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1372,23 +1372,27 @@ int diff_setup_done(struct diff_options 
>  	    (0 <= options->rename_limit && !options->detect_rename))
>  		return -1;
>  
> +	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
> +		options->output_format = 0;
> +
> +	if (options->output_format & (DIFF_FORMAT_NAME |
> +				      DIFF_FORMAT_NAME_STATUS |
> +				      DIFF_FORMAT_CHECKDIFF |
> +				      DIFF_FORMAT_NO_OUTPUT))

The DIFF_FORMAT_NO_OUTPUT here makes no sense (if it was set, you unset it above).

Show 19 quoted lines
> @@ -1671,15 +1674,17 @@ const char *diff_unique_abbrev(const uns
> [...]
>  
>  static void diff_flush_raw(struct diff_filepair *p,
> -			   int line_termination,
> -			   int inter_name_termination,
> -			   struct diff_options *options,
> -			   int output_format)
> +			   struct diff_options *options)
>  {
>  	int two_paths;
>  	char status[10];
>  	int abbrev = options->abbrev;
>  	const char *path_one, *path_two;
> +	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>
Show 13 quoted lines
> @@ -2041,55 +2028,61 @@ static void diff_summary(struct diff_fil
> [...]
>  
> -	if (options->with_raw) {
> +	if (output_format & (DIFF_FORMAT_RAW |
> +			     DIFF_FORMAT_NAME |
> +			     DIFF_FORMAT_NAME_STATUS |
> +			     DIFF_FORMAT_CHECKDIFF)) {
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			flush_one_pair(p, DIFF_FORMAT_RAW, options, NULL);
> +			if (check_pair_status(p))
> +				flush_one_pair(p, options);
This is a very nice cleanup.
Show 14 quoted lines
>  	}
> -	if (options->with_stat) {
> +
> +	if (output_format & DIFF_FORMAT_DIFFSTAT) {
> +		struct diffstat_t *diffstat;
> +
> +		diffstat = xcalloc(sizeof (struct diffstat_t), 1);
> +		diffstat->xm.consume = diffstat_consume;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			flush_one_pair(p, DIFF_FORMAT_DIFFSTAT, options,
> -				       diffstat);
> +			if (check_pair_status(p))
> +				diff_flush_stat(p, options, diffstat);
Again, very nice.
>  		}
>  		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.)

Show 7 quoted lines
> +	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?

All in all, I like this patch.

Ciao, Dscho

Previous: Timo HirvonenNext: Timo Hirvonen
Message 4 of 20 in “Rework diff options”
  1. 0/7 Rework diff optionsTimo Hirvonen, Jun 24, 2006
  2. 1/7 Clean up diff.cTimo Hirvonen, Jun 24, 2006
  3. 2/7 Merge with_raw, with_stat and summary variables to output_formatTimo Hirvonen, Jun 24, 2006
  4. Johannes SchindelinJun 24, 2006
  5. Timo HirvonenJun 24, 2006
  6. Johannes SchindelinJun 24, 2006
  7. Add msg_sep to diff_optionsTimo Hirvonen, Jun 25, 2006
  8. Junio C HamanoJun 25, 2006
  9. whatchanged: Default to DIFF_FORMAT_RAWTimo Hirvonen, Jun 25, 2006
  10. Junio C HamanoJun 25, 2006
  11. whatchanged: Default to DIFF_FORMAT_RAWTimo Hirvonen, Jun 25, 2006
  12. Don't xcalloc() struct diffstat_tTimo Hirvonen, Jun 25, 2006
  13. 3/7 Make --raw option available for all diff commandsTimo Hirvonen, Jun 24, 2006
  14. 4/7 Set default diff output format after parsing command lineTimo Hirvonen, Jun 24, 2006
  15. 5/7 DIFF_FORMAT_RAW is not default anymoreTimo Hirvonen, Jun 24, 2006
  16. 6/7 --name-only, --name-status, --check and -s are mutually exclusiveTimo Hirvonen, Jun 24, 2006
  17. 7/7 Remove awkward compatibility wartsTimo Hirvonen, Jun 24, 2006
  18. Junio C HamanoJun 25, 2006
  19. Timo HirvonenJun 25, 2006
  20. Junio C HamanoJun 26, 2006

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.