Skip to content

t3420 remove progress from output #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

t3420 remove progress from output #276

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions t/t3420-rebase-autostash.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ test_expect_success setup '
echo conflicting-change >file2 &&
git add . &&
test_tick &&
git commit -m "related commit"
git commit -m "related commit" &&
remove_progress_re="$(printf "s/.*\\r//")"
'

create_expected_success_am () {
Expand All @@ -48,8 +49,8 @@ create_expected_success_interactive () {
q_to_cr >expected <<-EOF
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <[email protected]> writes:

> progress output before comparing it to the expected output. We do this
> by removing everything before the final "\r" on each line as we don't
> care about the progress indicator, but we do care about what is printed
> immediately after it.

As long as sed implementation used here does not do anything funny
to CR, I think the approach to strip everything before the last CR
on the line is sensible.  As I am not familiar with how Windows port
of sed wants to treat a CR byte in the pattern, I am not sure about
the precondition of the above statement, though.

I also have to wonder if we can/want to do this without an extra
printf process every time we sanitize the output, though I do not
think I care too deeply about it.

> Signed-off-by: Phillip Wood <[email protected]>
> ---
>  t/t3420-rebase-autostash.sh | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 9186e90127..0454018584 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -48,8 +48,8 @@ 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.
> -	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
> +	Applied autostash.
> +	Successfully rebased and updated refs/heads/rebased-feature-branch.
>  	EOF
>  }
>  
> @@ -67,13 +67,13 @@ create_expected_failure_am () {
>  }
>  
>  create_expected_failure_interactive () {
> -	q_to_cr >expected <<-EOF
> +	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)QRebasing (2/2)QApplying autostash resulted in conflicts.
> +	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.
> -	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
> +	Successfully rebased and updated refs/heads/rebased-feature-branch.
>  	EOF
>  }
>  
> @@ -109,7 +109,8 @@ testrebase () {
>  			suffix=interactive
>  		fi &&
>  		create_expected_success_$suffix &&
> -		test_i18ncmp expected actual
> +		sed "$(printf "s/.*\\r//")" <actual >actual2 &&
> +		test_i18ncmp expected actual2
>  	'
>  
>  	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
> @@ -209,7 +210,8 @@ testrebase () {
>  			suffix=interactive
>  		fi &&
>  		create_expected_failure_$suffix &&
> -		test_i18ncmp expected actual
> +		sed "$(printf "s/.*\\r//")" <actual >actual2 &&
> +		test_i18ncmp expected actual2
>  	'
>  }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

On 01/07/2019 22:01, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> writes:
> 
>> progress output before comparing it to the expected output. We do this
>> by removing everything before the final "\r" on each line as we don't
>> care about the progress indicator, but we do care about what is printed
>> immediately after it.
> 
> As long as sed implementation used here does not do anything funny
> to CR, I think the approach to strip everything before the last CR
> on the line is sensible.  As I am not familiar with how Windows port
> of sed wants to treat a CR byte in the pattern, I am not sure about
> the precondition of the above statement, though.

I wondered about that too, but it passes the CI tests under windows.

> I also have to wonder if we can/want to do this without an extra
> printf process every time we sanitize the output, though I do not
> think I care too deeply about it.

I could add 're="$(printf ...)"' to the setup at the top of the file if 
you want

Best Wishes

Phillip

>> Signed-off-by: Phillip Wood <[email protected]>
>> ---
>>   t/t3420-rebase-autostash.sh | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index 9186e90127..0454018584 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -48,8 +48,8 @@ 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.
>> -	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
>> +	Applied autostash.
>> +	Successfully rebased and updated refs/heads/rebased-feature-branch.
>>   	EOF
>>   }
>>   
>> @@ -67,13 +67,13 @@ create_expected_failure_am () {
>>   }
>>   
>>   create_expected_failure_interactive () {
>> -	q_to_cr >expected <<-EOF
>> +	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)QRebasing (2/2)QApplying autostash resulted in conflicts.
>> +	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.
>> -	Q                                                                                QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
>> +	Successfully rebased and updated refs/heads/rebased-feature-branch.
>>   	EOF
>>   }
>>   
>> @@ -109,7 +109,8 @@ testrebase () {
>>   			suffix=interactive
>>   		fi &&
>>   		create_expected_success_$suffix &&
>> -		test_i18ncmp expected actual
>> +		sed "$(printf "s/.*\\r//")" <actual >actual2 &&
>> +		test_i18ncmp expected actual2
>>   	'
>>   
>>   	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
>> @@ -209,7 +210,8 @@ testrebase () {
>>   			suffix=interactive
>>   		fi &&
>>   		create_expected_failure_$suffix &&
>> -		test_i18ncmp expected actual
>> +		sed "$(printf "s/.*\\r//")" <actual >actual2 &&
>> +		test_i18ncmp expected actual2
>>   	'
>>   }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Phillip Wood <[email protected]> writes:

>> As long as sed implementation used here does not do anything funny
>> to CR, I think the approach to strip everything before the last CR
>> on the line is sensible.  As I am not familiar with how Windows port
>> of sed wants to treat a CR byte in the pattern, I am not sure about
>> the precondition of the above statement, though.
>
> I wondered about that too, but it passes the CI tests under windows.

Hopefully Git for Windows, MinGW, and CygWin would all behave
similarly.

>> I also have to wonder if we can/want to do this without an extra
>> printf process every time we sanitize the output, though I do not
>> think I care too deeply about it.
>
> I could add 're="$(printf ...)"' to the setup at the top of the file
> if you want

As I do not care too deeply about it, we recently saw a lot about
reducing number of processes in the tests, so apparently some folks
care and I presume they want to see something like that to happen.
I do not think $re is a good name for such a variable, though ;-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

On 02/07/2019 18:23, Junio C Hamano wrote:
> Phillip Wood <[email protected]> writes:
> 
>>> As long as sed implementation used here does not do anything funny
>>> to CR, I think the approach to strip everything before the last CR
>>> on the line is sensible.  As I am not familiar with how Windows port
>>> of sed wants to treat a CR byte in the pattern, I am not sure about
>>> the precondition of the above statement, though.
>>
>> I wondered about that too, but it passes the CI tests under windows.
> 
> Hopefully Git for Windows, MinGW, and CygWin would all behave
> similarly.
> 
>>> I also have to wonder if we can/want to do this without an extra
>>> printf process every time we sanitize the output, though I do not
>>> think I care too deeply about it.
>>
>> I could add 're="$(printf ...)"' to the setup at the top of the file
>> if you want
> 
> As I do not care too deeply about it, we recently saw a lot about
> reducing number of processes in the tests, so apparently some folks
> care and I presume they want to see something like that to happen.
> I do not think $re is a good name for such a variable, though ;-)

Yes, $re was just a place holder - naming is hard ...

Best Wishes

Phillip

$(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.
Q QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
Applied autostash.
Successfully rebased and updated refs/heads/rebased-feature-branch.
EOF
}

Expand All @@ -67,13 +68,13 @@ create_expected_failure_am () {
}

create_expected_failure_interactive () {
q_to_cr >expected <<-EOF
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)QRebasing (2/2)QApplying autostash resulted in conflicts.
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.
Q QSuccessfully rebased and updated refs/heads/rebased-feature-branch.
Successfully rebased and updated refs/heads/rebased-feature-branch.
EOF
}

Expand Down Expand Up @@ -109,7 +110,8 @@ testrebase () {
suffix=interactive
fi &&
create_expected_success_$suffix &&
test_i18ncmp expected actual
sed "$remove_progress_re" <actual >actual2 &&
test_i18ncmp expected actual2
'

test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
Expand Down Expand Up @@ -209,7 +211,8 @@ testrebase () {
suffix=interactive
fi &&
create_expected_failure_$suffix &&
test_i18ncmp expected actual
sed "$remove_progress_re" <actual >actual2 &&
test_i18ncmp expected actual2
'
}

Expand Down