From 426fd36f4a899b5c2299fe2eed82affc7385c8cf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:27:29 -0500 Subject: [PATCH 1/6] t4006: resurrect commented-out tests This set of tests was added by 4434e6ba6c (tests: check --[short]stat output after chmod, 2012-05-01), and is primarily about the handling of binary versus text files. Later, 74faaa16f0 (Fix "git diff --stat" for interesting - but empty - file changes, 2012-10-17) changed the stat output so that the empty text file is mentioned rather than omitted. That commit just comments out these tests. There's no discussion in the commit message, but the original email[1] says: NOTE! This does break two of our tests, so we clearly did this on purpose, or at least tested for it. I just uncommented the subtests that this makes irrelevant, and changed the output of another one. I don't think they're irrelevant, though. We should be testing this "mode change only" case and making sure that it has the post-74faaa16f0 behavior. So this commit brings back those tests, with the current expected output. [1] https://public-inbox.org/git/CA+55aFz88GPJcfMSqiyY+u0Cdm48bEyrsTGxHVJbGsYsDg=Q5w@mail.gmail.com/ Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4006-diff-mode.sh | 55 ++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh index a8e01eccd1..03489aff14 100755 --- a/t/t4006-diff-mode.sh +++ b/t/t4006-diff-mode.sh @@ -32,28 +32,37 @@ test_expect_success 'prepare binary file' ' git commit -m binbin ' -# test_expect_success '--stat output after text chmod' ' -# test_chmod -x rezrov && -# echo " 0 files changed" >expect && -# git diff HEAD --stat >actual && -# test_i18ncmp expect actual -# ' -# -# test_expect_success '--shortstat output after text chmod' ' -# git diff HEAD --shortstat >actual && -# test_i18ncmp expect actual -# ' -# -# test_expect_success '--stat output after binary chmod' ' -# test_chmod +x binbin && -# echo " 0 files changed" >expect && -# git diff HEAD --stat >actual && -# test_i18ncmp expect actual -# ' -# -# test_expect_success '--shortstat output after binary chmod' ' -# git diff HEAD --shortstat >actual && -# test_i18ncmp expect actual -# ' +test_expect_success '--stat output after text chmod' ' + test_chmod -x rezrov && + cat >expect <<-\EOF && + rezrov | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) + EOF + git diff HEAD --stat >actual && + test_i18ncmp expect actual +' + +test_expect_success '--shortstat output after text chmod' ' + tail -n 1 expect.short && + git diff HEAD --shortstat >actual && + test_i18ncmp expect.short actual +' + +test_expect_success '--stat output after binary chmod' ' + test_chmod +x binbin && + cat >expect <<-EOF && + binbin | Bin + rezrov | 0 + 2 files changed, 0 insertions(+), 0 deletions(-) + EOF + git diff HEAD --stat >actual && + test_i18ncmp expect actual +' + +test_expect_success '--shortstat output after binary chmod' ' + tail -n 1 expect.short && + git diff HEAD --shortstat >actual && + test_i18ncmp expect.short actual +' test_done From 48edf3a02addfa6385cd04c62a1c48671aad19aa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:32:41 -0500 Subject: [PATCH 2/6] diff: clear emitted_symbols flag after use There's an odd bug when "log --color-moved" is used with the combination of "--cc --stat -p": the stat for merge commits is erroneously shown with the diff of the _next_ commit. The included test demonstrates the issue. Our history looks something like this: A-B-M--D \ / C When we run "git log --cc --stat -p --color-moved" starting at D, we get this sequence of events: 1. The diff for D is using -p, so diff_flush() calls into diff_flush_patch_all_file_pairs(). There we see that o->color_moved is in effect, so we point o->emitted_symbols to a static local struct, causing diff_flush_patch() to queue the symbols instead of actually writing them out. We then do our move detection, emit the symbols, and clear the struct. But we leave o->emitted_symbols pointing to our struct. 2. Next we compute the diff for M. This is a merge, so we use the combined diff code. In find_paths_generic(), we compute the pairwise diff between each commit and its parent. Normally this is done with DIFF_FORMAT_NO_OUTPUT, since we're just looking for intersecting paths. But since "--stat --cc" shows the first-parent stat, and since we're computing that diff anyway, we enable DIFF_FORMAT_DIFFSTAT for the first parent. This outputs the stat information immediately, saving us from running a separate first-parent diff later. But where does that output go? Normally it goes directly to stdout, but because o->emitted_symbols is set, we queue it. As a result, we don't actually print the diffstat for the merge commit (yet), which is wrong. 3. Next we compute the diff for C. We're actually showing a patch again, so we end up in diff_flush_patch_all_file_pairs(), but this time we have the queued stat from step 2 waiting in our struct. We add new elements to it for C's diff, and then flush the whole thing. And we see the diffstat from M as part of C's diff, which is wrong. So triggering the bug really does require the combination of all of those options. To fix it, we can simply restore o->emitted_symbols to NULL after flushing it, so that it does not affect anything outside of diff_flush_patch_all_file_pairs(). This intuitively makes sense, since nobody outside of that function is going to bother flushing it, so we would not want them to write to it either. In fact, we could take this a step further and turn the local "esm" struct into a non-static variable that goes away after the function ends. However, since it contains a dynamically sized array, we benefit from amortizing the cost of allocations over many calls. So we'll leave it as static to retain that benefit. But let's push the zero-ing of esm.nr into the conditional for "if (o->emitted_symbols)" to make it clear that we do not expect esm to hold any values if we did not just try to use it. With the code as it is written now, if we did encounter such a case (which I think would be a bug), we'd silently leak those values without even bothering to display them. With this change, we'd at least eventually show them, and somebody would notice. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 4 +- t/t4066-diff-emit-delay.sh | 79 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100755 t/t4066-diff-emit-delay.sh diff --git a/diff.c b/diff.c index dc9965e836..a4016087df 100644 --- a/diff.c +++ b/diff.c @@ -5885,8 +5885,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o) for (i = 0; i < esm.nr; i++) free((void *)esm.buf[i].line); + esm.nr = 0; + + o->emitted_symbols = NULL; } - esm.nr = 0; } void diff_flush(struct diff_options *options) diff --git a/t/t4066-diff-emit-delay.sh b/t/t4066-diff-emit-delay.sh new file mode 100755 index 0000000000..5df6b5e64e --- /dev/null +++ b/t/t4066-diff-emit-delay.sh @@ -0,0 +1,79 @@ +#!/bin/sh + +test_description='test combined/stat/moved interaction' +. ./test-lib.sh + +# This test covers a weird 3-way interaction between "--cc -p", which will run +# the combined diff code, along with "--stat", which will be computed as a +# first-parent stat during the combined diff, and "--color-moved", which +# enables the emitted_symbols list to store the diff in memory. + +test_expect_success 'set up history with a merge' ' + test_commit A && + test_commit B && + git checkout -b side HEAD^ && + test_commit C && + git merge -m M master && + test_commit D +' + +test_expect_success 'log --cc -p --stat --color-moved' ' + cat >expect <<-\EOF && + commit D + --- + D.t | 1 + + 1 file changed, 1 insertion(+) + + diff --git a/D.t b/D.t + new file mode 100644 + index 0000000..1784810 + --- /dev/null + +++ b/D.t + @@ -0,0 +1 @@ + +D + commit M + + B.t | 1 + + 1 file changed, 1 insertion(+) + commit C + --- + C.t | 1 + + 1 file changed, 1 insertion(+) + + diff --git a/C.t b/C.t + new file mode 100644 + index 0000000..3cc58df + --- /dev/null + +++ b/C.t + @@ -0,0 +1 @@ + +C + commit B + --- + B.t | 1 + + 1 file changed, 1 insertion(+) + + diff --git a/B.t b/B.t + new file mode 100644 + index 0000000..223b783 + --- /dev/null + +++ b/B.t + @@ -0,0 +1 @@ + +B + commit A + --- + A.t | 1 + + 1 file changed, 1 insertion(+) + + diff --git a/A.t b/A.t + new file mode 100644 + index 0000000..f70f10e + --- /dev/null + +++ b/A.t + @@ -0,0 +1 @@ + +A + EOF + git log --format="commit %s" --cc -p --stat --color-moved >actual && + test_cmp expect actual +' + +test_done From 8817f0cc6751199a996342fbc28bd343d231a5ca Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:33:51 -0500 Subject: [PATCH 3/6] combine-diff: factor out stat-format mask There are several conditionals in the combine diff code that check if we're doing --stat or --numstat output. Since these must all remain in sync, let's pull them out into a separate bit-mask. Arguably this could go into diff.h along with the other DIFF_FORMAT macros, but it's not clear that the definition of "which formats are stat" is universal (e.g., does --dirstat count? --summary?). So let's keep this local to combine-diff.c, where the meaning is more clearly "stat formats that combine-diff supports". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/combine-diff.c b/combine-diff.c index ad7752ea6b..ebf9064e27 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1321,6 +1321,11 @@ static const char *path_path(void *obj) return path->path; } +/* + * Diff stat formats which we always compute solely against the first parent. + */ +#define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \ + | DIFF_FORMAT_DIFFSTAT) /* find set of paths that every parent touches */ static struct combine_diff_path *find_paths_generic(const struct object_id *oid, @@ -1342,8 +1347,7 @@ static struct combine_diff_path *find_paths_generic(const struct object_id *oid, * show stat against the first parent even when doing * combined diff. */ - int stat_opt = (output_format & - (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT)); + int stat_opt = output_format & STAT_FORMAT_MASK; if (i == 0 && stat_opt) opt->output_format = stat_opt; else @@ -1470,8 +1474,7 @@ void diff_tree_combined(const struct object_id *oid, * show stat against the first parent even * when doing combined diff. */ - stat_opt = (opt->output_format & - (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT)); + stat_opt = opt->output_format & STAT_FORMAT_MASK; if (stat_opt) { diffopts.output_format = stat_opt; @@ -1515,8 +1518,7 @@ void diff_tree_combined(const struct object_id *oid, show_raw_diff(p, num_parent, rev); needsep = 1; } - else if (opt->output_format & - (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT)) + else if (opt->output_format & STAT_FORMAT_MASK) needsep = 1; else if (opt->output_format & DIFF_FORMAT_CALLBACK) handle_combined_callback(opt, paths, num_parent, num_paths); From 8290faa077784a2518d0a77b448ca2987c3baf07 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:34:51 -0500 Subject: [PATCH 4/6] combine-diff: treat --shortstat like --stat The --stat of a combined diff is defined as the first-parent stat, going all the way back to 965f803c32 (combine-diff: show diffstat with the first parent., 2006-04-17). Naturally, we gave --numstat the same treatment in 74e2abe5b7 (diff --numstat, 2006-10-12). But --shortstat, which is really just the final line of --stat, does nothing, which produces confusing results: $ git show --oneline --stat eab7584e37 eab7584e37 Merge branch 'en/show-ref-doc-fix' Documentation/git-show-ref.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) $ git show --oneline --shortstat eab7584e37 eab7584e37 Merge branch 'en/show-ref-doc-fix' [nothing! We'd expect to see the "1 file changed..." line] This patch teaches combine-diff to treats the two formats identically. Reported-by: David Turner Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 1 + t/t4013-diff-various.sh | 1 + t/t4013/diff.diff-tree_--cc_--shortstat_master | 4 ++++ 3 files changed, 6 insertions(+) create mode 100644 t/t4013/diff.diff-tree_--cc_--shortstat_master diff --git a/combine-diff.c b/combine-diff.c index ebf9064e27..b4a92a8505 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1325,6 +1325,7 @@ static const char *path_path(void *obj) * Diff stat formats which we always compute solely against the first parent. */ #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \ + | DIFF_FORMAT_SHORTSTAT \ | DIFF_FORMAT_DIFFSTAT) /* find set of paths that every parent touches */ diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 7d985ff6b1..9ccdf08730 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -239,6 +239,7 @@ diff-tree --cc --stat --summary master # stat summary should show the diffstat and summary with the first parent diff-tree -c --stat --summary side diff-tree --cc --stat --summary side +diff-tree --cc --shortstat master # improved by Timo's patch diff-tree --cc --patch-with-stat master # improved by Timo's patch diff --git a/t/t4013/diff.diff-tree_--cc_--shortstat_master b/t/t4013/diff.diff-tree_--cc_--shortstat_master new file mode 100644 index 0000000000..a4ca42df2a --- /dev/null +++ b/t/t4013/diff.diff-tree_--cc_--shortstat_master @@ -0,0 +1,4 @@ +$ git diff-tree --cc --shortstat master +59d314ad6f356dd08601a4cd5e530381da3e3c64 + 2 files changed, 5 insertions(+) +$ From 04b19fcafd734245e61fd1c090cd7de8c2993eaa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:35:40 -0500 Subject: [PATCH 5/6] combine-diff: treat --summary like --stat Currently "--cc --summary" on a merge shows nothing. Since we show "--cc --stat" as a stat against the first parent, and because --summary is typically used in combination with --stat, it makes sense to treat them both the same way. Note that we have to tweak t4013's setup a bit to test this case, as the existing merges do not have any --summary results against their first parent. But since the merge at the tip of 'master' does add and remove files with respect to the second parent, we can just make a reversed doppelganger merge where the parents are swapped. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 1 + t/t4013-diff-various.sh | 7 +++++++ t/t4013/diff.diff-tree_--cc_--summary_REVERSE | 6 ++++++ 3 files changed, 14 insertions(+) create mode 100644 t/t4013/diff.diff-tree_--cc_--summary_REVERSE diff --git a/combine-diff.c b/combine-diff.c index b4a92a8505..ea79830f61 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1326,6 +1326,7 @@ static const char *path_path(void *obj) */ #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \ | DIFF_FORMAT_SHORTSTAT \ + | DIFF_FORMAT_SUMMARY \ | DIFF_FORMAT_DIFFSTAT) /* find set of paths that every parent touches */ diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 9ccdf08730..e28953975b 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -98,6 +98,12 @@ test_expect_success setup ' git commit -m "update mode" && git checkout -f master && + # Same merge as master, but with parents reversed. Hide it in a + # pseudo-ref to avoid impacting tests with --all. + commit=$(echo reverse | + git commit-tree -p master^2 -p master^1 master^{tree}) && + git update-ref REVERSE $commit && + git config diff.renames false && git show-branch @@ -240,6 +246,7 @@ diff-tree --cc --stat --summary master diff-tree -c --stat --summary side diff-tree --cc --stat --summary side diff-tree --cc --shortstat master +diff-tree --cc --summary REVERSE # improved by Timo's patch diff-tree --cc --patch-with-stat master # improved by Timo's patch diff --git a/t/t4013/diff.diff-tree_--cc_--summary_REVERSE b/t/t4013/diff.diff-tree_--cc_--summary_REVERSE new file mode 100644 index 0000000000..e208dd5682 --- /dev/null +++ b/t/t4013/diff.diff-tree_--cc_--summary_REVERSE @@ -0,0 +1,6 @@ +$ git diff-tree --cc --summary REVERSE +2562325a7ee916efb2481da93073b82cec801cbc + create mode 100644 file1 + delete mode 100644 file2 + delete mode 100644 file3 +$ From dac03b5518a053e52ed7ee20925970728d3ae4c3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jan 2019 07:36:47 -0500 Subject: [PATCH 6/6] combine-diff: treat --dirstat like --stat Currently "--cc --dirstat" will show nothing for a merge. Like --shortstat and --summary in the previous two patches, it probably makes sense to treat it like we do --stat, and show a stat against the first-parent. This case is less obviously correct than for --shortstat and --summary, as those are basically variants of --stat themselves. It's possible we could develop a multi-parent combined dirstat format, in which case we might regret defining this first-parent behavior. But the same could be said for --stat, and in the 12+ years of it showing first-parent stats, nobody has complained. So showing the first-parent dirstat is at least _useful_, and if we later develop a clever multi-parent stat format, we'd probably have to deal with --stat anyway. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 1 + t/t4013-diff-various.sh | 1 + t/t4013/diff.diff_--dirstat_--cc_master~1_master | 3 +++ 3 files changed, 5 insertions(+) create mode 100644 t/t4013/diff.diff_--dirstat_--cc_master~1_master diff --git a/combine-diff.c b/combine-diff.c index ea79830f61..dc9d986618 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1327,6 +1327,7 @@ static const char *path_path(void *obj) #define STAT_FORMAT_MASK (DIFF_FORMAT_NUMSTAT \ | DIFF_FORMAT_SHORTSTAT \ | DIFF_FORMAT_SUMMARY \ + | DIFF_FORMAT_DIRSTAT \ | DIFF_FORMAT_DIFFSTAT) /* find set of paths that every parent touches */ diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index e28953975b..9f8f0e84ad 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -358,6 +358,7 @@ diff --line-prefix=abc master master^ side diff --dirstat master~1 master~2 diff --dirstat initial rearrange diff --dirstat-by-file initial rearrange +diff --dirstat --cc master~1 master # No-index --abbrev and --no-abbrev diff --raw initial :noellipses diff --raw initial diff --git a/t/t4013/diff.diff_--dirstat_--cc_master~1_master b/t/t4013/diff.diff_--dirstat_--cc_master~1_master new file mode 100644 index 0000000000..fba4e34175 --- /dev/null +++ b/t/t4013/diff.diff_--dirstat_--cc_master~1_master @@ -0,0 +1,3 @@ +$ git diff --dirstat --cc master~1 master + 40.0% dir/ +$