{"thread":{"id":"11232","subject":"[PATCH] diff: Make numstat machine friendly also for renames (and copies)","startedAt":"2007-12-10T22:32:51Z","lastAt":"2007-12-12T19:07:40Z","messageCount":12,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"62625","messageId":"200712102332.53114.jnareb@gmail.com","threadId":"11232","inReplyTo":null,"subject":"[PATCH] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-10T22:32:51Z","receivedAt":"2007-12-10T22:32:51Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"git diff --numstat\" used the same format as \"git diff --stat\" for\nrenamed (and copied) files, except that filenames were not shortened\nwhen they didn't fit in the column width. This format is suitable for\nhuman consumption, but it cannot be unambiguously parsed.\n\nInstead of that always use final file name (\"to\" name) for numstat.\nIt is possible to find name before rename when name after is known.\n\nThis required to use pprint_rename (pretty print rename) during output\n(in the show_stats function) and not during parsing (in diffstat_add\nfunction).\n\nAdding from_name field to struct diffstat_t makes is_renamed bitfield\nredundant; nevertheless for the sake of clarity, readability and\nmaking this patch minimal (and because it would not reduce memory\nfootprint) it was not removed, and its used not replaced by checking\nfrom_name field.\n\nSigned-off-by: Jakub Narebski \n---\nThis would be useful for gitweb, later.\n\nI hope I have made it in time before feature freeze...\n\n diff.c | 15 ++++++++++++---\n 1 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f780e3e..38b9367 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -735,6 +735,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@@ -755,11 +756,14 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,\n \t}\n \tdiffstat->files[diffstat->nr++] = x;\n \tif (name_b) {\n-\t\tx->name = pprint_rename(name_a, name_b);\n+\t\tx->from_name = xstrdup(name_a);\n+\t\tx->name = xstrdup(name_b);\n \t\tx->is_renamed = 1;\n \t}\n-\telse\n+\telse {\n+\t\tx->from_name = NULL;\n \t\tx->name = xstrdup(name_a);\n+\t}\n \treturn x;\n }\n \n@@ -837,7 +841,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 will be quoted by pprint_rename */\n \t\t\tstruct strbuf buf;\n \t\t\tstrbuf_init(&buf, 0);\n \t\t\tif (quote_c_style(file->name, &buf, NULL, 0)) {\n@@ -846,6 +850,11 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\t} else {\n \t\t\t\tstrbuf_release(&buf);\n \t\t\t}\n+\t\t} else {\n+\t\t\tchar *qname = pprint_rename(file->from_name, file->name);\n+\t\t\tfree(file->name);\n+\t\t\tfree(file->from_name);\n+\t\t\tfile->name = qname;\n \t\t}\n \n \t\tlen = strlen(file->name);\n-- \n1.5.3.7\n"},{"id":"62628","messageId":"200712102355.39084.jnareb@gmail.com","threadId":"11232","inReplyTo":"200712102332.53114.jnareb@gmail.com","subject":"[PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-10T22:55:38Z","receivedAt":"2007-12-10T22:55:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"git diff --numstat\" used the same format as \"git diff --stat\" for\nrenamed (and copied) files, except that filenames were not shortened\nwhen they didn't fit in the column width. This format is suitable for\nhuman consumption, but it cannot be unambiguously parsed.\n\nInstead of that always use final file name (\"to\" name) for numstat.\nIt is possible to find name before rename when name after is known.\n\nThis required to use pprint_rename (pretty print rename) during output\n(in the show_stats function) and not during parsing (in diffstat_add\nfunction).\n\nAdding from_name field to struct diffstat_t makes is_renamed bitfield\nredundant; nevertheless for the sake of clarity, readability and\nmaking this patch minimal (and because it would not reduce memory\nfootprint) it was not removed, and its used not replaced by checking\nfrom_name field.\n\nSigned-off-by: Jakub Narebski \n---\nSorry for mistake: I have tested this commit, corrected it... and forgot\nto update patch to send.\n\n\nPrevious version of this patch (from 7 May 2007) used instead of current\nonly \"to_name\" format similar to git-diff-tree raw format for renames:\n\n added deleted TAB path for \"src\" TAB path for \"dst\" LF\n \nThe problem was when -z option was used: how to separate end of record\nfrom end of from_name and start of to_name. For git-diff we have status\nto distinguish those; no such thing for numstat output. Previous version\nof patch used (or was to use actually, because of error in the code)\n\n added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n\nwhen -z option was used.\n \nThis is left now for future --numstat-extended option...\n\n diff.c | 22 +++++++++++++---------\n 1 files changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f780e3e..8039ac7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -735,6 +735,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@@ -755,11 +756,14 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,\n \t}\n \tdiffstat->files[diffstat->nr++] = x;\n \tif (name_b) {\n-\t\tx->name = pprint_rename(name_a, name_b);\n+\t\tx->from_name = xstrdup(name_a);\n+\t\tx->name = xstrdup(name_b);\n \t\tx->is_renamed = 1;\n \t}\n-\telse\n+\telse {\n+\t\tx->from_name = NULL;\n \t\tx->name = xstrdup(name_a);\n+\t}\n \treturn x;\n }\n \n@@ -837,7 +841,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 will be quoted by pprint_rename */\n \t\t\tstruct strbuf buf;\n \t\t\tstrbuf_init(&buf, 0);\n \t\t\tif (quote_c_style(file->name, &buf, NULL, 0)) {\n@@ -846,6 +850,11 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\t} else {\n \t\t\t\tstrbuf_release(&buf);\n \t\t\t}\n+\t\t} else {\n+\t\t\tchar *qname = pprint_rename(file->from_name, file->name);\n+\t\t\tfree(file->name);\n+\t\t\tfree(file->from_name);\n+\t\t\tfile->name = qname;\n \t\t}\n \n \t\tlen = strlen(file->name);\n@@ -982,12 +991,7 @@ 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 (!file->is_renamed) {\n-\t\t\twrite_name_quoted(file->name, stdout, options->line_termination);\n-\t\t} else {\n-\t\t\tfputs(file->name, stdout);\n-\t\t\tputchar(options->line_termination);\n-\t\t}\n+\t\twrite_name_quoted(file->name, stdout, options->line_termination);\n \t}\n }\n \n-- \n1.5.3.7\n"},{"id":"62629","messageId":"7vir36jgty.fsf@gitster.siamese.dyndns.org","threadId":"11232","inReplyTo":"200712102332.53114.jnareb@gmail.com","subject":"Re: [PATCH] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-10T23:00:25Z","receivedAt":"2007-12-10T23:00:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> \"git diff --numstat\" used the same format as \"git diff --stat\" for\n> renamed (and copied) files, except that filenames were not shortened\n> when they didn't fit in the column width. This format is suitable for\n> human consumption, but it cannot be unambiguously parsed.\n\nAgreed about the (un)parsability, and --numstat is all about parsability\nso I would not object. A fix is really needed there.\n\nI do not have time to look at the patch right now, but if the changed\noutput is in line with what --name-status would show, that would be\ngreat. I'd call that \"the format that should have been from day one\".\n\nI.e. no '=>' rename marker, but show two names c-quoted (unless -z is\nused) and separated with inter_name_termination). IIRC, that is how\nrename/copy is shown with --name-status.\n"},{"id":"62631","messageId":"200712110014.48343.jnareb@gmail.com","threadId":"11232","inReplyTo":"7vir36jgty.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-10T23:14:47Z","receivedAt":"2007-12-10T23:14:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > \"git diff --numstat\" used the same format as \"git diff --stat\" for\n> > renamed (and copied) files, except that filenames were not shortened\n> > when they didn't fit in the column width. This format is suitable for\n> > human consumption, but it cannot be unambiguously parsed.\n> \n> Agreed about the (un)parsability, and --numstat is all about parsability\n> so I would not object. A fix is really needed there.\n> \n> I do not have time to look at the patch right now, but if the changed\n> output is in line with what --name-status would show, that would be\n> great. I'd call that \"the format that should have been from day one\".\n> \n> I.e. no '=>' rename marker, but show two names c-quoted (unless -z is\n> used) and separated with inter_name_termination). IIRC, that is how\n> rename/copy is shown with --name-status.\n\nUnfortunately this is not possible, at least if we want to retain\nthe assertion that -z output looks like normal output, only without\nquoting.\n\ndiff --name-status has _status_ field which can be used to distinguish\nif the NUL (for -z output) is the end of source filename, or the end\nof record.\n\nThe patch send changes --numstat to use only _destination_ name.\nWhat you want I'd left for futore --numstat-extended (basically --numstat,\nbut with status field.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"62633","messageId":"7vejdujazu.fsf@gitster.siamese.dyndns.org","threadId":"11232","inReplyTo":"200712102355.39084.jnareb@gmail.com","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-11T01:06:29Z","receivedAt":"2007-12-11T01:06:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Previous version of this patch (from 7 May 2007) used instead of current\n> only \"to_name\" format similar to git-diff-tree raw format for renames:\n>\n> added deleted TAB path for \"src\" TAB path for \"dst\" LF\n> \n> The problem was when -z option was used: how to separate end of record\n> from end of from_name and start of to_name. For git-diff we have status\n> to distinguish those; no such thing for numstat output. Previous version\n> of patch used (or was to use actually, because of error in the code)\n>\n> added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n>\n> when -z option was used.\n\nI think the cleanest at this point is to have --numstat-enhanced that\nshows\n\n\t \n \n\nAnything else would be a regression.\n"},{"id":"62635","messageId":"200712110226.35343.jnareb@gmail.com","threadId":"11232","inReplyTo":"7vejdujazu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-11T01:26:33Z","receivedAt":"2007-12-11T01:26:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > Previous version of this patch (from 7 May 2007) used instead of current\n> > only \"to_name\" format similar to git-diff-tree raw format for renames:\n> >\n> > added deleted TAB path for \"src\" TAB path for \"dst\" LF\n> > \n> > The problem was when -z option was used: how to separate end of record\n> > from end of from_name and start of to_name. For git-diff we have status\n> > to distinguish those; no such thing for numstat output. Previous version\n> > of patch used (or was to use actually, because of error in the code)\n> >\n> > added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n> >\n> > when -z option was used.\n> \n> I think the cleanest at this point is to have --numstat-enhanced that\n> shows\n> \n> \t \n> \t \n> \n> Anything else would be a regression.\n\nThat is the plan[*1*]. Nevertheless always using destination filename for\n\"ordinary\" numstat is a step in good direction. I don't think that would\nbreak _any_ scripts (as previous version was not good to be parsed by\na machine); I think it is even more probable that old version _broke_\nscripts if -M / -C was provided.\n\n[*1*] When I (or somebody else) find time for that.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"62639","messageId":"7vprxehrlv.fsf@gitster.siamese.dyndns.org","threadId":"11232","inReplyTo":"200712110226.35343.jnareb@gmail.com","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-11T02:50:36Z","receivedAt":"2007-12-11T02:50:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Junio C Hamano wrote:\n>> Jakub Narebski writes:\n>> \n>> > Previous version of this patch (from 7 May 2007) used instead of current\n>> > only \"to_name\" format similar to git-diff-tree raw format for renames:\n>> >\n>> > added deleted TAB path for \"src\" TAB path for \"dst\" LF\n\nThat's perfectly fine (that is without -z).\n\n>> > ... Previous version\n>> > of patch used (or was to use actually, because of error in the code)\n>> >\n>> > added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n>> >\n>> > when -z option was used.\n\nI think your \"previous\" one:\n\n\t NUL\t\t (no rename)\n\t NUL NUL NUL (with rename)\n\nwould be unambiguously parsable, but it would be cleaner and easier for\nthe parser if the latter were like this instead:\n\n\t NUL NUL NUL (with rename)\n\nThe reader can expect to find a NUL terminated src, finds the length is\nzero and notices it cannot be src and then reads two paths from that\npoint.\n\nWithout having the status letter, we cannot do \"optional two paths\"\nwithout breaking existing --numstat -z readers that do not care about\nrenames and copies, and I agree that existing --numstat -z readers that\npass -M or -C are already broken, so I think a format extension along\nthe above line (your \"previous\" and the above clean-up proposal have the\nsame power of expressiveness, I just think the latter is syntactically\ncleaner) would be reasonable. And at that point we probably would not\nneed --numstat-enhanced at all ;-)\n"},{"id":"62826","messageId":"200712120009.36560.jnareb@gmail.com","threadId":"11232","inReplyTo":"7vprxehrlv.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-11T23:09:35Z","receivedAt":"2007-12-11T23:09:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 11 Dec 2007, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> Junio C Hamano wrote:\n>>> Jakub Narebski writes:\n>>> \n>>>> Previous version of this patch (from 7 May 2007) used instead of current\n>>>> only \"to_name\" format similar to git-diff-tree raw format for renames:\n>>>>\n>>>> added deleted TAB path for \"src\" TAB path for \"dst\" LF\n> \n> That's perfectly fine (that is without -z).\n> \n>>>> ... Previous version\n>>>> of patch used (or was to use actually, because of error in the code)\n>>>>\n>>>> added deleted TAB path for \"src\" NUL NUL path for \"dst\" NUL\n>>>>\n>>>> when -z option was used.\n> \n> I think your \"previous\" one:\n> \n> \t NUL\t\t (no rename)\n> \t NUL NUL NUL (with rename)\n> \n> would be unambiguously parsable, but it would be cleaner and easier for\n> the parser if the latter were like this instead:\n> \n> \t NUL NUL NUL (with rename)\n> \n> The reader can expect to find a NUL terminated src, finds the length is\n> zero and notices it cannot be src and then reads two paths from that\n> point.\n\nVery good idea.\n\n> Without having the status letter, we cannot do \"optional two paths\"\n> without breaking existing --numstat -z readers that do not care about\n> renames and copies, and I agree that existing --numstat -z readers that\n> pass -M or -C are already broken, so I think a format extension along\n> the above line (your \"previous\" and the above clean-up proposal have the\n> same power of expressiveness, I just think the latter is syntactically\n> cleaner) would be reasonable. And at that point we probably would not\n> need --numstat-enhanced at all ;-)\n\nThe previous patch is a fix (usability fix) for numstat output in the\npresence of rename detection, making it truly machine friendly. Moreover\nit should not break any scripts which parsed numstat output not\nexpecting to deal with renames (for example if repository has\ndiff.renames config option set), and might even fix them.\n\nThe proposed - and implemented in patch below - change could break some\nscripts not expecting to deal with new numstat output. Adding status\nletter (with similarity/dissimilarity percentage info) would\nunfortunately require greater surgery... We can either allow scripts\n(do there exist any?) to break, or make the full rename info shown\nonly for --numstat-extended; but I'd like to have this status letter\nfor --numstat-extended / --numstat-enhanced.\n\nSo possible solution are:\n (a) Accept both patches, and accept remote possibility that some\n scripts might break\n (b) Add --numstat-extended option whoich would show rename info\n as implemented in this patch (and of course accept previous)\n (c) Implement --numstat-extended with status letter, as suggested,\n accept first patch but not this one.\n\n\nNote that gitweb would require --numstat with proper rename detection\nsupport if we want to e.g. provide graphical diffstat in 'commit' view\nfor merge commits.\n\n\nP.S. The numstat output format for renames should be probably described\nin documentation, and not only in commit message, but I was not sure\nwhere to put it (and even how it should be written).\n\n-- >8 --\nFrom: Jakub Narebski \nDate: Tue, 11 Dec 2007 23:52:18 +0100\nSubject: [RFC/PATCH] diff: Show rename info in numstat output, correctly\n\nMake numstat output of git-diff show both source and destination\nfilename for renames (and copies) in easily parseable format, similar\nto raw diff output format.\n\nThe numstat format for rename (or copy) is now\n\n TAB TAB LF\n\nor if -z option is used\n\n TAB NULL NULL NULL\n\nwhere and is number of added/deleted lines\n(or '-' for binary file).\n\nWhen -z option is not used, TAB, LF, and backslash characters in\npathnames are represented as \\t, \\n, and \\\\, respectively.\n\n\nIdea for -z numstat rename format by Junio C Hamano.\n\nSigned-off-by: Jakub Narebski \n---\n diff.c | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8039ac7..56cbf28 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -991,7 +991,14 @@ 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\twrite_name_quoted(file->name, stdout, options->line_termination);\n+\t\tif (file->is_renamed) {\n+\t\t\tchar sep = options->line_termination ? '\\t' : '\\0';\n+\t\t\tif (!options->line_termination)\n+\t\t\t\tputchar(options->line_termination);\n+\t\t\twrite_name_quoted(file->from_name, stdout, sep);\n+\t\t\twrite_name_quoted(file->name, stdout, options->line_termination);\n+\t\t} else\n+\t\t\twrite_name_quoted(file->name, stdout, options->line_termination);\n \t}\n }\n \n-- \n1.5.3.7\n"},{"id":"62865","messageId":"7vabog9wyx.fsf@gitster.siamese.dyndns.org","threadId":"11232","inReplyTo":"200712120009.36560.jnareb@gmail.com","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-12T07:46:30Z","receivedAt":"2007-12-12T07:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> The previous patch is a fix (usability fix) for numstat output in the\n> presence of rename detection, making it truly machine friendly. Moreover\n> it should not break any scripts which parsed numstat output not\n> expecting to deal with renames (for example if repository has\n> diff.renames config option set), and might even fix them.\n>\n> The proposed - and implemented in patch below - change could break some\n> scripts not expecting to deal with new numstat output.\n\nI do not quite follow. If a script wanted -M and/or -C output in the\nolder \"common/{a => b}/common\" format, either change would break it\nequally badly, either with or without -z. And I do not think adding\nstatus letter is necessary for --numstat; if the caller wants that\ninformation, it can ask for it explicitly with both options.\n\nSo I do not think there is anything more to solve, than the patch you\njust sent.\n\nThe patch looks good. Thanks.\n\n> P.S. The numstat output format for renames should be probably described\n> in documentation, and not only in commit message, but I was not sure\n> where to put it (and even how it should be written).\n\nI started writing this, and then reviewed the result of squashing your\ntwo patches.\n\nAlthough everybody complains that most of git is run-once-and-exit, I\nwrote the original diff part fairly conservatively to make it leak-free,\nbecause of a single command, \"diff-tree --stdin\", that can be told to\nrun millions of diffs inside a single process.\n\nBut the diffstat part is horribly leaky, especially after your patch,\nand it has ugly workarounds such as refusing to show both diffstat and\nshortstat at the same time, not because it does not make much sense\n(admittedly it doesn't), but because show_stats() discards necessary\ninformation when it is done, making it impossible for shortstat to run.\n\nSo I ended up restructuring the name allocation/free code a bit while at\nit.\n\n-- >8 --\ndiff --numstat -z: make it machine readable\n\nThe \"-z\" format is all about machine parsability, but showing renamed\npaths as \"common/{a => b}/suffix\" makes it impossible. The scripts would\nnever have successfully parsed \"--numstat -z -M\" in the old format.\n\nThis fixes the output format in a (hopefully minimally) backward\nincompatible way.\n\n * The output without -z is not changed. This has given a good way for\n humans to view added and deleted lines separately, and showing the\n path in combined, shorter way would preserve readability.\n\n * The output with -z is unchanged for paths that do not involve renames.\n Existing scripts that do not pass -M/-C are not affected at all.\n\n * The output with -z for a renamed path is shown in a format that can\n easily be distinguished from an unrenamed path.\n\nThis is based on Jakub Narebski's patch. Bugs and documentation typos\nare mine.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-format.txt | 61 +++++++++++++++++++++++++\n diff.c | 100 ++++++++++++++++++++++++++++-------------\n 2 files changed, 129 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt\nindex 2c3a4c4..400cbb3 100644\n--- a/Documentation/diff-format.txt\n+++ b/Documentation/diff-format.txt\n@@ -84,3 +84,64 @@ all parents.\n \n \n include::diff-generate-patch.txt[]\n+\n+\n+other diff formats\n+------------------\n+\n+The `--summary` option describes newly added, deleted, renamed and\n+copied files. The `--stat` option adds diffstat(1) graph to the\n+output. These options can be combined with other options, such as\n+`-p`, and are meant for human consumption.\n+\n+When showing a change that involves a rename or a copy, `--stat` output\n+formats the pathnames compactly by combining common prefix and suffix of\n+the pathnames. For example, a change that moves `arch/i386/Makefile` to\n+`arch/x86/Makefile` while modifying 4 lines will be shown like this:\n+\n+------------------------------------\n+arch/{i386 => x86}/Makefile | 4 +--\n+------------------------------------\n+\n+The `--numstat` option gives the diffstat(1) information but is designed\n+for easier machine consumption. An entry in `--numstat` output looks\n+like this:\n+\n+----------------------------------------\n+1\t2\tREADME\n+3\t1\tarch/{i386 => x86}/Makefile\n+----------------------------------------\n+\n+That is, from left to right:\n+\n+. the number of added lines;\n+. a tab;\n+. the number of deleted lines;\n+. a tab;\n+. pathname (possibly with rename/copy information);\n+. a newline.\n+\n+When `-z` output option is in effect, the output is formatted this way:\n+\n+----------------------------------------\n+1\t2\tREADME NUL\n+3\t1\tNUL arch/i386/Makefile NUL arch/x86/Makefile NUL\n+----------------------------------------\n+\n+That is:\n+\n+. the number of added lines;\n+. a tab;\n+. the number of deleted lines;\n+. a tab;\n+. a NUL (only exists if renamed/copied);\n+. pathname in preimage;\n+. a NUL (only exists if renamed/copied);\n+. pathname in postimage (only exists if renamed/copied);\n+. a NUL.\n+\n+The extra `NUL` before the preimage path in renamed case is to allow\n+scripts that read the output to tell if the current record being read is\n+a single-path record or a rename/copy record without reading ahead.\n+After reading added and deleted lines, reading up to `NUL` would yield\n+the pathname, but if that is `NUL`, the record will show two paths.\ndiff --git a/diff.c b/diff.c\nindex f780e3e..fc51506 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -734,7 +734,9 @@ struct diffstat_t {\n \tint nr;\n \tint alloc;\n \tstruct diffstat_file {\n+\t\tchar *from_name;\n \t\tchar *name;\n+\t\tchar *print_name;\n \t\tunsigned is_unmerged:1;\n \t\tunsigned is_binary:1;\n \t\tunsigned is_renamed:1;\n@@ -755,11 +757,14 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,\n \t}\n \tdiffstat->files[diffstat->nr++] = x;\n \tif (name_b) {\n-\t\tx->name = pprint_rename(name_a, name_b);\n+\t\tx->from_name = xstrdup(name_a);\n+\t\tx->name = xstrdup(name_b);\n \t\tx->is_renamed = 1;\n \t}\n-\telse\n+\telse {\n+\t\tx->from_name = NULL;\n \t\tx->name = xstrdup(name_a);\n+\t}\n \treturn x;\n }\n \n@@ -803,6 +808,28 @@ static void show_graph(char ch, int cnt, const char *set, const char *reset)\n \tprintf(\"%s\", reset);\n }\n \n+static void fill_print_name(struct diffstat_file *file)\n+{\n+\tchar *pname;\n+\n+\tif (file->print_name)\n+\t\treturn;\n+\n+\tif (!file->is_renamed) {\n+\t\tstruct strbuf buf;\n+\t\tstrbuf_init(&buf, 0);\n+\t\tif (quote_c_style(file->name, &buf, NULL, 0)) {\n+\t\t\tpname = strbuf_detach(&buf, NULL);\n+\t\t} else {\n+\t\t\tpname = file->name;\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n+\t} else {\n+\t\tpname = pprint_rename(file->from_name, file->name);\n+\t}\n+\tfile->print_name = pname;\n+}\n+\n static void show_stats(struct diffstat_t* data, struct diff_options *options)\n {\n \tint i, len, add, del, total, adds = 0, dels = 0;\n@@ -836,19 +863,8 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \tfor (i = 0; i < data->nr; i++) {\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\t\tstruct strbuf buf;\n-\t\t\tstrbuf_init(&buf, 0);\n-\t\t\tif (quote_c_style(file->name, &buf, NULL, 0)) {\n-\t\t\t\tfree(file->name);\n-\t\t\t\tfile->name = strbuf_detach(&buf, NULL);\n-\t\t\t} else {\n-\t\t\t\tstrbuf_release(&buf);\n-\t\t\t}\n-\t\t}\n-\n-\t\tlen = strlen(file->name);\n+\t\tfill_print_name(file);\n+\t\tlen = strlen(file->print_name);\n \t\tif (max_len < len)\n \t\t\tmax_len = len;\n \n@@ -873,7 +889,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \n \tfor (i = 0; i < data->nr; i++) {\n \t\tconst char *prefix = \"\";\n-\t\tchar *name = data->files[i]->name;\n+\t\tchar *name = data->files[i]->print_name;\n \t\tint added = data->files[i]->added;\n \t\tint deleted = data->files[i]->deleted;\n \t\tint name_len;\n@@ -901,17 +917,17 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\tprintf(\"%s%d%s\", add_c, added, reset);\n \t\t\tprintf(\" bytes\");\n \t\t\tprintf(\"\\n\");\n-\t\t\tgoto free_diffstat_file;\n+\t\t\tcontinue;\n \t\t}\n \t\telse if (data->files[i]->is_unmerged) {\n \t\t\tshow_name(prefix, name, len, reset, set);\n \t\t\tprintf(\" Unmerged\\n\");\n-\t\t\tgoto free_diffstat_file;\n+\t\t\tcontinue;\n \t\t}\n \t\telse if (!data->files[i]->is_renamed &&\n \t\t\t (added + deleted == 0)) {\n \t\t\ttotal_files--;\n-\t\t\tgoto free_diffstat_file;\n+\t\t\tcontinue;\n \t\t}\n \n \t\t/*\n@@ -933,11 +949,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\tshow_graph('+', add, add_c, reset);\n \t\tshow_graph('-', del, del_c, reset);\n \t\tputchar('\\n');\n-\tfree_diffstat_file:\n-\t\tfree(data->files[i]->name);\n-\t\tfree(data->files[i]);\n \t}\n-\tfree(data->files);\n \tprintf(\"%s %d files changed, %d insertions(+), %d deletions(-)%s\\n\",\n \t set, total_files, adds, dels, reset);\n }\n@@ -962,11 +974,7 @@ static void show_shortstats(struct diffstat_t* data)\n \t\t\t\tdels += deleted;\n \t\t\t}\n \t\t}\n-\t\tfree(data->files[i]->name);\n-\t\tfree(data->files[i]);\n \t}\n-\tfree(data->files);\n-\n \tprintf(\" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n \t total_files, adds, dels);\n }\n@@ -975,6 +983,9 @@ static void show_numstat(struct diffstat_t* data, struct diff_options *options)\n {\n \tint i;\n \n+\tif (data->nr == 0)\n+\t\treturn;\n+\n \tfor (i = 0; i < data->nr; i++) {\n \t\tstruct diffstat_file *file = data->files[i];\n \n@@ -982,15 +993,39 @@ 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 (!file->is_renamed) {\n-\t\t\twrite_name_quoted(file->name, stdout, options->line_termination);\n+\t\tif (options->line_termination) {\n+\t\t\tfill_print_name(file);\n+\t\t\tif (!file->is_renamed)\n+\t\t\t\twrite_name_quoted(file->name, stdout,\n+\t\t\t\t\t\t options->line_termination);\n+\t\t\telse {\n+\t\t\t\tfputs(file->print_name, stdout);\n+\t\t\t\tputchar(options->line_termination);\n+\t\t\t}\n \t\t} else {\n-\t\t\tfputs(file->name, stdout);\n-\t\t\tputchar(options->line_termination);\n+\t\t\tif (file->is_renamed) {\n+\t\t\t\tputchar('\\0');\n+\t\t\t\twrite_name_quoted(file->from_name, stdout, '\\0');\n+\t\t\t}\n+\t\t\twrite_name_quoted(file->name, stdout, '\\0');\n \t\t}\n \t}\n }\n \n+static void free_diffstat_info(struct diffstat_t *diffstat)\n+{\n+\tint i;\n+\tfor (i = 0; i < diffstat->nr; i++) {\n+\t\tstruct diffstat_file *f = diffstat->files[i];\n+\t\tif (f->name != f->print_name)\n+\t\t\tfree(f->print_name);\n+\t\tfree(f->name);\n+\t\tfree(f->from_name);\n+\t\tfree(f);\n+\t}\n+\tfree(diffstat->files);\n+}\n+\n struct checkdiff_t {\n \tstruct xdiff_emit_state xm;\n \tconst char *filename;\n@@ -2943,8 +2978,9 @@ void diff_flush(struct diff_options *options)\n \t\t\tshow_numstat(&diffstat, options);\n \t\tif (output_format & DIFF_FORMAT_DIFFSTAT)\n \t\t\tshow_stats(&diffstat, options);\n-\t\telse if (output_format & DIFF_FORMAT_SHORTSTAT)\n+\t\tif (output_format & DIFF_FORMAT_SHORTSTAT)\n \t\t\tshow_shortstats(&diffstat);\n+\t\tfree_diffstat_info(&diffstat);\n \t\tseparator++;\n \t}\n \n"},{"id":"62892","messageId":"200712121121.24613.jnareb@gmail.com","threadId":"11232","inReplyTo":"7vabog9wyx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-12T10:21:23Z","receivedAt":"2007-12-12T10:21:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > The previous patch is a fix (usability fix) for numstat output in the\n> > presence of rename detection, making it truly machine friendly. Moreover\n> > it should not break any scripts which parsed numstat output not\n> > expecting to deal with renames (for example if repository has\n> > diff.renames config option set), and might even fix them.\n> >\n> > The proposed - and implemented in patch below - change could break some\n> > scripts not expecting to deal with new numstat output.\n> \n> I do not quite follow. If a script wanted -M and/or -C output in the\n> older \"common/{a => b}/common\" format, either change would break it\n> equally badly, either with or without -z. And I do not think adding\n> status letter is necessary for --numstat; if the caller wants that\n> information, it can ask for it explicitly with both options.\n\nI meant it there that if an [older] script parsing numstat output\ndidn't expect to deal with renames (but had to for example because\nof diff.renames set), and expected the numstat output format to be\neither\n\n TAB TAB LF\n\nor (if running with -z option)\n\n TAB TAB NUL\n\nAfter first patch, but before second, those expectations were met, that\nI meant by \"and might even fix them\" (the scripts).\n\nBTW. beside the fact that for renames it was not filename (pathname),\nit could be half-quoted; see also below.\n\n> So I do not think there is anything more to solve, than the patch you\n> just sent.\n> \n> The patch looks good. Thanks.\n> \n> > P.S. The numstat output format for renames should be probably described\n> > in documentation, and not only in commit message, but I was not sure\n> > where to put it (and even how it should be written).\n> \n> I started writing this, and then reviewed the result of squashing your\n> two patches.\n\nAnd because of the above I wanted that you'd rather _not_ squash those\npatches, but apply them separately...\n\n> Although everybody complains that most of git is run-once-and-exit, I\n> wrote the original diff part fairly conservatively to make it leak-free,\n> because of a single command, \"diff-tree --stdin\", that can be told to\n> run millions of diffs inside a single process.\n> \n> But the diffstat part is horribly leaky, especially after your patch,\n> and it has ugly workarounds such as refusing to show both diffstat and\n> shortstat at the same time, not because it does not make much sense\n> (admittedly it doesn't), but because show_stats() discards necessary\n> information when it is done, making it impossible for shortstat to run.\n> \n> So I ended up restructuring the name allocation/free code a bit while at\n> it.\n\n...but as you did more surgery on diffstat, it is reasonable to do it\nin one commit, not two.\n\n> -- >8 --\n> diff --numstat -z: make it machine readable\n> \n> The \"-z\" format is all about machine parsability, but showing renamed\n> paths as \"common/{a => b}/suffix\" makes it impossible. The scripts would\n> never have successfully parsed \"--numstat -z -M\" in the old format.\n> \n> This fixes the output format in a (hopefully minimally) backward\n> incompatible way.\n> \n> * The output without -z is not changed. This has given a good way for\n> humans to view added and deleted lines separately, and showing the\n> path in combined, shorter way would preserve readability.\n> \n> * The output with -z is unchanged for paths that do not involve renames.\n> Existing scripts that do not pass -M/-C are not affected at all.\n> \n> * The output with -z for a renamed path is shown in a format that can\n> easily be distinguished from an unrenamed path.\n\nThis changes invariant what we had, that output with and without -z differ\nonly in [filename] quoting and record separator. Now those two have\ndifferent semantic: --numstat without -z is for easier reading, --numstat\nwith -z is for machine consumption.\n\nBut as output with and without -z has to differ (in the form) already\nfor renames, why not...\n\n[...]\n> include::diff-generate-patch.txt[]\n> +\n> +\n> +other diff formats\n> +------------------\n> +\n> +The `--summary` option describes newly added, deleted, renamed and\n> +copied files. The `--stat` option adds diffstat(1) graph to the\n> +output. These options can be combined with other options, such as\n> +`-p`, and are meant for human consumption.\n> +\n> +When showing a change that involves a rename or a copy, `--stat` output\n> +formats the pathnames compactly by combining common prefix and suffix of\n> +the pathnames. For example, a change that moves `arch/i386/Makefile` to\n> +`arch/x86/Makefile` while modifying 4 lines will be shown like this:\n\nPerhaps it could be added there word about \"table-like format\", and about\ncolumn widths (total width, filename [column] width).\n\n> +\n> +------------------------------------\n> +arch/{i386 => x86}/Makefile | 4 +--\n\nI would add there example of shortened filename there, for example\n\n +.../SubmittingPatches | 2 +\n\nDo I understand correctly that code first tries to strip directories,\nand only if the filename is too long it does shorten filename?\n\nBy the way the combining common prefix and suffix (pprint_rename) doesn't\nplay well with quoting: when only one of the source and destination names\nare quoted it wouldn't of course find any common prefix/suffix. I think\nthat if one of filenames has to be quoted, both should be.\n\nAnd shortening of filenames together with either combining common prefix,\nand/or with filename quoting produces output with might be a bit strange\nfor some. But I don't have idea how (and if) it could be corrected.\n\n> +------------------------------------\n> +\n> +The `--numstat` option gives the diffstat(1) information but is designed\n> +for easier machine consumption. An entry in `--numstat` output looks\n> +like this:\n> +\n> +----------------------------------------\n> +1\t2\tREADME\n> +3\t1\tarch/{i386 => x86}/Makefile\n> +----------------------------------------\n\nNote that filename is not shortened in --numstat (as it can be in --stat).\n\n> +\n> +When `-z` output option is in effect, the output is formatted this way:\n> +\n> +----------------------------------------\n> +1\t2\tREADME NUL\n> +3\t1\tNUL arch/i386/Makefile NUL arch/x86/Makefile NUL\n> +----------------------------------------\n\nI wanted to say that it would make it harder on \"line\"-based parsers...\nbut I have realized that it would not, as one can simply read next two\n\"lines\" (records) for pre-image and post-image filename if 0-length\nfilename is detected.\n\n[...]\n> +static void fill_print_name(struct diffstat_file *file)\n\nNice idea to separate this into function.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"62894","messageId":"200712121131.01471.jnareb@gmail.com","threadId":"11232","inReplyTo":"200712120009.36560.jnareb@gmail.com","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-12-12T10:31:01Z","receivedAt":"2007-12-12T10:31:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"BTW. I have noticed something strange with current (after my two\npatches) diffstat code:\n\n 3606:[gitweb/web@git]# ./git diff-tree -C -C -r --stat gitweb/test~8 \n 0456a2ba58efb0e1d5f7421d5a8a2278e3b15ebc\n .../test/file with spaces\\tand\\ttabs\" | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n 3607:[gitweb/web@git]# ./git diff-tree -C -C -r --numstat gitweb/test~8\n 0456a2ba58efb0e1d5f7421d5a8a2278e3b15ebc\n 3 1 gitweb/test/file with spaces \"gitweb/test/file with spaces\\tand\\ttabs\"\n\nbut:\n\n 3603:[gitweb/web@git]# ./git diff-tree -C -C -r --stat gitweb/test~6\n cfb9ef9ec73b37f44e9370c4f5d91e01d46ec6e4\n gitweb/test/Documentation-symlink1 | 1 +\n gitweb/test/Documentation-symlink2 | 1 +\n .../{git-shortlog.8.html => git-shortlog.html} | 0 \n 3 files changed, 2 insertions(+), 0 deletions(-)\n\nSo rename detection works. Strange.\n\nP.S. gitweb/test branch can be found on\n http://repo.or.cz/w/git/jnareb-git.git\n\n-- \nJakub Narebski\nPoland\n"},{"id":"62950","messageId":"7v4pen91fn.fsf@gitster.siamese.dyndns.org","threadId":"11232","inReplyTo":"200712121131.01471.jnareb@gmail.com","subject":"Re: [PATCH (amend)] diff: Make numstat machine friendly also for renames (and copies)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-12T19:07:40Z","receivedAt":"2007-12-12T19:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> BTW. I have noticed something strange with current (after my two\n> patches) diffstat code:\n>\n> 3606:[gitweb/web@git]# ./git diff-tree -C -C -r --stat gitweb/test~8 \n> 0456a2ba58efb0e1d5f7421d5a8a2278e3b15ebc\n> .../test/file with spaces\\tand\\ttabs\" | 4 +++-\n> 1 files changed, 3 insertions(+), 1 deletions(-)\n> 3607:[gitweb/web@git]# ./git diff-tree -C -C -r --numstat gitweb/test~8\n> 0456a2ba58efb0e1d5f7421d5a8a2278e3b15ebc\n> 3 1 gitweb/test/file with spaces \"gitweb/test/file with spaces\\tand\\ttabs\"\n\nIf you are wondering about the lack of => in --stat when --numstat is\nshowing rename, there is nothing strange going on. \"filename scaling\"\ndone in --stat happens way later than pprint_rename() in the current\ncode structure (essentially, it chomps at the last slash to keep the\nlong \"path\" fit within the given space). --stat (not --numstat) is for\nhuman consumption and showing longer part of the name of postimage is\nmore important if we do not have enough room, and most tools that enable\nrename use --summary with --stat anyway, so the rename information for\nsuch an oddball long path can be found out elsewhere in the output if\nneeded.\n"}]}