Re: [PATCH 11/18] branch-diff: add tests

[Johannes Schindelin]

Hi Philip,

On Fri, 4 May 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> > From: Thomas Rast 
> > 
> > These are essentially lifted from https://github.com/trast/tbdiff, with
> > light touch-ups to account for the new command name.
> > 
> > Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
> > to be adjusted: 11 - 'changed message'.
> > 
> > The underlying reason it had to be adjusted is that diff generation is
> > sometimes ambiguous. In this case, a comment line and an empty line are
> > added, but it is ambiguous whether they were added after the existing
> > empty line, or whether an empty line and the comment line are added
> > *before* the existing emtpy line. And apparently xdiff picks a different
> 
> s/emtpy/empty/

Yep, that has been pointed out by others, too... ;-)

It is good that this thing gets a really good review *grins*

Ciao,
Dscho

Re: [PATCH 11/18] branch-diff: add tests

[Philip Oakley]

From: "Johannes Schindelin" 

From: Thomas Rast 

These are essentially lifted from https://github.com/trast/tbdiff, with
light touch-ups to account for the new command name.

Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
to be adjusted: 11 - 'changed message'.

The underlying reason it had to be adjusted is that diff generation is
sometimes ambiguous. In this case, a comment line and an empty line are
added, but it is ambiguous whether they were added after the existing
empty line, or whether an empty line and the comment line are added
*before* the existing emtpy line. And apparently xdiff picks a different


s/emtpy/empty/


option here than Python's difflib.

Signed-off-by: Johannes Schindelin 

[...]
Philip

Re: [PATCH 11/18] branch-diff: add tests

[Johannes Schindelin]

Hi Stefan,

On Thu, 3 May 2018, Stefan Beller wrote:

> On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
>  wrote:
> > From: Thomas Rast 
> >
> > These are essentially lifted from https://github.com/trast/tbdiff, with
> > light touch-ups to account for the new command name.
> >
> > Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
> > to be adjusted: 11 - 'changed message'.
> >
> > The underlying reason it had to be adjusted is that diff generation is
> > sometimes ambiguous. In this case, a comment line and an empty line are
> > added, but it is ambiguous whether they were added after the existing
> > empty line, or whether an empty line and the comment line are added
> > *before* the existing emtpy line. And apparently xdiff picks a different
> > option here than Python's difflib.
> 
> I think that is the fallout of the diff heuristics. If you are keen on
> a 1:1 port, you can disable the diff sliding heuristics and it should
> produce the same diff with trailing new lines.

I am not keen on a 1:1 port. I am fine with having xdiff generate
different diffs than Python's difflib. That's par for the course when
relying on a not-quite-well-defined metric.

Ciao,
Dscho

Re: [PATCH 11/18] branch-diff: add tests

[Johannes Schindelin]

Hi Ævar,

On Thu, 3 May 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, May 03 2018, Johannes Schindelin wrote:
> 
> > *before* the existing emtpy line. And apparently xdiff picks a different
> 
> s/emtpy/empty/

Thanks for the spell check!
Dscho

Re: [PATCH 11/18] branch-diff: add tests

[Stefan Beller]

On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
 wrote:
> From: Thomas Rast 
>
> These are essentially lifted from https://github.com/trast/tbdiff, with
> light touch-ups to account for the new command name.
>
> Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
> to be adjusted: 11 - 'changed message'.
>
> The underlying reason it had to be adjusted is that diff generation is
> sometimes ambiguous. In this case, a comment line and an empty line are
> added, but it is ambiguous whether they were added after the existing
> empty line, or whether an empty line and the comment line are added
> *before* the existing emtpy line. And apparently xdiff picks a different
> option here than Python's difflib.

I think that is the fallout of the diff heuristics. If you are keen on
a 1:1 port, you can disable the diff sliding heuristics and it should produce
the same diff with trailing new lines.

Re: [PATCH 11/18] branch-diff: add tests

[Ævar Arnfjörð Bjarmason]

On Thu, May 03 2018, Johannes Schindelin wrote:

> *before* the existing emtpy line. And apparently xdiff picks a different

s/emtpy/empty/

[PATCH 11/18] branch-diff: add tests

[Johannes Schindelin]

From: Thomas Rast 

These are essentially lifted from https://github.com/trast/tbdiff, with
light touch-ups to account for the new command name.

Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
to be adjusted: 11 - 'changed message'.

The underlying reason it had to be adjusted is that diff generation is
sometimes ambiguous. In this case, a comment line and an empty line are
added, but it is ambiguous whether they were added after the existing
empty line, or whether an empty line and the comment line are added
*before* the existing emtpy line. And apparently xdiff picks a different
option here than Python's difflib.

Signed-off-by: Johannes Schindelin 
---
 t/.gitattributes   |   1 +
 t/t7910-branch-diff.sh | 144 ++
 t/t7910/history.export | 604 +
 3 files changed, 749 insertions(+)
 create mode 100755 t/t7910-branch-diff.sh
 create mode 100644 t/t7910/history.export

diff --git a/t/.gitattributes b/t/.gitattributes
index 3bd959ae523..af15d5aeedd 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -18,5 +18,6 @@ t[0-9][0-9][0-9][0-9]/* -whitespace
 /t5515/* eol=lf
 /t556x_common eol=lf
 /t7500/* eol=lf
+/t7910/* eol=lf
 /t8005/*.txt eol=lf
 /t9*/*.dump eol=lf
diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
new file mode 100755
index 000..a7fece88045
--- /dev/null
+++ b/t/t7910-branch-diff.sh
@@ -0,0 +1,144 @@
+#!/bin/sh
+
+test_description='branch-diff tests'
+
+. ./test-lib.sh
+
+# Note that because of git-branch-diff's heuristics, test_commit does more
+# harm than good.  We need some real history.
+
+test_expect_success 'setup' '
+   git fast-import < "$TEST_DIRECTORY"/t7910/history.export
+'
+
+test_expect_success 'simple A..B A..C (unmodified)' '
+   git branch-diff --no-color master..topic master..unmodified >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  35b9b25 s/5/A/
+   2:  fccce22 = 2:  de345ab s/4/A/
+   3:  147e64e = 3:  9af6654 s/11/B/
+   4:  a63e992 = 4:  2901f77 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'simple B...C (unmodified)' '
+   git branch-diff --no-color topic...unmodified >actual &&
+   # same "expected" as above
+   test_cmp expected actual
+'
+
+test_expect_success 'simple A B C (unmodified)' '
+   git branch-diff --no-color master topic unmodified >actual &&
+   # same "expected" as above
+   test_cmp expected actual
+'
+
+test_expect_success 'trivial reordering' '
+   git branch-diff --no-color master topic reordered >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  aca177a s/5/A/
+   3:  147e64e = 2:  14ad629 s/11/B/
+   4:  a63e992 = 3:  ee58208 s/12/B/
+   2:  fccce22 = 4:  307b27a s/4/A/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'removed a commit' '
+   git branch-diff --no-color master topic removed >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  7657159 s/5/A/
+   2:  fccce22 < -:  --- s/4/A/
+   3:  147e64e = 2:  43d84d3 s/11/B/
+   4:  a63e992 = 3:  a740396 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'added a commit' '
+   git branch-diff --no-color master topic added >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  2716022 s/5/A/
+   2:  fccce22 = 2:  b62accd s/4/A/
+   -:  --- > 3:  df46cfa s/6/A/
+   3:  147e64e = 4:  3e64548 s/11/B/
+   4:  a63e992 = 5:  12b4063 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'new base, A B C' '
+   git branch-diff --no-color master topic rebased >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  cc9c443 s/5/A/
+   2:  fccce22 = 2:  c5d9641 s/4/A/
+   3:  147e64e = 3:  28cc2b6 s/11/B/
+   4:  a63e992 = 4:  5628ab7 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'new base, B...C' '
+   # this syntax includes the commits from master!
+   git branch-diff --no-color topic...rebased >actual &&
+   cat >expected <<-EOF &&
+   -:  --- > 1:  a31b12e unrelated
+   1:  4de457d = 2:  cc9c443 s/5/A/
+   2:  fccce22 = 3:  c5d9641 s/4/A/
+   3:  147e64e = 4:  28cc2b6 s/11/B/
+   4:  a63e992 = 5:  5628ab7 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'changed commit' '
+   git branch-diff --no-color topic...changed >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  a4b s/5/A/
+   2:  fccce22 = 2:  f51d370 s/4/A/
+   3:  147e64e ! 3:  0559556 s/11/B/
+   @@ -10,7 +10,7 @@
+ 9
+ 10
+-11
+   -+B
+   ++BB
+ 12
+ 13
+ 14
+   4:  a63e992 ! 4:  d966c5c s/12/B/
+   @@ -8,7 +8,7 @@
+@@
+ 9
+