Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183283003:
Another attempt: gclient: delete mismatching checkouts (Closed)
|
Created:
6 years, 9 months ago by borenet Modified:
6 years, 9 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org, skiabot_google.com Visibility:
Public. |
DescriptionAnother attempt: gclient: delete mismatching checkouts
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=255930
Patch Set 1 : Re-land https://codereview.chromium.org/164053003 #Patch Set 2 : More minimal approach #
Total comments: 2
Patch Set 3 : Add GitWrapper tests for conflicting directories #
Total comments: 1
Patch Set 4 : Update smoketests: remove git-svn test, add Skia DEPS transition test #Patch Set 5 : Remove some unnecessary changes #
Total comments: 7
Patch Set 6 : Let existing logic handle deletion in SVN update() #Patch Set 7 : retry upload #Patch Set 8 : Remove IsGit #Patch Set 9 : retry upload #
Total comments: 2
Patch Set 10 : Use parens #Patch Set 11 : rebase #
Total comments: 7
Patch Set 12 : Address iannucci's comments #
Total comments: 2
Patch Set 13 : Fix for unmanaged git-svn, remove commented lines #
Messages
Total messages: 27 (0 generated)
Here's another try. Probably need to add a gclient_scm_test for the git version of the deletion logic and modify some of the smoketests. https://codereview.chromium.org/183283003/diff/20001/tests/gclient_smoketest.py File tests/gclient_smoketest.py (right): https://codereview.chromium.org/183283003/diff/20001/tests/gclient_smoketest.... tests/gclient_smoketest.py:783: def testSkipGitSvn(self): This test probably isn't needed. https://codereview.chromium.org/183283003/diff/20001/tests/gclient_smoketest.... tests/gclient_smoketest.py:1453: def testDeleteConflictingCheckout(self): This test probably isn't needed. It should maybe be replaced with something that simulates the Skia DEPS transition?
Patch set 3 adds tests for the relevant cases in GitWrapper.update. I'm going to modify the smoketests as suggested in my own comments: - Remove the "skip git-svn" smoketest as that case is now covered by a gclient_scm test. - Replace the "testDeleteConflictingCheckout" smoketest with one (or two) which simulates the behavior of bisect bots across the Skia DEPS transition. https://codereview.chromium.org/183283003/diff/40001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/183283003/diff/40001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1313: #gclient_scm.scm.GIT.Capture(['--version'], '.').AndReturn('version 1.6.6') I'm really confused by this; when I run all of the tests together (using "python tests/gclient_scm_test.py") these tests succeed as written. However, when I run them by themselves (using "python tests/gclient_scm_test.py ManagedGitWrapperTestCaseMox.testUpdateNoDotGit") they fail unless I uncomment these lines. I can't see why the behavior would change...
Patch set 5 adds a smoketest for the Skia DEPS transition. I think this is ready for review. https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode357 gclient_scm.py:357: if (current_url.rstrip('/') != url.rstrip('/') and The smoketest was behaving incorrectly because of a trailing '/'.
https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): Isn't the scm.GIT.IsGit check redundant? Can't you just use scm.GIT.IsGitSvn? And isn't 'exists' redundant too? I think you could squash this down to: if managed and scm.GIT.IsGitSvn(self.checkout_path): if svn url is unchanged: return else: delete checkout
https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): On 2014/02/28 22:17:52, szager1 wrote: > Isn't the scm.GIT.IsGit check redundant? Can't you just use scm.GIT.IsGitSvn? > And isn't 'exists' redundant too? > > I think you could squash this down to: > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > if svn url is unchanged: > return > else: > delete checkout Not quite. As written, the checkout is deleted if it's a git-svn checkout of the wrong repo OR if it's a pure-git checkout. The change you suggest would cause the checkout only to be deleted if it's a git-svn checkout of the wrong repo. The pure-git case is particularly important for the Skia DEPS transition.
https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): On 2014/03/03 18:31:11, borenet wrote: > On 2014/02/28 22:17:52, szager1 wrote: > > Isn't the scm.GIT.IsGit check redundant? Can't you just use scm.GIT.IsGitSvn? > > > And isn't 'exists' redundant too? > > > > I think you could squash this down to: > > > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > > if svn url is unchanged: > > return > > else: > > delete checkout > > Not quite. As written, the checkout is deleted if it's a git-svn checkout of > the wrong repo OR if it's a pure-git checkout. The change you suggest would > cause the checkout only to be deleted if it's a git-svn checkout of the wrong > repo. The pure-git case is particularly important for the Skia DEPS transition. But wouldn't the next stanza take care of the pure-git case?
https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): On 2014/03/03 18:50:40, szager1 wrote: > On 2014/03/03 18:31:11, borenet wrote: > > On 2014/02/28 22:17:52, szager1 wrote: > > > Isn't the scm.GIT.IsGit check redundant? Can't you just use > scm.GIT.IsGitSvn? > > > > > And isn't 'exists' redundant too? > > > > > > I think you could squash this down to: > > > > > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > > > if svn url is unchanged: > > > return > > > else: > > > delete checkout > > > > Not quite. As written, the checkout is deleted if it's a git-svn checkout of > > the wrong repo OR if it's a pure-git checkout. The change you suggest would > > cause the checkout only to be deleted if it's a git-svn checkout of the wrong > > repo. The pure-git case is particularly important for the Skia DEPS > transition. > > But wouldn't the next stanza take care of the pure-git case? > Ah, I see... the next stanza relies on options.reset and options.delete_unversioned_trees. I would prefer it if you could use that existing code to remove pure git checkouts, and align your flags with the existing logic.
https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): On 2014/03/03 18:54:20, szager1 wrote: > On 2014/03/03 18:50:40, szager1 wrote: > > On 2014/03/03 18:31:11, borenet wrote: > > > On 2014/02/28 22:17:52, szager1 wrote: > > > > Isn't the scm.GIT.IsGit check redundant? Can't you just use > > scm.GIT.IsGitSvn? > > > > > > > And isn't 'exists' redundant too? > > > > > > > > I think you could squash this down to: > > > > > > > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > > > > if svn url is unchanged: > > > > return > > > > else: > > > > delete checkout > > > > > > Not quite. As written, the checkout is deleted if it's a git-svn checkout > of > > > the wrong repo OR if it's a pure-git checkout. The change you suggest would > > > cause the checkout only to be deleted if it's a git-svn checkout of the > wrong > > > repo. The pure-git case is particularly important for the Skia DEPS > > transition. > > > > But wouldn't the next stanza take care of the pure-git case? > > > > Ah, I see... the next stanza relies on options.reset and > options.delete_unversioned_trees. I would prefer it if you could use that > existing code to remove pure git checkouts, and align your flags with the > existing logic. Okay, I can remove everything but the early return here. Assuming that the bots run with --reset, this will still work. Can I add a --force clause to the below as well? if (options.reset and options.delete_unversioned trees) or options.force: ?
On 2014/03/03 19:14:21, borenet wrote: > https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py > File gclient_scm.py (right): > > https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 > gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): > On 2014/03/03 18:54:20, szager1 wrote: > > On 2014/03/03 18:50:40, szager1 wrote: > > > On 2014/03/03 18:31:11, borenet wrote: > > > > On 2014/02/28 22:17:52, szager1 wrote: > > > > > Isn't the scm.GIT.IsGit check redundant? Can't you just use > > > scm.GIT.IsGitSvn? > > > > > > > > > And isn't 'exists' redundant too? > > > > > > > > > > I think you could squash this down to: > > > > > > > > > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > > > > > if svn url is unchanged: > > > > > return > > > > > else: > > > > > delete checkout > > > > > > > > Not quite. As written, the checkout is deleted if it's a git-svn checkout > > of > > > > the wrong repo OR if it's a pure-git checkout. The change you suggest > would > > > > cause the checkout only to be deleted if it's a git-svn checkout of the > > wrong > > > > repo. The pure-git case is particularly important for the Skia DEPS > > > transition. > > > > > > But wouldn't the next stanza take care of the pure-git case? > > > > > > > Ah, I see... the next stanza relies on options.reset and > > options.delete_unversioned_trees. I would prefer it if you could use that > > existing code to remove pure git checkouts, and align your flags with the > > existing logic. > > Okay, I can remove everything but the early return here. Assuming that the bots > run with --reset, this will still work. Can I add a --force clause to the below > as well? > > if (options.reset and options.delete_unversioned trees) or options.force: > > ? Yes, that sounds good to me.
Uploaded patch sets 6-9. https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/80001/gclient_scm.py#newcode1156 gclient_scm.py:1156: if scm.GIT.IsGit(self.checkout_path): On 2014/03/03 19:14:22, borenet wrote: > On 2014/03/03 18:54:20, szager1 wrote: > > On 2014/03/03 18:50:40, szager1 wrote: > > > On 2014/03/03 18:31:11, borenet wrote: > > > > On 2014/02/28 22:17:52, szager1 wrote: > > > > > Isn't the scm.GIT.IsGit check redundant? Can't you just use > > > scm.GIT.IsGitSvn? > > > > > > > > > And isn't 'exists' redundant too? > > > > > > > > > > I think you could squash this down to: > > > > > > > > > > if managed and scm.GIT.IsGitSvn(self.checkout_path): > > > > > if svn url is unchanged: > > > > > return > > > > > else: > > > > > delete checkout > > > > > > > > Not quite. As written, the checkout is deleted if it's a git-svn checkout > > of > > > > the wrong repo OR if it's a pure-git checkout. The change you suggest > would > > > > cause the checkout only to be deleted if it's a git-svn checkout of the > > wrong > > > > repo. The pure-git case is particularly important for the Skia DEPS > > > transition. > > > > > > But wouldn't the next stanza take care of the pure-git case? > > > > > > > Ah, I see... the next stanza relies on options.reset and > > options.delete_unversioned_trees. I would prefer it if you could use that > > existing code to remove pure git checkouts, and align your flags with the > > existing logic. > > Okay, I can remove everything but the early return here. Assuming that the bots > run with --reset, this will still work. Can I add a --force clause to the below > as well? > > if (options.reset and options.delete_unversioned trees) or options.force: > > ? Done.
lgtm with one final nit. https://codereview.chromium.org/183283003/diff/150001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/150001/gclient_scm.py#newcode1171 gclient_scm.py:1171: if options.force or options.reset and options.delete_unversioned_trees: Please add parentheses for grouping, don't rely on operator precedence rules.
Uploaded patch set 10. https://codereview.chromium.org/183283003/diff/150001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/150001/gclient_scm.py#newcode1171 gclient_scm.py:1171: if options.force or options.reset and options.delete_unversioned_trees: On 2014/03/03 21:20:17, szager1 wrote: > Please add parentheses for grouping, don't rely on operator precedence rules. Done.
At Chase's suggestion, adding mmoss@ since I fear gclient's potential for dealing damage.
omg, gclient is so painful :( https://chromiumcodereview.appspot.com/183283003/diff/190001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/183283003/diff/190001/gclient_scm.py#n... gclient_scm.py:333: return self._Capture(['rev-parse', '--verify', 'HEAD']) I really dislike this pattern in gclient (although it's very prevalent). Can't this logic move into the 'if not exists' block above? if not exists(path) or if not exists(path / .git): if not exists(path / .git): if force: rm_rf(path) else: raise Error() # clone fresh It would remove the duplicated contents of this block, which is what I'm worried about.
https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode333 gclient_scm.py:333: return self._Capture(['rev-parse', '--verify', 'HEAD']) On 2014/03/05 22:04:07, iannucci wrote: > I really dislike this pattern in gclient (although it's very prevalent). Can't > this logic move into the 'if not exists' block above? > > if not exists(path) or if not exists(path / .git): > if not exists(path / .git): > if force: > rm_rf(path) > else: > raise Error() > # clone fresh > > It would remove the duplicated contents of this block, which is what I'm worried > about. Done. It seems like this whole block is not needed, right? The above verifies that there's a .git directory and either clones or errors our, so we shouldn't get to this point if there's no .git directory. https://codereview.chromium.org/183283003/diff/200001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/183283003/diff/200001/tests/gclient_scm_test.... tests/gclient_scm_test.py:1262: #gclient_scm.scm.GIT.Capture(['--version'], '.').AndReturn('version 1.6.6') I'm still confused about these lines. The tests fail if I uncomment them and run: $ python tests/gclient_scm_test.py But they fail *unless* I uncomment them when I run: $ python tests/gclient_scm_test.py ManagedGitWrapperTestCaseMox.testUpdateNoDotGit Thoughts?
https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode333 gclient_scm.py:333: return self._Capture(['rev-parse', '--verify', 'HEAD']) On 2014/03/06 14:27:19, borenet wrote: > On 2014/03/05 22:04:07, iannucci wrote: > > I really dislike this pattern in gclient (although it's very prevalent). Can't > > this logic move into the 'if not exists' block above? > > > > if not exists(path) or if not exists(path / .git): > > if not exists(path / .git): > > if force: > > rm_rf(path) > > else: > > raise Error() > > # clone fresh > > > > It would remove the duplicated contents of this block, which is what I'm > worried > > about. > > Done. It seems like this whole block is not needed, right? The above verifies > that there's a .git directory and either clones or errors our, so we shouldn't > get to this point if there's no .git directory. I'm a little concerned about adding this logic above the early return for unmanaged checkouts. This will cause an unexpected error if, for example, .gclient has an unmanaged git checkout listed but there's actually an SVN checkout sitting there. That's a bizarre case, but it's roughly equivalent to one we've seen before.
lgtm https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode333 gclient_scm.py:333: return self._Capture(['rev-parse', '--verify', 'HEAD']) On 2014/03/06 18:04:49, borenet wrote: > On 2014/03/06 14:27:19, borenet wrote: > > On 2014/03/05 22:04:07, iannucci wrote: > > > I really dislike this pattern in gclient (although it's very prevalent). > Can't > > > this logic move into the 'if not exists' block above? > > > > > > if not exists(path) or if not exists(path / .git): > > > if not exists(path / .git): > > > if force: > > > rm_rf(path) > > > else: > > > raise Error() > > > # clone fresh > > > > > > It would remove the duplicated contents of this block, which is what I'm > > worried > > > about. > > > > Done. It seems like this whole block is not needed, right? The above verifies > > that there's a .git directory and either clones or errors our, so we shouldn't > > get to this point if there's no .git directory. > > I'm a little concerned about adding this logic above the early return for > unmanaged checkouts. This will cause an unexpected error if, for example, > .gclient has an unmanaged git checkout listed but there's actually an SVN > checkout sitting there. That's a bizarre case, but it's roughly equivalent to > one we've seen before. Hm... Couldn't we just check the managed boolean above as well? Though maybe it's ok because we're hiding behind --force? I also think that an unmanaged gclient svn checkout is HIGHLY unlikely. https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode1058 gclient_scm.py:1058: return # TODO(borenet): Get the svn revision number? Can you try this scenario with --json-output? It /should/ work, but I'm not sure.
Uploaded patch set 13 https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode333 gclient_scm.py:333: return self._Capture(['rev-parse', '--verify', 'HEAD']) On 2014/03/07 20:31:19, iannucci wrote: > On 2014/03/06 18:04:49, borenet wrote: > > On 2014/03/06 14:27:19, borenet wrote: > > > On 2014/03/05 22:04:07, iannucci wrote: > > > > I really dislike this pattern in gclient (although it's very prevalent). > > Can't > > > > this logic move into the 'if not exists' block above? > > > > > > > > if not exists(path) or if not exists(path / .git): > > > > if not exists(path / .git): > > > > if force: > > > > rm_rf(path) > > > > else: > > > > raise Error() > > > > # clone fresh > > > > > > > > It would remove the duplicated contents of this block, which is what I'm > > > worried > > > > about. > > > > > > Done. It seems like this whole block is not needed, right? The above > verifies > > > that there's a .git directory and either clones or errors our, so we > shouldn't > > > get to this point if there's no .git directory. > > > > I'm a little concerned about adding this logic above the early return for > > unmanaged checkouts. This will cause an unexpected error if, for example, > > .gclient has an unmanaged git checkout listed but there's actually an SVN > > checkout sitting there. That's a bizarre case, but it's roughly equivalent to > > one we've seen before. > > Hm... Couldn't we just check the managed boolean above as well? Though maybe > it's ok because we're hiding behind --force? > > I also think that an unmanaged gclient svn checkout is HIGHLY unlikely. I think you're right, but I've been surprised before :) At any rate, the early return itself is going to fail if there's an unmanaged, non-git checkout here, due to the use of "git rev-parse --verify HEAD". So I'm no longer worried about it. https://codereview.chromium.org/183283003/diff/190001/gclient_scm.py#newcode1058 gclient_scm.py:1058: return # TODO(borenet): Get the svn revision number? On 2014/03/07 20:31:19, iannucci wrote: > Can you try this scenario with --json-output? It /should/ work, but I'm not > sure. I tried this with a checkout of skia: $ git svn clone -r 13710 https://skia.googlecode.com/svn/trunk $ /hacked/gclient config https://skia.googlecode.com/svn/trunk --unmanaged $ /hacked/gclient sync --output-json json_output.txt (succeeds) https://codereview.chromium.org/183283003/diff/200001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/183283003/diff/200001/tests/gclient_scm_test.... tests/gclient_scm_test.py:1262: #gclient_scm.scm.GIT.Capture(['--version'], '.').AndReturn('version 1.6.6') On 2014/03/06 14:27:19, borenet wrote: > I'm still confused about these lines. The tests fail if I uncomment them and > run: > $ python tests/gclient_scm_test.py > > But they fail *unless* I uncomment them when I run: > $ python tests/gclient_scm_test.py > ManagedGitWrapperTestCaseMox.testUpdateNoDotGit > > Thoughts? Removed these lines in a subsequent patch set.
OK, lgtm. You'll land on monday 8 AM EST and watch the trees ?
On 2014/03/07 22:00:34, iannucci wrote: > OK, lgtm. You'll land on monday 8 AM EST and watch the trees ? Yes, sounds good to me.
Peanut gallery: Seems like it'd be easier to land this during off-hours (e.g. the weekend).
On 2014/03/07 22:27:51, DaleCurtis wrote: > Peanut gallery: Seems like it'd be easier to land this during off-hours (e.g. > the weekend). Yeah, it would, but there's also low enough traffic that it could break stuff and it would just be broken for the whole weekend. Early morning EST seems like a good mix of traffic + ability revert on breakage.
The CQ bit was checked by borenet@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/borenet@google.com/183283003/220001
Message was sent while issue was closed.
Change committed as 255930
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/192323006/ by thakis@chromium.org. The reason for reverting is: Broke `gclient sync` for me, failing with: Error: 1> Can't update/checkout /Volumes/MacintoshHD2/src/chrome-git/src if an unversioned directory is present. Delete the directory and try again. For someone else, it broke it with: % gclient sync ________ unmanaged solution; skipping src Error: Command svn info --xml returned non-zero exit status 1 in /Users/pawliger/chromium/src/. <?xml version="1.0" encoding="UTF-8"?> <info> svn: E155007: '/Users/pawliger/chromium/src' is not a working copy. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
