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

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> 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.

>> 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 ;-)

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