You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2023/05/02 15:49:46 UTC
[GitHub] [maven] cstamas opened a new pull request, #1100: [MNG-7038] Fixes for topDirectory
cstamas opened a new pull request, #1100:
URL: https://github.com/apache/maven/pull/1100
Changes:
* resolve file argument against workingDirectory explicitly
* the check Files.isRegularFile should happen against path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182874055
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
I still don't understand why would this loop enter 2 or more times? (the user specifies several -f parameters?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182897497
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
Missed the `break` ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182840100
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
Hm, while it IS inside a loop, it enters only ONCE, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182837938
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
That's really weird. That would make file argument order dependent as the `topDirectory` is modified inside the loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182875131
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
but even then, there is a break (or throws) so first entrance will either break or throw
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182855872
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
Even if this was not the case, the goal of the loop is to compute the `topDirectory`.
However, introducing a `workingDirectory` variable and using it to resolve the `path` could make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas merged pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #1100:
URL: https://github.com/apache/maven/pull/1100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1100: [MNG-7038] Fixes for topDirectory
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1100:
URL: https://github.com/apache/maven/pull/1100#discussion_r1182841201
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -332,10 +332,10 @@ void initialize(CliRequest cliRequest) throws ExitException {
for (String arg : cliRequest.args) {
if (isAltFile) {
// this is the argument following -f/--file
- Path path = Paths.get(arg);
+ Path path = topDirectory.resolve(arg);
Review Comment:
Hm, am wrong it seems, I assumed loop stops on "first found" but now as I look it does not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org