{"thread":{"id":"50305","subject":"[PATCH 0/6] some diff --cc --stat fixes","startedAt":"2019-01-24T12:26:06Z","lastAt":"2019-01-24T21:17:18Z","messageCount":19,"participants":["Jeff King","Stefan Beller","David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"367535","messageId":"20190124122603.GA10415@sigill.intra.peff.net","threadId":"50305","inReplyTo":null,"subject":"[PATCH 0/6] some diff --cc --stat fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:26:03Z","receivedAt":"2019-01-24T12:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This started as a follow-up to my \"something like this\" bug-fix from\nlast July:\n\n https://public-inbox.org/git/20180710203438.GB6886@sigill.intra.peff.net/\n\n(better late than never). But in the course of that, I discovered\nanother really weird and interesting bug with --color-moved. This fixes\nboth, along with some related cases.\n\n-Peff\n\n  [1/6]: t4006: resurrect commented-out tests\n  [2/6]: diff: clear emitted_symbols flag after use\n  [3/6]: combine-diff: factor out stat-format mask\n  [4/6]: combine-diff: treat --shortstat like --stat\n  [5/6]: combine-diff: treat --summary like --stat\n  [6/6]: combine-diff: treat --dirstat like --stat\n\n combine-diff.c                                | 17 ++--\n diff.c                                        |  4 +-\n t/t4006-diff-mode.sh                          | 55 +++++++------\n t/t4013-diff-various.sh                       |  9 +++\n .../diff.diff-tree_--cc_--shortstat_master    |  4 +\n t/t4013/diff.diff-tree_--cc_--summary_REVERSE |  6 ++\n .../diff.diff_--dirstat_--cc_master~1_master  |  3 +\n t/t4066-diff-emit-delay.sh                    | 79 +++++++++++++++++++\n 8 files changed, 147 insertions(+), 30 deletions(-)\n create mode 100644 t/t4013/diff.diff-tree_--cc_--shortstat_master\n create mode 100644 t/t4013/diff.diff-tree_--cc_--summary_REVERSE\n create mode 100644 t/t4013/diff.diff_--dirstat_--cc_master~1_master\n create mode 100755 t/t4066-diff-emit-delay.sh\n"},{"id":"367536","messageId":"20190124122728.GA11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 1/6] t4006: resurrect commented-out tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:27:29Z","receivedAt":"2019-01-24T12:27:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This set of tests was added by 4434e6ba6c (tests: check --[short]stat\noutput after chmod, 2012-05-01), and is primarily about the handling of\nbinary versus text files.\n\nLater, 74faaa16f0 (Fix \"git diff --stat\" for interesting - but empty -\nfile changes, 2012-10-17) changed the stat output so that the empty text\nfile is mentioned rather than omitted. That commit just comments out\nthese tests. There's no discussion in the commit message, but the\noriginal email[1] says:\n\n  NOTE! This does break two of our tests, so we clearly did this on\n  purpose, or at least tested for it. I just uncommented the subtests\n  that this makes irrelevant, and changed the output of another one.\n\nI don't think they're irrelevant, though. We should be testing this\n\"mode change only\" case and making sure that it has the post-74faaa16f0\nbehavior. So this commit brings back those tests, with the current\nexpected output.\n\n[1] https://public-inbox.org/git/CA+55aFz88GPJcfMSqiyY+u0Cdm48bEyrsTGxHVJbGsYsDg=Q5w@mail.gmail.com/\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot actually relevant to the rest of the series; I just found this while\nlooking for existing --shortstat tests.\n\n t/t4006-diff-mode.sh | 55 ++++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh\nindex a8e01eccd1..03489aff14 100755\n--- a/t/t4006-diff-mode.sh\n+++ b/t/t4006-diff-mode.sh\n@@ -32,28 +32,37 @@ test_expect_success 'prepare binary file' '\n \tgit commit -m binbin\n '\n \n-# test_expect_success '--stat output after text chmod' '\n-# \ttest_chmod -x rezrov &&\n-# \techo \" 0 files changed\" >expect &&\n-# \tgit diff HEAD --stat >actual &&\n-#\ttest_i18ncmp expect actual\n-# '\n-#\n-# test_expect_success '--shortstat output after text chmod' '\n-# \tgit diff HEAD --shortstat >actual &&\n-# \ttest_i18ncmp expect actual\n-# '\n-#\n-# test_expect_success '--stat output after binary chmod' '\n-# \ttest_chmod +x binbin &&\n-# \techo \" 0 files changed\" >expect &&\n-# \tgit diff HEAD --stat >actual &&\n-# \ttest_i18ncmp expect actual\n-# '\n-#\n-# test_expect_success '--shortstat output after binary chmod' '\n-# \tgit diff HEAD --shortstat >actual &&\n-# \ttest_i18ncmp expect actual\n-# '\n+test_expect_success '--stat output after text chmod' '\n+\ttest_chmod -x rezrov &&\n+\tcat >expect <<-\\EOF &&\n+\t rezrov | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\tEOF\n+\tgit diff HEAD --stat >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success '--shortstat output after text chmod' '\n+\ttail -n 1 <expect >expect.short &&\n+\tgit diff HEAD --shortstat >actual &&\n+\ttest_i18ncmp expect.short actual\n+'\n+\n+test_expect_success '--stat output after binary chmod' '\n+\ttest_chmod +x binbin &&\n+\tcat >expect <<-EOF &&\n+\t binbin | Bin\n+\t rezrov |   0\n+\t 2 files changed, 0 insertions(+), 0 deletions(-)\n+\tEOF\n+\tgit diff HEAD --stat >actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success '--shortstat output after binary chmod' '\n+\ttail -n 1 <expect >expect.short &&\n+\tgit diff HEAD --shortstat >actual &&\n+\ttest_i18ncmp expect.short actual\n+'\n \n test_done\n-- \n2.20.1.842.g8986705066\n\n"},{"id":"367537","messageId":"20190124123240.GB11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:32:41Z","receivedAt":"2019-01-24T12:32:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's an odd bug when \"log --color-moved\" is used with the combination\nof \"--cc --stat -p\": the stat for merge commits is erroneously shown\nwith the diff of the _next_ commit.\n\nThe included test demonstrates the issue. Our history looks something\nlike this:\n\n  A-B-M--D\n   \\ /\n    C\n\nWhen we run \"git log --cc --stat -p --color-moved\" starting at D, we get\nthis sequence of events:\n\n  1. The diff for D is using -p, so diff_flush() calls into\n     diff_flush_patch_all_file_pairs(). There we see that o->color_moved\n     is in effect, so we point o->emitted_symbols to a static local\n     struct, causing diff_flush_patch() to queue the symbols instead of\n     actually writing them out.\n\n     We then do our move detection, emit the symbols, and clear the\n     struct. But we leave o->emitted_symbols pointing to our struct.\n\n  2. Next we compute the diff for M. This is a merge, so we use the\n     combined diff code. In find_paths_generic(), we compute the\n     pairwise diff between each commit and its parent. Normally this is\n     done with DIFF_FORMAT_NO_OUTPUT, since we're just looking for\n     intersecting paths. But since \"--stat --cc\" shows the first-parent\n     stat, and since we're computing that diff anyway, we enable\n     DIFF_FORMAT_DIFFSTAT for the first parent. This outputs the stat\n     information immediately, saving us from running a separate\n     first-parent diff later.\n\n     But where does that output go? Normally it goes directly to stdout,\n     but because o->emitted_symbols is set, we queue it. As a result, we\n     don't actually print the diffstat for the merge commit (yet), which\n     is wrong.\n\n  3. Next we compute the diff for C. We're actually showing a patch\n     again, so we end up in diff_flush_patch_all_file_pairs(), but this\n     time we have the queued stat from step 2 waiting in our struct.\n\n     We add new elements to it for C's diff, and then flush the whole\n     thing. And we see the diffstat from M as part of C's diff, which is\n     wrong.\n\nSo triggering the bug really does require the combination of all of\nthose options.\n\nTo fix it, we can simply restore o->emitted_symbols to NULL after\nflushing it, so that it does not affect anything outside of\ndiff_flush_patch_all_file_pairs(). This intuitively makes sense, since\nnobody outside of that function is going to bother flushing it, so we\nwould not want them to write to it either.\n\nIn fact, we could take this a step further and turn the local \"esm\"\nstruct into a non-static variable that goes away after the function\nends. However, since it contains a dynamically sized array, we benefit\nfrom amortizing the cost of allocations over many calls. So we'll leave\nit as static to retain that benefit.\n\nBut let's push the zero-ing of esm.nr into the conditional for \"if\n(o->emitted_symbols)\" to make it clear that we do not expect esm to hold\nany values if we did not just try to use it. With the code as it is\nwritten now, if we did encounter such a case (which I think would be a\nbug), we'd silently leak those values without even bothering to display\nthem. With this change, we'd at least eventually show them, and somebody\nwould notice.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                     |  4 +-\n t/t4066-diff-emit-delay.sh | 79 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 1 deletion(-)\n create mode 100755 t/t4066-diff-emit-delay.sh\n\ndiff --git a/diff.c b/diff.c\nindex 1b5f276360..7b97739799 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5894,8 +5894,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \n \t\tfor (i = 0; i < esm.nr; i++)\n \t\t\tfree((void *)esm.buf[i].line);\n+\t\tesm.nr = 0;\n+\n+\t\to->emitted_symbols = NULL;\n \t}\n-\tesm.nr = 0;\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/t/t4066-diff-emit-delay.sh b/t/t4066-diff-emit-delay.sh\nnew file mode 100755\nindex 0000000000..5df6b5e64e\n--- /dev/null\n+++ b/t/t4066-diff-emit-delay.sh\n@@ -0,0 +1,79 @@\n+#!/bin/sh\n+\n+test_description='test combined/stat/moved interaction'\n+. ./test-lib.sh\n+\n+# This test covers a weird 3-way interaction between \"--cc -p\", which will run\n+# the combined diff code, along with \"--stat\", which will be computed as a\n+# first-parent stat during the combined diff, and \"--color-moved\", which\n+# enables the emitted_symbols list to store the diff in memory.\n+\n+test_expect_success 'set up history with a merge' '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit checkout -b side HEAD^ &&\n+\ttest_commit C &&\n+\tgit merge -m M master &&\n+\ttest_commit D\n+'\n+\n+test_expect_success 'log --cc -p --stat --color-moved' '\n+\tcat >expect <<-\\EOF &&\n+\tcommit D\n+\t---\n+\t D.t | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\n+\tdiff --git a/D.t b/D.t\n+\tnew file mode 100644\n+\tindex 0000000..1784810\n+\t--- /dev/null\n+\t+++ b/D.t\n+\t@@ -0,0 +1 @@\n+\t+D\n+\tcommit M\n+\n+\t B.t | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\tcommit C\n+\t---\n+\t C.t | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\n+\tdiff --git a/C.t b/C.t\n+\tnew file mode 100644\n+\tindex 0000000..3cc58df\n+\t--- /dev/null\n+\t+++ b/C.t\n+\t@@ -0,0 +1 @@\n+\t+C\n+\tcommit B\n+\t---\n+\t B.t | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\n+\tdiff --git a/B.t b/B.t\n+\tnew file mode 100644\n+\tindex 0000000..223b783\n+\t--- /dev/null\n+\t+++ b/B.t\n+\t@@ -0,0 +1 @@\n+\t+B\n+\tcommit A\n+\t---\n+\t A.t | 1 +\n+\t 1 file changed, 1 insertion(+)\n+\n+\tdiff --git a/A.t b/A.t\n+\tnew file mode 100644\n+\tindex 0000000..f70f10e\n+\t--- /dev/null\n+\t+++ b/A.t\n+\t@@ -0,0 +1 @@\n+\t+A\n+\tEOF\n+\tgit log --format=\"commit %s\" --cc -p --stat --color-moved >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.20.1.842.g8986705066\n\n"},{"id":"367538","messageId":"20190124123350.GC11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 3/6] combine-diff: factor out stat-format mask","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:33:51Z","receivedAt":"2019-01-24T12:33:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are several conditionals in the combine diff code that check if\nwe're doing --stat or --numstat output. Since these must all remain in\nsync, let's pull them out into a separate bit-mask.\n\nArguably this could go into diff.h along with the other DIFF_FORMAT\nmacros, but it's not clear that the definition of \"which formats are\nstat\" is universal (e.g., does --dirstat count? --summary?). So let's\nkeep this local to combine-diff.c, where the meaning is more clearly\n\"stat formats that combine-diff supports\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex a143c00634..b1d259d5a0 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1321,6 +1321,11 @@ static const char *path_path(void *obj)\n \treturn path->path;\n }\n \n+/*\n+ * Diff stat formats which we always compute solely against the first parent.\n+ */\n+#define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \\\n+\t\t\t  | DIFF_FORMAT_DIFFSTAT)\n \n /* find set of paths that every parent touches */\n static struct combine_diff_path *find_paths_generic(const struct object_id *oid,\n@@ -1342,8 +1347,7 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid,\n \t\t * show stat against the first parent even when doing\n \t\t * combined diff.\n \t\t */\n-\t\tint stat_opt = (output_format &\n-\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));\n+\t\tint stat_opt = output_format & STAT_FORMAT_MASK;\n \t\tif (i == 0 && stat_opt)\n \t\t\topt->output_format = stat_opt;\n \t\telse\n@@ -1470,8 +1474,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\t * show stat against the first parent even\n \t\t * when doing combined diff.\n \t\t */\n-\t\tstat_opt = (opt->output_format &\n-\t\t\t\t(DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));\n+\t\tstat_opt = opt->output_format & STAT_FORMAT_MASK;\n \t\tif (stat_opt) {\n \t\t\tdiffopts.output_format = stat_opt;\n \n@@ -1515,8 +1518,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\t\t\tshow_raw_diff(p, num_parent, rev);\n \t\t\tneedsep = 1;\n \t\t}\n-\t\telse if (opt->output_format &\n-\t\t\t (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))\n+\t\telse if (opt->output_format & STAT_FORMAT_MASK)\n \t\t\tneedsep = 1;\n \t\telse if (opt->output_format & DIFF_FORMAT_CALLBACK)\n \t\t\thandle_combined_callback(opt, paths, num_parent, num_paths);\n-- \n2.20.1.842.g8986705066\n\n"},{"id":"367539","messageId":"20190124123450.GD11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 4/6] combine-diff: treat --shortstat like --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:34:51Z","receivedAt":"2019-01-24T12:34:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --stat of a combined diff is defined as the first-parent stat,\ngoing all the way back to 965f803c32 (combine-diff: show diffstat with\nthe first parent., 2006-04-17).\n\nNaturally, we gave --numstat the same treatment in 74e2abe5b7 (diff\n--numstat, 2006-10-12).\n\nBut --shortstat, which is really just the final line of --stat, does\nnothing, which produces confusing results:\n\n  $ git show --oneline --stat eab7584e37\n  eab7584e37 Merge branch 'en/show-ref-doc-fix'\n\n   Documentation/git-show-ref.txt | 2 +-\n   1 file changed, 1 insertion(+), 1 deletion(-)\n\n  $ git show --oneline --shortstat eab7584e37\n  eab7584e37 Merge branch 'en/show-ref-doc-fix'\n\n  [nothing! We'd expect to see the \"1 file changed...\" line]\n\nThis patch teaches combine-diff to treats the two formats identically.\n\nReported-by: David Turner <novalis@novalis.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c                                 | 1 +\n t/t4013-diff-various.sh                        | 1 +\n t/t4013/diff.diff-tree_--cc_--shortstat_master | 4 ++++\n 3 files changed, 6 insertions(+)\n create mode 100644 t/t4013/diff.diff-tree_--cc_--shortstat_master\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex b1d259d5a0..83ec3dfffa 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1325,6 +1325,7 @@ static const char *path_path(void *obj)\n  * Diff stat formats which we always compute solely against the first parent.\n  */\n #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \\\n+\t\t\t  | DIFF_FORMAT_SHORTSTAT \\\n \t\t\t  | DIFF_FORMAT_DIFFSTAT)\n \n /* find set of paths that every parent touches */\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 7d985ff6b1..9ccdf08730 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -239,6 +239,7 @@ diff-tree --cc --stat --summary master\n # stat summary should show the diffstat and summary with the first parent\n diff-tree -c --stat --summary side\n diff-tree --cc --stat --summary side\n+diff-tree --cc --shortstat master\n # improved by Timo's patch\n diff-tree --cc --patch-with-stat master\n # improved by Timo's patch\ndiff --git a/t/t4013/diff.diff-tree_--cc_--shortstat_master b/t/t4013/diff.diff-tree_--cc_--shortstat_master\nnew file mode 100644\nindex 0000000000..a4ca42df2a\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--cc_--shortstat_master\n@@ -0,0 +1,4 @@\n+$ git diff-tree --cc --shortstat master\n+59d314ad6f356dd08601a4cd5e530381da3e3c64\n+ 2 files changed, 5 insertions(+)\n+$\n-- \n2.20.1.842.g8986705066\n\n"},{"id":"367540","messageId":"20190124123539.GE11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 5/6] combine-diff: treat --summary like --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:35:40Z","receivedAt":"2019-01-24T12:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently \"--cc --summary\" on a merge shows nothing. Since we show \"--cc\n--stat\" as a stat against the first parent, and because --summary is\ntypically used in combination with --stat, it makes sense to treat them\nboth the same way.\n\nNote that we have to tweak t4013's setup a bit to test this case, as the\nexisting merges do not have any --summary results against their first\nparent. But since the merge at the tip of 'master' does add and remove\nfiles with respect to the second parent, we can just make a reversed\ndoppelganger merge where the parents are swapped.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c                                | 1 +\n t/t4013-diff-various.sh                       | 7 +++++++\n t/t4013/diff.diff-tree_--cc_--summary_REVERSE | 6 ++++++\n 3 files changed, 14 insertions(+)\n create mode 100644 t/t4013/diff.diff-tree_--cc_--summary_REVERSE\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 83ec3dfffa..f42b41d884 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1326,6 +1326,7 @@ static const char *path_path(void *obj)\n  */\n #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \\\n \t\t\t  | DIFF_FORMAT_SHORTSTAT \\\n+\t\t\t  | DIFF_FORMAT_SUMMARY \\\n \t\t\t  | DIFF_FORMAT_DIFFSTAT)\n \n /* find set of paths that every parent touches */\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 9ccdf08730..e28953975b 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -98,6 +98,12 @@ test_expect_success setup '\n \tgit commit -m \"update mode\" &&\n \tgit checkout -f master &&\n \n+\t# Same merge as master, but with parents reversed. Hide it in a\n+\t# pseudo-ref to avoid impacting tests with --all.\n+\tcommit=$(echo reverse |\n+\t\t git commit-tree -p master^2 -p master^1 master^{tree}) &&\n+\tgit update-ref REVERSE $commit &&\n+\n \tgit config diff.renames false &&\n \n \tgit show-branch\n@@ -240,6 +246,7 @@ diff-tree --cc --stat --summary master\n diff-tree -c --stat --summary side\n diff-tree --cc --stat --summary side\n diff-tree --cc --shortstat master\n+diff-tree --cc --summary REVERSE\n # improved by Timo's patch\n diff-tree --cc --patch-with-stat master\n # improved by Timo's patch\ndiff --git a/t/t4013/diff.diff-tree_--cc_--summary_REVERSE b/t/t4013/diff.diff-tree_--cc_--summary_REVERSE\nnew file mode 100644\nindex 0000000000..e208dd5682\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--cc_--summary_REVERSE\n@@ -0,0 +1,6 @@\n+$ git diff-tree --cc --summary REVERSE\n+2562325a7ee916efb2481da93073b82cec801cbc\n+ create mode 100644 file1\n+ delete mode 100644 file2\n+ delete mode 100644 file3\n+$\n-- \n2.20.1.842.g8986705066\n\n"},{"id":"367541","messageId":"20190124123647.GF11354@sigill.intra.peff.net","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"[PATCH 6/6] combine-diff: treat --dirstat like --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T12:36:47Z","receivedAt":"2019-01-24T12:36:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently \"--cc --dirstat\" will show nothing for a merge.  Like\n--shortstat and --summary in the previous two patches, it probably makes\nsense to treat it like we do --stat, and show a stat against the\nfirst-parent.\n\nThis case is less obviously correct than for --shortstat and --summary,\nas those are basically variants of --stat themselves. It's possible we\ncould develop a multi-parent combined dirstat format, in which case we\nmight regret defining this first-parent behavior. But the same could be\nsaid for --stat, and in the 12+ years of it showing first-parent stats,\nnobody has complained.\n\nSo showing the first-parent dirstat is at least _useful_, and if we\nlater develop a clever multi-parent stat format, we'd probably have to\ndeal with --stat anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c                                   | 1 +\n t/t4013-diff-various.sh                          | 1 +\n t/t4013/diff.diff_--dirstat_--cc_master~1_master | 3 +++\n 3 files changed, 5 insertions(+)\n create mode 100644 t/t4013/diff.diff_--dirstat_--cc_master~1_master\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex f42b41d884..23d8fabe75 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1327,6 +1327,7 @@ static const char *path_path(void *obj)\n #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \\\n \t\t\t  | DIFF_FORMAT_SHORTSTAT \\\n \t\t\t  | DIFF_FORMAT_SUMMARY \\\n+\t\t\t  | DIFF_FORMAT_DIRSTAT \\\n \t\t\t  | DIFF_FORMAT_DIFFSTAT)\n \n /* find set of paths that every parent touches */\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex e28953975b..9f8f0e84ad 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -358,6 +358,7 @@ diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n+diff --dirstat --cc master~1 master\n # No-index --abbrev and --no-abbrev\n diff --raw initial\n :noellipses diff --raw initial\ndiff --git a/t/t4013/diff.diff_--dirstat_--cc_master~1_master b/t/t4013/diff.diff_--dirstat_--cc_master~1_master\nnew file mode 100644\nindex 0000000000..fba4e34175\n--- /dev/null\n+++ b/t/t4013/diff.diff_--dirstat_--cc_master~1_master\n@@ -0,0 +1,3 @@\n+$ git diff --dirstat --cc master~1 master\n+  40.0% dir/\n+$\n-- \n2.20.1.842.g8986705066\n"},{"id":"367570","messageId":"CAGZ79kZqKQ8KEjn84dr7pB-T==jcMj_z-9MxB8aCrtPfpyzD=w@mail.gmail.com","threadId":"50305","inReplyTo":"20190124122728.GA11354@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] t4006: resurrect commented-out tests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T18:18:07Z","receivedAt":"2019-01-24T18:18:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 4:27 AM Jeff King <peff@peff.net> wrote:\n\n> ---\n> Not actually relevant to the rest of the series; I just found this while\n> looking for existing --shortstat tests.\n\nNot actually relevant to the review, but my long term vision for\n--color-moved would work with this patch, too. ;)\n\nBut now that I look at the patch, there are actual changes, as you\nindicated in the commit message. Just more than I expected.\n\nThis patch is\n    Reviewed-by: Stefan Beller <sbeller@google.com>\n"},{"id":"367575","messageId":"CAGZ79kbHLvN252v-gNbcpsyGg8pZ9GPBtyZquX50HwhtYep5oA@mail.gmail.com","threadId":"50305","inReplyTo":"20190124123240.GB11354@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T18:55:10Z","receivedAt":"2019-01-24T18:55:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 4:32 AM Jeff King <peff@peff.net> wrote:\n>\n> There's an odd bug when \"log --color-moved\" is used with the combination\n> of \"--cc --stat -p\": the stat for merge commits is erroneously shown\n> with the diff of the _next_ commit.\n>\n> The included test demonstrates the issue. Our history looks something\n> like this:\n>\n>   A-B-M--D\n>    \\ /\n>     C\n>\n> When we run \"git log --cc --stat -p --color-moved\" starting at D, we get\n> this sequence of events:\n>\n>   1. The diff for D is using -p, so diff_flush() calls into\n>      diff_flush_patch_all_file_pairs(). There we see that o->color_moved\n>      is in effect, so we point o->emitted_symbols to a static local\n>      struct, causing diff_flush_patch() to queue the symbols instead of\n>      actually writing them out.\n>\n>      We then do our move detection, emit the symbols, and clear the\n>      struct. But we leave o->emitted_symbols pointing to our struct.\n>\n>   2. Next we compute the diff for M. This is a merge, so we use the\n>      combined diff code. In find_paths_generic(), we compute the\n>      pairwise diff between each commit and its parent. Normally this is\n>      done with DIFF_FORMAT_NO_OUTPUT, since we're just looking for\n>      intersecting paths. But since \"--stat --cc\" shows the first-parent\n>      stat, and since we're computing that diff anyway, we enable\n>      DIFF_FORMAT_DIFFSTAT for the first parent. This outputs the stat\n>      information immediately, saving us from running a separate\n>      first-parent diff later.\n>\n>      But where does that output go? Normally it goes directly to stdout,\n>      but because o->emitted_symbols is set, we queue it. As a result, we\n>      don't actually print the diffstat for the merge commit (yet),\n\nThanks for your analysis. As always a pleasant read.\nI understand and agree with what is written up to here remembering\nthe code vaguely.\n\n> which\n>      is wrong.\n\nI disagree with this sentiment. If we remember to flush the queued output\nthis is merely an inefficiency due to implementation details, but not wrong.\n\nWe could argue that it is wrong to have o->emitted_symbols set, as\nwe know we don't need it for producing a diffstat only.\n\n>\n>   3. Next we compute the diff for C. We're actually showing a patch\n>      again, so we end up in diff_flush_patch_all_file_pairs(), but this\n>      time we have the queued stat from step 2 waiting in our struct.\n\nRight, that is how the queueing can produce errors. I wonder if the\ntest that is included in this patch would work on top of\ne6e045f803 (\"diff.c: buffer all output if asked to\", 2017-06-29)\nas that commit specifically wanted to make sure these errors\nwould be caught.\n\n>\n>      We add new elements to it for C's diff, and then flush the whole\n>      thing. And we see the diffstat from M as part of C's diff, which is\n>      wrong.\n>\n> So triggering the bug really does require the combination of all of\n> those options.\n\nSimilarly we can trigger bugs by using options that enable buffering\n(so far only --color-moved) and options that do not fully buffer and flush.\n\n>\n> To fix it, we can simply restore o->emitted_symbols to NULL after\n> flushing it, so that it does not affect anything outside of\n> diff_flush_patch_all_file_pairs(). This intuitively makes sense, since\n> nobody outside of that function is going to bother flushing it, so we\n> would not want them to write to it either.\n\nThis would also cause the inefficiency I mentioned after (2) to disappear,\nas the merge commits diffstat would be just printed to stdout?\n\n>\n> In fact, we could take this a step further and turn the local \"esm\"\n> struct into a non-static variable that goes away after the function\n> ends. However, since it contains a dynamically sized array, we benefit\n> from amortizing the cost of allocations over many calls. So we'll leave\n> it as static to retain that benefit.\n\nokay.\n\n>\n> But let's push the zero-ing of esm.nr into the conditional for \"if\n> (o->emitted_symbols)\" to make it clear that we do not expect esm to hold\n> any values if we did not just try to use it. With the code as it is\n> written now, if we did encounter such a case (which I think would be a\n> bug), we'd silently leak those values without even bothering to display\n> them. With this change, we'd at least eventually show them, and somebody\n> would notice.\n\nWow. Good call.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n"},{"id":"367576","messageId":"f2a7e88a9b2f8fc014d4b945b8c9b489c6236436.camel@novalis.org","threadId":"50305","inReplyTo":"20190124123450.GD11354@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] combine-diff: treat --shortstat like --stat","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2019-01-24T18:58:00Z","receivedAt":"2019-01-24T18:58:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Thanks for the fix here.\n\nOn Thu, 2019-01-24 at 07:34 -0500, Jeff King wrote:\n> The --stat of a combined diff is defined as the first-parent stat,\n> going all the way back to 965f803c32 (combine-diff: show diffstat\n> with\n> the first parent., 2006-04-17).\n> \n> Naturally, we gave --numstat the same treatment in 74e2abe5b7 (diff\n> --numstat, 2006-10-12).\n> \n> But --shortstat, which is really just the final line of --stat, does\n> nothing, which produces confusing results:\n> \n>   $ git show --oneline --stat eab7584e37\n>   eab7584e37 Merge branch 'en/show-ref-doc-fix'\n> \n>    Documentation/git-show-ref.txt | 2 +-\n>    1 file changed, 1 insertion(+), 1 deletion(-)\n> \n>   $ git show --oneline --shortstat eab7584e37\n>   eab7584e37 Merge branch 'en/show-ref-doc-fix'\n> \n>   [nothing! We'd expect to see the \"1 file changed...\" line]\n> \n> This patch teaches combine-diff to treats the two formats\n> identically.\n> \n> Reported-by: David Turner <novalis@novalis.org>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  combine-diff.c                                 | 1 +\n>  t/t4013-diff-various.sh                        | 1 +\n>  t/t4013/diff.diff-tree_--cc_--shortstat_master | 4 ++++\n>  3 files changed, 6 insertions(+)\n>  create mode 100644 t/t4013/diff.diff-tree_--cc_--shortstat_master\n> \n> diff --git a/combine-diff.c b/combine-diff.c\n> index b1d259d5a0..83ec3dfffa 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1325,6 +1325,7 @@ static const char *path_path(void *obj)\n>   * Diff stat formats which we always compute solely against the\n> first parent.\n>   */\n>  #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \\\n> +\t\t\t  | DIFF_FORMAT_SHORTSTAT \\\n>  \t\t\t  | DIFF_FORMAT_DIFFSTAT)\n>  \n>  /* find set of paths that every parent touches */\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 7d985ff6b1..9ccdf08730 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -239,6 +239,7 @@ diff-tree --cc --stat --summary master\n>  # stat summary should show the diffstat and summary with the first\n> parent\n>  diff-tree -c --stat --summary side\n>  diff-tree --cc --stat --summary side\n> +diff-tree --cc --shortstat master\n>  # improved by Timo's patch\n>  diff-tree --cc --patch-with-stat master\n>  # improved by Timo's patch\n> diff --git a/t/t4013/diff.diff-tree_--cc_--shortstat_master\n> b/t/t4013/diff.diff-tree_--cc_--shortstat_master\n> new file mode 100644\n> index 0000000000..a4ca42df2a\n> --- /dev/null\n> +++ b/t/t4013/diff.diff-tree_--cc_--shortstat_master\n> @@ -0,0 +1,4 @@\n> +$ git diff-tree --cc --shortstat master\n> +59d314ad6f356dd08601a4cd5e530381da3e3c64\n> + 2 files changed, 5 insertions(+)\n> +$\n\n"},{"id":"367577","messageId":"CAGZ79kbz9QFLG1sr=WQ_kvLMis1hf41wCjK1Ngu4+1iAv3XO1g@mail.gmail.com","threadId":"50305","inReplyTo":"20190124123450.GD11354@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] combine-diff: treat --shortstat like --stat","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T19:02:16Z","receivedAt":"2019-01-24T19:02:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 4:34 AM Jeff King <peff@peff.net> wrote:\n>\n> The --stat of a combined diff is defined as the first-parent stat,\n> going all the way back to 965f803c32 (combine-diff: show diffstat with\n> the first parent., 2006-04-17).\n>\n> Naturally, we gave --numstat the same treatment in 74e2abe5b7 (diff\n> --numstat, 2006-10-12).\n>\n> But --shortstat, which is really just the final line of --stat, does\n> nothing, which produces confusing results:\n>\n>   $ git show --oneline --stat eab7584e37\n>   eab7584e37 Merge branch 'en/show-ref-doc-fix'\n>\n>    Documentation/git-show-ref.txt | 2 +-\n>    1 file changed, 1 insertion(+), 1 deletion(-)\n>\n>   $ git show --oneline --shortstat eab7584e37\n>   eab7584e37 Merge branch 'en/show-ref-doc-fix'\n>\n>   [nothing! We'd expect to see the \"1 file changed...\" line]\n>\n> This patch teaches combine-diff to treats the two formats identically.\n>\n> Reported-by: David Turner <novalis@novalis.org>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nBoth the previous patch and this one is reviewed by me.\n\nThanks,\nStefan\n"},{"id":"367579","messageId":"20190124191124.GB29828@sigill.intra.peff.net","threadId":"50305","inReplyTo":"CAGZ79kbHLvN252v-gNbcpsyGg8pZ9GPBtyZquX50HwhtYep5oA@mail.gmail.com","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T19:11:24Z","receivedAt":"2019-01-24T19:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 10:55:10AM -0800, Stefan Beller wrote:\n\n> >      But where does that output go? Normally it goes directly to stdout,\n> >      but because o->emitted_symbols is set, we queue it. As a result, we\n> >      don't actually print the diffstat for the merge commit (yet),\n> \n> Thanks for your analysis. As always a pleasant read.\n> I understand and agree with what is written up to here remembering\n> the code vaguely.\n> \n> > which\n> >      is wrong.\n> \n> I disagree with this sentiment. If we remember to flush the queued output\n> this is merely an inefficiency due to implementation details, but not wrong.\n> \n> We could argue that it is wrong to have o->emitted_symbols set, as\n> we know we don't need it for producing a diffstat only.\n\nIt's wrong in the sense that we finish printing that merge commit\nwithout having shown its diff. If it were the final commit, we would not\never print it at all!\n\nSo if you are arguing that it would be OK to queue it as long as we\nflushed it before deciding we were done with the diff, then I agree. But\ndoing that correctly would actually be non-trivial, because the\ncombined-diff code does not use the emitted_symbols queue for its diff\n(so the stat and the patch would appear out of order).\n\nI also wondered why diffstats go to o->emitted_symbols at all. We do not\ndo any analysis of them with --color-moved, I don't think. But I can\nalso see that having emitted_symbols hold everything makes sense from a\nmaintainability standpoint; future features may want to see more of what\nwe're emitting.\n\n> >   3. Next we compute the diff for C. We're actually showing a patch\n> >      again, so we end up in diff_flush_patch_all_file_pairs(), but this\n> >      time we have the queued stat from step 2 waiting in our struct.\n> \n> Right, that is how the queueing can produce errors. I wonder if the\n> test that is included in this patch would work on top of\n> e6e045f803 (\"diff.c: buffer all output if asked to\", 2017-06-29)\n> as that commit specifically wanted to make sure these errors\n> would be caught.\n\nI suspect that would not work with \"--cc\", because combine-diff outputs\ndirectly stdout. That's something that we might want to improve in the\nlong run (since obviously it cannot use --color-moved at this point).\n\n> > To fix it, we can simply restore o->emitted_symbols to NULL after\n> > flushing it, so that it does not affect anything outside of\n> > diff_flush_patch_all_file_pairs(). This intuitively makes sense, since\n> > nobody outside of that function is going to bother flushing it, so we\n> > would not want them to write to it either.\n> \n> This would also cause the inefficiency I mentioned after (2) to disappear,\n> as the merge commits diffstat would be just printed to stdout?\n\nYes, it avoids the overhead of even storing them in the emitted struct\nat all.\n\n> Reviewed-by: Stefan Beller <sbeller@google.com>\n\nThanks!\n\nI did quite a bit of head-scratching figuring out this bug, but at the\nend of it I now understand the flow of the color-moved code quite a bit\nbetter. :)\n\n-Peff\n"},{"id":"367580","messageId":"CAGZ79kY-xMDDgLgkWdc9CoZucd4S557NEPQdvPrd2+_LJAretA@mail.gmail.com","threadId":"50305","inReplyTo":"20190124123539.GE11354@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] combine-diff: treat --summary like --stat","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T19:14:15Z","receivedAt":"2019-01-24T19:14:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 4:35 AM Jeff King <peff@peff.net> wrote:\n\n> Note that we have to tweak t4013's setup a bit to test this case, as the\n> existing merges do not have any --summary results against their first\n> parent. But since the merge at the tip of 'master' does add and remove\n> files with respect to the second parent, we can just make a reversed\n> doppelganger merge where the parents are swapped.\n\n...\n\n> +       # Same merge as master, but with parents reversed. Hide it in a\n> +       # pseudo-ref to avoid impacting tests with --all.\n\nThere are 2 calls with --all, which may be worth testing for as well\nassuming we still have similar bugs as shown in the second patch,\nbut I guess this would also allow for other tests (how do we list all\npseudo refs for example?) to cover more corner cases.\n\nI am not sure I like this.\n"},{"id":"367592","messageId":"20190124192324.GB31073@sigill.intra.peff.net","threadId":"50305","inReplyTo":"CAGZ79kY-xMDDgLgkWdc9CoZucd4S557NEPQdvPrd2+_LJAretA@mail.gmail.com","subject":"Re: [PATCH 5/6] combine-diff: treat --summary like --stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T19:23:25Z","receivedAt":"2019-01-24T20:17:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 11:14:15AM -0800, Stefan Beller wrote:\n\n> On Thu, Jan 24, 2019 at 4:35 AM Jeff King <peff@peff.net> wrote:\n> \n> > Note that we have to tweak t4013's setup a bit to test this case, as the\n> > existing merges do not have any --summary results against their first\n> > parent. But since the merge at the tip of 'master' does add and remove\n> > files with respect to the second parent, we can just make a reversed\n> > doppelganger merge where the parents are swapped.\n> \n> ...\n> \n> > +       # Same merge as master, but with parents reversed. Hide it in a\n> > +       # pseudo-ref to avoid impacting tests with --all.\n> \n> There are 2 calls with --all, which may be worth testing for as well\n> assuming we still have similar bugs as shown in the second patch,\n> but I guess this would also allow for other tests (how do we list all\n> pseudo refs for example?) to cover more corner cases.\n> \n> I am not sure I like this.\n\nThe --all tests aren't actually very thorough. In fact, they don't\ngenerate diffs at all, making it especially silly that they are in\nt4013-diff-various. They are only looking at --decorate.\n\nIt also would not be the end of the world to modify the expected output\nfor those tests. You can see the extend of the damage by applying the\npatch below on top and running t4013 with \"-v\".\n\n---\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 9f8f0e84ad..742c3cdbcb 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -98,11 +98,10 @@ test_expect_success setup '\n \tgit commit -m \"update mode\" &&\n \tgit checkout -f master &&\n \n-\t# Same merge as master, but with parents reversed. Hide it in a\n-\t# pseudo-ref to avoid impacting tests with --all.\n+\t# Same merge as master, but with parents reversed.\n \tcommit=$(echo reverse |\n \t\t git commit-tree -p master^2 -p master^1 master^{tree}) &&\n-\tgit update-ref REVERSE $commit &&\n+\tgit update-ref refs/heads/reverse $commit &&\n \n \tgit config diff.renames false &&\n \n@@ -246,7 +245,7 @@ diff-tree --cc --stat --summary master\n diff-tree -c --stat --summary side\n diff-tree --cc --stat --summary side\n diff-tree --cc --shortstat master\n-diff-tree --cc --summary REVERSE\n+diff-tree --cc --summary reverse\n # improved by Timo's patch\n diff-tree --cc --patch-with-stat master\n # improved by Timo's patch\ndiff --git a/t/t4013/diff.diff-tree_--cc_--summary_REVERSE b/t/t4013/diff.diff-tree_--cc_--summary_reverse\nsimilarity index 75%\nrename from t/t4013/diff.diff-tree_--cc_--summary_REVERSE\nrename to t/t4013/diff.diff-tree_--cc_--summary_reverse\nindex e208dd5682..35da01cf46 100644\n--- a/t/t4013/diff.diff-tree_--cc_--summary_REVERSE\n+++ b/t/t4013/diff.diff-tree_--cc_--summary_reverse\n@@ -1,4 +1,4 @@\n-$ git diff-tree --cc --summary REVERSE\n+$ git diff-tree --cc --summary reverse\n 2562325a7ee916efb2481da93073b82cec801cbc\n  create mode 100644 file1\n  delete mode 100644 file2\n"},{"id":"367593","messageId":"xmqqy379hkri.fsf@gitster-ct.c.googlers.com","threadId":"50305","inReplyTo":"20190124123240.GB11354@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-24T20:18:41Z","receivedAt":"2019-01-24T20:18:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When we run \"git log --cc --stat -p --color-moved\" starting at D, we get\n> this sequence of events:\n>\n>   1. The diff for D is using -p, so diff_flush() calls into\n>      diff_flush_patch_all_file_pairs(). There we see that o->color_moved\n>      is in effect, so we point o->emitted_symbols to a static local\n>      struct, causing diff_flush_patch() to queue the symbols instead of\n>      actually writing them out.\n>\n>      We then do our move detection, emit the symbols, and clear the\n>      struct. But we leave o->emitted_symbols pointing to our struct.\n\nWow, that was nasty.  \n\nI did not like the complexity of that \"emitted symbols\" conversion\nwe had to do recently and never trusted the code.  There still is\nsomething funny in diff_flush_patch_all_file_pairs() even after this\npatch, though.\n\n - We first check o->color_moved and unconditionally point\n   o->emitted_symbols to &esm.\n\n - In an if() block we enter when o->emitted_symbols is set, there\n   is a check to see if o->color_moved is set.  This makes sense\n   only if we are trying to be prepared to handle a case where we\n   are not the one that assigned a non-NULL to o->emitted_symbols\n   due to o->color_moved.  So it certainly is possible that\n   o->emitted_symbols is set before we enter this function.\n\n - But then, it means that o->emitted_symbols we may have had\n   non-NULL when the function is called may be overwritten if\n   o->color_moved is set.\n\nThe above observation does not necessarily indicate any bug; it just\nshows that the code structure is messier than necessary.\n\n> To fix it, we can simply restore o->emitted_symbols to NULL after\n> flushing it, so that it does not affect anything outside of\n> diff_flush_patch_all_file_pairs(). This intuitively makes sense, since\n> nobody outside of that function is going to bother flushing it, so we\n> would not want them to write to it either.\n\nPerhaps.  I see word-diff codepath gives an allocated buffer to\no->emitted_symbols, so assigning NULL without freeing would mean a\nleak, but I guess this helper function is not designed to be called\n"},{"id":"367594","messageId":"CAGZ79kboXBvd5kNHmHzJCsmoE7GWmYg9v5GhE4b3kPNwVqCTVw@mail.gmail.com","threadId":"50305","inReplyTo":"20190124122603.GA10415@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] some diff --cc --stat fixes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T19:21:29Z","receivedAt":"2019-01-24T20:19:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 4:26 AM Jeff King <peff@peff.net> wrote:\n>\n> This started as a follow-up to my \"something like this\" bug-fix from\n> last July:\n>\n>  https://public-inbox.org/git/20180710203438.GB6886@sigill.intra.peff.net/\n>\n> (better late than never). But in the course of that, I discovered\n> another really weird and interesting bug with --color-moved. This fixes\n> both, along with some related cases.\n\nI reviewed the whole series, and I like it except for the small nit\nin the test setup. Arguably we can fix the --all tests once the time\ncomes, so I am in favor of taking this series as-is.\n\nThanks,\nStefan\n"},{"id":"367597","messageId":"CAGZ79kZR--VN2BiG=usyy=J=u59Up3XYpK4c9pJYQkX1Gby06Q@mail.gmail.com","threadId":"50305","inReplyTo":"xmqqy379hkri.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-24T20:36:38Z","receivedAt":"2019-01-24T20:36:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 24, 2019 at 12:18 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> > When we run \"git log --cc --stat -p --color-moved\" starting at D, we get\n> > this sequence of events:\n> >\n> >   1. The diff for D is using -p, so diff_flush() calls into\n> >      diff_flush_patch_all_file_pairs(). There we see that o->color_moved\n> >      is in effect, so we point o->emitted_symbols to a static local\n> >      struct, causing diff_flush_patch() to queue the symbols instead of\n> >      actually writing them out.\n> >\n> >      We then do our move detection, emit the symbols, and clear the\n> >      struct. But we leave o->emitted_symbols pointing to our struct.\n>\n> Wow, that was nasty.\n>\n> I did not like the complexity of that \"emitted symbols\" conversion\n> we had to do recently and never trusted the code.  There still is\n> something funny in diff_flush_patch_all_file_pairs() even after this\n> patch, though.\n>\n>  - We first check o->color_moved and unconditionally point\n>    o->emitted_symbols to &esm.\n\nI do not recall if we discussed this or if I just had talking voices in my\nhead, but this was done deliberately to separate the implementation\nof the buffering feature and its user. I thought once we had the buffering\nin place, we might add options that also require buffering;\nhence the explicit \"move coloring implies using a buffer\".\n\nHaving the buffer as a local static might be an iffy implementation\nthat should be fixed for clarify.\n\n>\n>  - In an if() block we enter when o->emitted_symbols is set, there\n>    is a check to see if o->color_moved is set.  This makes sense\n>    only if we are trying to be prepared to handle a case where we\n>    are not the one that assigned a non-NULL to o->emitted_symbols\n>    due to o->color_moved.  So it certainly is possible that\n>    o->emitted_symbols is set before we enter this function.\n\nAh I see where you are coming from, as the code was written\nI imagined:\n\n    if (o->color_moved)\n        o->emitted_symbols = &esm;\n    if (o->distim_gostaks)\n        o->emitted_symbols = &esm;\n\n    if (o->emitted_symbols) {\n         if (o->color_moved)\n            handle_color_moved(o);\n        if (o->distim_gostaks)\n            handle_distimming(o);\n\n        ... flush symbols...\n        ... free &cleanup ...\n    }\n\n\n>  - But then, it means that o->emitted_symbols we may have had\n>    non-NULL when the function is called may be overwritten if\n>    o->color_moved is set.\n\nI see. So either we'd want to have\n\n    if (o->emitted_symbols)\n        BUG(\"should not be already set\");\n\nor as Peff points out, make it non-static.\n\n> The above observation does not necessarily indicate any bug; it just\n> shows that the code structure is messier than necessary.\n\nMakes sense. I should not have anticipated any new feature to be\nadded, yet.\n\n>\n> > To fix it, we can simply restore o->emitted_symbols to NULL after\n> > flushing it, so that it does not affect anything outside of\n> > diff_flush_patch_all_file_pairs(). This intuitively makes sense, since\n> > nobody outside of that function is going to bother flushing it, so we\n> > would not want them to write to it either.\n>\n> Perhaps.  I see word-diff codepath gives an allocated buffer to\n> o->emitted_symbols, so assigning NULL without freeing would mean a\n> leak, but I guess this helper function is not designed to be called\n\nYes, this was done as to make the buffering complete.\nWe would not have to buffer any word diff as the color move doesn't yet\nwork on that.\n\nSee the NEEDSWORK in diff_words_flush, where we could implement\na word based move coloring.\n"},{"id":"367601","messageId":"20190124211505.GA16114@sigill.intra.peff.net","threadId":"50305","inReplyTo":"xmqqy379hkri.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T21:15:06Z","receivedAt":"2019-01-24T21:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 12:18:41PM -0800, Junio C Hamano wrote:\n\n> I did not like the complexity of that \"emitted symbols\" conversion\n> we had to do recently and never trusted the code.  There still is\n> something funny in diff_flush_patch_all_file_pairs() even after this\n> patch, though.\n> \n>  - We first check o->color_moved and unconditionally point\n>    o->emitted_symbols to &esm.\n> \n>  - In an if() block we enter when o->emitted_symbols is set, there\n>    is a check to see if o->color_moved is set.  This makes sense\n>    only if we are trying to be prepared to handle a case where we\n>    are not the one that assigned a non-NULL to o->emitted_symbols\n>    due to o->color_moved.  So it certainly is possible that\n>    o->emitted_symbols is set before we enter this function.\n\nYeah, I noticed that, too. I assumed it was preparing for a day when the\nlogic for \"are we collecting symbols\" becomes more complex than just\nbeing equivalent to \"o->color_moved\".\n\nUnder that rationale, I was OK leaving it.\n\n>  - But then, it means that o->emitted_symbols we may have had\n>    non-NULL when the function is called may be overwritten if\n>    o->color_moved is set.\n\nYeah, that is true. I think in the new world order proposed by this\npatch, we'd always assume that it's NULL coming in, possibly assign it,\nand re-NULL it going\nout.\n\n> The above observation does not necessarily indicate any bug; it just\n> shows that the code structure is messier than necessary.\n\nYeah, I don't think it's a bug currently, although...\n\n> > To fix it, we can simply restore o->emitted_symbols to NULL after\n> > flushing it, so that it does not affect anything outside of\n> > diff_flush_patch_all_file_pairs(). This intuitively makes sense, since\n> > nobody outside of that function is going to bother flushing it, so we\n> > would not want them to write to it either.\n> \n> Perhaps.  I see word-diff codepath gives an allocated buffer to\n> o->emitted_symbols, so assigning NULL without freeing would mean a\n> leak, but I guess this helper function is not designed to be called\n\nHrm, I'm embarrassed to say I did not notice that it also uses the\nemitted_symbols system.\n\nI think we only do it there, though, in the sub-diff_options that\nword-diff uses, in which case we make a separate emitted_diff_symbols\nstruct instead of re-using the one from the parent diff_options.\n\nSo I think the general idea still holds, which is that whoever assigns\nthe emitted_symbols flag is responsible for flushing it. For\n--color-moved, that happens in a single function, but for word-diff,\nit's split across the init/flush functions.\n\n-Peff\n"},{"id":"367604","messageId":"20190124211715.GB16114@sigill.intra.peff.net","threadId":"50305","inReplyTo":"CAGZ79kZR--VN2BiG=usyy=J=u59Up3XYpK4c9pJYQkX1Gby06Q@mail.gmail.com","subject":"Re: [PATCH 2/6] diff: clear emitted_symbols flag after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-24T21:17:15Z","receivedAt":"2019-01-24T21:17:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2019 at 12:36:38PM -0800, Stefan Beller wrote:\n\n> >  - In an if() block we enter when o->emitted_symbols is set, there\n> >    is a check to see if o->color_moved is set.  This makes sense\n> >    only if we are trying to be prepared to handle a case where we\n> >    are not the one that assigned a non-NULL to o->emitted_symbols\n> >    due to o->color_moved.  So it certainly is possible that\n> >    o->emitted_symbols is set before we enter this function.\n> \n> Ah I see where you are coming from, as the code was written\n> I imagined:\n> \n>     if (o->color_moved)\n>         o->emitted_symbols = &esm;\n>     if (o->distim_gostaks)\n>         o->emitted_symbols = &esm;\n> \n>     if (o->emitted_symbols) {\n>          if (o->color_moved)\n>             handle_color_moved(o);\n>         if (o->distim_gostaks)\n>             handle_distimming(o);\n> \n>         ... flush symbols...\n>         ... free &cleanup ...\n>     }\n\nYeah, FWIW this is what I took to be the reason for the code being laid\nout as it is.\n\n> >  - But then, it means that o->emitted_symbols we may have had\n> >    non-NULL when the function is called may be overwritten if\n> >    o->color_moved is set.\n> \n> I see. So either we'd want to have\n> \n>     if (o->emitted_symbols)\n>         BUG(\"should not be already set\");\n> \n> or as Peff points out, make it non-static.\n\nEven if it's non-static, you'd still want to ensure that it's not set\ncoming into the function (because somebody like diff-words might have\nleft it set, confusing us). So I think that BUG() may be worth having\neither way.\n\n-Peff\n"}]}