Uploaded image for project: 'Maven SCM'
  1. Maven SCM
  2. SCM-709

REGRESSION: git status doesn't work if repository root is not the working directory

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.8, 1.8.1
    • Fix Version/s: 1.9
    • Labels:
      None

      Description

      SCM-686 introduced the --porcelain option to make the status result language independend.
      Without the -porcelain option files were listed relative to the working directory, but with -porcelain files are listed relative to the repository root. In most cases these are the same, but not always.

        Issue Links

          Activity

          Hide
          gastonc GastonC added a comment -

          Hi Robert;
          I am experiencing problems while using the maven-scm-provider-gitexe as part of the Maven release plugin.

          The issue occurs this maven-scm-provider-gitexe tried to parse git output which contains spaces, as follows:

          git status --porcelain
          M "dir a/pom.xml"

          I am getting the exception "Illegal character in path at index 0"
          I understand that the quotes around the returned path are not expected by maven-scm-provider-gitexe.

          Version:
          git version 2.14.1.windows.1
          maven-scm-provider-gitexe-1.9.5

          Thanks in advance.

          Show
          gastonc GastonC added a comment - Hi Robert; I am experiencing problems while using the maven-scm-provider-gitexe as part of the Maven release plugin. The issue occurs this maven-scm-provider-gitexe tried to parse git output which contains spaces, as follows: git status --porcelain M "dir a/pom.xml" I am getting the exception "Illegal character in path at index 0" I understand that the quotes around the returned path are not expected by maven-scm-provider-gitexe. Version: git version 2.14.1.windows.1 maven-scm-provider-gitexe-1.9.5 Thanks in advance.
          Hide
          rfscholte Robert Scholte added a comment -

          Let's close this issue, so I can also close the depending issue of the maven-release-plugin for the next release.

          Show
          rfscholte Robert Scholte added a comment - Let's close this issue, so I can also close the depending issue of the maven-release-plugin for the next release.
          Hide
          rfscholte Robert Scholte added a comment -

          IIRC this issue should already be partly solved, at least for the git status part. However, nobody has tested the current snapshot and came with good feedback. As long as nobody has confirmed that it is fixed, I need to keep this issue open.

          Show
          rfscholte Robert Scholte added a comment - IIRC this issue should already be partly solved, at least for the git status part. However, nobody has tested the current snapshot and came with good feedback. As long as nobody has confirmed that it is fixed, I need to keep this issue open.
          Hide
          znerd Ernst de Haan added a comment -

          Note that I incentified this issue via Donay on Sept 30, 2013. If anyone would like to claim it, let me know, otherwise I might reallocate it to a different issue.

          Show
          znerd Ernst de Haan added a comment - Note that I incentified this issue via Donay on Sept 30, 2013. If anyone would like to claim it, let me know, otherwise I might reallocate it to a different issue.
          Hide
          znerd Ernst de Haan added a comment - - edited

          Reproduction steps for my issue:

          1. Have a small Maven project:
            • root folder is project A, containing modules B and C
            • subfolder B contains module B
            • subfolder C contains module C
            • module C has module B as parent
          2. Module A defines a Git repository in an <scm> tag
          3. Go into module C's subdirectory
          4. Execute mvn release:prepare release:perform

          Expected:

          • Tag is created on repository R defined in the <scm> tag in module A

          Actual:

          • Tagging fails, because the release plugin attempts to create a tag on R/C (where R is the repository defined in the <scm> tag in module A and C is the name of module C)

          Workaround:

          • Copy the <scm> tag from module A's POM to module C's POM
          Show
          znerd Ernst de Haan added a comment - - edited Reproduction steps for my issue: Have a small Maven project: root folder is project A, containing modules B and C subfolder B contains module B subfolder C contains module C module C has module B as parent Module A defines a Git repository in an <scm> tag Go into module C's subdirectory Execute mvn release:prepare release:perform Expected: Tag is created on repository R defined in the <scm> tag in module A Actual: Tagging fails, because the release plugin attempts to create a tag on R / C (where R is the repository defined in the <scm> tag in module A and C is the name of module C) Workaround: Copy the <scm> tag from module A's POM to module C's POM
          Hide
          rfscholte Robert Scholte added a comment -

          There are indeed more places where this needs to be fixed. I'll try to refactor the code a bit, so this logic can stay at one place.
          I've just checked out the project in a folder with spaces and that causes several tests to fail, so I need to make those tests space-safe too.

          Show
          rfscholte Robert Scholte added a comment - There are indeed more places where this needs to be fixed. I'll try to refactor the code a bit, so this logic can stay at one place. I've just checked out the project in a folder with spaces and that causes several tests to fail, so I need to make those tests space-safe too.
          Hide
          tensberg Michael Koch added a comment -

          Thanks for taking the time to look at this. I think that even with your fix there is still a problem where the Git*Command classes calculate the relativeRepositoryPath if the repository root path (i. e. the output of git rev-parse --show-toplevel) contains spaces.

          Show
          tensberg Michael Koch added a comment - Thanks for taking the time to look at this. I think that even with your fix there is still a problem where the Git*Command classes calculate the relativeRepositoryPath if the repository root path (i. e. the output of git rev-parse --show-toplevel ) contains spaces.
          Hide
          rfscholte Robert Scholte added a comment -

          Spaces-issue fixed in aff6ce41

          Show
          rfscholte Robert Scholte added a comment - Spaces-issue fixed in aff6ce41
          Hide
          rfscholte Robert Scholte added a comment -

          Spaces issue confirmed, I'll have a look at it

          Show
          rfscholte Robert Scholte added a comment - Spaces issue confirmed, I'll have a look at it
          Hide
          tensberg Michael Koch added a comment -

          I just found out that URI.create generally does not work as intended because it does not create a file: URL. Therefore

          URI.create( stdout.getOutput().trim() ).relativize( fileSet.getBasedir().toURI() )

          does not create a relative URL and the git status command fails. You can test this if in GitStatusCommandTest.testResolvePath() you change

          URI path = repositoryRoot.toURI().relativize( workingDirectory.toURI() );

          to

          URI path = URI.create(repositoryRoot.getAbsolutePath()).relativize(workingDirectory.toURI());.

          Show
          tensberg Michael Koch added a comment - I just found out that URI.create generally does not work as intended because it does not create a file: URL. Therefore URI.create( stdout.getOutput().trim() ).relativize( fileSet.getBasedir().toURI() ) does not create a relative URL and the git status command fails. You can test this if in GitStatusCommandTest.testResolvePath() you change URI path = repositoryRoot.toURI().relativize( workingDirectory.toURI() ); to URI path = URI.create(repositoryRoot.getAbsolutePath()).relativize(workingDirectory.toURI()); .
          Hide
          tensberg Michael Koch added a comment -

          The current 1.9-SNAPSHOT fails in GitStatusCommand.executeStatusCommand line 68 if there are spaces in the directory path.

          [ERROR] Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.4.1:prepare (default-cli) on project codequality: An error occurred during the status check process: Exception while executing SCM command. Illegal character in path at index 29: /home/michael/Projekte/JGloss Test -> [Help 1]
          org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.4.1:prepare (default-cli) on project codequality: An error occurred during the status check process: Exception while executing SCM command.
          	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:217)
          [snip]
          Caused by: java.lang.IllegalArgumentException: Illegal character in path at index 29: /home/michael/Projekte/JGloss Test
          	at java.net.URI.create(URI.java:859)
          	at org.apache.maven.scm.provider.git.gitexe.command.status.GitStatusCommand.executeStatusCommand(GitStatusCommand.java:68)
          	at org.apache.maven.scm.command.status.AbstractStatusCommand.executeCommand(AbstractStatusCommand.java:44)
          	at org.apache.maven.scm.command.AbstractCommand.execute(AbstractCommand.java:59)
          	... 31 more
          Caused by: java.net.URISyntaxException: Illegal character in path at index 29: /home/michael/Projekte/JGloss Test
          	at java.net.URI$Parser.fail(URI.java:2829)
          

          This is because URI.create does not handle spaces. Using new File(path).toURI() instead fixes this bug. The GitAddCommand and GitCheckInCommand should also be affected, though I haven't tested it. I've attached a patch which fixes all occurrences.

          Show
          tensberg Michael Koch added a comment - The current 1.9-SNAPSHOT fails in GitStatusCommand.executeStatusCommand line 68 if there are spaces in the directory path. [ERROR] Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.4.1:prepare (default-cli) on project codequality: An error occurred during the status check process: Exception while executing SCM command. Illegal character in path at index 29: /home/michael/Projekte/JGloss Test -> [Help 1] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.4.1:prepare (default-cli) on project codequality: An error occurred during the status check process: Exception while executing SCM command. at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:217) [snip] Caused by: java.lang.IllegalArgumentException: Illegal character in path at index 29: /home/michael/Projekte/JGloss Test at java.net.URI.create(URI.java:859) at org.apache.maven.scm.provider.git.gitexe.command.status.GitStatusCommand.executeStatusCommand(GitStatusCommand.java:68) at org.apache.maven.scm.command.status.AbstractStatusCommand.executeCommand(AbstractStatusCommand.java:44) at org.apache.maven.scm.command.AbstractCommand.execute(AbstractCommand.java:59) ... 31 more Caused by: java.net.URISyntaxException: Illegal character in path at index 29: /home/michael/Projekte/JGloss Test at java.net.URI$Parser.fail(URI.java:2829) This is because URI.create does not handle spaces. Using new File(path).toURI() instead fixes this bug. The GitAddCommand and GitCheckInCommand should also be affected, though I haven't tested it. I've attached a patch which fixes all occurrences.
          Hide
          rfscholte Robert Scholte added a comment -

          r.43bc1325 and r.10099c04 committed. I'll let some devs test it first.

          Show
          rfscholte Robert Scholte added a comment - r.43bc1325 and r.10099c04 committed. I'll let some devs test it first.
          Hide
          rfscholte Robert Scholte added a comment -
          Show
          rfscholte Robert Scholte added a comment - http://stackoverflow.com/questions/957928/is-there-a-way-to-get-the-git-root-directory-in-one-command Tested on Windows and Mac, so that could be something to include
          Hide
          rfscholte Robert Scholte added a comment -

          No, this doesn't fix the issue. Now only the files within the working directory are checked, not does outside the working directory but inside the repository.

          Show
          rfscholte Robert Scholte added a comment - No, this doesn't fix the issue. Now only the files within the working directory are checked, not does outside the working directory but inside the repository.
          Hide
          andrei.pozolotin Andrei Pozolotin added a comment -

          wow! it it all that needed? is part of unit tests now? where can I get snapshot to try this?

          Show
          andrei.pozolotin Andrei Pozolotin added a comment - wow! it it all that needed? is part of unit tests now? where can I get snapshot to try this?
          Show
          rfscholte Robert Scholte added a comment - Dot has been added in https://git-wip-us.apache.org/repos/asf?p=maven-scm.git;a=commit;h=4518cfc5983669cf5f4bcf90a4a4963897afc739
          Hide
          tik Tim Kettler added a comment -

          @Darryl
          That's what the command should be changed to then.

          Show
          tik Tim Kettler added a comment - @Darryl That's what the command should be changed to then.
          Hide
          dlmiles Darryl L. Miles added a comment - - edited

          @Tim

          cd subdir/in/one/git/repo
          git status --porcelain .

          Note the use of the "." file name.
          This can be used to get SVN like behaviour where only the files changes below the current directly are shown.

          Show
          dlmiles Darryl L. Miles added a comment - - edited @Tim cd subdir/in/one/git/repo git status --porcelain . Note the use of the "." file name. This can be used to get SVN like behaviour where only the files changes below the current directly are shown.
          Hide
          rfscholte Robert Scholte added a comment -

          SCM-686 broke it

          Show
          rfscholte Robert Scholte added a comment - SCM-686 broke it
          Hide
          andrei.pozolotin Andrei Pozolotin added a comment -

          I just remembered that it worked fine with maven-release-plugin v 2.3.2 (and whatever SCM dependency was there)
          is there easy way to see what specific change to v 2.4 (and related SCM) broke this?

          Show
          andrei.pozolotin Andrei Pozolotin added a comment - I just remembered that it worked fine with maven-release-plugin v 2.3.2 (and whatever SCM dependency was there) is there easy way to see what specific change to v 2.4 (and related SCM) broke this?
          Hide
          rfscholte Robert Scholte added a comment -

          @Andrei:
          1. Although it looks like a hack, this will probably work. It's either knowing the relative path between the workingdirectory and the repository root and check the actual file, or get the information from the status-entries. Git claims that the output of porcelain is consistent. After a chat with Mark we decided to try this. We have several different CI systems to verify that this works. I would expect that the status-entry would already have enough information to decide if the file exists or not.
          2. That was another idea, but I'm pretty sure that it will break the tests right now.
          3. Probably not. As a Windows user (probably the most critical OS in this case) I can confirm that the GIT output is consistent and uses forward slashes.
          4. Do we need to check if the file exists, if we better analyze the status output
          5. See 4
          6. That was my question to Mark
          7. how?

          @Tim
          Good point. AFAIK scm status expects all modified files inside (relative to?) the working directory.

          Show
          rfscholte Robert Scholte added a comment - @Andrei: 1. Although it looks like a hack, this will probably work. It's either knowing the relative path between the workingdirectory and the repository root and check the actual file, or get the information from the status-entries. Git claims that the output of porcelain is consistent. After a chat with Mark we decided to try this. We have several different CI systems to verify that this works. I would expect that the status-entry would already have enough information to decide if the file exists or not. 2. That was another idea, but I'm pretty sure that it will break the tests right now. 3. Probably not. As a Windows user (probably the most critical OS in this case) I can confirm that the GIT output is consistent and uses forward slashes. 4. Do we need to check if the file exists, if we better analyze the status output 5. See 4 6. That was my question to Mark 7. how? @Tim Good point. AFAIK scm status expects all modified files inside (relative to?) the working directory.
          Hide
          tik Tim Kettler added a comment -

          I started looking into how to fix this yesterday, too. The most natural solution for me seemed to be to find the repository root and create the files based on that. Either by invoking git rev-parse --show-toplevel or by walking up the directory tree and looking for '.git'.

          The other issue to consider is the actual semantics of the status command. Invoked from the repository root all provider implementations behave the same but from a subdirectory svn status only shows changes in the subdirectory and below while git shows changes from the whole repository. Is this intended or should the scm status command behave uniform regardless of the provider?

          Show
          tik Tim Kettler added a comment - I started looking into how to fix this yesterday, too. The most natural solution for me seemed to be to find the repository root and create the files based on that. Either by invoking git rev-parse --show-toplevel or by walking up the directory tree and looking for '.git' . The other issue to consider is the actual semantics of the status command. Invoked from the repository root all provider implementations behave the same but from a subdirectory svn status only shows changes in the subdirectory and below while git shows changes from the whole repository. Is this intended or should the scm status command behave uniform regardless of the provider?
          Hide
          andrei.pozolotin Andrei Pozolotin added a comment -

          I took a look. few ideas:

          1) "/" feels like a hack; who guarantees its presence ?

          2) could you differentiate via all of: File.exists() File.isFile() File.isDirectory() ?

          3) "/" probably should be File.separator ?

          4) need File.getCanonicalFile() to guard against symlinks ?

          5) need File.getAbsolutePath() to actually render File.separator suffix ?

          6) only one of oldFilePath or newFilePath is actually present on file system for File.exists() to work,
          could be logic error in :: if ( status == ScmFileStatus.RENAMED ) {} :: block, if treating both same way ?

          7) if original issue is path/file overlap, may be should detect specifically only that?

          Show
          andrei.pozolotin Andrei Pozolotin added a comment - I took a look. few ideas: 1) "/" feels like a hack; who guarantees its presence ? 2) could you differentiate via all of: File.exists() File.isFile() File.isDirectory() ? 3) "/" probably should be File.separator ? 4) need File.getCanonicalFile() to guard against symlinks ? 5) need File.getAbsolutePath() to actually render File.separator suffix ? 6) only one of oldFilePath or newFilePath is actually present on file system for File.exists() to work, could be logic error in :: if ( status == ScmFileStatus.RENAMED ) {} :: block, if treating both same way ? 7) if original issue is path/file overlap, may be should detect specifically only that?
          Hide
          rfscholte Robert Scholte added a comment -

          My latest commit was https://git-wip-us.apache.org/repos/asf?p=maven-scm.git;a=commitdiff;h=6aff3431817108139d29914dc81d8d2dc53e3c6a
          The idea is to check for a forward slash at the end. If so, it should be a directory, otherwise a file.
          But if I switch the lines of private boolean isFile( String file ), some tests fail.
          The reason: if a file does not exist, it returns false, but that's not the same as being a directory.
          Mark Struberg offered his help within a couple of weeks to check if the tests are wrong or not.

          Or you could speed it up by having a look at it.

          Show
          rfscholte Robert Scholte added a comment - My latest commit was https://git-wip-us.apache.org/repos/asf?p=maven-scm.git;a=commitdiff;h=6aff3431817108139d29914dc81d8d2dc53e3c6a The idea is to check for a forward slash at the end. If so, it should be a directory, otherwise a file. But if I switch the lines of private boolean isFile( String file ) , some tests fail. The reason: if a file does not exist, it returns false , but that's not the same as being a directory. Mark Struberg offered his help within a couple of weeks to check if the tests are wrong or not. Or you could speed it up by having a look at it.
          Hide
          andrei.pozolotin Andrei Pozolotin added a comment -

          I am curious what next steps would be?

          Show
          andrei.pozolotin Andrei Pozolotin added a comment - I am curious what next steps would be?
          Hide
          rfscholte Robert Scholte added a comment -

          Differences between --short and --porcelain:

          1. The user’s color.status configuration is not respected; color will always be off.

          2. The user’s status.relativePaths configuration is not respected; paths shown will always be relative to the repository root.

          So this is actually saying that the working directory is ignored. And if the working directory is not the repository root, you're having an issue.

          Show
          rfscholte Robert Scholte added a comment - Differences between --short and --porcelain: 1. The user’s color.status configuration is not respected; color will always be off. 2. The user’s status.relativePaths configuration is not respected; paths shown will always be relative to the repository root. So this is actually saying that the working directory is ignored. And if the working directory is not the repository root, you're having an issue.
          Hide
          rfscholte Robert Scholte added a comment -

          Now we seem to have to root cause. Still weird that only a few people seem to have trouble with it. The next question would be: which of the 2 values is wrong?

          Show
          rfscholte Robert Scholte added a comment - Now we seem to have to root cause. Still weird that only a few people seem to have trouble with it. The next question would be: which of the 2 values is wrong?
          Hide
          tik Tim Kettler added a comment -

          The problem is not with the regex but with the processing afterwards.

          GitStatusConsumer tries to construct a new File object from the workingDirectory and the extracted path from the regex and then checks for existence.

          Values for me are:

          workingDirectory = /home/tik/Develop/barchart-bugs/barchart-bugs-MRELEASE-812/build-tester/barchart-bugs-MRELEASE-812-case-02
          files.get( 0 ) = barchart-bugs-MRELEASE-812/build-tester/barchart-bugs-MRELEASE-812-case-02/pom.xml

          Obviously, the paths overlap and hence the check fails and the file is discarded.

          Show
          tik Tim Kettler added a comment - The problem is not with the regex but with the processing afterwards. GitStatusConsumer tries to construct a new File object from the workingDirectory and the extracted path from the regex and then checks for existence. Values for me are: workingDirectory = /home/tik/Develop/barchart-bugs/barchart-bugs- MRELEASE-812 /build-tester/barchart-bugs- MRELEASE-812 -case-02 files.get( 0 ) = barchart-bugs- MRELEASE-812 /build-tester/barchart-bugs- MRELEASE-812 -case-02/pom.xml Obviously, the paths overlap and hence the check fails and the file is discarded.
          Hide
          rfscholte Robert Scholte added a comment -

          from http://markmail.org/message/trozsqiym4zruj2u

          My git version is: 1.7.7.5 (Apple Git-26)

          Here's a sample output of git status --porcelain:

          M pom.xml
          A foo-archetype/pom.xml
          ?? phonebook/

          As expected I would say...

          Show
          rfscholte Robert Scholte added a comment - from http://markmail.org/message/trozsqiym4zruj2u My git version is: 1.7.7.5 (Apple Git-26) Here's a sample output of git status --porcelain: M pom.xml A foo-archetype/pom.xml ?? phonebook/ As expected I would say...
          Show
          rfscholte Robert Scholte added a comment - Reference: http://www.kernel.org/pub/software/scm/git/docs/git-status.html

            People

            • Assignee:
              rfscholte Robert Scholte
              Reporter:
              rfscholte Robert Scholte
            • Votes:
              7 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development