Skip to content

Back to Shell usage for Windows command line (+ minor Win UT fix) #20

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Back to Shell usage for Windows command line (+ minor Win UT fix) #20

wants to merge 1 commit into from

Conversation

axel3rd
Copy link
Contributor

@axel3rd axel3rd commented Feb 7, 2017

PR for #17 : Back to Shell usage for Windows command line (only).
Allow embedded CMD command (echo, ...) and binary on PATH usage.
Regression since v3.0.15 (b38a1b3 : improvement avoiding some shell injection).

In addition, minor UT fix on DirectoryScanner (were platform dependant)

michael-o
michael-o reviewed Jun 18, 2017
View reviewed changes
src/main/java/org/codehaus/plexus/util/cli/Commandline.java
@@ -139,7 +139,7 @@
* Create a new command line object.
* Shell is autodetected from operating system
*
* Shell usage is only desirable when generating code for remote execution.
* For Linux, shell usage is only desirable when generating code for remote execution.
Copy link
Member

Choose a reason for hiding this comment

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

Not Linux, but Unix-like. There are other OSes too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-o : Good catch, you are right. Do you want I update the PR (there is 6 mentioned in file) ? (I'm not sure to understand, you have closed the PR).

Copy link
Member

Choose a reason for hiding this comment

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

I have already merged a fixed version of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks

@michael-o michael-o closed this Jun 18, 2017
@axel3rd axel3rd deleted the #17 branch June 18, 2017 21:19
@rfscholte
Copy link
Member

When trying to run this with gitbash (mingw64) I get the following result:

Failed tests:
  DirectoryScannerTest.testFollowSymlinks:186 null
  DirectoryScannerTest.testFollowSymlinksFalse:155 expected:<9> but was:<11>

Tests run: 303, Failures: 2, Errors: 0, Skipped: 0

@axel3rd axel3rd mentioned this pull request Jun 19, 2017
Closed
@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 19, 2017

@rfscholte : Thanks for the feedback.

I have tested on 3.0.24 (GitBash, Windows 10), this two tests fail previously your report (on the assertAlwaysIncluded):

DirectoryScannerTest.testFollowSymlinks:185->assertAlwaysIncluded:167 nul
DirectoryScannerTest.testFollowSymlinksFalse:154->assertAlwaysIncluded:167 null

=> I have open #24, I will try to have a look on it.

@axel3rd axel3rd restored the #17 branch June 20, 2017 09:15
@axel3rd axel3rd deleted the #17 branch June 20, 2017 10:01
@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 20, 2017 •
edited
Loading

@rfscholte : After investigation, the unit tests fail when symlink resources are not preserved during git clone.

The core.symlinks option should be used:

git clone -c core.symlinks=true https://github.com/codehaus-plexus/plexus-utils.git

I will reorient #24 and provide a PR for this check, to avoid some people loose time like us next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@axel3rd @rfscholte @michael-o