{"thread":{"id":"32164","subject":"[PATCH] diff: Fixes shortstat number of files","startedAt":"2012-11-21T21:26:49Z","lastAt":"2012-11-27T19:14:49Z","messageCount":4,"participants":["Antoine Pelisse","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"203639","messageId":"1353533210-29684-1-git-send-email-apelisse@gmail.com","threadId":"32164","inReplyTo":null,"subject":"[PATCH] diff: Fixes shortstat number of files","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-21T21:26:49Z","receivedAt":"2012-11-21T21:26:49Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"There is a discrepancy between the last line of `git diff --stat`\nand `git diff --shortstat` in case of a merge.\nThe unmerged files are actually counted twice, thus doubling the\nvalue of \"file changed\".\n\nIn fact, while stat decrements number of files when seeing an unmerged\nfile, shortstat doesn't.\n\nSigned-off-by: Antoine Pelisse \n---\n diff.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e89a201..5c6bcbd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1704,9 +1704,8 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \t\tint added = data->files[i]->added;\n \t\tint deleted= data->files[i]->deleted;\n \n-\t\tif (data->files[i]->is_unmerged)\n-\t\t\tcontinue;\n-\t\tif (!data->files[i]->is_renamed && (added + deleted == 0)) {\n+\t\tif (data->files[i]->is_unmerged ||\n+\t\t (!data->files[i]->is_renamed && (added + deleted == 0))) {\n \t\t\ttotal_files--;\n \t\t} else if (!data->files[i]->is_binary) { /* don't count bytes */\n \t\t\tadds += added;\n-- \n1.7.9.5\n"},{"id":"203845","messageId":"7vhaodxctq.fsf@alter.siamese.dyndns.org","threadId":"32164","inReplyTo":"1353533210-29684-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] diff: Fixes shortstat number of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T03:28:17Z","receivedAt":"2012-11-26T03:28:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> Subject: Re: [PATCH] diff: Fixes shortstat number of files\n\nPlease replace \"Fixes\" with \"Fix at least (because our log messages\nare written as if a patch is giving an order to the codebase, iow,\nin imperative mood), but we would prefer to see a concrete\ndescription on what is fixed, when we can. And in this case, I\nthink we can, perhaps:\n\n diff: do not count unmerged paths twice in --shortstat/--numstat\n\nor something.\n\n> There is a discrepancy between the last line of `git diff --stat`\n> and `git diff --shortstat` in case of a merge.\n> The unmerged files are actually counted twice, thus doubling the\n> value of \"file changed\".\n\nI think the current 'master' and upward is broken with respect to\nthis; I am consistently getting two entries for unmerged paths\nacross --stat, --shortstat and --numstat options (iow, not just\nshortstat and numstat but the '--stat' seems to be broken as well).\n\nThanks.\n"},{"id":"203862","messageId":"CALWbr2z+9fJGg34q7zp3knZVWgaTwtvzFBvFHRSmXfH+hc-jNw@mail.gmail.com","threadId":"32164","inReplyTo":"7vhaodxctq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: Fixes shortstat number of files","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-11-26T09:10:41Z","receivedAt":"2012-11-26T09:10:41Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Indeed stat seems to be broken on master by commit 74faaa16 from Linus Torvalds\n\nThere are three separated issues here:\n - unmerged files are marked as \"interesting\" in stat and probably\nshouldn't, with some patch like this:\n\n data->is_interesting = p->status != 0;\n\n if (!one || !two) {\n data->is_unmerged = 1;\n+ data->is_interesting = 0;\n return;\n }\n\nBy the way, I don't get the point of this code then:\n\n else if (data->files[i]->is_unmerged) {\n fprintf(options->file, \"%s\", line_prefix);\n show_name(options->file, prefix, name, len);\n fprintf(options->file, \" Unmerged\\n\");\n continue;\n }\n\nand\n\n if (file->is_unmerged) {\n /* \"Unmerged\" is 8 characters */\n bin_width = bin_width < 8 ? 8 : bin_width;\n continue;\n }\n\nAre we ever supposed to print that ? I feel like it could be removed.\n\n - Unmerged files are not filtered out in shortstat, thus counted\ntwice (addressed by the patch)\n - no file has ever been filtered out of numstat, and probably should\nthe way it's done in stat. That is with something like this:\n\n if (!data->files[i]->is_interesting &&\n (added + deleted == 0)) {\n continue;\n }\n\n\nCheers,\nAntoine Pelisse\n\n\n---------- Forwarded message ----------\nFrom: Junio C Hamano \nDate: Mon, Nov 26, 2012 at 4:28 AM\nSubject: Re: [PATCH] diff: Fixes shortstat number of files\nTo: Antoine Pelisse \nCc: git@vger.kernel.org\n\n\nAntoine Pelisse writes:\n\n> Subject: Re: [PATCH] diff: Fixes shortstat number of files\n\nPlease replace \"Fixes\" with \"Fix at least (because our log messages\nare written as if a patch is giving an order to the codebase, iow,\nin imperative mood), but we would prefer to see a concrete\ndescription on what is fixed, when we can. And in this case, I\nthink we can, perhaps:\n\n diff: do not count unmerged paths twice in --shortstat/--numstat\n\nor something.\n\n> There is a discrepancy between the last line of `git diff --stat`\n> and `git diff --shortstat` in case of a merge.\n> The unmerged files are actually counted twice, thus doubling the\n> value of \"file changed\".\n\nI think the current 'master' and upward is broken with respect to\nthis; I am consistently getting two entries for unmerged paths\nacross --stat, --shortstat and --numstat options (iow, not just\nshortstat and numstat but the '--stat' seems to be broken as well).\n\nThanks.\n"},{"id":"204016","messageId":"7vhaoan9hy.fsf@alter.siamese.dyndns.org","threadId":"32164","inReplyTo":"CALWbr2z+9fJGg34q7zp3knZVWgaTwtvzFBvFHRSmXfH+hc-jNw@mail.gmail.com","subject":"Re: [PATCH] diff: Fixes shortstat number of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-27T19:14:49Z","receivedAt":"2012-11-27T19:14:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> Indeed stat seems to be broken on master by commit 74faaa16 from Linus Torvalds\n>\n> There are three separated issues here:\n> - unmerged files are marked as \"interesting\" in stat and probably\n> shouldn't, with some patch like this:\n>\n> data->is_interesting = p->status != 0;\n>\n> if (!one || !two) {\n> data->is_unmerged = 1;\n> + data->is_interesting = 0;\n> return;\n> }\n>\n> By the way, I don't get the point of this code then:\n>\n> else if (data->files[i]->is_unmerged) {\n> fprintf(options->file, \"%s\", line_prefix);\n> show_name(options->file, prefix, name, len);\n> fprintf(options->file, \" Unmerged\\n\");\n> continue;\n> }\n>\n> and\n>\n> if (file->is_unmerged) {\n> /* \"Unmerged\" is 8 characters */\n> bin_width = bin_width < 8 ? 8 : bin_width;\n> continue;\n> }\n>\n> Are we ever supposed to print that ? I feel like it could be removed.\n\nYes, we have been showing two entries in --stat output:\n\n file | Unmerged\n file | 4 ++++\n\nand that is not going to change.\n\nThere are a few problems in diff.c around --stat area, partially\ncaused by Linus's patch (like s/is_rename/is_interesting/ change\nstarted ignoring unmerged entries in the diff queue and made the\nexisting loop not go into the codepath we see above), and largely\ncaused by the earlier change that introduced when --stat-count was\nadded (the second loop that decrements total_files does so only for\nthe paths within the \"count\" horizon determined by the first loop;\ntotal_files must be adjusted for _all_ uninteresting and unchanged\nfilepairs and exclude unmerged entries).\n\nAlso the initialization of data->is_interesting is wrong. These\ndays, p->status is never zero.\n\nI'll send out a fix later today.\n"}]}