[PATCH 0/3] Add regression tests for recent rebase -i fixes

[PATCH 0/3] Add regression tests for recent rebase -i fixes

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

These patches add some tests for the fixes I sent the week before
last. The output tests check the messages for stashes that apply and
fail for all three variants of rebase. They are quite strict but I
couldn't think of a better way to check that the output is correct and
doesn't contain anything it isn't meant to (such as the output of git
status).


Phillip Wood (3):
  rebase -i: Add test for reflog message
  rebase: Add tests for console output
  rebase: Add tests for console output with conflicting stash

 t/t3404-rebase-interactive.sh        |  7 +++++++
 t/t3420-rebase-autostash.sh          | 20 ++++++++++++++++++--
 t/t3420/expected-fail-am             |  8 ++++++++
 t/t3420/expected-fail-interactive    |  6 ++++++
 t/t3420/expected-fail-merge          | 32 ++++++++++++++++++++++++++++++++
 t/t3420/expected-success-am          |  6 ++++++
 t/t3420/expected-success-interactive |  4 ++++
 t/t3420/expected-success-merge       | 30 ++++++++++++++++++++++++++++++
 t/t3420/remove-ids.sed               |  6 ++++++
 9 files changed, 117 insertions(+), 2 deletions(-)
 create mode 100644 t/t3420/expected-fail-am
 create mode 100644 t/t3420/expected-fail-interactive
 create mode 100644 t/t3420/expected-fail-merge
 create mode 100644 t/t3420/expected-success-am
 create mode 100644 t/t3420/expected-success-interactive
 create mode 100644 t/t3420/expected-success-merge
 create mode 100644 t/t3420/remove-ids.sed

-- 
2.13.0

[PATCH 1/3] rebase -i: Add test for reflog message

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check that the reflog message written to the branch reflog when the
rebase is completed is correct. This checks for regressions for the
fix in commit
4ab867b8fc rebase -i: fix reflog message

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5bd0275930b715c25507fc9744c8946e7f73900b..37821d245433f757fa13f0a3e27da0312bebb7db 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -169,6 +169,13 @@ test_expect_success 'reflog for the branch shows state before rebase' '
 	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
 '
 
+test_expect_success 'reflog for the branch shows correct finish message' '
+	printf "rebase -i (finish): refs/heads/branch1 onto %s\n" \
+		"$(git rev-parse branch2)" >expected &&
+	git log -g --pretty=%gs -1 refs/heads/branch1 >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'exchange two commits' '
 	set_fake_editor &&
 	FAKE_LINES="2 1" git rebase -i HEAD~2 &&
-- 
2.13.0

[PATCH 2/3] rebase: Add tests for console output

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check the console output when using --autostash and the stash applies
cleanly is what we expect. To avoid this test depending on commit and
stash hashes it uses sed to replace them with XXX. The sed script also
replaces carriage returns in the output with '\r' to avoid embedded
'^M's in the expected output files. Unfortunately this means we still
end up with an embedded '^M' in the sed script which may not be
preserved when sending this. The last line of the sed script should be
+s/^M/\\r/g

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh          | 10 +++++++++-
 t/t3420/expected-success-am          |  6 ++++++
 t/t3420/expected-success-interactive |  4 ++++
 t/t3420/expected-success-merge       | 30 ++++++++++++++++++++++++++++++
 t/t3420/remove-ids.sed               |  6 ++++++
 5 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index ab8a63e8d6dc643b28eb0c74ba3f032b7532226f..886be63c6d13e1ac4197a1b185659fb3d7d7eb26 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -53,12 +53,20 @@ testrebase() {
 		git checkout -b rebased-feature-branch feature-branch &&
 		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >>file3 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >tmp 2>&1 &&
 		grep unrelated file4 &&
 		grep dirty file3 &&
 		git checkout feature-branch
 	'
 
+	test_expect_success "rebase$type --autostash: check output" '
+		suffix=${type#\ -} && suffix=${suffix:--am} &&
+		sed -f $TEST_DIRECTORY/t3420/remove-ids.sed tmp \
+			>actual-success$suffix &&
+		test_cmp $TEST_DIRECTORY/t3420/expected-success$suffix \
+			actual-success$suffix
+	'
+
 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
 		test_config rebase.autostash true &&
 		git reset --hard &&
diff --git a/t/t3420/expected-success-am b/t/t3420/expected-success-am
new file mode 100644
index 0000000000000000000000000000000000000000..c18ded04f703ed2aa83d5e62589a908d0a44cf7e
--- /dev/null
+++ b/t/t3420/expected-success-am
@@ -0,0 +1,6 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+First, rewinding head to replay your work on top of it...
+Applying: second commit
+Applying: third commit
+Applied autostash.
diff --git a/t/t3420/expected-success-interactive b/t/t3420/expected-success-interactive
new file mode 100644
index 0000000000000000000000000000000000000000..b31f71c95ddc9c18ce9956c1aadf53cedd966801
--- /dev/null
+++ b/t/t3420/expected-success-interactive
@@ -0,0 +1,4 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+Rebasing (1/2)\rRebasing (2/2)\rSuccessfully rebased and updated refs/heads/rebased-feature-branch.
+Applied autostash.
diff --git a/t/t3420/expected-success-merge b/t/t3420/expected-success-merge
new file mode 100644
index 0000000000000000000000000000000000000000..66386f7cb5242a255d9cc64aad741e651ec7ec1e
--- /dev/null
+++ b/t/t3420/expected-success-merge
@@ -0,0 +1,30 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+First, rewinding head to replay your work on top of it...
+Merging unrelated-onto-branch with HEAD~1
+Merging:
+XXX unrelated commit
+XXX second commit
+found 1 common ancestor:
+XXX initial commit
+[detached HEAD XXX] second commit
+ Author: A U Thor <author@example.com>
+ Date: Thu Apr 7 15:14:13 2005 -0700
+ 2 files changed, 2 insertions(+)
+ create mode 100644 file1
+ create mode 100644 file2
+Committed: 0001 second commit
+Merging unrelated-onto-branch with HEAD~0
+Merging:
+XXX second commit
+XXX third commit
+found 1 common ancestor:
+XXX second commit
+[detached HEAD XXX] third commit
+ Author: A U Thor <author@example.com>
+ Date: Thu Apr 7 15:15:13 2005 -0700
+ 1 file changed, 1 insertion(+)
+ create mode 100644 file3
+Committed: 0002 third commit
+All done.
+Applied autostash.
diff --git a/t/t3420/remove-ids.sed b/t/t3420/remove-ids.sed
new file mode 100644
index 0000000000000000000000000000000000000000..9e9048b02bd04d287461543d85db0bb715b89f8c
--- /dev/null
+++ b/t/t3420/remove-ids.sed
@@ -0,0 +1,6 @@
+s/^\(Created autostash: \)[0-9a-f]\{6,\}$/\1XXX/
+s/^\(HEAD is now at \)[0-9a-f]\{6,\}\( .* commit\)$/\1XXX\2/
+s/^[0-9a-f]\{6,\}\( .* commit\)$/XXX\1/
+s/\(detached HEAD \)[0-9a-f]\{6,\}/\1XXX/
+s/\(could not apply \)[0-9a-f]\{6,\}/\1XXX/g
+s/\r/\\r/g
-- 
2.13.0

[PATCH 3/3] rebase: Add tests for console output with conflicting stash

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check that the console output is what we expect when restoring
autostashed changes fails.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh       | 10 +++++++++-
 t/t3420/expected-fail-am          |  8 ++++++++
 t/t3420/expected-fail-interactive |  6 ++++++
 t/t3420/expected-fail-merge       | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 886be63c6d13e1ac4197a1b185659fb3d7d7eb26..4ff0d7aebd66b0ce091ceb11986b3928503c8c5f 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -148,7 +148,7 @@ testrebase() {
 		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >file4 &&
 		git add file4 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >tmp 2>&1 &&
 		test_path_is_missing $dotest &&
 		git reset --hard &&
 		grep unrelated file4 &&
@@ -157,6 +157,14 @@ testrebase() {
 		git stash pop &&
 		grep dirty file4
 	'
+
+	test_expect_success "rebase$type: check output with conflicting stash" '
+		suffix=${type#\ -} && suffix=${suffix:--am} &&
+		sed -f $TEST_DIRECTORY/t3420/remove-ids.sed tmp \
+			>actual-fail$suffix &&
+		test_cmp $TEST_DIRECTORY/t3420/expected-fail$suffix \
+			actual-fail$suffix
+	'
 }
 
 test_expect_success "rebase: fast-forward rebase" '
diff --git a/t/t3420/expected-fail-am b/t/t3420/expected-fail-am
new file mode 100644
index 0000000000000000000000000000000000000000..1d643fa2ef7e67f1ad1a49a02e81d2d6c66e6920
--- /dev/null
+++ b/t/t3420/expected-fail-am
@@ -0,0 +1,8 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+First, rewinding head to replay your work on top of it...
+Applying: second commit
+Applying: third commit
+Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
diff --git a/t/t3420/expected-fail-interactive b/t/t3420/expected-fail-interactive
new file mode 100644
index 0000000000000000000000000000000000000000..9d1dd5264237e8f58e1960c6c0fb5b736df6f86d
--- /dev/null
+++ b/t/t3420/expected-fail-interactive
@@ -0,0 +1,6 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+Rebasing (1/2)\rRebasing (2/2)\rSuccessfully rebased and updated refs/heads/rebased-feature-branch.
+Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
diff --git a/t/t3420/expected-fail-merge b/t/t3420/expected-fail-merge
new file mode 100644
index 0000000000000000000000000000000000000000..5dcb2ff3acdb93f55131d67ae227d231e097a2a2
--- /dev/null
+++ b/t/t3420/expected-fail-merge
@@ -0,0 +1,32 @@
+Created autostash: XXX
+HEAD is now at XXX third commit
+First, rewinding head to replay your work on top of it...
+Merging unrelated-onto-branch with HEAD~1
+Merging:
+XXX unrelated commit
+XXX second commit
+found 1 common ancestor:
+XXX initial commit
+[detached HEAD XXX] second commit
+ Author: A U Thor <author@example.com>
+ Date: Thu Apr 7 15:14:13 2005 -0700
+ 2 files changed, 2 insertions(+)
+ create mode 100644 file1
+ create mode 100644 file2
+Committed: 0001 second commit
+Merging unrelated-onto-branch with HEAD~0
+Merging:
+XXX second commit
+XXX third commit
+found 1 common ancestor:
+XXX second commit
+[detached HEAD XXX] third commit
+ Author: A U Thor <author@example.com>
+ Date: Thu Apr 7 15:15:13 2005 -0700
+ 1 file changed, 1 insertion(+)
+ create mode 100644 file3
+Committed: 0002 third commit
+All done.
+Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
-- 
2.13.0

Re: [PATCH 2/3] rebase: Add tests for console output

[Phillip Wood]

On 31/05/17 11:42, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Check the console output when using --autostash and the stash applies
> cleanly is what we expect. To avoid this test depending on commit and
> stash hashes it uses sed to replace them with XXX. The sed script also
> replaces carriage returns in the output with '\r' to avoid embedded
> '^M's in the expected output files. Unfortunately this means we still
> end up with an embedded '^M' in the sed script which may not be
> preserved when sending this. The last line of the sed script should be
> +s/^M/\\r/g

Thinking about this it might be better to create the sed script with 
printf when running the test

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>   t/t3420-rebase-autostash.sh          | 10 +++++++++-
>   t/t3420/expected-success-am          |  6 ++++++
>   t/t3420/expected-success-interactive |  4 ++++
>   t/t3420/expected-success-merge       | 30 ++++++++++++++++++++++++++++++
>   t/t3420/remove-ids.sed               |  6 ++++++
>   5 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index ab8a63e8d6dc643b28eb0c74ba3f032b7532226f..886be63c6d13e1ac4197a1b185659fb3d7d7eb26 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -53,12 +53,20 @@ testrebase() {
>   		git checkout -b rebased-feature-branch feature-branch &&
>   		test_when_finished git branch -D rebased-feature-branch &&
>   		echo dirty >>file3 &&
> -		git rebase$type unrelated-onto-branch &&
> +		git rebase$type unrelated-onto-branch >tmp 2>&1 &&
>   		grep unrelated file4 &&
>   		grep dirty file3 &&
>   		git checkout feature-branch
>   	'
>   
> +	test_expect_success "rebase$type --autostash: check output" '
> +		suffix=${type#\ -} && suffix=${suffix:--am} &&
> +		sed -f $TEST_DIRECTORY/t3420/remove-ids.sed tmp \
> +			>actual-success$suffix &&
> +		test_cmp $TEST_DIRECTORY/t3420/expected-success$suffix \
> +			actual-success$suffix
> +	'
> +
>   	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
>   		test_config rebase.autostash true &&
>   		git reset --hard &&
> diff --git a/t/t3420/expected-success-am b/t/t3420/expected-success-am
> new file mode 100644
> index 0000000000000000000000000000000000000000..c18ded04f703ed2aa83d5e62589a908d0a44cf7e
> --- /dev/null
> +++ b/t/t3420/expected-success-am
> @@ -0,0 +1,6 @@
> +Created autostash: XXX
> +HEAD is now at XXX third commit
> +First, rewinding head to replay your work on top of it...
> +Applying: second commit
> +Applying: third commit
> +Applied autostash.
> diff --git a/t/t3420/expected-success-interactive b/t/t3420/expected-success-interactive
> new file mode 100644
> index 0000000000000000000000000000000000000000..b31f71c95ddc9c18ce9956c1aadf53cedd966801
> --- /dev/null
> +++ b/t/t3420/expected-success-interactive
> @@ -0,0 +1,4 @@
> +Created autostash: XXX
> +HEAD is now at XXX third commit
> +Rebasing (1/2)\rRebasing (2/2)\rSuccessfully rebased and updated refs/heads/rebased-feature-branch.
> +Applied autostash.
> diff --git a/t/t3420/expected-success-merge b/t/t3420/expected-success-merge
> new file mode 100644
> index 0000000000000000000000000000000000000000..66386f7cb5242a255d9cc64aad741e651ec7ec1e
> --- /dev/null
> +++ b/t/t3420/expected-success-merge
> @@ -0,0 +1,30 @@
> +Created autostash: XXX
> +HEAD is now at XXX third commit
> +First, rewinding head to replay your work on top of it...
> +Merging unrelated-onto-branch with HEAD~1
> +Merging:
> +XXX unrelated commit
> +XXX second commit
> +found 1 common ancestor:
> +XXX initial commit
> +[detached HEAD XXX] second commit
> + Author: A U Thor <author@example.com>
> + Date: Thu Apr 7 15:14:13 2005 -0700
> + 2 files changed, 2 insertions(+)
> + create mode 100644 file1
> + create mode 100644 file2
> +Committed: 0001 second commit
> +Merging unrelated-onto-branch with HEAD~0
> +Merging:
> +XXX second commit
> +XXX third commit
> +found 1 common ancestor:
> +XXX second commit
> +[detached HEAD XXX] third commit
> + Author: A U Thor <author@example.com>
> + Date: Thu Apr 7 15:15:13 2005 -0700
> + 1 file changed, 1 insertion(+)
> + create mode 100644 file3
> +Committed: 0002 third commit
> +All done.
> +Applied autostash.
> diff --git a/t/t3420/remove-ids.sed b/t/t3420/remove-ids.sed
> new file mode 100644
> index 0000000000000000000000000000000000000000..9e9048b02bd04d287461543d85db0bb715b89f8c
> --- /dev/null
> +++ b/t/t3420/remove-ids.sed
> @@ -0,0 +1,6 @@
> +s/^\(Created autostash: \)[0-9a-f]\{6,\}$/\1XXX/
> +s/^\(HEAD is now at \)[0-9a-f]\{6,\}\( .* commit\)$/\1XXX\2/
> +s/^[0-9a-f]\{6,\}\( .* commit\)$/XXX\1/
> +s/\(detached HEAD \)[0-9a-f]\{6,\}/\1XXX/
> +s/\(could not apply \)[0-9a-f]\{6,\}/\1XXX/g
> +s//\\r/g
> 

Re: [PATCH 2/3] rebase: Add tests for console output

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> On 31/05/17 11:42, Phillip Wood wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Check the console output when using --autostash and the stash applies
>> cleanly is what we expect. To avoid this test depending on commit and
>> stash hashes it uses sed to replace them with XXX. The sed script also
>> replaces carriage returns in the output with '\r' to avoid embedded
>> '^M's in the expected output files. Unfortunately this means we still
>> end up with an embedded '^M' in the sed script which may not be
>> preserved when sending this.

Or such a sed script may simply be not portable.

Re: [PATCH 1/3] rebase -i: Add test for reflog message

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Check that the reflog message written to the branch reflog when the
> rebase is completed is correct. This checks for regressions for the
> fix in commit
> 4ab867b8fc rebase -i: fix reflog message
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

Good idea.  Thanks.

>  t/t3404-rebase-interactive.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 5bd0275930b715c25507fc9744c8946e7f73900b..37821d245433f757fa13f0a3e27da0312bebb7db 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -169,6 +169,13 @@ test_expect_success 'reflog for the branch shows state before rebase' '
>  	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
>  '
>  
> +test_expect_success 'reflog for the branch shows correct finish message' '
> +	printf "rebase -i (finish): refs/heads/branch1 onto %s\n" \
> +		"$(git rev-parse branch2)" >expected &&
> +	git log -g --pretty=%gs -1 refs/heads/branch1 >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'exchange two commits' '
>  	set_fake_editor &&
>  	FAKE_LINES="2 1" git rebase -i HEAD~2 &&

Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

Hi Phillip,

On Wed, 31 May 2017, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Check the console output when using --autostash and the stash applies
> cleanly is what we expect. To avoid this test depending on commit and
> stash hashes it uses sed to replace them with XXX. The sed script also
> replaces carriage returns in the output with '\r' to avoid embedded
> '^M's in the expected output files. Unfortunately this means we still
> end up with an embedded '^M' in the sed script which may not be
> preserved when sending this. The last line of the sed script should be
> +s/^M/\\r/g

Like Junio pointed out, this sed script would not be portable. To
elaborate: there are two major variants of sed out there, GNU sed and BSD
sed. Typically GNU sed allows a little more powerful instructions, e.g. \t
and \r.

But we should simply take a step back and ask why test_cmp is not enough
to ignore the CRs in the output?

Also, about the commit IDs. As long as the tests are consistent (i.e. they
use test_commit rather than plain `git commit`, or at least call
`test_tick` beforehand), the commit IDs should actually be identical
between runs and not depend on the time of day or the date.

The only exception to that rule is when some previous test cases call
`test_commit` but are guarded behind some prereq and may not be executed
at all. In that case, the precise commit IDs depend on the particular set
of active prereqs.

But as far as I can tell, t3420 does not have any test cases guarded by
prereqs.

Taking an additional step back, I wonder whether we have to hard-code the
commit IDs (or XXX) to begin with. Why not generate the `expect` files
with the information at hand? We can simply ask `git rev-parse --short`.

For the stash's commit ID, there is no record in the ref space, of course
(because it was created with `git stash create`). But I think in this
case, it is legitimate to simply grep the output.

That way, the test would be precise and resilient.

So for example instead of adding the t/t3420/expected-success-am verbatim,
you could generate the output via

	cat >expect <<-EOF
	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\\$" output)
	HEAD is now at $(git rev-parse --short HEAD~2) third commit
	First, rewinding head to replay your work on top of it...
	Applying: second commit
	Applying: third commit
	Applied autostash.
	EOF

Ciao,
Johannes

Re: [PATCH 2/3] rebase: Add tests for console output

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Also, about the commit IDs. As long as the tests are consistent (i.e. they
> use test_commit rather than plain `git commit`, or at least call
> `test_tick` beforehand), the commit IDs should actually be identical
> between runs and not depend on the time of day or the date.
>
> The only exception to that rule is when some previous test cases call
> `test_commit` but are guarded behind some prereq and may not be executed
> at all. In that case, the precise commit IDs depend on the particular set
> of active prereqs.

Good observation.  The tests written such a way may make later
introduction of new hash function troublesome, though (we already
have tons of them, and it is already a hassle just imagining that we
will have to migrate them X-<).  

And what you gave below is an excellent suggestion to even solve
that future headaches.

> But as far as I can tell, t3420 does not have any test cases guarded by
> prereqs.
>
> Taking an additional step back, I wonder whether we have to hard-code the
> commit IDs (or XXX) to begin with. Why not generate the `expect` files
> with the information at hand? We can simply ask `git rev-parse --short`.
>
> For the stash's commit ID, there is no record in the ref space, of course
> (because it was created with `git stash create`). But I think in this
> case, it is legitimate to simply grep the output.
>
> That way, the test would be precise and resilient.
>
> So for example instead of adding the t/t3420/expected-success-am verbatim,
> you could generate the output via
>
> 	cat >expect <<-EOF
> 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\\$" output)
> 	HEAD is now at $(git rev-parse --short HEAD~2) third commit
> 	First, rewinding head to replay your work on top of it...
> 	Applying: second commit
> 	Applying: third commit
> 	Applied autostash.
> 	EOF
>
> Ciao,
> Johannes

Re: [PATCH 2/3] rebase: Add tests for console output

[Stefan Beller]

On Thu, Jun 1, 2017 at 4:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Also, about the commit IDs. As long as the tests are consistent (i.e. they
>> use test_commit rather than plain `git commit`, or at least call
>> `test_tick` beforehand), the commit IDs should actually be identical
>> between runs and not depend on the time of day or the date.
>>
>> The only exception to that rule is when some previous test cases call
>> `test_commit` but are guarded behind some prereq and may not be executed
>> at all. In that case, the precise commit IDs depend on the particular set
>> of active prereqs.
>
> Good observation.  The tests written such a way may make later
> introduction of new hash function troublesome, though (we already
> have tons of them, and it is already a hassle just imagining that we
> will have to migrate them X-<).
>
> And what you gave below is an excellent suggestion to even solve
> that future headaches.
>

We had a discussion off list how much of the test suite is in bad shape,
and "$ git grep ^index" points out a lot of places as well.

pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

Hi,

On Thu, 1 Jun 2017, Stefan Beller wrote:

> On Thu, Jun 1, 2017 at 4:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> Also, about the commit IDs. As long as the tests are consistent (i.e. they
> >> use test_commit rather than plain `git commit`, or at least call
> >> `test_tick` beforehand), the commit IDs should actually be identical
> >> between runs and not depend on the time of day or the date.
> >>
> >> The only exception to that rule is when some previous test cases call
> >> `test_commit` but are guarded behind some prereq and may not be executed
> >> at all. In that case, the precise commit IDs depend on the particular set
> >> of active prereqs.
> >
> > Good observation.  The tests written such a way may make later
> > introduction of new hash function troublesome, though (we already
> > have tons of them, and it is already a hassle just imagining that we
> > will have to migrate them X-<).
> >
> > And what you gave below is an excellent suggestion to even solve
> > that future headaches.
> 
> We had a discussion off list how much of the test suite is in bad shape,
> and "$ git grep ^index" points out a lot of places as well.

Maybe we should call out a specific month (or even a longer period) during
which we try to push toward that new hash function, and focus more on
those tasks (and on critical bug fixes, if any) than anything else.

I also wonder how we can attract (back) cryptographic talent to help us
avoid repeating mistakes when picking a new hash algorithm.

So far, the only undisputable expert opinion I read was from the Keccak
team, and I did not have the impression that their opinion had any impact
on the discussion. Needless to say: I think it should. Cryptography is
hard. We proved it ;-)

Ciao,
Dscho

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Jonathan Nieder]

Hi Dscho,

Johannes Schindelin wrote:
> On Thu, 1 Jun 2017, Stefan Beller wrote:

>> We had a discussion off list how much of the test suite is in bad shape,
>> and "$ git grep ^index" points out a lot of places as well.
>
> Maybe we should call out a specific month (or even a longer period) during
> which we try to push toward that new hash function, and focus more on
> those tasks (and on critical bug fixes, if any) than anything else.

Thanks for offering. ;-)

Here's a rough list of some useful tasks, in no particular order:

1. bc/object-id: This patch series continues, eliminating assumptions
   about the size of object ids by encapsulating them in a struct.
   One straightforward way to find code that still needs to be
   converted is to grep for "sha" --- often the conversion patches
   change function and variable names to refer to oid_ where they used
   to use sha1_, making the stragglers easier to spot.

2. Hard-coded object ids in tests: As Stefan hinted, many tests beyond
   t00* make assumptions about the exact values of object ids.  That's
   bad for maintainability for other reasons beyond the hash function
   transition, too.

   It should be possible to suss them out by patching git's sha1
   routine to use the ones-complement of sha1 (~sha1) instead and
   seeing which tests fail.

3. Repository format extension to use a different hash function: we
   want git to be able to work with two hash functions: sha1 and
   something else.  For interoperability and simplity, it is useful
   for a single git binary to support both hash functions.

   That means a repository needs to be able to specify what hash
   function is used for the objects in that repository.  This can be
   configured by setting '[core] repositoryformatversion=1' (to avoid
   confusing old versions of git) and
   '[extensions] experimentalNewHashFunction = true'.
   Documentation/technical/repository-version.txt has more details.

   We can start experimenting with this using e.g. the ~sha1 function
   described at (2), or the 160-bit hash of the patch author's choice
   (e.g. truncated blake2bp-256).

4. When choosing a hash function, people may argue about performance.
   It would be useful for run some benchmarks for git (running
   the test suite, t/perf tests, etc) using a variety of hash
   functions as input to such a discussion.

5. Longer hash: Even once all object id references in git use struct
   object_id (see (1)), we need to tackle other assumptions about
   object id size in git and its tests.

   It should be possible to suss them out by replacing git's sha1
   routine with a 40-byte hash: sha1 with each byte repeated (sha1+sha1)
   and seeing what fails.

6. Repository format extension for longer hash: As in (3), we could
   add a repository format extension to experiment with using the
   sha1+sha1 function.

7. Avoiding wasted memory from unused hash functions: struct object_id
   has definition 'unsigned char hash[GIT_MAX_RAWSZ]', where
   GIT_MAX_RAWSZ is the size of the largest supported hash function.
   When operating on a repository that only uses sha1, this wastes
   memory.

   Avoid that by making object identifiers variable-sized.  That is,
   something like

     struct object_id {
     	union {
	  unsigned char hash20[20];
	  unsigned char hash32[32];
	} *hash;
     }

   or

     struct object_id {
       unsigned char *hash;
     }

   The hard part is that allocation and destruction have to be
   explicit instead of happening automatically when an object_id is an
   automatic variable.

8. Implement
   http://public-inbox.org/git/20170307001709.GC26789@aiede.mtv.corp.google.com/
   :)  I'd like to send a breakdown of that too, but that probably
   should happen in a separate message.

9. We can use help from security experts in all of this.  Fuzzing,
   analysis of how we use cryptography, security review of other parts
   of the design, and information to help choose a hash function are
   all appreciated.

> I also wonder how we can attract (back) cryptographic talent to help us
> avoid repeating mistakes when picking a new hash algorithm.
>
> So far, the only undisputable expert opinion I read was from the Keccak
> team, and I did not have the impression that their opinion had any impact
> on the discussion. Needless to say: I think it should. Cryptography is
> hard. We proved it ;-)

Do you have some ideas in mind here?

How did you get the impression that their opinion had no impact?  We
have been getting feedback about the choice of hash function both on
and off list from a variety of people, some indisputably security
experts.  Sometimes the best one can do is to just listen.

For what it's worth my personal opinion is currently leaning toward
blake2bp-256 as choice of hash function, not SHA2 or SHA3.  But we
still have time to learn more and make a better decision.

Thanks and hope that helps,
Jonathan

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Jonathan Nieder]

Jonathan Nieder wrote:

> Here's a rough list of some useful tasks, in no particular order:
>
> 1. bc/object-id: This patch series continues, eliminating assumptions
>    about the size of object ids by encapsulating them in a struct.
>    One straightforward way to find code that still needs to be
>    converted is to grep for "sha" --- often the conversion patches
>    change function and variable names to refer to oid_ where they used
>    to use sha1_, making the stragglers easier to spot.

I forgot to say: please coordinate with Brian M. Carlson if working on
this one.  That makes it possible to divide up the work in a way that
doesn't lead to people patching the same files and stepping on each
other's toes.

This is one of the larger tasks and contributions to it are very
useful.  Just do coordinate.

For comparison, the other suggestions in the list should be possible
for a rogue agent to experiment with alone, with advice from the list
where they need it. :)

Jonathan

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 2, 2017 at 7:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi Dscho,
>
> Johannes Schindelin wrote:
>> On Thu, 1 Jun 2017, Stefan Beller wrote:
>
>>> We had a discussion off list how much of the test suite is in bad shape,
>>> and "$ git grep ^index" points out a lot of places as well.
>>
>> Maybe we should call out a specific month (or even a longer period) during
>> which we try to push toward that new hash function, and focus more on
>> those tasks (and on critical bug fixes, if any) than anything else.
>
> Thanks for offering. ;-)
>
> Here's a rough list of some useful tasks, in no particular order:
>
> 1. bc/object-id: This patch series continues, eliminating assumptions
>    about the size of object ids by encapsulating them in a struct.
>    One straightforward way to find code that still needs to be
>    converted is to grep for "sha" --- often the conversion patches
>    change function and variable names to refer to oid_ where they used
>    to use sha1_, making the stragglers easier to spot.
>
> 2. Hard-coded object ids in tests: As Stefan hinted, many tests beyond
>    t00* make assumptions about the exact values of object ids.  That's
>    bad for maintainability for other reasons beyond the hash function
>    transition, too.
>
>    It should be possible to suss them out by patching git's sha1
>    routine to use the ones-complement of sha1 (~sha1) instead and
>    seeing which tests fail.

I just hacked this up locally. While a lot of stuff fails, it's not
exactly an out of control garbage fire and could be churned through by
someone interested. A lot of it's just lazy sha1 hardcoding for no
good reason where we could use a tag, or e.g. test_cmp on ls-tree
output without the test really caring about the hash. Things that
really need to test the sha1 could be guarded by some new prereq.

Both of my attempts to fuzz SHA1DCInit though resulted in some
segfaults, I tried both changing "0x" to "~0x" in the ihv assignment,
and just calling SHA1DCUpdate(ctx, "x", 1) at the end of the function,
not sure why that's happening.

If someone knows offhand what I'm doing wrong here I might be
interested in going through this if I don't have to sift through the
segfaults. I.e. what's an easy hack to make all of git pretend that
"foo" hashes to "xfoo", "" to "x" etc.

> 4. When choosing a hash function, people may argue about performance.
>    It would be useful for run some benchmarks for git (running
>    the test suite, t/perf tests, etc) using a variety of hash
>    functions as input to such a discussion.

To the extent that such benchmarks matter, it seems prudent to heavily
weigh them in favor of whatever seems to be likely to be the more
common hash function going forward, since those are likely to get
faster through future hardware acceleration.

E.g. Intel announced Goldmont last year which according to one SHA-1
implementation improved from 9.5 cycles per byte to 2.7 cpb[1]. They
only have acceleration for SHA-1 and SHA-256[2]

1. https://github.com/weidai11/cryptopp/issues/139#issuecomment-264283385

2. https://en.wikipedia.org/wiki/Goldmont

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 2. Hard-coded object ids in tests: As Stefan hinted, many tests beyond
>    t00* make assumptions about the exact values of object ids.  That's
>    bad for maintainability for other reasons beyond the hash function
>    transition, too.
>
>    It should be possible to suss them out by patching git's sha1
>    routine to use the ones-complement of sha1 (~sha1) instead and
>    seeing which tests fail.

One particularly nasty one is t1512-rev-parse-disambiguation that
ensures that the abbreviation and disambiguation works correctly.
It uses a set of objects (tags, commits, trees and blobs) whose
object names all begin with number of "0"; which will of course
become useless once we change the hash function.

No matter what new hash function is chosen, we'd need a similar
test to ensure that disambiguation works correctly, so one of the
tasks for hash migration is to port (not drop) this test.

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

Hi Jonathan,

On Fri, 2 Jun 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> > On Thu, 1 Jun 2017, Stefan Beller wrote:
> 
> >> We had a discussion off list how much of the test suite is in bad shape,
> >> and "$ git grep ^index" points out a lot of places as well.
> >
> > Maybe we should call out a specific month (or even a longer period) during
> > which we try to push toward that new hash function, and focus more on
> > those tasks (and on critical bug fixes, if any) than anything else.
> 
> Thanks for offering. ;-)

Undoubtedly my lack of command of the English language is to blame for
this misunderstanding.

By no means did I try to indicate that I am ready to accept the
responsibility of working toward a new hash dumped on me.

What I wanted to suggest instead was that the current direction looks very
unfocused to me, and that I do not see anything going forward in a
coherent manner. Hence my suggestion to make it public known that a
certain time period would be dedicated (and contributions would be highly
encouraged) to work on replacing SHA-1 by something else.

But:

1) this cannot be a one-person effort, it is too large

2) it cannot even be as uncoordinated an effort as it is now, because that
leads only to bikeshedding instead of progress

3) the only person who could make that call is Junio

4) we still have the problem that there is no cryptography expert among
those who in the Git project are listened to

> How did you get the impression that their opinion had no impact? We have
> been getting feedback about the choice of hash function both on and off
> list from a variety of people, some indisputably security experts.
> Sometimes the best one can do is to just listen.

I did get the impression by talking at length to a cryptography expert who
successfully resisted any suggestions to get involved in the Git mailing
list.

There were also accounts floating around on Twitter that a certain
cryptography expert who dared to mention already back in 2005 how
dangerous it would be to hardcode SHA-1 into Git was essentially shown the
finger, and I cannot fault him for essentially saying "I told you so"
publicly.

In my mind, it would have made sense to ask well-respected cryptographers
about their opinions and then try to figure out a consensus among them (as
opposed to what I saw so far, a lot of enthusastic talk by developers with
little standing in the cryptography community, mostly revolving around
hash size and speed as opposed to security). And then try to implement
that consensus in Git. Given my recent success rate with SHA-1 related
concerns, I am unfortunately not the person who can bring that about.

But maybe you are.

Ciao,
Dscho

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Fri, 2 Jun 2017, Jonathan Nieder wrote:
>> Johannes Schindelin wrote:

>>> Maybe we should call out a specific month (or even a longer period) during
>>> which we try to push toward that new hash function, and focus more on
>>> those tasks (and on critical bug fixes, if any) than anything else.
>>
>> Thanks for offering. ;-)
>
> Undoubtedly my lack of command of the English language is to blame for
> this misunderstanding.
>
> By no means did I try to indicate that I am ready to accept the
> responsibility of working toward a new hash dumped on me.

It was a joke.  More seriously, I do appreciate your questions to get
this discussion going.

[...]
> 3) the only person who could make that call is Junio

I strongly disagree with this.

> 4) we still have the problem that there is no cryptography expert among
> those who in the Git project are listened to

*shrug* I still don't know what you are suggesting here.  Are you
saying we should find a cryptography expert to pay?  Or do you have
other specific suggestions of how to attract them?

>> How did you get the impression that their opinion had no impact? We have
>> been getting feedback about the choice of hash function both on and off
>> list from a variety of people, some indisputably security experts.
>> Sometimes the best one can do is to just listen.
>
> I did get the impression by talking at length to a cryptography expert who
> successfully resisted any suggestions to get involved in the Git mailing
> list.

I know of other potential Git contributors that have resisted getting
involved in the Git mailing list, too.  I still don't know what you
are suggesting here.  Forgive me for being dense.

> There were also accounts floating around on Twitter that a certain
> cryptography expert who dared to mention already back in 2005 how
> dangerous it would be to hardcode SHA-1 into Git was essentially shown the
> finger, and I cannot fault him for essentially saying "I told you so"
> publicly.

I think there is a concrete suggestion embedded here: when discussions
go in an unproductive direction, my usual practice has been to keep
away from them.  This means that to a casual observer there can appear
to be a consensus that doesn't really exist.  We need to do better
than that: when a prominent contributor like Linus and people newer to
the project are emphatically dismissing the security impact of using a
broken hash function, others in the project need to speak up to make
it clear that those are not the actual opinions of the project.

To put it another way: "The standard you walk past is the standard you
accept".  I have failed at this.

It is a very hard problem to solve, but it is worth solving.

> In my mind, it would have made sense to ask well-respected cryptographers
> about their opinions and then try to figure out a consensus among them (as
> opposed to what I saw so far, a lot of enthusastic talk by developers with
> little standing in the cryptography community, mostly revolving around
> hash size and speed as opposed to security). And then try to implement
> that consensus in Git. Given my recent success rate with SHA-1 related
> concerns, I am unfortunately not the person who can bring that about.
>
> But maybe you are.

I think you are being a bit dismissive of both the work done so far
and the value of your own work.

I am happy to solicit more input from security researchers, though,
and your suggestion to do so is good advice.

Thanks and hope that helps,
Jonathan

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Stefan Beller]

On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> Thanks for offering. ;-)
>
> Undoubtedly my lack of command of the English language is to blame for
> this misunderstanding.

Sometimes it is best to not be a native speaker, just fluent enough to
get by. :)

> What I wanted to suggest instead was that the current direction looks very
> unfocused to me

That is unfortunate but reality of being a *real* community project.
Neither you nor me (nor Junio) can command people to do things.
The best we can do is reject an idea going off.

>, and that I do not see anything going forward in a
> coherent manner.

But is this bad?

> 1) this cannot be a one-person effort, it is too large

I agree. But there are efforts by multiple people.
See Brians series (lots of different reviewers), also Brandon picked
up parts of it (origin/bw/object-id). Or the design that was discussed
on list, which was lots of people participation.

>
> 2) it cannot even be as uncoordinated an effort as it is now, because that
> leads only to bikeshedding instead of progress

Jonathan presented a list of things, that can be done in parallel in an
uncoordinated effort, because that is how the project works.
(C.f. he mentioned "rogue agents")

> 3) the only person who could make that call is Junio

Occasionally I think the same, but in fact it is not true. As said above,
Junio has strong veto power for things going off rails, but in his role
as a maintainer he does not coordinate people. (He occasionally asks
them to coordinate between themselves, though)

>
> 4) we still have the problem that there is no cryptography expert among
> those who in the Git project are listened to

I can assure you that Jonathan listened to crypto experts. It just did not
happen on the mailing list, which is sad regarding openness and transparency.


5. The timeline you seem to favor would be really great for people working
on Git at $BIG_CORP, as big corps usually plan things by the quarter. So maybe
by having a timeline (known in advance of the quarter) can convince managers
easier.

>
>> How did you get the impression that their opinion had no impact? We have
>> been getting feedback about the choice of hash function both on and off
>> list from a variety of people, some indisputably security experts.
>> Sometimes the best one can do is to just listen.
>
> I did get the impression by talking at length to a cryptography expert who
> successfully resisted any suggestions to get involved in the Git mailing
> list.
>
> There were also accounts floating around on Twitter that a certain
> cryptography expert who dared to mention already back in 2005 how
> dangerous it would be to hardcode SHA-1 into Git was essentially shown the
> finger, and I cannot fault him for essentially saying "I told you so"
> publicly.

Heh. The community between 2005 and now has changed. (I was not there
for example. ;-) ) So let's hope the community changes for the better.

> In my mind, it would have made sense to ask well-respected cryptographers
> about their opinions and then try to figure out a consensus among them (as
> opposed to what I saw so far, a lot of enthusastic talk by developers with
> little standing in the cryptography community, mostly revolving around
> hash size and speed as opposed to security). And then try to implement
> that consensus in Git.

Sounds good to me. That is why I personally think point (4) from
Jonathans list above over-emphasizes performance/size over security.

On the other hand if we find a smart way now, then this hash function
transition will open the road to switching the hash function down the road
once again with less or even no penalty if we make mistakes in choosing
yet another bad hash function now.

> Given my recent success rate with SHA-1 related
> concerns, I am unfortunately not the person who can bring that about.
>
> But maybe you are.
>
> Ciao,
> Dscho

Thanks for bringing the discussion back to life,
Stefan

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Jonathan Nieder]

Stefan Beller wrote:
> On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:

>> In my mind, it would have made sense to ask well-respected cryptographers
>> about their opinions and then try to figure out a consensus among them (as
>> opposed to what I saw so far, a lot of enthusastic talk by developers with
>> little standing in the cryptography community, mostly revolving around
>> hash size and speed as opposed to security). And then try to implement
>> that consensus in Git.
>
> Sounds good to me. That is why I personally think point (4) from
> Jonathans list above over-emphasizes performance/size over security.

The very least the only kind of replies my example task (4) led to
were of this kind, so you can get a clear sense of whether the
community values performance over security. :)

I happen to think that performance and security both matter and are
related (since if performance regresses enough, then people end up
using the faster but insecure thing).  This has shown up in the
history of SSL, for example.  But I am very happy to see people
focusing more on the security properties than the performance
properties --- that is a correct prioritization.

Jonathan

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Samuel Lijin]

On Tue, Jun 6, 2017 at 6:45 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>
>> 4) we still have the problem that there is no cryptography expert among
>> those who in the Git project are listened to
>
> I can assure you that Jonathan listened to crypto experts. It just did not
> happen on the mailing list, which is sad regarding openness and transparency.

In the interest of openness and transparency, perhaps a blue doc
should be put together to outline and document the hash function that
succeeds SHA1, and the rationales for doing so? It would, ideally,
cite (preferably by including, and not just linking to) any
discussions with crypto experts that have chimed in off-list (given
said experts' consent for any such communication to be publicized,
naturally).

If I'm not mistaken, the only such doc behind the transition right now
is the Git hash function transition document, which covers the
technical barriers to replacing SHA1, but not why we might choose X to
replace SHA1.

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> 3) the only person who could make that call is Junio
>
> I strongly disagree with this.

If it helps, I _can_ make any set of declarations to make it sound
more official, e.g. (the remainder of) June is the "make sure our
tests are ready" month where we try to eradiate uchar[20] by
advancing the object-id effort, replace EMPTY_TREE_SHA1_HEX and
friends with EMPTY_TREE_OID_HEX, add a build-time configuration that
allows us to build a Git binary that uses a phony hash e.g. ~sha1
truncated to 16 bytes, and build Git with such a configuration and
then run tests, and we concentrate on this effort without adding new
things until we make the tests pass.

And make similar declarations for the remainder of the year.

But the actual input for formulating what each step does and the
timeline we aim for needs to come from the list wisdom; it won't
have much impact if the project's official declaration is not
followed-thru, and a unilateral declaration that is pulled out of
thin-air likely will not be.

> I am happy to solicit more input from security researchers, though,
> and your suggestion to do so is good advice.

One good thing is that we can prepare the framework to adopt a new
hash before knowing what the exact hash function is; crypto-minded
folks can work on the hash selection in parallel without waiting for
the framework to become ready.  And I do agree with Dscho that
having crypto experts would be very helpful for the latter.

[PATCH] t4005: modernize style and drop hard coded sha1

[Stefan Beller]

Use modern style in the test t4005. Remove hard coded sha1 values.
Combine test prep work and the actual test. Rename the first
test to contain the word "setup".

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Junio wrote:
> If it helps, I _can_ make any set of declarations to make it sound
> more official, e.g. (the remainder of) June is the "make sure our
> tests are ready" 

If it helps, I can write code for that. :)

Do get a good grasp on which tests need to be fixed, I changed the seed
value for the sha1 computation and then run the test suite. There are a lot
of tests passing for this, but also quite a few failing. Then I picked t4005
randomly to start with. This patch works even with a crippled hash function
as we use hash-object to get the object id.

Thanks,
Stefan

 t/t4005-diff-rename-2.sh | 95 ++++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 52 deletions(-)

diff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh
index 135addbfbd..f542d2929d 100755
--- a/t/t4005-diff-rename-2.sh
+++ b/t/t4005-diff-rename-2.sh
@@ -3,84 +3,75 @@
 # Copyright (c) 2005 Junio C Hamano
 #
 
-test_description='Same rename detection as t4003 but testing diff-raw.
+test_description='Same rename detection as t4003 but testing diff-raw.'
 
-'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh ;# test-lib chdir's into trash
 
-test_expect_success \
-    'prepare reference tree' \
-    'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
-     echo frotz >rezrov &&
-    git update-index --add COPYING rezrov &&
-    tree=$(git write-tree) &&
-    echo $tree'
-
-test_expect_success \
-    'prepare work tree' \
-    'sed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&
-    sed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&
-    rm -f COPYING &&
-    git update-index --add --remove COPYING COPYING.?'
+test_expect_success 'setup reference tree' '
+	cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
+	echo frotz >rezrov &&
+	git update-index --add COPYING rezrov &&
+	tree=$(git write-tree) &&
+	echo $tree &&
+	sed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&
+	sed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&
+	origoid=$(git hash-object COPYING) &&
+	oid1=$(git hash-object COPYING.1) &&
+	oid2=$(git hash-object COPYING.2)
+'
 
+################################################################
 # tree has COPYING and rezrov.  work tree has COPYING.1 and COPYING.2,
 # both are slightly edited, and unchanged rezrov.  We say COPYING.1
 # and COPYING.2 are based on COPYING, and do not say anything about
 # rezrov.
 
-git diff-index -C $tree >current
-
-cat >expected <<\EOF
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 06c67961bbaed34a127f76d261f4c0bf73eda471 R1234	COPYING	COPYING.2
-EOF
+test_expect_success 'validate output from rename/copy detection (#1)' '
+	rm -f COPYING &&
+	git update-index --add --remove COPYING COPYING.? &&
 
-test_expect_success \
-    'validate output from rename/copy detection (#1)' \
-    'compare_diff_raw current expected'
+	cat <<-EOF >expected &&
+	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
+	:100644 100644 $origoid $oid2 R1234	COPYING	COPYING.2
+	EOF
+	git diff-index -C $tree >current &&
+	compare_diff_raw expected current
+'
 
 ################################################################
-
-test_expect_success \
-    'prepare work tree again' \
-    'mv COPYING.2 COPYING &&
-     git update-index --add --remove COPYING COPYING.1 COPYING.2'
-
 # tree has COPYING and rezrov.  work tree has COPYING and COPYING.1,
 # both are slightly edited, and unchanged rezrov.  We say COPYING.1
 # is based on COPYING and COPYING is still there, and do not say anything
 # about rezrov.
 
-git diff-index -C $tree >current
-cat >expected <<\EOF
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 06c67961bbaed34a127f76d261f4c0bf73eda471 M	COPYING
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
-EOF
+test_expect_success 'validate output from rename/copy detection (#2)' '
+	mv COPYING.2 COPYING &&
+	git update-index --add --remove COPYING COPYING.1 COPYING.2 &&
 
-test_expect_success \
-    'validate output from rename/copy detection (#2)' \
-    'compare_diff_raw current expected'
+	cat <<-EOF >expected &&
+	:100644 100644 $origoid $oid2 M	COPYING
+	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
+	EOF
+	git diff-index -C $tree >current &&
+	compare_diff_raw current expected
+'
 
 ################################################################
-
 # tree has COPYING and rezrov.  work tree has the same COPYING and
 # copy-edited COPYING.1, and unchanged rezrov.  We should not say
 # anything about rezrov or COPYING, since the revised again diff-raw
 # nows how to say Copy.
 
-test_expect_success \
-    'prepare work tree once again' \
-    'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
-     git update-index --add --remove COPYING COPYING.1'
-
-git diff-index -C --find-copies-harder $tree >current
-cat >expected <<\EOF
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
-EOF
+test_expect_success 'validate output from rename/copy detection (#3)' '
+	cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
+	git update-index --add --remove COPYING COPYING.1 &&
 
-test_expect_success \
-    'validate output from rename/copy detection (#3)' \
-    'compare_diff_raw current expected'
+	cat <<-EOF >expected &&
+	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
+	EOF
+	git diff-index -C --find-copies-harder $tree >current &&
+	compare_diff_raw current expected
+'
 
 test_done
-- 
2.13.0.17.gf3d7728391

Re: [PATCH 2/3] rebase: Add tests for console output

[Phillip Wood]

Hi Johannes

Thanks for your feedback
On 01/06/17 13:56, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 31 May 2017, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Check the console output when using --autostash and the stash applies
>> cleanly is what we expect. To avoid this test depending on commit and
>> stash hashes it uses sed to replace them with XXX. The sed script also
>> replaces carriage returns in the output with '\r' to avoid embedded
>> '^M's in the expected output files. Unfortunately this means we still
>> end up with an embedded '^M' in the sed script which may not be
>> preserved when sending this. The last line of the sed script should be
>> +s/^M/\\r/g
> 
> Like Junio pointed out, this sed script would not be portable. To
> elaborate: there are two major variants of sed out there, GNU sed and BSD
> sed. Typically GNU sed allows a little more powerful instructions, e.g. \t
> and \r.

I'm confused by this as my script does not use the escape sequence "\r"
out of portability concerns. It has a literal carriage return as you get
from typing Ctrl-Q Ctrl-M in emacs or Ctrl-V Ctrl-M in bash which I
think should be portable and replaces the carriage returns in git's
output with the literal string '\r'. I did this so that the expected
files don't have control characters in them that mess up the display
when you cat them or read them in an email client

> But we should simply take a step back and ask why test_cmp is not enough
> to ignore the CRs in the output?
> 
> Also, about the commit IDs. As long as the tests are consistent (i.e. they
> use test_commit rather than plain `git commit`, or at least call
> `test_tick` beforehand), the commit IDs should actually be identical
> between runs and not depend on the time of day or the date.
> 
> The only exception to that rule is when some previous test cases call
> `test_commit` but are guarded behind some prereq and may not be executed
> at all. In that case, the precise commit IDs depend on the particular set
> of active prereqs.

The other exceptions are when the commit hash algorithm changes or the
contents of the commits changes because of some update to the test
script. That's why I didn't want to rely on matching fixed SHA1s

> 
> But as far as I can tell, t3420 does not have any test cases guarded by
> prereqs.
> 
> Taking an additional step back, I wonder whether we have to hard-code the
> commit IDs (or XXX) to begin with. Why not generate the `expect` files
> with the information at hand? We can simply ask `git rev-parse --short`.
> 
> For the stash's commit ID, there is no record in the ref space, of course
> (because it was created with `git stash create`). But I think in this
> case, it is legitimate to simply grep the output.

That's a good approach to handling the stash hash if we want to generate
the expected files from the test script

> That way, the test would be precise and resilient.
> 
> So for example instead of adding the t/t3420/expected-success-am verbatim,
> you could generate the output via
> 
> 	cat >expect <<-EOF
> 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\\$" output)
> 	HEAD is now at $(git rev-parse --short HEAD~2) third commit
> 	First, rewinding head to replay your work on top of it...
> 	Applying: second commit
> 	Applying: third commit
> 	Applied autostash.
> 	EOF

This approach works well for the cases where there aren't control
characters embedded in git's output, but for the interactive tests there
are so we'd end up with control characters in the test script which I
wanted to avoid or doing $(printf '\r'). I steered clear of generating
the expected file in the test as i) it was more work (both for me
(rebase --merge has a few commit hashes in it's output) and when the
script is running) and ii) it's a bit messy to implement given the way
the tests are structured in a helper function that's called with a
parameter indicating the type of rebase to test.

I can go ahead with generating the expected files from the script if you
really want but I wonder if changing the test to generate the sed script
with printf as below might be a simpler way to mitigate the carriage
return problem, though it would be less strict than generating the real
hashes with rev-parse.

--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -189,6 +189,16 @@ test_expect_success "rebase: noop rebase" '
 	git checkout feature-branch
 '

+test_expect_sucess 'rebase: create sed script to sanitize output' '
+	printf "\
+s/^\(Created autostash: \)[0-9a-f]\{6,\}$/\1XXX/
+s/^\(HEAD is now at \)[0-9a-f]\{6,\}\( .* commit\)$/\1XXX\2/
+s/^[0-9a-f]\{6,\}\( .* commit\)$/XXX\1/
+s/\(detached HEAD \)[0-9a-f]\{6,\}/\1XXX/
+s/\(could not apply \)[0-9a-f]\{6,\}/\1XXX/g
+s/\\r/\\r/g" >$TEST_DIRECTORY/t4320/remove-ids.sed
+'
+

Let me know what you think,

Best Wishes

Phillip

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

Hi Stefan,

On Tue, 6 Jun 2017, Stefan Beller wrote:

> On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > 3) the only person who could make that call is Junio
> 
> Occasionally I think the same, but in fact it is not true.

Again my poor English skillz make sure I get misunderstood. So bear with
me, please, and let me try again.

The current What's cooking mails are full of stuff other than the
transition from SHA-1 to a new function. In fact, every once in a while I
see brian carlson's patch series with the remark "Needs review" while
other patch series get reviewed even by Junio.

In my mind, this sends a message.

If, hypothetically, a couple of What's cooking mails would have in their
header some language to the extent that we need to focus on transitioning
away from SHA-1, and maybe even have the promise that Junio would not
review other patch series as long as there are patches to review that
prepare the tests for the transition, that convert more 20 and 40
constants, that convert more users to object_ids (and maybe strongly
encourage to coordinate with brian so as not to trip over each others'
toes), to implement a command to convert a SHA-1 based repository to a
repository based on a different hash, to implement caching of legacy SHA-1
<=> new hash mapping, then that would send a wholly different message.

And in my mind, if anybody else than Junio sent this message, it would
sound ludicrous. For example, if I sent a mail to that extent, I would
find it ridiculous myself, in particular since I am a very unprolific
reviewer, and the promise to focus on favoring reviews of SHA-1 transition
related patches would sound very unsincere from somebody like me.

> As said above, Junio has strong veto power for things going off rails,
> but in his role as a maintainer he does not coordinate people. (He
> occasionally asks them to coordinate between themselves, though)

I never had in mind that Junio would coordinate people or distribute
tasks.

Instead, I had in mind that a certain time period could be called out as
focusing on that pretty important direction.

That would be mostly symbolic, of course. And encouraging. In a positive
way. With a direction.

> > 4) we still have the problem that there is no cryptography expert among
> > those who in the Git project are listened to
> 
> I can assure you that Jonathan listened to crypto experts. It just did
> not happen on the mailing list, which is sad regarding openness and
> transparency.

True. Same goes for me, of course. I just felt pretty uncomfortable
sharing the contents of my private conversation publicly, when I tried
very hard to convince my conversation partner to join the discussion on
this mailing list, and they refused.

The gist of it was: SHA-256 should be preferred to SHA3-256 because we
will soon have good hardware support (and performance is really, really
important when you need to work on the largest Git repository on this
planet). And if there is no consensus about that, BLAKE should be
considered over other algorithms because it has been studied pretty well.

Ciao,
Dscho
> 
> 
> 5. The timeline you seem to favor would be really great for people working
> on Git at $BIG_CORP, as big corps usually plan things by the quarter. So maybe
> by having a timeline (known in advance of the quarter) can convince managers
> easier.
> 
> >
> >> How did you get the impression that their opinion had no impact? We have
> >> been getting feedback about the choice of hash function both on and off
> >> list from a variety of people, some indisputably security experts.
> >> Sometimes the best one can do is to just listen.
> >
> > I did get the impression by talking at length to a cryptography expert who
> > successfully resisted any suggestions to get involved in the Git mailing
> > list.
> >
> > There were also accounts floating around on Twitter that a certain
> > cryptography expert who dared to mention already back in 2005 how
> > dangerous it would be to hardcode SHA-1 into Git was essentially shown the
> > finger, and I cannot fault him for essentially saying "I told you so"
> > publicly.
> 
> Heh. The community between 2005 and now has changed. (I was not there
> for example. ;-) ) So let's hope the community changes for the better.
> 
> > In my mind, it would have made sense to ask well-respected cryptographers
> > about their opinions and then try to figure out a consensus among them (as
> > opposed to what I saw so far, a lot of enthusastic talk by developers with
> > little standing in the cryptography community, mostly revolving around
> > hash size and speed as opposed to security). And then try to implement
> > that consensus in Git.
> 
> Sounds good to me. That is why I personally think point (4) from
> Jonathans list above over-emphasizes performance/size over security.
> 
> On the other hand if we find a smart way now, then this hash function
> transition will open the road to switching the hash function down the road
> once again with less or even no penalty if we make mistakes in choosing
> yet another bad hash function now.
> 
> > Given my recent success rate with SHA-1 related
> > concerns, I am unfortunately not the person who can bring that about.
> >
> > But maybe you are.
> >
> > Ciao,
> > Dscho
> 
> Thanks for bringing the discussion back to life,
> Stefan
> 

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Stefan Beller]

On Wed, Jun 7, 2017 at 7:47 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Tue, 6 Jun 2017, Stefan Beller wrote:
>
>> On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > 3) the only person who could make that call is Junio
>>
>> Occasionally I think the same, but in fact it is not true.
>
> Again my poor English skillz make sure I get misunderstood. So bear with
> me, please, and let me try again.

I don't think it is a language thing, but a matter of perspective.

>
> The current What's cooking mails are full of stuff other than the
> transition from SHA-1 to a new function.

True, but there is also

    * bw/object-id (2017-06-05) 33 commits
    ...

     Conversion from uchar[20] to struct object_id continues.

     Will merge to 'next'.


>  In fact, every once in a while I
> see brian carlson's patch series with the remark "Needs review" while
> other patch series get reviewed even by Junio.

So are you trying to impose priorities on what Junio has to review?
(It sounds like so, but maybe you are just stating an observation,
and a conclusion with an actionable item comes next)

Sometimes I disagree with what Junio does and in which order, too.
But he has more experience in how to guide a successful community,
so I respect what he does even if I would have done it differently (such
as a different order).

>
> In my mind, this sends a message.

In the simplest form the message could be understood as a call for help
to review the patches Brian sent.

And the fact that YOU are not reviewing the patches, tells me that
you have more important things to do, such as running a fork of Git.

In my perception the conversion is picking up speed. It used to be Brian
who decided to start this years ago as a one-man show, but now we
have multiple people working on it
* Brian sending out more patches, as more review happens:
https://public-inbox.org/git/5973919a-e282-a02e-9b04-d313c77e250d@google.com/
https://public-inbox.org/git/20170509221322.GA106700@google.com/
* Brandon picking up one part of the conversion series (mentioned before, see
current cooking email)
* A potential migration plan has been made
  "Git hash function transition" (https://goo.gl/gh2Mzc).
  See the note atop the document "Note: this draft is somewhat out
  of date and is being reworked (in particular to use a different hash
function).
  See public-inbox.org/git for more current discussion."
* There are not a lot of patches, which do not have written
  "SHA1 CONVERISON" all over their face. (This one has, I made it last night
  as a one off response to this thread:
https://public-inbox.org/git/20170607021805.11849-1-sbeller@google.com/)

>
> If, hypothetically, a couple of What's cooking mails would have in their
> header some language to the extent that we need to focus on transitioning
> away from SHA-1,  and maybe even have the promise that Junio would not
> review other patch series as long as there are patches to review that
> prepare the tests for the transition, that convert more 20 and 40
> constants, that convert more users to object_ids (and maybe strongly
> encourage to coordinate with brian so as not to trip over each others'
> toes), to implement a command to convert a SHA-1 based repository to a
> repository based on a different hash, to implement caching of legacy SHA-1
> <=> new hash mapping, then that would send a wholly different message.

That message sent there would be "Junio thinks the SHA1 conversion is
the most important thing now, so he does the work (the work a maintainer
does to guide the project)".

You could send the same message "Johannes thinks the SHA1 conversion
is the most important thing and do the work (Johannes being a well respected
contributor would send patches, and that would attract a lot of
reviews for sure.
-- I don't mean this snarky, please don't read any snark in this.)

> And in my mind, if anybody else than Junio sent this message, it would
> sound ludicrous.

Yes I have seen a couple of these messages (unrelated topics).
"Git should do X. Git should not do Y. k, thx bye!" and I ignored them,
because these one-offs do not convince me to invest my own time
in it to produce a reasonable ROI.

If there are patches attached, it is not ludicrous any more as the "proof
of work done" shows that the voice raised is more than just hot talk, but
actual a genuine interest in moving things towards the right direction.

Another big one: "Move Git away from global state all over the place".
If you think about all subsystems, it may even reach the order of
magnitude to the sha1 conversion, but the way the message was sent
it did not seem ludicrous to me:
https://public-inbox.org/git/20170530171217.GB2798@google.com/

Or the other big project "Protocol v2, that scales and serves large
repos well (number of refs, large binary files omitted, refs in wants
and so on)" took a different approach, and mostly discussed design
(I recall emails both from Microsoft as well as Google discussing the
design, most of them having RFC patches attached, such that it very
much looked like "proof of work done, so it is not ludicrous")

> For example, if I sent a mail to that extent, I would
> find it ridiculous myself, in particular since I am a very unprolific
> reviewer, and the promise to focus on favoring reviews of SHA-1 transition
> related patches would sound very unsincere from somebody like me.

If you were to actually follow through after such an announcement,
and in fact review the sha1 conversion patches thoroughly and in a timely
manner, I would think such a call up front would be well received.

I thought about doing that myself, but I dread my future commitment.
Specifically as my $DAY_JOBs wants me to work on submodules
instead of the sha1 conversion. ("Submodules are more important
than the SHA1 conversion [for me] ", if I were to trust the infinite wisdom
of our management process. )


>
>> As said above, Junio has strong veto power for things going off rails,
>> but in his role as a maintainer he does not coordinate people. (He
>> occasionally asks them to coordinate between themselves, though)
>
> I never had in mind that Junio would coordinate people or distribute
> tasks.

Coordinate is a strong word here. Most recent observed examples:
https://public-inbox.org/git/xmqq60gpfvqj.fsf@gitster.mtv.corp.google.com/
(My patch series conflicted with Ævars series, so we had to figure
out how to fix it)

https://public-inbox.org/git/20170602182215.GA57260@google.com/
(Aforementioned object id conversion series having merge conflicts)

Note that this is only about coordination "You should talk to person Y
so we can figure out how to make these 2 patches work well together,
not about distributing tasks, as in "You must do X".

> Instead, I had in mind that a certain time period could be called out as
> focusing on that pretty important direction.

As remarked in an earlier email, if such a thing is called out, I would very
much prefer it in the next quarter, as then I can convince my manager to
have more time following such a goal. Otherwise *I* would ignore it.
The community at large may be different and jump on it like crazy.

> That would be mostly symbolic, of course. And encouraging. In a positive
> way. With a direction.

So you want a project roadmap?

As hinted at before the best would be to lead a good example here
and show *your* roadmap such that others see how valuable of a tool
it is to have a roadmap in the open.

>> > 4) we still have the problem that there is no cryptography expert among
>> > those who in the Git project are listened to
>>
>> I can assure you that Jonathan listened to crypto experts. It just did
>> not happen on the mailing list, which is sad regarding openness and
>> transparency.
>
> True. Same goes for me, of course. I just felt pretty uncomfortable
> sharing the contents of my private conversation publicly, when I tried
> very hard to convince my conversation partner to join the discussion on
> this mailing list, and they refused.
>
> The gist of it was: SHA-256 should be preferred to SHA3-256 because we
> will soon have good hardware support (and performance is really, really
> important when you need to work on the largest Git repository on this
> planet). And if there is no consensus about that, BLAKE should be
> considered over other algorithms because it has been studied pretty well.

BLAKE is what we're currently leaning on. (we=authors of "Git hash function
transition"; Leaning in the sense: If nobody ever speaks up until all work is
done, we'd just go with that. As soon as someone comes up with any
reasonable argument either publicly or privately on why other hashes are
better, we're easily persuaded to go with that)

> Again my poor English skillz make sure I get misunderstood. So bear with
> me, please, and let me try again.

Same for me, if I misunderstood you please point out.

tl;dr: Discussions are nice, but someone has to do the actual work, too.

Thanks,
Stefan

Re: [PATCH] t4005: modernize style and drop hard coded sha1

[Brandon Williams]

On 06/06, Stefan Beller wrote:
> Use modern style in the test t4005. Remove hard coded sha1 values.
> Combine test prep work and the actual test. Rename the first
> test to contain the word "setup".
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Junio wrote:
> > If it helps, I _can_ make any set of declarations to make it sound
> > more official, e.g. (the remainder of) June is the "make sure our
> > tests are ready" 
> 
> If it helps, I can write code for that. :)
> 
> Do get a good grasp on which tests need to be fixed, I changed the seed
> value for the sha1 computation and then run the test suite. There are a lot
> of tests passing for this, but also quite a few failing. Then I picked t4005
> randomly to start with. This patch works even with a crippled hash function
> as we use hash-object to get the object id.
> 
> Thanks,
> Stefan
> 
>  t/t4005-diff-rename-2.sh | 95 ++++++++++++++++++++++--------------------------
>  1 file changed, 43 insertions(+), 52 deletions(-)
> 
> diff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh
> index 135addbfbd..f542d2929d 100755
> --- a/t/t4005-diff-rename-2.sh
> +++ b/t/t4005-diff-rename-2.sh
> @@ -3,84 +3,75 @@
>  # Copyright (c) 2005 Junio C Hamano
>  #
>  
> -test_description='Same rename detection as t4003 but testing diff-raw.
> +test_description='Same rename detection as t4003 but testing diff-raw.'
>  
> -'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/diff-lib.sh ;# test-lib chdir's into trash
>  
> -test_expect_success \
> -    'prepare reference tree' \
> -    'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
> -     echo frotz >rezrov &&
> -    git update-index --add COPYING rezrov &&
> -    tree=$(git write-tree) &&
> -    echo $tree'
> -
> -test_expect_success \
> -    'prepare work tree' \
> -    'sed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&
> -    sed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&
> -    rm -f COPYING &&
> -    git update-index --add --remove COPYING COPYING.?'
> +test_expect_success 'setup reference tree' '
> +	cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
> +	echo frotz >rezrov &&
> +	git update-index --add COPYING rezrov &&
> +	tree=$(git write-tree) &&
> +	echo $tree &&
> +	sed -e 's/HOWEVER/However/' <COPYING >COPYING.1 &&
> +	sed -e 's/GPL/G.P.L/g' <COPYING >COPYING.2 &&
> +	origoid=$(git hash-object COPYING) &&
> +	oid1=$(git hash-object COPYING.1) &&
> +	oid2=$(git hash-object COPYING.2)
> +'

This conversation looks good to me.  The only thing that made me scratch
my head a bit were the shell variables origoid, oid1, and oid2.  It was
just slightly confusing figuring out where they came from in the tests
below before I noticed they were initialized up here.

>  
> +################################################################
>  # tree has COPYING and rezrov.  work tree has COPYING.1 and COPYING.2,
>  # both are slightly edited, and unchanged rezrov.  We say COPYING.1
>  # and COPYING.2 are based on COPYING, and do not say anything about
>  # rezrov.
>  
> -git diff-index -C $tree >current
> -
> -cat >expected <<\EOF
> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 06c67961bbaed34a127f76d261f4c0bf73eda471 R1234	COPYING	COPYING.2
> -EOF
> +test_expect_success 'validate output from rename/copy detection (#1)' '
> +	rm -f COPYING &&
> +	git update-index --add --remove COPYING COPYING.? &&
>  
> -test_expect_success \
> -    'validate output from rename/copy detection (#1)' \
> -    'compare_diff_raw current expected'
> +	cat <<-EOF >expected &&
> +	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
> +	:100644 100644 $origoid $oid2 R1234	COPYING	COPYING.2
> +	EOF
> +	git diff-index -C $tree >current &&
> +	compare_diff_raw expected current
> +'
>  
>  ################################################################
> -
> -test_expect_success \
> -    'prepare work tree again' \
> -    'mv COPYING.2 COPYING &&
> -     git update-index --add --remove COPYING COPYING.1 COPYING.2'
> -
>  # tree has COPYING and rezrov.  work tree has COPYING and COPYING.1,
>  # both are slightly edited, and unchanged rezrov.  We say COPYING.1
>  # is based on COPYING and COPYING is still there, and do not say anything
>  # about rezrov.
>  
> -git diff-index -C $tree >current
> -cat >expected <<\EOF
> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 06c67961bbaed34a127f76d261f4c0bf73eda471 M	COPYING
> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
> -EOF
> +test_expect_success 'validate output from rename/copy detection (#2)' '
> +	mv COPYING.2 COPYING &&
> +	git update-index --add --remove COPYING COPYING.1 COPYING.2 &&
>  
> -test_expect_success \
> -    'validate output from rename/copy detection (#2)' \
> -    'compare_diff_raw current expected'
> +	cat <<-EOF >expected &&
> +	:100644 100644 $origoid $oid2 M	COPYING
> +	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
> +	EOF
> +	git diff-index -C $tree >current &&
> +	compare_diff_raw current expected
> +'
>  
>  ################################################################
> -
>  # tree has COPYING and rezrov.  work tree has the same COPYING and
>  # copy-edited COPYING.1, and unchanged rezrov.  We should not say
>  # anything about rezrov or COPYING, since the revised again diff-raw
>  # nows how to say Copy.
>  
> -test_expect_success \
> -    'prepare work tree once again' \
> -    'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
> -     git update-index --add --remove COPYING COPYING.1'
> -
> -git diff-index -C --find-copies-harder $tree >current
> -cat >expected <<\EOF
> -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
> -EOF
> +test_expect_success 'validate output from rename/copy detection (#3)' '
> +	cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
> +	git update-index --add --remove COPYING COPYING.1 &&
>  
> -test_expect_success \
> -    'validate output from rename/copy detection (#3)' \
> -    'compare_diff_raw current expected'
> +	cat <<-EOF >expected &&
> +	:100644 100644 $origoid $oid1 C1234	COPYING	COPYING.1
> +	EOF
> +	git diff-index -C --find-copies-harder $tree >current &&
> +	compare_diff_raw current expected
> +'
>  
>  test_done
> -- 
> 2.13.0.17.gf3d7728391
> 

-- 
Brandon Williams

Re: [PATCH 2/3] rebase: Add tests for console output

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> I'm confused by this as my script does not use the escape sequence "\r"
> out of portability concerns. It has a literal carriage return as you get
> from typing Ctrl-Q Ctrl-M in emacs or Ctrl-V Ctrl-M in bash ...

I think the worry is that some implementations of sed may be unhappy
to see these raw control characters in the sed script.

Re: [PATCH 2/3] rebase: Add tests for console output

[Phillip Wood]

On 09/06/17 17:39, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> I'm confused by this as my script does not use the escape sequence "\r"
>> out of portability concerns. It has a literal carriage return as you get
>> from typing Ctrl-Q Ctrl-M in emacs or Ctrl-V Ctrl-M in bash ...
> 
> I think the worry is that some implementations of sed may be unhappy
> to see these raw control characters in the sed script.
> 

Ah, I'd misunderstood the problem, thanks for taking the time to point
that out. I'll redo the tests as Johannes suggested

Thanks

Phillip

[PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I've revised the second two tests as Johannes suggested to drop the
sed script. The first one is unchanged.

Phillip Wood (3):
  rebase -i: Add test for reflog message
  rebase: Add regression tests for console output
  rebase: Add more regression tests for console output

 t/t3404-rebase-interactive.sh |   7 +++
 t/t3420-rebase-autostash.sh   | 138 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 141 insertions(+), 4 deletions(-)

-- 
2.13.0

[PATCH v2 1/3] rebase -i: Add test for reflog message

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check that the reflog message written to the branch reflog when the
rebase is completed is correct

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5bd0275930b715c25507fc9744c8946e7f73900b..37821d245433f757fa13f0a3e27da0312bebb7db 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -169,6 +169,13 @@ test_expect_success 'reflog for the branch shows state before rebase' '
 	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
 '
 
+test_expect_success 'reflog for the branch shows correct finish message' '
+	printf "rebase -i (finish): refs/heads/branch1 onto %s\n" \
+		"$(git rev-parse branch2)" >expected &&
+	git log -g --pretty=%gs -1 refs/heads/branch1 >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'exchange two commits' '
 	set_fake_editor &&
 	FAKE_LINES="2 1" git rebase -i HEAD~2 &&
-- 
2.13.0

[PATCH v2 2/3] rebase: Add regression tests for console output

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check the console output when using --autostash and the stash applies
cleanly is what we expect. The test is quite strict but should catch
any changes to the console output from the various rebase flavors.

Thanks-to: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh | 66 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index ab8a63e8d6dc643b28eb0c74ba3f032b7532226f..2cbc2e89cd026c370a86da35e181d77f27081c7a 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -33,6 +33,62 @@ test_expect_success setup '
 	git commit -m "related commit"
 '
 
+create_expected_success_am() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Applying: second commit
+	Applying: third commit
+	Applied autostash.
+	EOF
+}
+
+create_expected_success_interactive() {
+	cr=$'\r' &&
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	Rebasing (1/2)${cr}Rebasing (2/2)${cr}Successfully rebased and updated refs/heads/rebased-feature-branch.
+	Applied autostash.
+	EOF
+}
+
+create_expected_success_merge() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Merging unrelated-onto-branch with HEAD~1
+	Merging:
+	$(git rev-parse --short unrelated-onto-branch) unrelated commit
+	$(git rev-parse --short feature-branch^) second commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~2) initial commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:14:13 2005 -0700
+	 2 files changed, 2 insertions(+)
+	 create mode 100644 file1
+	 create mode 100644 file2
+	Committed: 0001 second commit
+	Merging unrelated-onto-branch with HEAD~0
+	Merging:
+	$(git rev-parse --short rebased-feature-branch~1) second commit
+	$(git rev-parse --short feature-branch) third commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~1) second commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:15:13 2005 -0700
+	 1 file changed, 1 insertion(+)
+	 create mode 100644 file3
+	Committed: 0002 third commit
+	All done.
+	Applied autostash.
+	EOF
+}
+
 testrebase() {
 	type=$1
 	dotest=$2
@@ -51,14 +107,20 @@ testrebase() {
 		test_config rebase.autostash true &&
 		git reset --hard &&
 		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >>file3 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >actual 2>&1 &&
 		grep unrelated file4 &&
 		grep dirty file3 &&
 		git checkout feature-branch
 	'
 
+	test_expect_success "rebase$type --autostash: check output" '
+		test_when_finished git branch -D rebased-feature-branch &&
+		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		create_expected_success_$suffix &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
 		test_config rebase.autostash true &&
 		git reset --hard &&
-- 
2.13.0

[PATCH v2 3/3] rebase: Add more regression tests for console output

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check the console output when using --autostash and the stash does not
apply is what we expect. The test is quite strict but should catch any
changes to the console output from the various rebase flavors.

Thanks-to: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh | 72 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 2cbc2e89cd026c370a86da35e181d77f27081c7a..325ec75353b200bb88ab223e4a3b87d885cf2cf2 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -89,6 +89,68 @@ create_expected_success_merge() {
 	EOF
 }
 
+create_expected_failure_am() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Applying: second commit
+	Applying: third commit
+	Applying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	EOF
+}
+
+create_expected_failure_interactive() {
+	cr=$'\r' &&
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	Rebasing (1/2)${cr}Rebasing (2/2)${cr}Successfully rebased and updated refs/heads/rebased-feature-branch.
+	Applying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	EOF
+}
+
+create_expected_failure_merge() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Merging unrelated-onto-branch with HEAD~1
+	Merging:
+	$(git rev-parse --short unrelated-onto-branch) unrelated commit
+	$(git rev-parse --short feature-branch^) second commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~2) initial commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:14:13 2005 -0700
+	 2 files changed, 2 insertions(+)
+	 create mode 100644 file1
+	 create mode 100644 file2
+	Committed: 0001 second commit
+	Merging unrelated-onto-branch with HEAD~0
+	Merging:
+	$(git rev-parse --short rebased-feature-branch~1) second commit
+	$(git rev-parse --short feature-branch) third commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~1) second commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:15:13 2005 -0700
+	 1 file changed, 1 insertion(+)
+	 create mode 100644 file3
+	Committed: 0002 third commit
+	All done.
+	Applying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	EOF
+}
+
 testrebase() {
 	type=$1
 	dotest=$2
@@ -199,10 +261,9 @@ testrebase() {
 		test_config rebase.autostash true &&
 		git reset --hard &&
 		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >file4 &&
 		git add file4 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >actual 2>&1 &&
 		test_path_is_missing $dotest &&
 		git reset --hard &&
 		grep unrelated file4 &&
@@ -211,6 +272,13 @@ testrebase() {
 		git stash pop &&
 		grep dirty file4
 	'
+
+	test_expect_success "rebase$type: check output with conflicting stash" '
+		test_when_finished git branch -D rebased-feature-branch &&
+		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		create_expected_failure_$suffix &&
+		test_cmp expected actual
+	'
 }
 
 test_expect_success "rebase: fast-forward rebase" '
-- 
2.13.0

Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

Hi Phillip,

On Wed, 7 Jun 2017, Phillip Wood wrote:

> On 01/06/17 13:56, Johannes Schindelin wrote:
> > 
> > On Wed, 31 May 2017, Phillip Wood wrote:
> > 
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>
> >> Check the console output when using --autostash and the stash applies
> >> cleanly is what we expect. To avoid this test depending on commit and
> >> stash hashes it uses sed to replace them with XXX. The sed script also
> >> replaces carriage returns in the output with '\r' to avoid embedded
> >> '^M's in the expected output files. Unfortunately this means we still
> >> end up with an embedded '^M' in the sed script which may not be
> >> preserved when sending this. The last line of the sed script should be
> >> +s/^M/\\r/g
> > 
> > Like Junio pointed out, this sed script would not be portable. To
> > elaborate: there are two major variants of sed out there, GNU sed and BSD
> > sed. Typically GNU sed allows a little more powerful instructions, e.g. \t
> > and \r.
> 
> I'm confused by this as my script does not use the escape sequence "\r"
> out of portability concerns. It has a literal carriage return as you get
> from typing Ctrl-Q Ctrl-M in emacs or Ctrl-V Ctrl-M in bash which I
> think should be portable and replaces the carriage returns in git's
> output with the literal string '\r'. I did this so that the expected
> files don't have control characters in them that mess up the display
> when you cat them or read them in an email client

Junio elaborated elsewhere what my main concern is: while a literal
backslash followed by a lower-case r may be unportable, a plain CR byte is
almost certainly unportable.

> > Taking an additional step back, I wonder whether we have to hard-code
> > the commit IDs (or XXX) to begin with. Why not generate the `expect`
> > files with the information at hand? We can simply ask `git rev-parse
> > --short`.
> > 
> > For the stash's commit ID, there is no record in the ref space, of
> > course (because it was created with `git stash create`). But I think
> > in this case, it is legitimate to simply grep the output.
> 
> That's a good approach to handling the stash hash if we want to generate
> the expected files from the test script

I would strongly favor that, especially since it would make the transition
away from SHA-1 easier rather than more difficult.

> > That way, the test would be precise and resilient.
> > 
> > So for example instead of adding the t/t3420/expected-success-am verbatim,
> > you could generate the output via
> > 
> > 	cat >expect <<-EOF
> > 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\\$" output)
> > 	HEAD is now at $(git rev-parse --short HEAD~2) third commit
> > 	First, rewinding head to replay your work on top of it...
> > 	Applying: second commit
> > 	Applying: third commit
> > 	Applied autostash.
> > 	EOF
> 
> This approach works well for the cases where there aren't control
> characters embedded in git's output, but for the interactive tests there
> are so we'd end up with control characters in the test script which I
> wanted to avoid or doing $(printf '\r'). I steered clear of generating
> the expected file in the test as i) it was more work (both for me
> (rebase --merge has a few commit hashes in it's output) and when the
> script is running) and ii) it's a bit messy to implement given the way
> the tests are structured in a helper function that's called with a
> parameter indicating the type of rebase to test.
> 
> I can go ahead with generating the expected files from the script if you
> really want but I wonder if changing the test to generate the sed script
> with printf as below might be a simpler way to mitigate the carriage
> return problem, though it would be less strict than generating the real
> hashes with rev-parse.
> 
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -189,6 +189,16 @@ test_expect_success "rebase: noop rebase" '
>  	git checkout feature-branch
>  '
> 
> +test_expect_sucess 'rebase: create sed script to sanitize output' '
> +	printf "\
> +s/^\(Created autostash: \)[0-9a-f]\{6,\}$/\1XXX/
> +s/^\(HEAD is now at \)[0-9a-f]\{6,\}\( .* commit\)$/\1XXX\2/
> +s/^[0-9a-f]\{6,\}\( .* commit\)$/XXX\1/
> +s/\(detached HEAD \)[0-9a-f]\{6,\}/\1XXX/
> +s/\(could not apply \)[0-9a-f]\{6,\}/\1XXX/g
> +s/\\r/\\r/g" >$TEST_DIRECTORY/t4320/remove-ids.sed
> +'
> +
> 
> Let me know what you think,

I'd rather generate the expected output. If there are control characters
to be embedded anywhere, we have good prior art to do that: see the
q_to_nul(), q_to_cr() and q_to_tab() functions in test-lib-functions.

If you still think that it is too daunting, please point me to a branch
(you're on GitHub, right?) and I can try my hand at a PR.

Ciao,
Dscho

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Schindelin]

Hi Phillip,

On Wed, 14 Jun 2017, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> I've revised the second two tests as Johannes suggested to drop the
> sed script. The first one is unchanged.

This iteration looks pretty good to me!

Ciao,
Johannes

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

Hi Ævar,

On Fri, 2 Jun 2017, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Jun 2, 2017 at 7:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> >
> > Johannes Schindelin wrote:
> >> On Thu, 1 Jun 2017, Stefan Beller wrote:
> >
> >>> We had a discussion off list how much of the test suite is in bad
> >>> shape, and "$ git grep ^index" points out a lot of places as well.
> >>
> >> Maybe we should call out a specific month (or even a longer period)
> >> during which we try to push toward that new hash function, and focus
> >> more on those tasks (and on critical bug fixes, if any) than anything
> >> else.
> >
> > Thanks for offering. ;-)
> >
> > Here's a rough list of some useful tasks, in no particular order:
> >
> > 1. bc/object-id: This patch series continues, eliminating assumptions
> >    about the size of object ids by encapsulating them in a struct.
> >    One straightforward way to find code that still needs to be
> >    converted is to grep for "sha" --- often the conversion patches
> >    change function and variable names to refer to oid_ where they used
> >    to use sha1_, making the stragglers easier to spot.
> >
> > 2. Hard-coded object ids in tests: As Stefan hinted, many tests beyond
> >    t00* make assumptions about the exact values of object ids.  That's
> >    bad for maintainability for other reasons beyond the hash function
> >    transition, too.
> >
> >    It should be possible to suss them out by patching git's sha1
> >    routine to use the ones-complement of sha1 (~sha1) instead and
> >    seeing which tests fail.
> 
> I just hacked this up locally.

Would you mind turning that into a patch? I figure this could be a
compile-time switch and applied to Git's `master` so that it is easier to
find those assumptions, as well as verify fixes on multiple platforms.

> > 4. When choosing a hash function, people may argue about performance.
> >    It would be useful for run some benchmarks for git (running
> >    the test suite, t/perf tests, etc) using a variety of hash
> >    functions as input to such a discussion.
> 
> To the extent that such benchmarks matter, it seems prudent to heavily
> weigh them in favor of whatever seems to be likely to be the more
> common hash function going forward, since those are likely to get
> faster through future hardware acceleration.

Exactly.

As I just mentioned elsewhere, the cryptographers I talked to expect
SHA-256 and SHA3-256 to see the most focus of all hash functions, by far.

> E.g. Intel announced Goldmont last year which according to one SHA-1
> implementation improved from 9.5 cycles per byte to 2.7 cpb[1]. They
> only have acceleration for SHA-1 and SHA-256[2]
> 
> 1. https://github.com/weidai11/cryptopp/issues/139#issuecomment-264283385
> 
> 2. https://en.wikipedia.org/wiki/Goldmont

Thanks for digging that up! Very valuable information.

Ciao,
Dscho

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> I've revised the second two tests as Johannes suggested to drop the
> sed script. The first one is unchanged.
>
> Phillip Wood (3):
>   rebase -i: Add test for reflog message
>   rebase: Add regression tests for console output
>   rebase: Add more regression tests for console output
>
>  t/t3404-rebase-interactive.sh |   7 +++
>  t/t3420-rebase-autostash.sh   | 138 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 141 insertions(+), 4 deletions(-)

Thanks (and thanks for Dscho for reading it over).

Unfortunately this breaks unless your shell is bash (I didn't have
time to look further into it), i.e. "make SHELL_PATH=/bin/dash test"

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> I've revised the second two tests as Johannes suggested to drop the
>> sed script. The first one is unchanged.
>>
>> Phillip Wood (3):
>>   rebase -i: Add test for reflog message
>>   rebase: Add regression tests for console output
>>   rebase: Add more regression tests for console output
>>
>>  t/t3404-rebase-interactive.sh |   7 +++
>>  t/t3420-rebase-autostash.sh   | 138 ++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 141 insertions(+), 4 deletions(-)
>
> Thanks (and thanks for Dscho for reading it over).
>
> Unfortunately this breaks unless your shell is bash (I didn't have
> time to look further into it), i.e. "make SHELL_PATH=/bin/dash test"

This is the bash-ism that broke it, I think.

    create_expected_success_interactive() {
            cr=$'\r' &&
            cat >expected <<-EOF

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> I've revised the second two tests as Johannes suggested to drop the
>>> sed script. The first one is unchanged.
>>>
>>> Phillip Wood (3):
>>>   rebase -i: Add test for reflog message
>>>   rebase: Add regression tests for console output
>>>   rebase: Add more regression tests for console output
>>>
>>>  t/t3404-rebase-interactive.sh |   7 +++
>>>  t/t3420-rebase-autostash.sh   | 138 ++++++++++++++++++++++++++++++++++++++++--
>>>  2 files changed, 141 insertions(+), 4 deletions(-)
>>
>> Thanks (and thanks for Dscho for reading it over).
>>
>> Unfortunately this breaks unless your shell is bash (I didn't have
>> time to look further into it), i.e. "make SHELL_PATH=/bin/dash test"
>
> This is the bash-ism that broke it, I think.
>
>     create_expected_success_interactive() {
>             cr=$'\r' &&
>             cat >expected <<-EOF

FYI, I've queued the following as a fix-up and pushed the result
out.

 t/t3420-rebase-autostash.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 325ec75353..801bce25da 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -45,7 +45,7 @@ create_expected_success_am() {
 }
 
 create_expected_success_interactive() {
-	cr=$'\r' &&
+	cr=$(echo . | tr '.' '\015') &&
 	cat >expected <<-EOF
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
 	HEAD is now at $(git rev-parse --short feature-branch) third commit
@@ -103,7 +103,7 @@ create_expected_failure_am() {
 }
 
 create_expected_failure_interactive() {
-	cr=$'\r' &&
+	cr=$(echo . | tr '.' '\015') &&
 	cat >expected <<-EOF
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
 	HEAD is now at $(git rev-parse --short feature-branch) third commit
-- 
2.13.1-591-gf514f8f1c0

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

Hi Junio,

On Thu, 15 Jun 2017, Junio C Hamano wrote:

> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 325ec75353..801bce25da 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -45,7 +45,7 @@ create_expected_success_am() {
>  }
>  
>  create_expected_success_interactive() {
> -	cr=$'\r' &&
> +	cr=$(echo . | tr '.' '\015') &&
>  	cat >expected <<-EOF
>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>  	HEAD is now at $(git rev-parse --short feature-branch) third commit

This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
would emit) is interpreted correctly as a line break on Windows, meaning
that cr is now *empty*. Not what we want.

What I did is to replace the `cat` by `q_to_cr` (we have that lovely
function, might just as well use it), replace `${cr}` by `Q` and skip the
cr variable altogether.

Additionally, there is another huge problem: the "Applied autostash." is
printed to stdout, not stderr, in line with how things were done in the
shell version of rebase -i.

While this was just a minor bug previously, now we exercise that bug, by
redirecting stderr to stdout and redirecting stdout to the file `actual`.
Nothing says that stderr should be printed to that file before stdout, but
that is exactly what the test case tries to verify.

There is only one slight problem: in my Git for Windows SDK, stdout is
printed first.

The quick fix would be to redirect stderr and stdout independently.

However, I think it is time for that bug to be fixed: autostash messages
should really go to stderr, just like the rest of them rebase messages.

I attached the patch, together with the two fixups to Phillip's patches,
and a fixup for the autostash-messages-to-stderr patch that I think should
be squashed in but I really ran out of time testing this.

Phillip, would you mind picking those changes up as you deem appropriate?

Ciao,
Dscho

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-sequencer-print-autostash-messages-to-stderr.patch, Size: 1874 bytes --]

From c5a8319f0378d93be5aea05b833bb5e23c9f0b3d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 16 Jun 2017 15:34:50 +0200
Subject: [PATCH 1/4] sequencer: print autostash messages to stderr

The rebase messages are printed to stderr traditionally. It was a bug
introduced in 587947750bd (rebase: implement --[no-]autostash and
rebase.autostash, 2013-05-12) and that bug has been faithfully copied
when reimplementing parts of the interactive rebase in the sequencer.

It is time to fix that: let's print the autostash messages to stderr
instead of stdout.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a59408a23a2..8713cc8d1d5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1923,7 +1923,7 @@ static int apply_autostash(struct replay_opts *opts)
 	argv_array_push(&child.args, "apply");
 	argv_array_push(&child.args, stash_sha1.buf);
 	if (!run_command(&child))
-		printf(_("Applied autostash.\n"));
+		fprintf(stderr, _("Applied autostash.\n"));
 	else {
 		struct child_process store = CHILD_PROCESS_INIT;
 
@@ -1937,10 +1937,11 @@ static int apply_autostash(struct replay_opts *opts)
 		if (run_command(&store))
 			ret = error(_("cannot store %s"), stash_sha1.buf);
 		else
-			printf(_("Applying autostash resulted in conflicts.\n"
-				"Your changes are safe in the stash.\n"
-				"You can run \"git stash pop\" or"
-				" \"git stash drop\" at any time.\n"));
+			fprintf(stderr,
+				_("Applying autostash resulted in conflicts.\n"
+				  "Your changes are safe in the stash.\n"
+				  "You can run \"git stash pop\" or"
+				  " \"git stash drop\" at any time.\n"));
 	}
 
 	strbuf_release(&stash_sha1);
-- 
2.12.2.windows.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0002-fixup-rebase-add-regression-tests-for-console-output.patch, Size: 1203 bytes --]

From 3425f7c9bfb62c3d2d4adaff41a664dd4ae2efa9 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 16 Jun 2017 15:39:20 +0200
Subject: [PATCH 2/4] fixup! rebase: add regression tests for console output

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3420-rebase-autostash.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 801bce25da4..64904bef069 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -45,12 +45,11 @@ create_expected_success_am() {
 }
 
 create_expected_success_interactive() {
-	cr=$(echo . | tr '.' '\015') &&
-	cat >expected <<-EOF
+	q_to_cr >expected <<-EOF
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
 	HEAD is now at $(git rev-parse --short feature-branch) third commit
-	Rebasing (1/2)${cr}Rebasing (2/2)${cr}Successfully rebased and updated refs/heads/rebased-feature-branch.
-	Applied autostash.
+	Rebasing (1/2)QRebasing (2/2)QApplied autostash.
+	Successfully rebased and updated refs/heads/rebased-feature-branch.
 	EOF
 }
 
-- 
2.12.2.windows.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Type: text/x-diff; name=0003-fixup-rebase-add-more-regression-tests-for-console-o.patch, Size: 1361 bytes --]

From e16ad989c85c55bdfcf45fe561911a699e962b44 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 16 Jun 2017 15:39:35 +0200
Subject: [PATCH 3/4] fixup! rebase: add more regression tests for console
 output

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3420-rebase-autostash.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 64904bef069..2c01ae6ad2a 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -102,14 +102,13 @@ create_expected_failure_am() {
 }
 
 create_expected_failure_interactive() {
-	cr=$(echo . | tr '.' '\015') &&
-	cat >expected <<-EOF
+	q_to_cr >expected <<-EOF
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
 	HEAD is now at $(git rev-parse --short feature-branch) third commit
-	Rebasing (1/2)${cr}Rebasing (2/2)${cr}Successfully rebased and updated refs/heads/rebased-feature-branch.
-	Applying autostash resulted in conflicts.
+	Rebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.
 	Your changes are safe in the stash.
 	You can run "git stash pop" or "git stash drop" at any time.
+	Successfully rebased and updated refs/heads/rebased-feature-branch.
 	EOF
 }
 
-- 
2.12.2.windows.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: Type: text/x-diff; name=0004-fixup-sequencer-print-autostash-messages-to-stderr.patch, Size: 1246 bytes --]

From 128c4a66ab3413b2135264f17024e1f5e9250f03 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 16 Jun 2017 15:40:46 +0200
Subject: [PATCH 4/4] fixup! sequencer: print autostash messages to stderr

This is the companion to the proposed patch, but I had no time to verify
that the test suite still passes (it may actually test for the buggy
behavior...)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index db1deed8464..2cf73b88e8e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -166,14 +166,14 @@ apply_autostash () {
 		stash_sha1=$(cat "$state_dir/autostash")
 		if git stash apply $stash_sha1 2>&1 >/dev/null
 		then
-			echo "$(gettext 'Applied autostash.')"
+			echo "$(gettext 'Applied autostash.')" >&2
 		else
 			git stash store -m "autostash" -q $stash_sha1 ||
 			die "$(eval_gettext "Cannot store \$stash_sha1")"
 			gettext 'Applying autostash resulted in conflicts.
 Your changes are safe in the stash.
 You can run "git stash pop" or "git stash drop" at any time.
-'
+' >&2
 		fi
 	fi
 }
-- 
2.12.2.windows.1

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Sixt]

Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:
> On Thu, 15 Jun 2017, Junio C Hamano wrote:
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index 325ec75353..801bce25da 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -45,7 +45,7 @@ create_expected_success_am() {
>>   }
>>   
>>   create_expected_success_interactive() {
>> -	cr=$'\r' &&
>> +	cr=$(echo . | tr '.' '\015') &&
>>   	cat >expected <<-EOF
>>   	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>>   	HEAD is now at $(git rev-parse --short feature-branch) third commit
> 
> This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
> would emit) is interpreted correctly as a line break on Windows, meaning
> that cr is now *empty*. Not what we want.
> 
> What I did is to replace the `cat` by `q_to_cr` (we have that lovely
> function, might just as well use it), replace `${cr}` by `Q` and skip the
> cr variable altogether.

You beat me to it. I came up with the identical q_to_cr changes, but 
haven't dug the remaining failure regarding the swapped output lines. 
You seem to have nailed it. Will test your proposed changes tomorrow.

-- Hannes

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:
>> On Thu, 15 Jun 2017, Junio C Hamano wrote:
>>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>>> index 325ec75353..801bce25da 100755
>>> --- a/t/t3420-rebase-autostash.sh
>>> +++ b/t/t3420-rebase-autostash.sh
>>> @@ -45,7 +45,7 @@ create_expected_success_am() {
>>>   }
>>>     create_expected_success_interactive() {
>>> -	cr=$'\r' &&
>>> +	cr=$(echo . | tr '.' '\015') &&
>>>   	cat >expected <<-EOF
>>>   	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>>>   	HEAD is now at $(git rev-parse --short feature-branch) third commit
>>
>> This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
>> would emit) is interpreted correctly as a line break on Windows, meaning
>> that cr is now *empty*. Not what we want.
>>
>> What I did is to replace the `cat` by `q_to_cr` (we have that lovely
>> function, might just as well use it), replace `${cr}` by `Q` and skip the
>> cr variable altogether.
>
> You beat me to it. I came up with the identical q_to_cr changes, but
> haven't dug the remaining failure regarding the swapped output
> lines. You seem to have nailed it. Will test your proposed changes
> tomorrow.

Ouch.  Thanks, both of you.

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Phillip Wood]

On 16/06/17 14:49, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Thu, 15 Jun 2017, Junio C Hamano wrote:
> 
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index 325ec75353..801bce25da 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -45,7 +45,7 @@ create_expected_success_am() {
>>  }
>>  
>>  create_expected_success_interactive() {
>> -	cr=$'\r' &&
>> +	cr=$(echo . | tr '.' '\015') &&
>>  	cat >expected <<-EOF
>>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>>  	HEAD is now at $(git rev-parse --short feature-branch) third commit
> 
> This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
> would emit) is interpreted correctly as a line break on Windows, meaning
> that cr is now *empty*. Not what we want.
> 
> What I did is to replace the `cat` by `q_to_cr` (we have that lovely
> function, might just as well use it), replace `${cr}` by `Q` and skip the
> cr variable altogether.
> 
> Additionally, there is another huge problem: the "Applied autostash." is
> printed to stdout, not stderr, in line with how things were done in the
> shell version of rebase -i.
> 
> While this was just a minor bug previously, now we exercise that bug, by
> redirecting stderr to stdout and redirecting stdout to the file `actual`.
> Nothing says that stderr should be printed to that file before stdout, but
> that is exactly what the test case tries to verify.
> 
> There is only one slight problem: in my Git for Windows SDK, stdout is
> printed first.
> 
> The quick fix would be to redirect stderr and stdout independently.
> 
> However, I think it is time for that bug to be fixed: autostash messages
> should really go to stderr, just like the rest of them rebase messages.
> 
> I attached the patch, together with the two fixups to Phillip's patches,
> and a fixup for the autostash-messages-to-stderr patch that I think should
> be squashed in but I really ran out of time testing this.
> 
> Phillip, would you mind picking those changes up as you deem appropriate?

Will do, thanks for the patches

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Phillip Wood]

On 16/06/17 00:29, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>>
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>
>>>> I've revised the second two tests as Johannes suggested to drop the
>>>> sed script. The first one is unchanged.
>>>>
>>>> Phillip Wood (3):
>>>>   rebase -i: Add test for reflog message
>>>>   rebase: Add regression tests for console output
>>>>   rebase: Add more regression tests for console output
>>>>
>>>>  t/t3404-rebase-interactive.sh |   7 +++
>>>>  t/t3420-rebase-autostash.sh   | 138 ++++++++++++++++++++++++++++++++++++++++--
>>>>  2 files changed, 141 insertions(+), 4 deletions(-)
>>>
>>> Thanks (and thanks for Dscho for reading it over).
>>>
>>> Unfortunately this breaks unless your shell is bash (I didn't have
>>> time to look further into it), i.e. "make SHELL_PATH=/bin/dash test"
>>
>> This is the bash-ism that broke it, I think.
>>
>>     create_expected_success_interactive() {
>>             cr=$'\r' &&
>>             cat >expected <<-EOF

Sorry about that, for some reason I thought $' was in the posix shell
standard but it's not. I'll redo the patches with the changes Johannes
suggested.

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

>> Phillip, would you mind picking those changes up as you deem appropriate?
>
> Will do, thanks for the patches

Thanks, both.

[PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I've updated the second two tests to be portable using q_to_cr() as
Johannes suggested and added his patch to fix the autostash messages
going to stdout rather than stderr. The reflog message test is
unchanged. Thanks to Johannes for his help and to Junio for picking up
the bashism in the last iteration.

Johannes Schindelin (1):
  sequencer: print autostash messages to stderr

Phillip Wood (3):
  rebase -i: Add test for reflog message
  rebase: Add regression tests for console output
  rebase: Add more regression tests for console output

 git-rebase.sh                 |   4 +-
 sequencer.c                   |  11 ++--
 t/t3404-rebase-interactive.sh |   7 +++
 t/t3420-rebase-autostash.sh   | 136 ++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 147 insertions(+), 11 deletions(-)

-- 
2.13.0

[PATCH v3 1/4] sequencer: print autostash messages to stderr

[Phillip Wood]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The rebase messages are printed to stderr traditionally. However due
to a bug introduced in 587947750bd (rebase: implement --[no-]autostash
and rebase.autostash, 2013-05-12) which was faithfully copied when
reimplementing parts of the interactive rebase in the sequencer the
autostash messages are printed to stdout instead.

It is time to fix that: let's print the autostash messages to stderr
instead of stdout.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase.sh |  4 ++--
 sequencer.c   | 11 ++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index db1deed8464f0643763ed6e3c5e54221cad8c985..2cf73b88e8e83ca34b9eb319dbc2b0a220139b0f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -166,14 +166,14 @@ apply_autostash () {
 		stash_sha1=$(cat "$state_dir/autostash")
 		if git stash apply $stash_sha1 2>&1 >/dev/null
 		then
-			echo "$(gettext 'Applied autostash.')"
+			echo "$(gettext 'Applied autostash.')" >&2
 		else
 			git stash store -m "autostash" -q $stash_sha1 ||
 			die "$(eval_gettext "Cannot store \$stash_sha1")"
 			gettext 'Applying autostash resulted in conflicts.
 Your changes are safe in the stash.
 You can run "git stash pop" or "git stash drop" at any time.
-'
+' >&2
 		fi
 	fi
 }
diff --git a/sequencer.c b/sequencer.c
index a23b948ac148304dbebfe38955ec8b40cab3e1e5..606750b1e0c900a9fb43cea224d27ab36ca29ab9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1921,7 +1921,7 @@ static int apply_autostash(struct replay_opts *opts)
 	argv_array_push(&child.args, "apply");
 	argv_array_push(&child.args, stash_sha1.buf);
 	if (!run_command(&child))
-		printf(_("Applied autostash.\n"));
+		fprintf(stderr, _("Applied autostash.\n"));
 	else {
 		struct child_process store = CHILD_PROCESS_INIT;
 
@@ -1935,10 +1935,11 @@ static int apply_autostash(struct replay_opts *opts)
 		if (run_command(&store))
 			ret = error(_("cannot store %s"), stash_sha1.buf);
 		else
-			printf(_("Applying autostash resulted in conflicts.\n"
-				"Your changes are safe in the stash.\n"
-				"You can run \"git stash pop\" or"
-				" \"git stash drop\" at any time.\n"));
+			fprintf(stderr,
+				_("Applying autostash resulted in conflicts.\n"
+				  "Your changes are safe in the stash.\n"
+				  "You can run \"git stash pop\" or"
+				  " \"git stash drop\" at any time.\n"));
 	}
 
 	strbuf_release(&stash_sha1);
-- 
2.13.0

[PATCH v3 2/4] rebase -i: Add test for reflog message

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check that the reflog message written to the branch reflog when the
rebase is completed is correct

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5bd0275930b715c25507fc9744c8946e7f73900b..37821d245433f757fa13f0a3e27da0312bebb7db 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -169,6 +169,13 @@ test_expect_success 'reflog for the branch shows state before rebase' '
 	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
 '
 
+test_expect_success 'reflog for the branch shows correct finish message' '
+	printf "rebase -i (finish): refs/heads/branch1 onto %s\n" \
+		"$(git rev-parse branch2)" >expected &&
+	git log -g --pretty=%gs -1 refs/heads/branch1 >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'exchange two commits' '
 	set_fake_editor &&
 	FAKE_LINES="2 1" git rebase -i HEAD~2 &&
-- 
2.13.0

[PATCH v3 3/4] rebase: Add regression tests for console output

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check the console output when using --autostash and the stash applies
cleanly is what we expect. The test is quite strict but should catch
any changes to the console output from the various rebase flavors.

Thanks-to: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh | 65 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index ab8a63e8d6dc643b28eb0c74ba3f032b7532226f..cd1012798cb300f4f1ddeba6fdcad544ca9ea1d9 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -33,6 +33,61 @@ test_expect_success setup '
 	git commit -m "related commit"
 '
 
+create_expected_success_am() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Applying: second commit
+	Applying: third commit
+	Applied autostash.
+	EOF
+}
+
+create_expected_success_interactive() {
+	q_to_cr >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	Rebasing (1/2)QRebasing (2/2)QApplied autostash.
+	Successfully rebased and updated refs/heads/rebased-feature-branch.
+	EOF
+}
+
+create_expected_success_merge() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Merging unrelated-onto-branch with HEAD~1
+	Merging:
+	$(git rev-parse --short unrelated-onto-branch) unrelated commit
+	$(git rev-parse --short feature-branch^) second commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~2) initial commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:14:13 2005 -0700
+	 2 files changed, 2 insertions(+)
+	 create mode 100644 file1
+	 create mode 100644 file2
+	Committed: 0001 second commit
+	Merging unrelated-onto-branch with HEAD~0
+	Merging:
+	$(git rev-parse --short rebased-feature-branch~1) second commit
+	$(git rev-parse --short feature-branch) third commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~1) second commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:15:13 2005 -0700
+	 1 file changed, 1 insertion(+)
+	 create mode 100644 file3
+	Committed: 0002 third commit
+	All done.
+	Applied autostash.
+	EOF
+}
+
 testrebase() {
 	type=$1
 	dotest=$2
@@ -51,14 +106,20 @@ testrebase() {
 		test_config rebase.autostash true &&
 		git reset --hard &&
 		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >>file3 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >actual 2>&1 &&
 		grep unrelated file4 &&
 		grep dirty file3 &&
 		git checkout feature-branch
 	'
 
+	test_expect_success "rebase$type --autostash: check output" '
+		test_when_finished git branch -D rebased-feature-branch &&
+		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		create_expected_success_$suffix &&
+		test_cmp expected actual
+	'
+
 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
 		test_config rebase.autostash true &&
 		git reset --hard &&
-- 
2.13.0

[PATCH v3 4/4] rebase: Add more regression tests for console output

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Check the console output when using --autostash and the stash does not
apply is what we expect. The test is quite strict but should catch any
changes to the console output from the various rebase flavors.

Thanks-to: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3420-rebase-autostash.sh | 71 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index cd1012798cb300f4f1ddeba6fdcad544ca9ea1d9..2c01ae6ad2a104940e388013984e7fa2a0d5aed5 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -88,6 +88,67 @@ create_expected_success_merge() {
 	EOF
 }
 
+create_expected_failure_am() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Applying: second commit
+	Applying: third commit
+	Applying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	EOF
+}
+
+create_expected_failure_interactive() {
+	q_to_cr >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	Rebasing (1/2)QRebasing (2/2)QApplying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	Successfully rebased and updated refs/heads/rebased-feature-branch.
+	EOF
+}
+
+create_expected_failure_merge() {
+	cat >expected <<-EOF
+	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
+	HEAD is now at $(git rev-parse --short feature-branch) third commit
+	First, rewinding head to replay your work on top of it...
+	Merging unrelated-onto-branch with HEAD~1
+	Merging:
+	$(git rev-parse --short unrelated-onto-branch) unrelated commit
+	$(git rev-parse --short feature-branch^) second commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~2) initial commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:14:13 2005 -0700
+	 2 files changed, 2 insertions(+)
+	 create mode 100644 file1
+	 create mode 100644 file2
+	Committed: 0001 second commit
+	Merging unrelated-onto-branch with HEAD~0
+	Merging:
+	$(git rev-parse --short rebased-feature-branch~1) second commit
+	$(git rev-parse --short feature-branch) third commit
+	found 1 common ancestor:
+	$(git rev-parse --short feature-branch~1) second commit
+	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
+	 Author: A U Thor <author@example.com>
+	 Date: Thu Apr 7 15:15:13 2005 -0700
+	 1 file changed, 1 insertion(+)
+	 create mode 100644 file3
+	Committed: 0002 third commit
+	All done.
+	Applying autostash resulted in conflicts.
+	Your changes are safe in the stash.
+	You can run "git stash pop" or "git stash drop" at any time.
+	EOF
+}
+
 testrebase() {
 	type=$1
 	dotest=$2
@@ -198,10 +259,9 @@ testrebase() {
 		test_config rebase.autostash true &&
 		git reset --hard &&
 		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >file4 &&
 		git add file4 &&
-		git rebase$type unrelated-onto-branch &&
+		git rebase$type unrelated-onto-branch >actual 2>&1 &&
 		test_path_is_missing $dotest &&
 		git reset --hard &&
 		grep unrelated file4 &&
@@ -210,6 +270,13 @@ testrebase() {
 		git stash pop &&
 		grep dirty file4
 	'
+
+	test_expect_success "rebase$type: check output with conflicting stash" '
+		test_when_finished git branch -D rebased-feature-branch &&
+		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		create_expected_failure_$suffix &&
+		test_cmp expected actual
+	'
 }
 
 test_expect_success "rebase: fast-forward rebase" '
-- 
2.13.0

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Sixt]

Am 16.06.2017 um 20:43 schrieb Johannes Sixt:
> Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:
>> On Thu, 15 Jun 2017, Junio C Hamano wrote:
>>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>>> index 325ec75353..801bce25da 100755
>>> --- a/t/t3420-rebase-autostash.sh
>>> +++ b/t/t3420-rebase-autostash.sh
>>> @@ -45,7 +45,7 @@ create_expected_success_am() {
>>>   }
>>>   create_expected_success_interactive() {
>>> -    cr=$'\r' &&
>>> +    cr=$(echo . | tr '.' '\015') &&
>>>       cat >expected <<-EOF
>>>       $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>>>       HEAD is now at $(git rev-parse --short feature-branch) third 
>>> commit
>>
>> This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
>> would emit) is interpreted correctly as a line break on Windows, meaning
>> that cr is now *empty*. Not what we want.
>>
>> What I did is to replace the `cat` by `q_to_cr` (we have that lovely
>> function, might just as well use it), replace `${cr}` by `Q` and skip the
>> cr variable altogether.
> 
> You beat me to it. I came up with the identical q_to_cr changes, but 
> haven't dug the remaining failure regarding the swapped output lines. 
> You seem to have nailed it. Will test your proposed changes tomorrow.

As expected, the patches fix the observed test failures for me, too, if 
that's still relevant.

-- Hannes

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 16.06.2017 um 20:43 schrieb Johannes Sixt:
>> Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:
>>> On Thu, 15 Jun 2017, Junio C Hamano wrote:
>>>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>>>> index 325ec75353..801bce25da 100755
>>>> --- a/t/t3420-rebase-autostash.sh
>>>> +++ b/t/t3420-rebase-autostash.sh
>>>> @@ -45,7 +45,7 @@ create_expected_success_am() {
>>>>   }
>>>>   create_expected_success_interactive() {
>>>> -    cr=$'\r' &&
>>>> +    cr=$(echo . | tr '.' '\015') &&
>>>>       cat >expected <<-EOF
>>>>       $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>>>>       HEAD is now at $(git rev-parse --short feature-branch) third
>>>> commit
>>>
>>> This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
>>> would emit) is interpreted correctly as a line break on Windows, meaning
>>> that cr is now *empty*. Not what we want.
>>>
>>> What I did is to replace the `cat` by `q_to_cr` (we have that lovely
>>> function, might just as well use it), replace `${cr}` by `Q` and skip the
>>> cr variable altogether.
>>
>> You beat me to it. I came up with the identical q_to_cr changes, but
>> haven't dug the remaining failure regarding the swapped output
>> lines. You seem to have nailed it. Will test your proposed changes
>> tomorrow.
>
> As expected, the patches fix the observed test failures for me, too,
> if that's still relevant.

Thanks for double-checking.

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> I've updated the second two tests to be portable using q_to_cr() as
> Johannes suggested and added his patch to fix the autostash messages
> going to stdout rather than stderr. The reflog message test is
> unchanged. Thanks to Johannes for his help and to Junio for picking up
> the bashism in the last iteration.
>
> Johannes Schindelin (1):
>   sequencer: print autostash messages to stderr
>
> Phillip Wood (3):
>   rebase -i: Add test for reflog message
>   rebase: Add regression tests for console output
>   rebase: Add more regression tests for console output
>
>  git-rebase.sh                 |   4 +-
>  sequencer.c                   |  11 ++--
>  t/t3404-rebase-interactive.sh |   7 +++
>  t/t3420-rebase-autostash.sh   | 136 ++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 147 insertions(+), 11 deletions(-)

I've merged this to 'next' but I probably shouldn't have before
making sure that Travis tests passes 'pu' while this was still in
there.  

At least t3420 seems to fail under GETTEXT_POISON build.

  https://travis-ci.org/git/git/jobs/245990993

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>  git-rebase.sh                 |   4 +-
>>  sequencer.c                   |  11 ++--
>>  t/t3404-rebase-interactive.sh |   7 +++
>>  t/t3420-rebase-autostash.sh   | 136 ++++++++++++++++++++++++++++++++++++++++--
>>  4 files changed, 147 insertions(+), 11 deletions(-)
>
> I've merged this to 'next' but I probably shouldn't have before
> making sure that Travis tests passes 'pu' while this was still in
> there.  
>
> At least t3420 seems to fail under GETTEXT_POISON build.
>
>   https://travis-ci.org/git/git/jobs/245990993

This should be sufficient to make t3420 pass.  It seems that t3404
is also broken under GETTEXT_POISON build, but I won't have time to
look at it, at least tonight.

    $ make GETTEXT_POISON=YesPlease
    $ cd t && sh ./t3404-*.sh -i -v

to see how it breaks.

Thanks.

 t/t3420-rebase-autostash.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 6826c38cbd..e243700660 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -178,7 +178,7 @@ testrebase () {
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
 		create_expected_success_$suffix &&
-		test_cmp expected actual
+		test_i18ncmp expected actual
 	'
 
 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
@@ -275,7 +275,7 @@ testrebase () {
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
 		create_expected_failure_$suffix &&
-		test_cmp expected actual
+		test_i18ncmp expected actual
 	'
 }
 

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Phillip Wood]

On 23/06/17 06:07, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>>>  git-rebase.sh                 |   4 +-
>>>  sequencer.c                   |  11 ++--
>>>  t/t3404-rebase-interactive.sh |   7 +++
>>>  t/t3420-rebase-autostash.sh   | 136 ++++++++++++++++++++++++++++++++++++++++--
>>>  4 files changed, 147 insertions(+), 11 deletions(-)
>>
>> I've merged this to 'next' but I probably shouldn't have before
>> making sure that Travis tests passes 'pu' while this was still in
>> there.  
>>
>> At least t3420 seems to fail under GETTEXT_POISON build.
>>
>>   https://travis-ci.org/git/git/jobs/245990993
> 
> This should be sufficient to make t3420 pass.  It seems that t3404
> is also broken under GETTEXT_POISON build, but I won't have time to
> look at it, at least tonight.
> 
>     $ make GETTEXT_POISON=YesPlease
>     $ cd t && sh ./t3404-*.sh -i -v
> 
> to see how it breaks.

t3404 passes for me,
$ make GETTEXT_POISON=YesPlease
$ cd t &&sh t3404-rebase-interactive.sh -i -v
...
# still have 1 known breakage(s)
# passed all remaining 95 test(s)
1..96

Also as far as I can see it passes on travis -
https://travis-ci.org/git/git/jobs/245990993#L910 have I missed
something? Do you want me to submit a fixup patch for t3420 or have you
got one already?

Thanks

Phillip

> Thanks.
> 
>  t/t3420-rebase-autostash.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 6826c38cbd..e243700660 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -178,7 +178,7 @@ testrebase () {
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>  		create_expected_success_$suffix &&
> -		test_cmp expected actual
> +		test_i18ncmp expected actual
>  	'
>  
>  	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
> @@ -275,7 +275,7 @@ testrebase () {
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>  		create_expected_failure_$suffix &&
> -		test_cmp expected actual
> +		test_i18ncmp expected actual
>  	'
>  }
>  
> 

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> t3404 passes for me,
> $ make GETTEXT_POISON=YesPlease
> $ cd t &&sh t3404-rebase-interactive.sh -i -v
> ...
> # still have 1 known breakage(s)
> # passed all remaining 95 test(s)
> 1..96

Interesting.  The tip of 'pu', which includes this series, does pass
for me, too, but the tip of this topic 7d70e6b9 ("rebase: add more
regression tests for console output", 2017-06-19) tested in
isolation fails, and gives the output at the end of this message.

> Do you want me to submit a fixup patch for t3420 or have you
> got one already?

For 3420, I can wrap the two-liner patch I showed here earlier into
a commit on top of the series.  

3404 needs a similar fix-up for the series to be able to stand on
its own.  Alternatively, at least we need to understand what in 'pu'
makes the result of the merge pass---the symptom indicates that this
topic cannot be merged to a released version without that unknown
other topic in 'pu' merged if we want to keep POISON build passing
the tests.

Thanks.

-- output from 3404 follows --

Initialized empty Git repository in /home/gitster/git/t/trash directory.t3404-rebase-interactive/.git/
expecting success: 
	test_commit A file1 &&
	test_commit B file1 &&
	test_commit C file2 &&
	test_commit D file1 &&
	test_commit E file3 &&
	git checkout -b branch1 A &&
	test_commit F file4 &&
	test_commit G file1 &&
	test_commit H file5 &&
	git checkout -b branch2 F &&
	test_commit I file6 &&
	git checkout -b conflict-branch A &&
	test_commit one conflict &&
	test_commit two conflict &&
	test_commit three conflict &&
	test_commit four conflict &&
	git checkout -b no-conflict-branch A &&
	test_commit J fileJ &&
	test_commit K fileK &&
	test_commit L fileL &&
	test_commit M fileM &&
	git checkout -b no-ff-branch A &&
	test_commit N fileN &&
	test_commit O fileO &&
	test_commit P fileP

[master# GETTEXT POISON # 6e62bf8] A
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file1
[master 313fe96] B
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[master d0f65f2] C
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file2
[master 0547e3f] D
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[master 8f99a4f] E
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file3
# GETTEXT POISON #[branch1 cfefd94] F
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file4
[branch1 83751a6] G
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[branch1 4373208] H
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file5
# GETTEXT POISON #[branch2 615be62] I
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file6
# GETTEXT POISON #[conflict-branch b895952] one
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 conflict
[conflict-branch 766a798] two
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[conflict-branch 1eadf03] three
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[conflict-branch f91a2b3] four
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
# GETTEXT POISON #[no-conflict-branch 808874f] J
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileJ
[no-conflict-branch 265b89e] K
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileK
[no-conflict-branch 6b0f5e6] L
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileL
[no-conflict-branch 3389558] M
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileM
# GETTEXT POISON #[no-ff-branch 53b4423] N
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileN
[no-ff-branch cc47714] O
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileO
[no-ff-branch faef1a5] P
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 fileP
ok 1 - setup

expecting success: 
	git checkout -b emptybranch master &&
	git commit --allow-empty -m "empty" &&
	git rebase --keep-empty -i HEAD~2 &&
	git log --oneline >actual &&
	test_line_count = 6 actual

# GETTEXT POISON #[emptybranch da33401] empty
 Author: A U Thor <author@example.com>
Successfully rebased and updated refs/heads/emptybranch.
ok 2 - rebase --keep-empty

expecting success: 
	git checkout master &&
	(
	set_fake_editor &&
	FAKE_LINES="1 exec_>touch-one
		2 exec_>touch-two exec_false exec_>touch-three
		3 4 exec_>\"touch-file__name_with_spaces\";_>touch-after-semicolon 5" &&
	export FAKE_LINES &&
	test_must_fail git rebase -i A
	) &&
	test_path_is_file touch-one &&
	test_path_is_file touch-two &&
	test_path_is_missing touch-three " (should have stopped before)" &&
	test_cmp_rev C HEAD &&
	git rebase --continue &&
	test_path_is_file touch-three &&
	test_path_is_file "touch-file  name with spaces" &&
	test_path_is_file touch-after-semicolon &&
	test_cmp_rev master HEAD &&
	rm -f touch-*

# GETTEXT POISON #rebase -i script before editing:
pick 313fe96 B
pick d0f65f2 C
pick 0547e3f D
pick 8f99a4f E

rebase -i script after editing:
pick 313fe96 B
exec >touch-one
pick d0f65f2 C
exec >touch-two
exec false
exec >touch-three
pick 0547e3f D
pick 8f99a4f E
exec >"touch-file  name with spaces"; >touch-after-semicolon
Rebasing (2/9)Executing: >touch-one
Rebasing (3/9)Rebasing (4/9)Executing: >touch-two
Rebasing (5/9)Executing: false
warning: # GETTEXT POISON #
Rebasing (6/9)Executing: >touch-three
Rebasing (7/9)Rebasing (8/9)Rebasing (9/9)Executing: >"touch-file  name with spaces"; >touch-after-semicolon
Successfully rebased and updated refs/heads/master.
ok 3 - rebase -i with the exec command

expecting success: 
	git checkout master &&
	mkdir subdir && (cd subdir &&
	set_fake_editor &&
	FAKE_LINES="1 exec_>touch-subdir" \
		git rebase -i HEAD^
	) &&
	test_path_is_file touch-subdir &&
	rm -fr subdir

# GETTEXT POISON #rebase -i script before editing:
pick 8f99a4f E

rebase -i script after editing:
pick 8f99a4f E
exec >touch-subdir
Rebasing (2/2)Executing: >touch-subdir
Successfully rebased and updated refs/heads/master.
ok 4 - rebase -i with the exec command runs from tree root

expecting success: 
	git checkout master &&
	set_fake_editor &&
	test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i HEAD^ &&
	test_cmp_rev master^ HEAD &&
	git reset --hard &&
	git rebase --continue

# GETTEXT POISON #rebase -i script before editing:
pick 8f99a4f E

rebase -i script after editing:
exec echo foo >file1
pick 8f99a4f E
Rebasing (1/2)Executing: echo foo >file1
error: # GETTEXT POISON #
warning: # GETTEXT POISON #
# GETTEXT POISON # D
Rebasing (2/2)Successfully rebased and updated refs/heads/master.
ok 5 - rebase -i with the exec command checks tree cleanness

expecting success: 
	git checkout master &&
	test_when_finished "git rebase --abort" &&
	set_fake_editor &&
	test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
	git rebase -i HEAD^ >actual 2>&1 &&
	! grep "Maybe git-rebase is broken" actual

# GETTEXT POISON #ok 6 - rebase -i with exec of inexistent command

expecting success: 
	git checkout branch2 &&
	set_fake_editor &&
	git rebase -i F &&
	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
	test $(git rev-parse I) = $(git rev-parse HEAD)

# GETTEXT POISON #Successfully rebased and updated refs/heads/branch2.
ok 7 - no changes are a nop

expecting success: 
	git checkout -b dead-end &&
	git rm file6 &&
	git commit -m "stop here" &&
	set_fake_editor &&
	git rebase -i F branch2 &&
	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
	test $(git rev-parse I) = $(git rev-parse branch2) &&
	test $(git rev-parse I) = $(git rev-parse HEAD)

# GETTEXT POISON #rm 'file6'
[dead-end f814f58] stop here
 Author: A U Thor <author@example.com>
 1 file changed, 1 deletion(-)
 delete mode 100644 file6
Successfully rebased and updated refs/heads/branch2.
ok 8 - test the [branch] option

expecting success: 
	git checkout -b test-onto branch2 &&
	set_fake_editor &&
	git rebase -i --onto branch1 F &&
	test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
	test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
	test $(git rev-parse I) = $(git rev-parse branch2)

# GETTEXT POISON #Rebasing (1/1)Successfully rebased and updated refs/heads/test-onto.
ok 9 - test --onto <branch>

expecting success: 
	git checkout branch1 &&
	git tag original-branch1 &&
	set_fake_editor &&
	git rebase -i branch2 &&
	test file6 = $(git diff --name-only original-branch1) &&
	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
	test $(git rev-parse I) = $(git rev-parse branch2) &&
	test $(git rev-parse I) = $(git rev-parse HEAD~2)

# GETTEXT POISON #Rebasing (1/2)Rebasing (2/2)Successfully rebased and updated refs/heads/branch1.
ok 10 - rebase on top of a non-conflicting commit

expecting success: 
	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)

ok 11 - reflog for the branch shows state before rebase

expecting success: 
	printf "rebase -i (finish): refs/heads/branch1 onto %s\n" \
		"$(git rev-parse branch2)" >expected &&
	git log -g --pretty=%gs -1 refs/heads/branch1 >actual &&
	test_cmp expected actual

ok 12 - reflog for the branch shows correct finish message

expecting success: 
	set_fake_editor &&
	FAKE_LINES="2 1" git rebase -i HEAD~2 &&
	test H = $(git cat-file commit HEAD^ | sed -ne \$p) &&
	test G = $(git cat-file commit HEAD | sed -ne \$p)

rebase -i script before editing:
pick ae8f65e G
pick f5f5249 H

rebase -i script after editing:
pick f5f5249 H
pick ae8f65e G
Rebasing (1/2)Rebasing (2/2)Successfully rebased and updated refs/heads/branch1.
ok 13 - exchange two commits

expecting success: 
	git tag new-branch1 &&
	set_fake_editor &&
	test_must_fail git rebase -i master &&
	test "$(git rev-parse HEAD~3)" = "$(git rev-parse master)" &&
	test_cmp expect .git/rebase-merge/patch &&
	test_cmp expect2 file1 &&
	test "$(git diff --name-status |
		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)

Rebasing (1/4)Rebasing (2/4)Rebasing (3/4)Rebasing (4/4)error: # GETTEXT POISON #

# GETTEXT POISON #

Could not apply 5d18e54... G
# GETTEXT POISON #
# GETTEXT POISON #
ok 14 - stop on conflicting pick

expecting success: 
	git rebase --abort &&
	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
	test_path_is_missing .git/rebase-merge

ok 15 - abort

expecting success: 
	git rm --cached file1 &&
	git commit -m "remove file in base" &&
	set_fake_editor &&
	test_must_fail git rebase -i master > output 2>&1 &&
	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
		output &&
	test_i18ngrep "file1" output &&
	test_path_is_missing .git/rebase-merge &&
	git reset --hard HEAD^

rm 'file1'
[branch1 2dd5570] remove file in base
 Author: A U Thor <author@example.com>
 1 file changed, 1 deletion(-)
 delete mode 100644 file1
# GETTEXT POISON # G
ok 16 - abort with error when new base cannot be checked out

expecting success: 
	echo A > file7 &&
	git add file7 &&
	test_tick &&
	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
	git tag twerp &&
	set_fake_editor &&
	git rebase -i --onto master HEAD^ &&
	git show HEAD | grep "^Author: Twerp Snog"

[branch1 2596307] different author
 Author: Twerp Snog <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 file7
Rebasing (1/1)Successfully rebased and updated refs/heads/branch1.
Author: Twerp Snog <author@example.com>
ok 17 - retain authorship

expecting success: 
	git reset --hard twerp &&
	test_commit a conflict a conflict-a &&
	git reset --hard twerp &&
	GIT_AUTHOR_NAME=AttributeMe \
	test_commit b conflict b conflict-b &&
	set_fake_editor &&
	test_must_fail git rebase -i conflict-a &&
	echo resolved >conflict &&
	git add conflict &&
	git rebase --continue &&
	test $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&
	git show >out &&
	grep AttributeMe out

# GETTEXT POISON # different author
[branch1 748a43d] a
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 conflict
# GETTEXT POISON # different author
[branch1 34d1a6c] b
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 conflict
Rebasing (1/1)error: # GETTEXT POISON #

# GETTEXT POISON #

Could not apply 34d1a6c... b
# GETTEXT POISON #
# GETTEXT POISON #
[# GETTEXT POISON # 7866b12] b
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/branch1.
Author: AttributeMe <author@example.com>
ok 18 - retain authorship w/ conflicts

expecting success: 
	git reset --hard twerp &&
	echo B > file7 &&
	test_tick &&
	GIT_AUTHOR_NAME="Nitfol" git commit -m "nitfol" file7 &&
	echo "******************************" &&
	set_fake_editor &&
	FAKE_LINES="1 squash 2" EXPECT_HEADER_COUNT=2 \
		git rebase -i --onto master HEAD~2 &&
	test B = $(cat file7) &&
	test $(git rev-parse HEAD^) = $(git rev-parse master)

# GETTEXT POISON # different author
[branch1 cdf577a] nitfol
 Author: Nitfol <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
******************************
rebase -i script before editing:
pick 2596307 different author
pick cdf577a nitfol

rebase -i script after editing:
pick 2596307 different author
squash cdf577a nitfol
Rebasing (1/2)Rebasing (2/2)[# GETTEXT POISON # d2d5ba7] different author
 Author: Twerp Snog <author@example.com>
 Date: Thu Apr 7 15:33:13 2005 -0700
 1 file changed, 1 insertion(+)
 create mode 100644 file7
Successfully rebased and updated refs/heads/branch1.
ok 19 - squash

expecting success: 
	git show HEAD | grep "^Author: Twerp Snog"

Author: Twerp Snog <author@example.com>
ok 20 - retain authorship when squashing

expecting success: 
	HEAD=$(git rev-parse HEAD) &&
	set_fake_editor &&
	git rebase -i -p HEAD^ &&
	git update-index --refresh &&
	git diff-files --quiet &&
	git diff-index --quiet --cached HEAD -- &&
	test $HEAD = $(git rev-parse HEAD)

# GETTEXT POISON ## GETTEXT POISON #
ok 21 - -p handles "no changes" gracefully

checking known breakage: 
	git checkout H &&
	set_fake_editor &&
	FAKE_LINES="2 1" git rebase -i -p HEAD~2 &&
	test H = $(git cat-file commit HEAD^ | sed -ne \$p) &&
	test G = $(git cat-file commit HEAD | sed -ne \$p)

# GETTEXT POISON ## GETTEXT POISON # 4373208... H
rebase -i script before editing:
pick 83751a6 G
pick 4373208 H

rebase -i script after editing:
pick 4373208 H
pick 83751a6 G
# GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON #
not ok 22 - exchange two commits with -p # TODO known breakage

expecting success: 
	git checkout -b to-be-preserved master^ &&
	: > unrelated-file &&
	git add unrelated-file &&
	test_tick &&
	git commit -m "unrelated" &&
	git checkout -b another-branch master &&
	echo B > file1 &&
	test_tick &&
	git commit -m J file1 &&
	test_tick &&
	git merge to-be-preserved &&
	echo C > file1 &&
	test_tick &&
	git commit -m K file1 &&
	echo D > file1 &&
	test_tick &&
	git commit -m L1 file1 &&
	git checkout HEAD^ &&
	echo 1 > unrelated-file &&
	test_tick &&
	git commit -m L2 unrelated-file &&
	test_tick &&
	git merge another-branch &&
	echo E > file1 &&
	test_tick &&
	git commit -m M file1 &&
	git checkout -b to-be-rebased &&
	test_tick &&
	set_fake_editor &&
	git rebase -i -p --onto branch1 master &&
	git update-index --refresh &&
	git diff-files --quiet &&
	git diff-index --quiet --cached HEAD -- &&
	test $(git rev-parse HEAD~6) = $(git rev-parse branch1) &&
	test $(git rev-parse HEAD~4^2) = $(git rev-parse to-be-preserved) &&
	test $(git rev-parse HEAD^^2^) = $(git rev-parse HEAD^^^) &&
	test $(git show HEAD~5:file1) = B &&
	test $(git show HEAD~3:file1) = C &&
	test $(git show HEAD:file1) = E &&
	test $(git show HEAD:unrelated-file) = 1

# GETTEXT POISON # 83751a6... G
# GETTEXT POISON #[to-be-preserved 4b3f608] unrelated
 Author: AttributeMe <author@example.com>
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 unrelated-file
# GETTEXT POISON #[another-branch 8e3f1bd] J
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
# GETTEXT POISON #
8e3f1bd J
virtual to-be-preserved
# GETTEXT POISON #
0547e3f D
Merge made by the 'recursive' strategy.
 unrelated-file | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 unrelated-file
[another-branch 0f839ac] K
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
[another-branch 916bd1f] L1
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
# GETTEXT POISON ## GETTEXT POISON # 0f839ac... K
[# GETTEXT POISON # 8fa3ce5] L2
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+)
# GETTEXT POISON #
8fa3ce5 L2
virtual another-branch
# GETTEXT POISON #
0f839ac K
Merge made by the 'recursive' strategy.
 file1 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
[# GETTEXT POISON # e12d8fb] M
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
# GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON #
ok 23 - preserve merges with -p

expecting success: 
	set_fake_editor &&
	FAKE_LINES="1 2 edit 3 4" git rebase -i -p HEAD~3 &&
	echo 2 > unrelated-file &&
	test_tick &&
	git commit -m L2-modified --amend unrelated-file &&
	git rebase --continue &&
	git update-index --refresh &&
	git diff-files --quiet &&
	git diff-index --quiet --cached HEAD -- &&
	test $(git show HEAD:unrelated-file) = 2

rebase -i script before editing:
pick 60cb1bd L2
pick f2d0f15 L1
pick 8b9d7f7 Merge branch 'another-branch' into HEAD
pick ecf20bb M

rebase -i script after editing:
pick 60cb1bd L2
pick f2d0f15 L1
edit 8b9d7f7 Merge branch 'another-branch' into HEAD
pick ecf20bb M
# GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON ## GETTEXT POISON #
# GETTEXT POISON #

[# GETTEXT POISON # 82f9bab] L2-modified
 Author: AttributeMe <author@example.com>
 Date: Thu Apr 7 15:43:13 2005 -0700
# GETTEXT POISON ## GETTEXT POISON #
ok 24 - edit ancestor with -p

expecting success: 
	test_tick &&
	set_fake_editor &&
	test_must_fail git rebase -i --onto new-branch1 HEAD^ &&
	echo resolved > file1 &&
	git add file1 &&
	FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue &&
	test $(git rev-parse HEAD^) = $(git rev-parse new-branch1) &&
	git show HEAD | grep chouette

Rebasing (1/1)error: # GETTEXT POISON #

# GETTEXT POISON #

Could not apply 23e2f4c... M
# GETTEXT POISON #
# GETTEXT POISON #
[# GETTEXT POISON # be87a39] chouette!
 Author: AttributeMe <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/to-be-rebased.
    chouette!
ok 25 - --continue tries to commit

expecting success: 
	git reset --hard master@{1} &&
	test_tick &&
	set_fake_editor &&
	test_must_fail git rebase -v -i --onto new-branch1 HEAD^ &&
	echo resolved > file1 &&
	git add file1 &&
	git rebase --continue > output &&
	grep "^ file1 | 2 +-$" output

# GETTEXT POISON # D
# GETTEXT POISON #
 file1 | 2 +-
 file4 | 1 +
 file5 | 1 +
 file6 | 1 +
 4 files changed, 4 insertions(+), 1 deletion(-)
 create mode 100644 file4
 create mode 100644 file5
 create mode 100644 file6
# GETTEXT POISON ## GETTEXT POISON # 5d18e54... G
Rebasing (1/1)
error: # GETTEXT POISON #

# GETTEXT POISON #

Could not apply 0547e3f... D
# GETTEXT POISON #
# GETTEXT POISON #
Successfully rebased and updated refs/heads/to-be-rebased.
 file1 | 2 +-
ok 26 - verbose flag is heeded, even after --continue

expecting success: 
	base=$(git rev-parse HEAD~4) &&
	set_fake_editor &&
	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
		EXPECT_HEADER_COUNT=4 \
		git rebase -i $base &&
	test $base = $(git rev-parse HEAD^) &&
	test 1 = $(git show | grep ONCE | wc -l)

rebase -i script before editing:
pick 615be62 I
pick 0626e8d H
pick 5d18e54 G
pick 2e6badc D

rebase -i script after editing:
pick 615be62 I
squash 0626e8d H
squash 5d18e54 G
squash 2e6badc D
Rebasing (2/4)Rebasing (3/4)error: # GETTEXT POISON #
Could not apply 5d18e54... G
not ok 27 - multi-squash only fires up editor once
#	
#		base=$(git rev-parse HEAD~4) &&
#		set_fake_editor &&
#		FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
#			EXPECT_HEADER_COUNT=4 \
#			git rebase -i $base &&
#		test $base = $(git rev-parse HEAD^) &&
#		test 1 = $(git show | grep ONCE | wc -l)
#	

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 3404 needs a similar fix-up for the series to be able to stand on
> its own.  Alternatively, at least we need to understand what in 'pu'
> makes the result of the merge pass---the symptom indicates that this
> topic cannot be merged to a released version without that unknown
> other topic in 'pu' merged if we want to keep POISON build passing
> the tests.

Ah, no worries.  I think I figured it out.  

The topic "rebase -i regression fix", which this "regression fix
tests" builds on, is queued on an older codebase than 0d75bfe6
("tests: fix tests broken under GETTEXT_POISON=YesPlease",
2017-05-05); it is natural these old test breakages can be seen when
the topic is tested alone.

So we can safely merge this topic down.

Thanks for prodding me to take a deeper look.

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For 3420, I can wrap the two-liner patch I showed here earlier into
> a commit on top of the series.  

So, here is what I'll queue on top before merging the topic down to
'master'.

-- >8 --
Subject: [PATCH] t3420: fix under GETTEXT_POISON build

Newly added tests to t3420 in this series prepare expected
human-readable output from "git rebase -i" and then compare the
actual output with it.  As the output from the command is designed
to go through i18n/l10n, we need to use test_i18ncmp to tell
GETTEXT_POISON build that it is OK the output does not match.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3420-rebase-autostash.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 6826c38cbd..e243700660 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -178,7 +178,7 @@ testrebase () {
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
 		create_expected_success_$suffix &&
-		test_cmp expected actual
+		test_i18ncmp expected actual
 	'
 
 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
@@ -275,7 +275,7 @@ testrebase () {
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
 		create_expected_failure_$suffix &&
-		test_cmp expected actual
+		test_i18ncmp expected actual
 	'
 }
 
-- 
2.13.1-678-g93553a431c

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Phillip Wood]

On 23/06/17 19:53, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 3404 needs a similar fix-up for the series to be able to stand on
>> its own.  Alternatively, at least we need to understand what in 'pu'
>> makes the result of the merge pass---the symptom indicates that this
>> topic cannot be merged to a released version without that unknown
>> other topic in 'pu' merged if we want to keep POISON build passing
>> the tests.
> 
> Ah, no worries.  I think I figured it out.  
> 
> The topic "rebase -i regression fix", which this "regression fix
> tests" builds on, is queued on an older codebase than 0d75bfe6
> ("tests: fix tests broken under GETTEXT_POISON=YesPlease",
> 2017-05-05); it is natural these old test breakages can be seen when
> the topic is tested alone.

Oh, that explains it, I was pretty sure the reflog messages were not
translated so couldn't understand why it would fail under
GETTEXT_POISON=YesPlease

> So we can safely merge this topic down.

That's great, thanks for taking the time to track down the reason for
the test failure

Best Wishes

Phillip

Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes

[Phillip Wood]

On 23/06/17 20:01, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> For 3420, I can wrap the two-liner patch I showed here earlier into
>> a commit on top of the series.  
> 
> So, here is what I'll queue on top before merging the topic down to
> 'master'.

Thanks for creating this fixup, I'll remember to think about
GETTEXT_POISON when I'm writing tests in the future.

> -- >8 --
> Subject: [PATCH] t3420: fix under GETTEXT_POISON build
> 
> Newly added tests to t3420 in this series prepare expected
> human-readable output from "git rebase -i" and then compare the
> actual output with it.  As the output from the command is designed
> to go through i18n/l10n, we need to use test_i18ncmp to tell
> GETTEXT_POISON build that it is OK the output does not match.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t3420-rebase-autostash.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 6826c38cbd..e243700660 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -178,7 +178,7 @@ testrebase () {
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>  		create_expected_success_$suffix &&
> -		test_cmp expected actual
> +		test_i18ncmp expected actual
>  	'
>  
>  	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
> @@ -275,7 +275,7 @@ testrebase () {
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>  		create_expected_failure_$suffix &&
> -		test_cmp expected actual
> +		test_i18ncmp expected actual
>  	'
>  }
>  
>