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

[Junio C Hamano]

Johannes Sixt  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 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]

Phillip Wood  writes:

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

Thanks, both.

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  writes:
> 
>> Junio C Hamano  writes:
>>
>>> Phillip Wood  writes:
>>>
 From: Phillip Wood 

 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

[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

[Junio C Hamano]

Johannes Sixt  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

[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

[Johannes Schindelin]

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,
DschoFrom c5a8319f0378d93be5aea05b833bb5e23c9f0b3d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin 
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 
---
 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(, "apply");
 	argv_array_push(, stash_sha1.buf);
 	if (!run_command())
-		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())
 			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(_sha1);
-- 
2.12.2.windows.1

From 3425f7c9bfb62c3d2d4adaff41a664dd4ae2efa9 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin 
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 
---
 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

From e16ad989c85c55bdfcf45fe561911a699e962b44 Mon Sep 17 00:00:00 2001

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

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> Phillip Wood  writes:
>>
>>> From: Phillip Wood 
>>>
>>> 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

[Junio C Hamano]

Junio C Hamano  writes:

> Phillip Wood  writes:
>
>> From: Phillip Wood 
>>
>> 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]

Phillip Wood  writes:

> From: Phillip Wood 
>
> 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

[Johannes Schindelin]

Hi Phillip,

On Wed, 14 Jun 2017, Phillip Wood wrote:

> From: Phillip Wood 
> 
> 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

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

[Phillip Wood]

From: Phillip Wood 

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