| Message ID | 20190124123539.GE11354@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some diff --cc --stat fixes | expand |
On Thu, Jan 24, 2019 at 4:35 AM Jeff King <peff@peff.net> wrote: > 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. ... > + # Same merge as master, but with parents reversed. Hide it in a > + # pseudo-ref to avoid impacting tests with --all. There are 2 calls with --all, which may be worth testing for as well assuming we still have similar bugs as shown in the second patch, but I guess this would also allow for other tests (how do we list all pseudo refs for example?) to cover more corner cases. I am not sure I like this.
On Thu, Jan 24, 2019 at 11:14:15AM -0800, Stefan Beller wrote: > On Thu, Jan 24, 2019 at 4:35 AM Jeff King <peff@peff.net> wrote: > > > 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. > > ... > > > + # Same merge as master, but with parents reversed. Hide it in a > > + # pseudo-ref to avoid impacting tests with --all. > > There are 2 calls with --all, which may be worth testing for as well > assuming we still have similar bugs as shown in the second patch, > but I guess this would also allow for other tests (how do we list all > pseudo refs for example?) to cover more corner cases. > > I am not sure I like this. The --all tests aren't actually very thorough. In fact, they don't generate diffs at all, making it especially silly that they are in t4013-diff-various. They are only looking at --decorate. It also would not be the end of the world to modify the expected output for those tests. You can see the extend of the damage by applying the patch below on top and running t4013 with "-v". --- diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 9f8f0e84ad..742c3cdbcb 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -98,11 +98,10 @@ 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. + # Same merge as master, but with parents reversed. commit=$(echo reverse | git commit-tree -p master^2 -p master^1 master^{tree}) && - git update-ref REVERSE $commit && + git update-ref refs/heads/reverse $commit && git config diff.renames false && @@ -246,7 +245,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 +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 similarity index 75% rename from t/t4013/diff.diff-tree_--cc_--summary_REVERSE rename to t/t4013/diff.diff-tree_--cc_--summary_reverse index e208dd5682..35da01cf46 100644 --- a/t/t4013/diff.diff-tree_--cc_--summary_REVERSE +++ b/t/t4013/diff.diff-tree_--cc_--summary_reverse @@ -1,4 +1,4 @@ -$ git diff-tree --cc --summary REVERSE +$ git diff-tree --cc --summary reverse 2562325a7ee916efb2481da93073b82cec801cbc create mode 100644 file1 delete mode 100644 file2
diff --git a/combine-diff.c b/combine-diff.c index 83ec3dfffa..f42b41d884 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 +$
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 <peff@peff.net> --- 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