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

Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

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?

That was the only way I could think of to separate pre-image name
from posi-image name for renames. Note that file name might look like
(part of) diffstat line, and there is no 'status' field in the
numstat to mark rename (as there is in "git diff-tree --raw" output).

But it doesn't mean that this is the only way, that is why it is RFC.
Well that and the fact that this patch increases slightly memory
footprint.

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

Right, I forgot to run "make test" to check which tests scripts
would need adjusting.

But result of "git grep -e -M --and -e --numstat -- t/" is empty,
so I don't think that any script test --numstat with rename detection.

quoted

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?

Did you mean --stat here? Because

  --summary::
        Output a condensed summary of extended header information
        such as creations, renames and mode changes.

I'd like to have diffstat for merge, similar to what "git pull <repo>"
does when doing true merge, not what "git commit" does.

And --stat is meant for human consumption, not for machine consumption.
File name may contain " => " inside. And there is no way to differentiate
between " => " in file name, and " => " separating "src" path name from
"dst" path name.

Have you actually _tested_ your patch?

Compiled, but forgot to run "make test". But I have checked that it passes
"make test", which probably mean that we don't have enough coverage ;-)

quoted

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

It passes "make test". I should perhaps mark more strongly that some
of _ideas_ are untested...

-- 
Jakub Narebski
Poland