{"thread":{"id":"8021","subject":"[PATCH/RFC] diff: Make numstat machine friendly also for renames","startedAt":"2007-05-08T00:43:42Z","lastAt":"2007-05-09T04:59:23Z","messageCount":6,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"41375","messageId":"11785850223782-git-send-email-jnareb@gmail.com","threadId":"8021","inReplyTo":null,"subject":"[PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-08T00:43:42Z","receivedAt":"2007-05-08T00:43:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Instead of saving human readable rename information in the 'name'\nfield when diffstat info is generated, do it when writing --stat\noutput. Change --numstat output to be machine friendly.\n\nThis makes result of git-diff --numstat more suitable for machines\nalso when renames are involved, by using format similar to the one for\nrenames in the raw diff format, instead of the format more suited for\nhumans.\n\nThe numstat format for rename is now\n\n  added deleted TAB path for \"src\" TAB path for \"dst\" LF\n\nor if -z option is used\n\n  added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n\nWhen -z option is not used, \", TAB, LF, and backslash characters in\npathnames are represented as \\\", \\t, \\n, and \\\\, respectively. If any\ncharacter needs to be quoted then pathnames are enclosed in double\nquotes.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis change increases memory footprint a bit, as struct diffstat_file\nis wider by sizeof(char *) wide field, which is NULL except for\nrenames.\n\nI have thought about storing it using one pointer to fragment of\nmemory of the form \"dst name \\0 src name \\0\", but this trades a little\nmemory for CPU time.\n\n\nThe goal of this change is to make it possible to generate HTML\ndiffstat against first parent for merge commits in gitweb. The current\nnotation for renames, which looks for example like below:\n\n  t/{t6030-bisect-run.sh => t6030-bisect-porcelain.sh}\n\nis not easy to parse by machines (note that filename may contain\n\"=>\"), but easy to understand _usually_ by humans.\n\n\nP.S. By the way, what is the difference between quote_one and\nquote_c_style, i.e. when one calls one and when the other?\n\n\n diff.c |   22 ++++++++++++++++++----\n 1 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7bbe759..568c59b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -703,6 +703,7 @@ struct diffstat_t {\n \tint alloc;\n \tstruct diffstat_file {\n \t\tchar *name;\n+\t\tchar *from_name;\n \t\tunsigned is_unmerged:1;\n \t\tunsigned is_binary:1;\n \t\tunsigned is_renamed:1;\n@@ -722,12 +723,13 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,\n \t\t\t\tdiffstat->alloc * sizeof(x));\n \t}\n \tdiffstat->files[diffstat->nr++] = x;\n+\tx->name = xstrdup(name_a);\n \tif (name_b) {\n-\t\tx->name = pprint_rename(name_a, name_b);\n+\t\tx->from_name = xstrdup(name_b);\n \t\tx->is_renamed = 1;\n \t}\n \telse\n-\t\tx->name = xstrdup(name_a);\n+\t\tx->from_name = NULL;\n \treturn x;\n }\n \n@@ -805,7 +807,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\tstruct diffstat_file *file = data->files[i];\n \t\tint change = file->added + file->deleted;\n \n-\t\tif (!file->is_renamed) {  /* renames are already quoted by pprint_rename */\n+\t\tif (!file->is_renamed) {  /* renames are quoted by pprint_rename */\n \t\t\tlen = quote_c_style(file->name, NULL, NULL, 0);\n \t\t\tif (len) {\n \t\t\t\tchar *qname = xmalloc(len + 1);\n@@ -813,6 +815,10 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\t\tfree(file->name);\n \t\t\t\tfile->name = qname;\n \t\t\t}\n+\t\t} else {\n+\t\t\tchar *qname = pprint_rename(file->name, file->from_name);\n+\t\t\tfree(file->name);\n+\t\t\tfile->name = qname;\n \t\t}\n \n \t\tlen = strlen(file->name);\n@@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data, struct diff_options *options)\n \t\t\tprintf(\"-\\t-\\t\");\n \t\telse\n \t\t\tprintf(\"%d\\t%d\\t\", file->added, file->deleted);\n-\t\tif (options->line_termination && !file->is_renamed &&\n+\t\tif (options->line_termination &&\n \t\t    quote_c_style(file->name, NULL, NULL, 0))\n \t\t\tquote_c_style(file->name, NULL, stdout, 0);\n \t\telse\n \t\t\tfputs(file->name, stdout);\n+\t\tif (file->is_renamed) {\n+\t\t\tprintf(\"%s\", options->line_termination ? \"\\t\" : \"\\0\\0\");\n+\t\t\tif (options->line_termination &&\n+\t\t\t    quote_c_style(file->from_name, NULL, NULL, 0))\n+\t\t\t\tquote_c_style(file->from_name, NULL, stdout, 0);\n+\t\t\telse\n+\t\t\t\tfputs(file->from_name, stdout);\n+\t\t}\n \t\tputchar(options->line_termination);\n \t}\n }\n-- \n1.5.1.3\n"},{"id":"41377","messageId":"7vzm4g5ddu.fsf@assigned-by-dhcp.cox.net","threadId":"8021","inReplyTo":"11785850223782-git-send-email-jnareb@gmail.com","subject":"Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-08T01:10:21Z","receivedAt":"2007-05-08T01:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Instead of saving human readable rename information in the 'name'\n> field when diffstat info is generated, do it when writing --stat\n> output. Change --numstat output to be machine friendly.\n>\n> This makes result of git-diff --numstat more suitable for machines\n> also when renames are involved, by using format similar to the one for\n> renames in the raw diff format, instead of the format more suited for\n> humans.\n>\n> The numstat format for rename is now\n>\n>   added deleted TAB path for \"src\" TAB path for \"dst\" LF\n>\n> or if -z option is used\n>\n>   added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n\nWhy two NULs?\n\nThere are already a handful in-tree users of --numstat, and also\na few tests scripts.  I think you would need to adjust them.\n\n> The goal of this change is to make it possible to generate HTML\n> diffstat against first parent for merge commits in gitweb. The current\n> notation for renames, which looks for example like below:\n>\n>   t/{t6030-bisect-run.sh => t6030-bisect-porcelain.sh}\n\nI do not have much objection against teaching --numstat to show\nthe preimage pathnames.  I do not disagree with \"the goal\" of\nshowing \"git diff --stat -M $commit^1 $commit\" even for merge\ncommit.\n\nBut I do not see the connection between the two.  Why aren't you\nparsing --summary?\n\nHave you actually _tested_ your patch?\n\n> @@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data,\n>  \t\t\tprintf(\"-\\t-\\t\");\n>  \t\telse\n>  \t\t\tprintf(\"%d\\t%d\\t\", file->added, file->deleted);\n> -\t\tif (options->line_termination && !file->is_renamed &&\n> +\t\tif (options->line_termination &&\n>  \t\t    quote_c_style(file->name, NULL, NULL, 0))\n>  \t\t\tquote_c_style(file->name, NULL, stdout, 0);\n>  \t\telse\n>  \t\t\tfputs(file->name, stdout);\n> +\t\tif (file->is_renamed) {\n> +\t\t\tprintf(\"%s\", options->line_termination ? \"\\t\" : \"\\0\\0\");\n\nI know you marked it as RFC; but it is impolite to request\ncomments from other people on a patch that does not do what you\nintended to do, without marking \"this is untested\".  It would\nwaste people's time.\n"},{"id":"41381","messageId":"200705080345.26817.jnareb@gmail.com","threadId":"8021","inReplyTo":"7vzm4g5ddu.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-08T01:45:25Z","receivedAt":"2007-05-08T01:45:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> Instead of saving human readable rename information in the 'name'\n>> field when diffstat info is generated, do it when writing --stat\n>> output. Change --numstat output to be machine friendly.\n>>\n>> This makes result of git-diff --numstat more suitable for machines\n>> also when renames are involved, by using format similar to the one for\n>> renames in the raw diff format, instead of the format more suited for\n>> humans.\n>>\n>> The numstat format for rename is now\n>>\n>>   added deleted TAB path for \"src\" TAB path for \"dst\" LF\n>>\n>> or if -z option is used\n>>\n>>   added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n> \n> Why two NULs?\n\nThat was the only way I could think of to separate pre-image name\nfrom posi-image name for renames. Note that file name might look like\n(part of) diffstat line, and there is no 'status' field in the\nnumstat to mark rename (as there is in \"git diff-tree --raw\" output).\n\nBut it doesn't mean that this is the only way, that is why it is RFC.\nWell that and the fact that this patch increases slightly memory\nfootprint.\n\n> There are already a handful in-tree users of --numstat, and also\n> a few tests scripts.  I think you would need to adjust them.\n\nRight, I forgot to run \"make test\" to check which tests scripts\nwould need adjusting.\n\nBut result of \"git grep -e -M --and -e --numstat -- t/\" is empty,\nso I don't think that any script test --numstat with rename detection.\n\n>> The goal of this change is to make it possible to generate HTML\n>> diffstat against first parent for merge commits in gitweb. The current\n>> notation for renames, which looks for example like below:\n>>\n>>   t/{t6030-bisect-run.sh => t6030-bisect-porcelain.sh}\n> \n> I do not have much objection against teaching --numstat to show\n> the preimage pathnames.  I do not disagree with \"the goal\" of\n> showing \"git diff --stat -M $commit^1 $commit\" even for merge\n> commit.\n> \n> But I do not see the connection between the two.  Why aren't you\n> parsing --summary?\n\nDid you mean --stat here? Because\n\n  --summary::\n        Output a condensed summary of extended header information\n        such as creations, renames and mode changes.\n\nI'd like to have diffstat for merge, similar to what \"git pull <repo>\"\ndoes when doing true merge, not what \"git commit\" does.\n\nAnd --stat is meant for human consumption, not for machine consumption.\nFile name may contain \" => \" inside. And there is no way to differentiate\nbetween \" => \" in file name, and \" => \" separating \"src\" path name from\n\"dst\" path name.\n\n> Have you actually _tested_ your patch?\n\nCompiled, but forgot to run \"make test\". But I have checked that it passes\n\"make test\", which probably mean that we don't have enough coverage ;-)\n\n>> @@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data,\n>>  \t\t\tprintf(\"-\\t-\\t\");\n>>  \t\telse\n>>  \t\t\tprintf(\"%d\\t%d\\t\", file->added, file->deleted);\n>> -\t\tif (options->line_termination && !file->is_renamed &&\n>> +\t\tif (options->line_termination &&\n>>  \t\t    quote_c_style(file->name, NULL, NULL, 0))\n>>  \t\t\tquote_c_style(file->name, NULL, stdout, 0);\n>>  \t\telse\n>>  \t\t\tfputs(file->name, stdout);\n>> +\t\tif (file->is_renamed) {\n>> +\t\t\tprintf(\"%s\", options->line_termination ? \"\\t\" : \"\\0\\0\");\n> \n> I know you marked it as RFC; but it is impolite to request\n> comments from other people on a patch that does not do what you\n> intended to do, without marking \"this is untested\".  It would\n> waste people's time.\n\nIt passes \"make test\". I should perhaps mark more strongly that some\nof _ideas_ are untested...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"41389","messageId":"7vhcqo5b64.fsf@assigned-by-dhcp.cox.net","threadId":"8021","inReplyTo":"200705080345.26817.jnareb@gmail.com","subject":"Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-08T01:58:11Z","receivedAt":"2007-05-08T01:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>>> The numstat format for rename is now\n>>>\n>>>   added deleted TAB path for \"src\" TAB path for \"dst\" LF\n>>>\n>>> or if -z option is used\n>>>\n>>>   added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n>> \n>> Why two NULs?\n>\n> That was the only way I could think of to separate pre-image name\n> from posi-image name for renames. Note that file name might look like\n> (part of) diffstat line, and there is no 'status' field in the\n> numstat to mark rename (as there is in \"git diff-tree --raw\" output).\n\nThe --stat format is for human consumption, and --numstat (be it\nwith -z or without) is for machines, so I am not opposed to a\nformat change that gives information that is already computed\nbut currently is hard to parse.  If the format change breaks\nexisting scripts, we might want to do --numstat-extended,\nthough...\n\nFor example, I do not see a reason not to add \"R98\" in there.\nI.e.\n\n\tadded deleted status TAB \"src\" (TAB \"dst\"){0,1} LF\n\tadded deleted status NUL \"src\" (NUL \"dst\"){0,1} NUL\n\nwhere the dst path is present only when status says it is a\nrename/copy, just like the --raw format.\n\n> Did you mean --stat here?\n\nNo, I did mean --summary.  But that was foolish of me.  I forgot\nthat it had the same { namepart => namepart } issue.\n\n>>> @@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data,\n>>>  \t\t\tprintf(\"-\\t-\\t\");\n>>>  \t\telse\n>>>  \t\t\tprintf(\"%d\\t%d\\t\", file->added, file->deleted);\n>>> -\t\tif (options->line_termination && !file->is_renamed &&\n>>> +\t\tif (options->line_termination &&\n>>>  \t\t    quote_c_style(file->name, NULL, NULL, 0))\n>>>  \t\t\tquote_c_style(file->name, NULL, stdout, 0);\n>>>  \t\telse\n>>>  \t\t\tfputs(file->name, stdout);\n>>> +\t\tif (file->is_renamed) {\n>>> +\t\t\tprintf(\"%s\", options->line_termination ? \"\\t\" : \"\\0\\0\");\n>\n> It passes \"make test\".\n\nBut you already grepped to make sure this codepath is never\nexercised ;-).\n\nWhat I was hoping you to notice was that printf(\"%s\", \"\\0\\0\")\nthing.  %s would not even notice that the const char[] literal\nis 2 bytes long.\n"},{"id":"41447","messageId":"200705081433.58931.jnareb@gmail.com","threadId":"8021","inReplyTo":"7vhcqo5b64.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-08T12:33:57Z","receivedAt":"2007-05-08T12:33:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>>>> The numstat format for rename is now\n>>>>\n>>>>   added deleted TAB path for \"src\" TAB path for \"dst\" LF\n>>>>\n>>>> or if -z option is used\n>>>>\n>>>>   added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n>>> \n>>> Why two NULs?\n>>\n>> That was the only way I could think of to separate pre-image name\n>> from posi-image name for renames. Note that file name might look like\n>> (part of) diffstat line, and there is no 'status' field in the\n>> numstat to mark rename (as there is in \"git diff-tree --raw\" output).\n> \n> The --stat format is for human consumption, and --numstat (be it\n> with -z or without) is for machines, so I am not opposed to a\n> format change that gives information that is already computed\n> but currently is hard to parse.  If the format change breaks\n> existing scripts, we might want to do --numstat-extended,\n> though...\n> \n> For example, I do not see a reason not to add \"R98\" in there.\n> I.e.\n> \n> \tadded deleted status TAB \"src\" (TAB \"dst\"){0,1} LF\n> \tadded deleted status NUL \"src\" (NUL \"dst\"){0,1} NUL\n> \n> where the dst path is present only when status says it is a\n> rename/copy, just like the --raw format.\n\nThat is a good idea, but wouldn't it break existing scripts? Well,\nbreak more than a bit hacky idea of using NUL NUL as separator between\npre-image name and post-image name.\n\nThis would change output in every case, while my proposal doesn't change\noutput for the case without renames. '-M' should also work correctly,\nperhaps scripts using it getting wrong filename. It is '-z -M' that changes\nmost.\n\n>> Did you mean --stat here?\n> \n> No, I did mean --summary.  But that was foolish of me.  I forgot\n> that it had the same { namepart => namepart } issue.\n\nAh. I somehow didn't get then that you meant for --numstat to have only\npost-image names, and get pre-image names from rename information in\n--summary. But as you have noticed it wouldn't help: rename information\nis in the same for-humans format.\n\n>>>> @@ -949,11 +955,19 @@ static void show_numstat(struct diffstat_t* data,\n>>>>  \t\t\tprintf(\"-\\t-\\t\");\n>>>>  \t\telse\n>>>>  \t\t\tprintf(\"%d\\t%d\\t\", file->added, file->deleted);\n>>>> -\t\tif (options->line_termination && !file->is_renamed &&\n>>>> +\t\tif (options->line_termination &&\n>>>>  \t\t    quote_c_style(file->name, NULL, NULL, 0))\n>>>>  \t\t\tquote_c_style(file->name, NULL, stdout, 0);\n>>>>  \t\telse\n>>>>  \t\t\tfputs(file->name, stdout);\n>>>> +\t\tif (file->is_renamed) {\n>>>> +\t\t\tprintf(\"%s\", options->line_termination ? \"\\t\" : \"\\0\\0\");\n>\n> What I was hoping you to notice was that printf(\"%s\", \"\\0\\0\")\n> thing.  %s would not even notice that the const char[] literal\n> is 2 bytes long.\n\nOoops. Shame on me. That is the result of trying to be too smart... \nand changing separator between pathnames for rename from NUL to NUL NUL.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"41535","messageId":"7vd51azj6c.fsf@assigned-by-dhcp.cox.net","threadId":"8021","inReplyTo":"200705081433.58931.jnareb@gmail.com","subject":"Re: [PATCH/RFC] diff: Make numstat machine friendly also for renames","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-09T04:59:23Z","receivedAt":"2007-05-09T04:59:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> The --stat format is for human consumption, and --numstat (be it\n>> with -z or without) is for machines, so I am not opposed to a\n>> format change that gives information that is already computed\n>> but currently is hard to parse.  If the format change breaks\n>> existing scripts, we might want to do --numstat-extended,\n>> though...\n>> \n>> For example, I do not see a reason not to add \"R98\" in there.\n>> I.e.\n>> \n>> \tadded deleted status TAB \"src\" (TAB \"dst\"){0,1} LF\n>> \tadded deleted status NUL \"src\" (NUL \"dst\"){0,1} NUL\n>> \n>> where the dst path is present only when status says it is a\n>> rename/copy, just like the --raw format.\n>\n> That is a good idea, but wouldn't it break existing scripts? Well,\n> break more than a bit hacky idea of using NUL NUL as separator between\n> pre-image name and post-image name.\n\nI think both would break equally.  That's why I hinted --numstat-extended,\nbut I think the information is getting to be about --machine-readable, and\nnot necessarily about \"stat\" anymore.\n"}]}