t3420 remove progress from output #276
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can actually use a different PR base branch; just hit the Of course, it wouldn't hurt to mention the base branch name explicitly in the cover letter... |
phillipwood
force-pushed
the
wip/t3420-remove-progress-from-output
branch
from
c9a1adb
to
5250162
Compare
I think so. |
|
/submit |
|
Submitted as pull.276.git.gitgitgadget@gmail.com |
|
@dscho - do you know why didn't this Cc Gábor? |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -48,8 +48,8 @@ create_expected_success_interactive () { | |||
| q_to_cr >expected <<-EOF | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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 <phillip.wood@dunelm.org.uk>
> ---
> 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
> '
> }
There was a problem hiding this comment.
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" <gitgitgadget@gmail.com> 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 <phillip.wood@dunelm.org.uk>
>> ---
>> 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
>> '
>> }
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> 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 ;-)
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 02/07/2019 18:23, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> 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
|
This branch is now known as |
|
This patch series was integrated into pu via git@b103ab3. |
Yes, because I did a poor job implementing the |
|
This patch series was integrated into pu via git@3a446b1. |
Some of the tests check the output of rebase is what we expect. These were added after a regression that added unwanted stash output when using --autostash. They are useful as they prevent unintended changes to the output of the various rebase commands. However they also include all the progress output which is less useful as it only tests what would be written to a dumb terminal which is not the normal use case. The recent changes to fix clearing the line when printing progress necessarily meant making an ugly change to these tests. Address this my removing the 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. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Thanks, I'll covert it to a list |
phillipwood
force-pushed
the
wip/t3420-remove-progress-from-output
branch
from
5250162
to
e1b4023
Compare
|
This patch series was integrated into pu via git@f96d5be. |
|
This patch series was integrated into pu via git@5fc7643. |
|
/submit |
|
Submitted as pull.276.v2.git.gitgitgadget@gmail.com |
|
This patch series was integrated into pu via git@9350b89. |
|
This patch series was integrated into pu via git@daef3da. |
|
This patch series was integrated into pu via git@171857f. |
|
This patch series was integrated into next via git@1363de0. |
|
This patch series was integrated into pu via git@28dd4a6. |
|
This patch series was integrated into pu via git@b954692. |
|
This patch series was integrated into next via git@b954692. |
|
This patch series was integrated into master via git@b954692. |
|
Closed via b954692. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove ugliness in the tests that check the output of git rebase
I've updated this to avoid repeated calls to printf as suggested by Junio
Based-On: sg/rebase-progress
Cc: SZEDER Gábor szeder.dev@gmail.com, Johannes Schindelin Johannes.Schindelin@gmx.de