Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-22510

Clean After Checkout Results in Failed to Checkout Revision

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      We're executing a job, and trying to perform a git clean. One of our submodules is dirty, and unfortunately since the Jenkins GIT plugin cleans after checkout, we get the following error:

      FATAL: Command "git submodule update --init --recursive" returned status code 1:
      stdout:
      stderr: error: Your local changes to the following files would be overwritten by checkout:
      minified/sidecar.js
      minified/sidecar.lite.js
      minified/sidecar.lite.min.js
      minified/sidecar.min.js
      Please, commit your changes or stash them before you can switch branches.
      Aborting
      Unable to checkout 'd58c3304230e4bf26ffb3fa6986d4b6ba90d4c66' in submodule path 'sugarcrm/sidecar'

      I tested using the Pre-SCM Buildstep plugin and performing a git clean before the checkout fixes the problem. Specifically having it perform:
      git submodule foreach --recursive git clean -fdx
      git submodule foreach --recursive git reset --hard

      before we do the checkout fixes it.

        Attachments

          Issue Links

            Activity

            Hide
            rsennewald Ray Sennewald added a comment -

            David Wang has implemented functionality to Clean Before Checkout, which actually helps fix this scenario. His pull request is here: https://github.com/jenkinsci/git-plugin/pull/220.

            Show
            rsennewald Ray Sennewald added a comment - David Wang has implemented functionality to Clean Before Checkout, which actually helps fix this scenario. His pull request is here: https://github.com/jenkinsci/git-plugin/pull/220 .
            Hide
            markewaite Mark Waite added a comment - - edited

            Considering the number of cases related to this topic over the course of the evolution of the git plugin, I'd very much prefer that we have a very deep set of tests created which will assure that submodules behave as desired. Some of the related bugs (JENKINS-8315, JENKINS-7445) are listed as fixed, yet those fixes either did not survive, or do not meet your need. I'd really like more tests that express the failure cases, since the git-client-plugin has very few submodule tests.

            Ultimately, we may need a "clean before checkout" operation, but it seems like we should first invest effort to create unit tests which will assure that submodules are behaving as expected in the git client plugin (and possibly in the git plugin).

            Relying on the user to know when they should select "clean before checkout" and when they should "clean after checkout" seems like we're expecting too much from the users. I would very much prefer to find a sequence which avoids requiring the addition of yet another form of clean. We have "wipe workspace" which removes everything, we have "clean after checkout", and this will add "clean before checkout".

            Since you and David are active users of submodules, would you be willing to provide one or more test cases which illustrate this failure mode, then use that test case to evaluate alternatives to fix the test?

            As an example of an alternative, would the "git clean -ffxd" command (double f arguments clean untracked directories managed by another repository) resolve the issue without requiring the addition of a new command?

            Show
            markewaite Mark Waite added a comment - - edited Considering the number of cases related to this topic over the course of the evolution of the git plugin, I'd very much prefer that we have a very deep set of tests created which will assure that submodules behave as desired. Some of the related bugs ( JENKINS-8315 , JENKINS-7445 ) are listed as fixed, yet those fixes either did not survive, or do not meet your need. I'd really like more tests that express the failure cases, since the git-client-plugin has very few submodule tests. Ultimately, we may need a "clean before checkout" operation, but it seems like we should first invest effort to create unit tests which will assure that submodules are behaving as expected in the git client plugin (and possibly in the git plugin). Relying on the user to know when they should select "clean before checkout" and when they should "clean after checkout" seems like we're expecting too much from the users. I would very much prefer to find a sequence which avoids requiring the addition of yet another form of clean. We have "wipe workspace" which removes everything, we have "clean after checkout", and this will add "clean before checkout". Since you and David are active users of submodules, would you be willing to provide one or more test cases which illustrate this failure mode, then use that test case to evaluate alternatives to fix the test? As an example of an alternative, would the " git clean -ffxd " command (double f arguments clean untracked directories managed by another repository) resolve the issue without requiring the addition of a new command?
            Hide
            markewaite Mark Waite added a comment - - edited

            I've been trying to understand the sequence of git operations which arrive at that state described in the bug report, and haven't yet understood how to do that. My attempts have persuaded me that I don't understand submodules. I used my fork of the git-client-plugin for my experiments.

            The failing sequence I see currently is:

            • checkout master - ok
            • checkout tests/notSubmodules - ok
            • checkout tests/getSubmodules - ok
            • checkout tests/notSubmodules - fails

            I defined a bash shell function "checkout" for my testing convenience. It is

            checkout ()
            {
                branch=$1;
                git checkout -f $branch;
                git clean -xffd;
                git submodule update --init --recursive;
                git status
            }
            

            Clone the repository - works

            git clone git://github.com/MarkEWaite/git-client-plugin
            cd git-client-plugin
            ls modules/ntp # reports an error, no content in modules/ntp
            

            Checkout tests/notSubmodules - works

            checkout tests/notSubmodules
            

            Checkout tests/getSubmodules - works

            checkout tests/getSubmodules
            

            Checkout tests/notSubmodules - works

            checkout tests/notSubmodules
            

            Checkout tests/getSubmodules - fails

            checkout tests/getSubmodules
            

            The "mostly" working sequence in my case seems to be "force checkout, git clean -xffd, submodule update". That seems to work for all the cases I've tested except the master to notSubmodules to getSubmodules to notSubmodules to getSubmodules case. However, since my case does not duplicate your message, I'm not sure if you'll see the same behavior as I see.

            In that repository, the branch to branch transitions seem to behave like this:

            Current Branch Branch to Checkout Result
            master tests/getSubmodules ok
            master tests/notSubmodules ok
            tests/getSubmodules master ok
            tests/notSubmodules master ok
            tests/getSubmodules tests/notSubmodules fails in some cases
            tests/notSubmodules tests/getSubmodules fails in some cases

            However, the failure message I'm seeing is not the same as the failure message you're seeing. I'm confident that means I don't understand enough about submodules. Can you provide a similar series of steps (preferably using a fork of the git-client-plugin repository) to show your case?

            If you're interested in my first attempt to express that failure mode as a unit test, refer to the proto-submodule-tests branch in my fork of the git-client-plugin source. The test is "not yet ready for prime time".

            Show
            markewaite Mark Waite added a comment - - edited I've been trying to understand the sequence of git operations which arrive at that state described in the bug report, and haven't yet understood how to do that. My attempts have persuaded me that I don't understand submodules. I used my fork of the git-client-plugin for my experiments. The failing sequence I see currently is: checkout master - ok checkout tests/notSubmodules - ok checkout tests/getSubmodules - ok checkout tests/notSubmodules - fails I defined a bash shell function "checkout" for my testing convenience. It is checkout () { branch=$1; git checkout -f $branch; git clean -xffd; git submodule update --init --recursive; git status } Clone the repository - works git clone git://github.com/MarkEWaite/git-client-plugin cd git-client-plugin ls modules/ntp # reports an error, no content in modules/ntp Checkout tests/notSubmodules - works checkout tests/notSubmodules Checkout tests/getSubmodules - works checkout tests/getSubmodules Checkout tests/notSubmodules - works checkout tests/notSubmodules Checkout tests/getSubmodules - fails checkout tests/getSubmodules The "mostly" working sequence in my case seems to be "force checkout, git clean -xffd, submodule update". That seems to work for all the cases I've tested except the master to notSubmodules to getSubmodules to notSubmodules to getSubmodules case. However, since my case does not duplicate your message, I'm not sure if you'll see the same behavior as I see. In that repository, the branch to branch transitions seem to behave like this: Current Branch Branch to Checkout Result master tests/getSubmodules ok master tests/notSubmodules ok tests/getSubmodules master ok tests/notSubmodules master ok tests/getSubmodules tests/notSubmodules fails in some cases tests/notSubmodules tests/getSubmodules fails in some cases However, the failure message I'm seeing is not the same as the failure message you're seeing. I'm confident that means I don't understand enough about submodules. Can you provide a similar series of steps (preferably using a fork of the git-client-plugin repository) to show your case? If you're interested in my first attempt to express that failure mode as a unit test, refer to the proto-submodule-tests branch in my fork of the git-client-plugin source. The test is "not yet ready for prime time".
            Hide
            markewaite Mark Waite added a comment -

            I think git.clean() can't be switched method to always use "clean -xffd" instead of "clean -xfd" because that may damage existing use cases which depend on the current behavior.

            I haven't been able to find any better solution than the solution you've proposed. I agree with your recommendation that we should add "Clean before checkout" as an additional behavior. We'll rely on users to choose which ever of the two best meets the need of their use case.

            I think we'll still need the switch branch of submodule before checkout pull request as well, but I think that "Clean before checkout" is also a needed addition to the git plugin.

            Over the course of the next week, I'll perform some additional tests to better understand the impact of the change.

            Show
            markewaite Mark Waite added a comment - I think git.clean() can't be switched method to always use "clean -xffd" instead of "clean -xfd" because that may damage existing use cases which depend on the current behavior. I haven't been able to find any better solution than the solution you've proposed. I agree with your recommendation that we should add "Clean before checkout" as an additional behavior. We'll rely on users to choose which ever of the two best meets the need of their use case. I think we'll still need the switch branch of submodule before checkout pull request as well, but I think that "Clean before checkout" is also a needed addition to the git plugin. Over the course of the next week, I'll perform some additional tests to better understand the impact of the change.
            Hide
            rsennewald Ray Sennewald added a comment - - edited

            Mark, I appreciate your willingness to help us out here. TBH, I don't feel that my situation is unique to submodules. It appears to me that the problem would arise anytime that you have a conflict of a file between what you have in your working copy, and what your trying to checkout. I'd love to be able to add some tests around this, but have been quite busy at work in other regards lately. My 2 cents are, do we really need clean after checkout? Is there any situation that this behavior is needed, that can't be taken care of by doing a clean before checkout? Perhaps we should change Clean After Checkout to be called Clean Before Checkout, and change its behavior to do what we implemented in clean before checkout? What do you think? I'm more than happy to have David or I help out here.

            I should mention, what worked for us was the correct order of operations. We didn't need a git clean -ffdx, we just needed git clean -fdx and git reset --hard to be executed on all submodules BEFORE we do a checkout on those submodules.

            Show
            rsennewald Ray Sennewald added a comment - - edited Mark, I appreciate your willingness to help us out here. TBH, I don't feel that my situation is unique to submodules. It appears to me that the problem would arise anytime that you have a conflict of a file between what you have in your working copy, and what your trying to checkout. I'd love to be able to add some tests around this, but have been quite busy at work in other regards lately. My 2 cents are, do we really need clean after checkout? Is there any situation that this behavior is needed, that can't be taken care of by doing a clean before checkout? Perhaps we should change Clean After Checkout to be called Clean Before Checkout, and change its behavior to do what we implemented in clean before checkout? What do you think? I'm more than happy to have David or I help out here. I should mention, what worked for us was the correct order of operations. We didn't need a git clean -ffdx, we just needed git clean -fdx and git reset --hard to be executed on all submodules BEFORE we do a checkout on those submodules.
            Hide
            markewaite Mark Waite added a comment -

            I'm somewhat fanatical about backward compatibility, and removing "Clean after checkout" (or mutating it to be "Clean before checkout") would risk all sorts of compatibility problems. The git plugin (and the git client plugin) are two of the most used Jenkins plugins. Many users depend on them to remain compatible from one version to the next, and to retain their old behavior (no matter how odd that behavior may appear).

            I'm glad the "git clean -fdx" was sufficient, since that retains compatibility with the existing code and existing job definitions.

            Show
            markewaite Mark Waite added a comment - I'm somewhat fanatical about backward compatibility, and removing "Clean after checkout" (or mutating it to be "Clean before checkout") would risk all sorts of compatibility problems. The git plugin (and the git client plugin) are two of the most used Jenkins plugins. Many users depend on them to remain compatible from one version to the next, and to retain their old behavior (no matter how odd that behavior may appear). I'm glad the "git clean -fdx" was sufficient, since that retains compatibility with the existing code and existing job definitions.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: David S Wang
            Path:
            src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java
            src/main/resources/hudson/plugins/git/extensions/impl/CleanBeforeCheckout/help.html
            src/test/java/hudson/plugins/git/AbstractGitTestCase.java
            src/test/java/hudson/plugins/git/GitSCMTest.java
            http://jenkins-ci.org/commit/git-plugin/fb12fed4c1c8af11db68ec236961401d3d11eb7b
            Log:
            Files/changes added to allow clean before checkout resolving JENKINS-22510

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: David S Wang Path: src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java src/main/resources/hudson/plugins/git/extensions/impl/CleanBeforeCheckout/help.html src/test/java/hudson/plugins/git/AbstractGitTestCase.java src/test/java/hudson/plugins/git/GitSCMTest.java http://jenkins-ci.org/commit/git-plugin/fb12fed4c1c8af11db68ec236961401d3d11eb7b Log: Files/changes added to allow clean before checkout resolving JENKINS-22510
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Mark Waite
            Path:
            src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java
            src/main/resources/hudson/plugins/git/extensions/impl/CleanBeforeCheckout/help.html
            src/test/java/hudson/plugins/git/AbstractGitTestCase.java
            src/test/java/hudson/plugins/git/GitSCMTest.java
            http://jenkins-ci.org/commit/git-plugin/743d715f21183e473adae4b4d969f3ccf16d3578
            Log:
            Merge branch 'master' of https://github.com/ds2wang/git-plugin

            [Fixed JENKINS-7376] Clean after checkout cleans submodules
            [Fixed JENKINS-13910] Clean operates on submodules
            [Fixed JENKINS-22510] Clean after checkout causes checkout failure

            Compare: https://github.com/jenkinsci/git-plugin/compare/fd2bc21c8951...743d715f2118

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Mark Waite Path: src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java src/main/resources/hudson/plugins/git/extensions/impl/CleanBeforeCheckout/help.html src/test/java/hudson/plugins/git/AbstractGitTestCase.java src/test/java/hudson/plugins/git/GitSCMTest.java http://jenkins-ci.org/commit/git-plugin/743d715f21183e473adae4b4d969f3ccf16d3578 Log: Merge branch 'master' of https://github.com/ds2wang/git-plugin [Fixed JENKINS-7376] Clean after checkout cleans submodules [Fixed JENKINS-13910] Clean operates on submodules [Fixed JENKINS-22510] Clean after checkout causes checkout failure Compare: https://github.com/jenkinsci/git-plugin/compare/fd2bc21c8951...743d715f2118
            Hide
            markewaite Mark Waite added a comment -

            Fix will be available in next release after git plugin 2.2.0

            Show
            markewaite Mark Waite added a comment - Fix will be available in next release after git plugin 2.2.0
            Hide
            mcrooney mcrooney added a comment -

            "Relying on the user to know when they should select "clean before checkout" and when they should "clean after checkout" seems like we're expecting too much from the users."

            Totally, I agree, I ended up having to search on Google and then read through this bug report, to figure out what the difference was, and I'm still not sure I understand what the difference would be using one or the other. The root of this problem is that the help texts for these checkboxes are exactly the same except for the word "before" and "after" I'm happy to file an issue for improving the help texts if that seems reasonable?

            Show
            mcrooney mcrooney added a comment - "Relying on the user to know when they should select "clean before checkout" and when they should "clean after checkout" seems like we're expecting too much from the users." Totally, I agree, I ended up having to search on Google and then read through this bug report, to figure out what the difference was, and I'm still not sure I understand what the difference would be using one or the other. The root of this problem is that the help texts for these checkboxes are exactly the same except for the word "before" and "after" I'm happy to file an issue for improving the help texts if that seems reasonable?
            Hide
            markewaite Mark Waite added a comment -

            That would be fine. I'm not sure how you'll make the help text describe the things you have learned, but you'll make my life easier evaluating your proposed changes if you submit your proposed change as a pull request. You can do that by:

            Show
            markewaite Mark Waite added a comment - That would be fine. I'm not sure how you'll make the help text describe the things you have learned, but you'll make my life easier evaluating your proposed changes if you submit your proposed change as a pull request. You can do that by: Create a free GitHub account for yourself Fork the git plugin repository with the "Fork" button on the left of the repo Edit the help text (from the web interface is easy enough) in your forked copy of the repository ( https://github.com/YourUserNameGoesHere/git-plugin/blob/master/src/main/resources/hudson/plugins/git/extensions/impl/CleanBeforeCheckout/help.html and https://github.com/YourUserNameGoesHere/git-plugin/blob/master/src/main/resources/hudson/plugins/git/extensions/impl/CleanCheckout/help.html ) Save your changes (from the web Submit a pull request (using the GitHub submit pull request from the web)
            Hide
            mcrooney mcrooney added a comment -

            Thanks Mark! I guess my takeaway from this is: "clean after was originally the only cleaning behavior, is the only one offered by job-dsl, and should generally work. Use clean before if clean after isn't working for you." Though, that's pretty ambiguous. I still don't totally understand myself, except that that understanding was enough for me to decide to use "after". What does "after" solve that "before" doesn't? Is "before" strictly better?

            Show
            mcrooney mcrooney added a comment - Thanks Mark! I guess my takeaway from this is: "clean after was originally the only cleaning behavior, is the only one offered by job-dsl, and should generally work. Use clean before if clean after isn't working for you." Though, that's pretty ambiguous. I still don't totally understand myself, except that that understanding was enough for me to decide to use "after". What does "after" solve that "before" doesn't? Is "before" strictly better?
            Hide
            markewaite Mark Waite added a comment -

            One is not better than the other (as far as I can tell). You're correct that "Clean after checkout" was the first clean behavior, and was implemented very early in the development of the plugin.

            The specific benefit sought by the original submitter of the bug report was to better handle clean after a recursive submodule checkout had left the repository in a "dirty" state. In the submodule case, performing the clean before the checkout seems to have a better chance of succeeding than performing the clean after the checkout. I assume one of the reasons for a better chance of success is that the recursive submodule checkout may fail due to the "dirty" state of the working directory, which then might prevent the "clean after" from even being executed.

            I assume the job DSL includes one and not the other because it was initially created when "clean after" was the only option available.

            Show
            markewaite Mark Waite added a comment - One is not better than the other (as far as I can tell). You're correct that "Clean after checkout" was the first clean behavior, and was implemented very early in the development of the plugin. The specific benefit sought by the original submitter of the bug report was to better handle clean after a recursive submodule checkout had left the repository in a "dirty" state. In the submodule case, performing the clean before the checkout seems to have a better chance of succeeding than performing the clean after the checkout. I assume one of the reasons for a better chance of success is that the recursive submodule checkout may fail due to the "dirty" state of the working directory, which then might prevent the "clean after" from even being executed. I assume the job DSL includes one and not the other because it was initially created when "clean after" was the only option available.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Mark Waite
            Path:
            src/test/java/org/jenkinsci/plugins/gitclient/GitAPITestCase.java
            http://jenkins-ci.org/commit/git-client-plugin/eec1a16bc7ddff1ffd022f5c9e5f631f1a63f329
            Log:
            Add two detailed submodule tests

            Tests describe the implementation as it currently exists, in hopes
            of detecting future regressions with test execution. The tests show
            inconsistencies between the CliGitAPIImpl and JGitAPIImpl classes,
            and inconsistencies between command line git and JGit behavior.

            Command line git clean as implemented in CliGitAPIImpl does not remove
            untracked submodules or files contained in untracked submodule dirs.
            JGit clean as implemented in JGitAPIImpl removes untracked submodules.
            This test captures that surprising difference between the implementations.

            CliGitAPIImpl supports renamed submodules. JGitAPIImpl does not support
            renamed submodules. One of these tests captures that difference.

            See bug reports such as:
            JENKINS-22510 - Clean After Checkout Results in Failed to Checkout Revision
            JENKINS-8053 - Git submodules are cloned too early and not removed once the revToBuild has been checked out
            JENKINS-14083 - Build can't recover from broken submodule path
            JENKINS-15399 - Changing remote URL doesn't update submodules

            Compare: https://github.com/jenkinsci/git-client-plugin/compare/cdcbfd56f49f...eec1a16bc7dd

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Mark Waite Path: src/test/java/org/jenkinsci/plugins/gitclient/GitAPITestCase.java http://jenkins-ci.org/commit/git-client-plugin/eec1a16bc7ddff1ffd022f5c9e5f631f1a63f329 Log: Add two detailed submodule tests Tests describe the implementation as it currently exists, in hopes of detecting future regressions with test execution. The tests show inconsistencies between the CliGitAPIImpl and JGitAPIImpl classes, and inconsistencies between command line git and JGit behavior. Command line git clean as implemented in CliGitAPIImpl does not remove untracked submodules or files contained in untracked submodule dirs. JGit clean as implemented in JGitAPIImpl removes untracked submodules. This test captures that surprising difference between the implementations. CliGitAPIImpl supports renamed submodules. JGitAPIImpl does not support renamed submodules. One of these tests captures that difference. See bug reports such as: JENKINS-22510 - Clean After Checkout Results in Failed to Checkout Revision JENKINS-8053 - Git submodules are cloned too early and not removed once the revToBuild has been checked out JENKINS-14083 - Build can't recover from broken submodule path JENKINS-15399 - Changing remote URL doesn't update submodules Compare: https://github.com/jenkinsci/git-client-plugin/compare/cdcbfd56f49f...eec1a16bc7dd

              People

              Assignee:
              ndeloof Nicolas De Loof
              Reporter:
              rsennewald Ray Sennewald
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: