Skip to content
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.

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

t0061: fix test for argv[0] with spaces (MINGW only) #356

Closed

Conversation

SyntevoAlex
Copy link

The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

  1. It succeeds with AND without fix eb7c786 that addressed user's
    problem. This happens because the core problem was misunderstood,
    leading to conclusion that git is unable to start any programs with
    spaces in path on Win7. But in fact
    a) Bug only affected cmd.exe scripts, such as .bat scripts
    b) Bug only happened when cmd.exe received at least two quoted args
    c) Bug happened on any Windows (verified on Win10).
    Therefore, correct test must involve .bat script and two quoted args.
  2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
    is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy [email protected]

@SyntevoAlex
Copy link
Author

@dscho thanks for the pointer about gcc build! Didn't realize that default makefile doesn't use VS at all. I have updated the commit message; does that look good now?

@SyntevoAlex
Copy link
Author

This PR is continuation of git#651 which I have created on wrong repo by mistake.

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning up my mess! I offered a couple suggestions I hope you agree with. If not, this is already good to go onto the Git mailing list.

t/t0061-run-command.sh Outdated Show resolved Hide resolved
t/t0061-run-command.sh Outdated Show resolved Hide resolved
t/t0061-run-command.sh Outdated Show resolved Hide resolved
The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

1. It succeeds with AND without fix eb7c786 that addressed user's
   problem. This happens because the core problem was misunderstood,
   leading to conclusion that git is unable to start any programs with
   spaces in path on Win7. But in fact
     a) Bug only affected cmd.exe scripts, such as .bat scripts
     b) Bug only happened when cmd.exe received at least two quoted args
     c) Bug happened on any Windows (verified on Win10).
   Therefore, correct test must involve .bat script and two quoted args.
2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
   is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy <[email protected]>
@SyntevoAlex
Copy link
Author

SyntevoAlex commented Sep 30, 2019

OK, I have updated the patch. Different code style preferences, I guess.
Does it look good now?

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 1, 2019

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 2, 2019

This branch is now known as js/mingw-spawn-with-spaces-in-path.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 2, 2019

This patch series was integrated into pu via git@e73c6f3.

@gitgitgadget gitgitgadget bot added the pu label Oct 2, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 3, 2019

This patch series was integrated into pu via git@e698a13.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2019

This patch series was integrated into pu via git@0f47be2.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2019

This patch series was integrated into pu via git@661c071.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2019

This patch series was integrated into next via git@0fdb87d.

@gitgitgadget gitgitgadget bot added the next label Oct 4, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 6, 2019

This patch series was integrated into pu via git@4487d83.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 7, 2019

This patch series was integrated into pu via git@b6d1e92.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 8, 2019

This patch series was integrated into pu via git@df1c917.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into pu via git@cdadaa3.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into pu via git@424663d.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into next via git@424663d.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into master via git@424663d.

@gitgitgadget gitgitgadget bot added the master label Oct 9, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

Closed via 424663d.

@gitgitgadget gitgitgadget bot closed this Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants