Fix both working_directory and commit_with_message functions.
Created attachment 181878 [details] Patch
tansell@tansell-z620-l:/fast/chrome/src/third_party/WebKit/Tools/Scripts$ ./test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitSVNTest Suppressing most webkitpy logging while running unit tests. [12/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_git_commit erred: Traceback (most recent call last): File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1205, in test_commit_with_message_git_commit commit_text = self.scm.commit_with_message("another test commit", git_commit="HEAD^") File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 364, in commit_with_message raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.") ScriptError: Working copy is modified. Cannot commit individual git_commits. [18/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_git_commit_range erred: Traceback (most recent call last): File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1215, in test_commit_with_message_git_commit_range commit_text = self.scm.commit_with_message("another test commit", git_commit="HEAD~2..HEAD") File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 364, in commit_with_message raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.") ScriptError: Working copy is modified. Cannot commit individual git_commits. [28/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_only_local_commit erred: Traceback (most recent call last): File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1225, in test_commit_with_message_only_local_commit commit_text = self.scm.commit_with_message("another test commit") File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 368, in commit_with_message self._assert_can_squash(working_directory_is_clean) File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 345, in _assert_can_squash raise AmbiguousCommitError(num_local_commits, working_directory_is_clean) AmbiguousCommitError Ran 63 tests in 34.061s FAILED (failures=0, errors=3) after $ ./test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitSVNTest Suppressing most webkitpy logging while running unit tests. Ran 63 tests in 36.501s OK
Comment on attachment 181878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review > Tools/Scripts/webkitpy/common/checkout/scm/git.py:152 > - return self._run_git(['diff', self.remote_merge_base(), '--no-renames', '--name-only']) == "" > + return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == "" Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted). You'd have to ask dpranke.
If I understand this correctly the following changes have occured. The working_directory_is_clean and clean_working_directory functions now deal with the normal meaning of "working directory", IE stuff which has yet to be committed. The commit_with_message function has been modify so it does one of the following options (which is what the tests are currently testing for): If you give it a commit range, then it'll dcommit the git commits referred too. If you have asked it to squash (via either the force_squash or the webkit-patch.commit-should-always-squash config) then it will squash all yet to dcommitted git commits and anything in the working directory together, then dcommit the result. Otherwise, it will check there is only a single thing to dcommit, either a single git commit, or stuff in the working directory, and dcommit that. Does that match what you actually want it to do? Tim
(In reply to comment #4) > If I understand this correctly the following changes have occured. > > The working_directory_is_clean and clean_working_directory functions now deal with the normal meaning of "working directory", IE stuff which has yet to be committed. To be clear, I actually mean changes which have yet to be committed to EITHER subversion OR git.
Comment on attachment 181878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152 >> + return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == "" > > Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted). You'd have to ask dpranke. Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change.
Comment on attachment 181878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review >>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152 >>> + return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == "" >> >> Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted). You'd have to ask dpranke. > > Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change. Yeah, the problem is that if we have local commits (i.e., HEAD points to something we've committed locally), then clean_working_directory will leave those commits around instead of blowing them away. This happens all the time on the commit-queue because the way it lands patches is to first commit them locally and then dcommit them to svn. If the dcommit fails, it will have the local commit. If clean doesn't blow the local commit away, it will bork the queue until it does its big working copy reset.
Comment on attachment 181878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review >>>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152 >>>> + return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == "" >>> >>> Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted). You'd have to ask dpranke. >> >> Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change. > > Yeah, the problem is that if we have local commits (i.e., HEAD points to something we've committed locally), then clean_working_directory will leave those commits around instead of blowing them away. > > This happens all the time on the commit-queue because the way it lands patches is to first commit them locally and then dcommit them to svn. If the dcommit fails, it will have the local commit. If clean doesn't blow the local commit away, it will bork the queue until it does its big working copy reset. Your change is redefining "working_directory" to be something very unexpected (at least for experienced git users). It seems like you also want a "reset to remote_merge_base" type thing?
Yes, the SCM API name should likely change. SCM was "designed" to be an abstraction around the current checkout's scm system, and is thus a strange compromise between svn and git.
(In reply to comment #9) > Yes, the SCM API name should likely change. SCM was "designed" to be an abstraction around the current checkout's scm system, and is thus a strange compromise between svn and git. You already have the ability to discard local commits using Git.discard_local_commits? Maybe we should change the caller to do a "if self.supports_local_commits(): self.discard_local_commits()"?
You should probably check out the previous discussion in bug 104032 (and bug 104198 that we filed about the fact that the method name is now confusing and/or wrong ;).
The right path forward I think is to revert 104032 and use the following to functions; def ensure_clean_working_directory(self, force_clean): if self.working_directory_is_clean(): return if not force_clean: print self.run(self.status_command(), error_handler=Executive.ignore_error, cwd=self.checkout_root) raise ScriptError(message="Working directory has modifications, pass --force-clean or --no-clean to continue.") _log.info("Cleaning working directory") self.clean_working_directory() def ensure_no_local_commits(self, force): if not self.supports_local_commits(): return commits = self.local_commits() if not len(commits): return if not force: _log.error("Working directory has local commits, pass --force-clean to continue.") sys.exit(1) #### WHY!? Oh god why!? self.discard_local_commits() ??
Scripts/webkitpy/tool/steps/cleanworkingdirectory.py then should be change to something like "resettoupstream" or something?
Created attachment 182099 [details] Patch
Comment on attachment 182099 [details] Patch Won't this patch break the commit-queue as described above?
I don't really understand what problem you're trying to solve. "Fix both working_directory and commit_with_message functions" doesn't mean anything to me and your change log does not provide any additional information.
(In reply to comment #16) > I don't really understand what problem you're trying to solve. "Fix both working_directory and commit_with_message functions" doesn't mean anything to me and your change log does not provide any additional information. I think the subject refers to the failing tests and then the belief that the working_directory_is_clean() function was doing the wrong thing.
Perhaps we should retitle the bug them? These functions are working as intended (even if they could benefit from more git-centric names).
sure :).
Created attachment 183099 [details] Patch
Comment on attachment 183099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183099&action=review OK. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:129 > + executive = executive or Executive() This is still a FIXME, but way better, thank you! > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1551 > + maxDiff = None Was this intentional?
Created attachment 183346 [details] Patch
Comment on attachment 183346 [details] Patch Clearing flags on attachment: 183346 Committed r140089: <http://trac.webkit.org/changeset/140089>
All reviewed patches have been landed. Closing bug.