Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames

From: Junio C Hamano <junkio@cox.net>
Date: May 8, 2007, 01:10 UTC
Message-ID: <7vzm4g5ddu.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11785850223782-git-send-email-jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 16 quoted lines

> Instead of saving human readable rename information in the 'name'
> field when diffstat info is generated, do it when writing --stat
> output. Change --numstat output to be machine friendly.
>
> This makes result of git-diff --numstat more suitable for machines
> also when renames are involved, by using format similar to the one for
> renames in the raw diff format, instead of the format more suited for
> humans.
>
> The numstat format for rename is now
>
>   added deleted TAB path for "src" TAB path for "dst" LF
>
> or if -z option is used
>
>   added deleted TAB path for "src" NUL NUL path for "dst" NUL

Why two NULs?

There are already a handful in-tree users of --numstat, and also a few tests scripts. I think you would need to adjust them.

Show 5 quoted lines

> The goal of this change is to make it possible to generate HTML
> diffstat against first parent for merge commits in gitweb. The current
> notation for renames, which looks for example like below:
>
>   t/{t6030-bisect-run.sh => t6030-bisect-porcelain.sh}

I do not have much objection against teaching --numstat to show the preimage pathnames. I do not disagree with "the goal" of showing "git diff --stat -M $commit^1 $commit" even for merge commit.

But I do not see the connection between the two. Why aren't you parsing --summary?

Have you actually _tested_ your patch?

Show 12 quoted lines

> @@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data,
>  			printf("-\t-\t");
>  		else
>  			printf("%d\t%d\t", file->added, file->deleted);
> -		if (options->line_termination && !file->is_renamed &&
> +		if (options->line_termination &&
>  		    quote_c_style(file->name, NULL, NULL, 0))
>  			quote_c_style(file->name, NULL, stdout, 0);
>  		else
>  			fputs(file->name, stdout);
> +		if (file->is_renamed) {
> +			printf("%s", options->line_termination ? "\t" : "\0\0");

I know you marked it as RFC; but it is impolite to request comments from other people on a patch that does not do what you intended to do, without marking "this is untested". It would waste people's time.