Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8382030:
depot_tools: Add git svn find-rev for safesync_url parsing (commonly LKGR link). (Closed)
Description[depot_tools] Adding safesync_url for git and git-svn checkouts.
R=maruel@chromium.org
TEST=Configure a checkout using the NewGitWorkflow with a safesync_url and everything works (though possibly with a really long git svn fetch time).
BUG=106015
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115011
Patch Set 1 : '' #
Total comments: 2
Patch Set 2 : review comments, still need to add test #
Total comments: 14
Patch Set 3 : '' #
Total comments: 24
Patch Set 4 : code review changes #
Total comments: 26
Patch Set 5 : yep, you guessed it - more code review changes! 😃 #Patch Set 6 : updating local copy #Patch Set 7 : adding git and git-svn integration tests #
Total comments: 10
Patch Set 8 : maruel's review comments on first round of tests #Patch Set 9 : adding scm.SVN.IsValidRevision test #Patch Set 10 : adding SVN version of GetUsableRev test + logic fix #Patch Set 11 : adding GetUsableRev tests for git/git-svn #Patch Set 12 : changing more occurrences of SVN to Svn #
Messages
Total messages: 33 (0 generated)
NOTE: pylint passed but presubmit unit tests didn't. DOUBLENOTE: I suck at Python and this is fragile (as it requires git-svn to be set up like in http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout). However, I think it'd be really useful for me to be able to use a safesync_url with .DEPS.git (new git workflow) - maybe others agree?
On 2011/10/25 08:26:36, Dan Beam wrote: > NOTE: pylint passed but presubmit unit tests didn't. What OS? I'm trying to get the tests to work again on Windows. They should pass on linux. > DOUBLENOTE: I suck at Python and this is fragile (as it requires git-svn to be > set up like in > http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout). However, > I think it'd be really useful for me to be able to use a safesync_url with > .DEPS.git (new git workflow) - maybe others agree? I think this is valuable, but the implementation needs a test.
http://codereview.chromium.org/8382030/diff/2001/gclient.py File gclient.py (right): http://codereview.chromium.org/8382030/diff/2001/gclient.py#newcode928 gclient.py:928: svn_rev = scm.Sha1FromSvnRevision(rev) What happens when upstream is git and not git-svn? http://codereview.chromium.org/8382030/diff/2001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/2001/gclient_scm.py#newcode487 gclient_scm.py:487: return self._Capture(['svn', 'find-rev', 'r' + rev]).split('\n')[-1] return self._Capture(['svn', 'find-rev', 'r' + rev]).splitlines()[-1] I feel like this should go in scm.py but gclient.py doesn't use this module directly.
On 2011/10/25 13:10:58, Marc-Antoine Ruel wrote: > On 2011/10/25 08:26:36, Dan Beam wrote: > > NOTE: pylint passed but presubmit unit tests didn't. > > What OS? I'm trying to get the tests to work again on Windows. They should pass > on linux. Ubuntu 11.10 > > DOUBLENOTE: I suck at Python and this is fragile (as it requires git-svn to be > > set up like in > > http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout). > However, > > I think it'd be really useful for me to be able to use a safesync_url with > > .DEPS.git (new git workflow) - maybe others agree? > > I think this is valuable, but the implementation needs a test. I agree. We could probably test this by creating a fake git-svn checkout.
On 2011/10/25 13:11:14, Marc-Antoine Ruel wrote: > http://codereview.chromium.org/8382030/diff/2001/gclient.py > File gclient.py (right): > > http://codereview.chromium.org/8382030/diff/2001/gclient.py#newcode928 > gclient.py:928: svn_rev = scm.Sha1FromSvnRevision(rev) > What happens when upstream is git and not git-svn? This is a current assumption, otherwise I'd assume git svn find-rev rXXXX would probably fail. [Update:] Yep, it does with this message Migrating from a git-svn v1 layout... Data from a previous version of git-svn exists, but .git/svn (required for this version (1.7.3.1) of git-svn) does not exist. Done migrating from a git-svn v1 layout Unable to determine upstream SVN information from HEAD history [/Update] I could probably check for this output and yell at a user to set up their git-svn remote correctly, if you'd like. I will not that at the moment gclient sync with a safesync_url just fails, so in the words of the adage, "Something's better than nothing" (maybe). > http://codereview.chromium.org/8382030/diff/2001/gclient_scm.py > File gclient_scm.py (right): > > http://codereview.chromium.org/8382030/diff/2001/gclient_scm.py#newcode487 > gclient_scm.py:487: return self._Capture(['svn', 'find-rev', 'r' + > rev]).split('\n')[-1] > return self._Capture(['svn', 'find-rev', 'r' + rev]).splitlines()[-1] Will do tonight when I get back home. > I feel like this should go in scm.py but gclient.py doesn't use this module > directly. I'm not really familiar with the layout of depot_tools so I'll move it to wherever you want me to move it (if here's not a good place).
On 2011/10/25 23:06:54, Dan Beam wrote: > I could probably check for this output and yell at a user to set up their > git-svn remote correctly, if you'd like. git config --get svn-remote.svn.url is not empty? > > I feel like this should go in scm.py but gclient.py doesn't use this module > > directly. > > I'm not really familiar with the layout of depot_tools so I'll move it to > wherever you want me to move it (if here's not a good place). Add the function to scm.py and don't create a GitWrapper, just use the scm.GIT.Sha1FromSvnRevision() directly. It'd be nice if it could be abstracted nicely, I'm not sure.
http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode480 gclient_scm.py:480: # If this doesn't look like a Subversion revision (just numbers), pass it Please add a proper docstring. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode482 gclient_scm.py:482: if not rev.isdigit(): Also look for "or len(rev) > 7" We'll revision once we have more than 9999999 revisions. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode492 gclient_scm.py:492: return '' No, '01234567' could still be a valid short hash. It would be nice to verify the hash is valid. Something like git rev-parse $(rev) should return a valid hash. In any case, silently dropping the value is not a valid option. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode1008 gclient_scm.py:1008: def FindSafesyncRev(self, rev): Make it a @staticmethod instead of disabling the warning. Also, just return rev without touching it. It could be a {DATE}, BASE, HEAD, COMMITTED, PREV, or god who knows. You could verify that the revision is valid with svn info -r $(rev) $(url) but I don't think it's useful to do a network request at this point. http://codereview.chromium.org/8382030/diff/14003/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode391 scm.py:391: return '' return None in case of failure. http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode393 scm.py:393: GIT.Capture(['svn', 'rebase', '--local'], cwd=cwd) rebase (!) You need to use svn fetch instead, then "git svn find-rev 'r%s' % rev".
Other nits should be pretty straight forward (and should be fixed as soon as I have time). http://codereview.chromium.org/8382030/diff/14003/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode393 scm.py:393: GIT.Capture(['svn', 'rebase', '--local'], cwd=cwd) On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > rebase (!) Yeah, I guess this isn't a good idea, but it's the only way I found to convert git-svn metadata that seems to be guaranteed to not hit network (other than calling git svn find-rev once to rebuild and maybe once more to find). I haven't been able to find a way to exactly replicate what git svn find-rev does before it searches for a sha1 *just* using git-svn's public API, but perhaps this is the wrong thing to do. > You need to use svn fetch instead Are we OK with git svn fetch possibly requiring a painfully long fetching and converting of svn revisions linearly until hitting HEAD? Imagine someone running this the first time with never fetched SVN remote. I'd like to try only locally pulled stuff first, but there's no --local flag for git svn fetch (and I didn't want to have to start looking at git config --get <git-svn-remote>.fetch). I guess I'm assuming people have set up git-svn like we mention in our guide, but maybe they're not using a fetch = refs/remotes/origin/master to convert already fetched git revs (which is too bad if so...). Is it not common for people to git fetch && git svn rebase --local to convert and apply changes (or git pull && git svn fetch)? > , then "git svn find-rev 'r%s' % rev". I'm doing that 4 lines after this.
http://codereview.chromium.org/8382030/diff/14003/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode393 scm.py:393: GIT.Capture(['svn', 'rebase', '--local'], cwd=cwd) On 2011/10/31 18:32:28, Dan Beam wrote: > On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > > rebase (!) > > Yeah, I guess this isn't a good idea, but it's the only way I found to convert > git-svn metadata that seems to be guaranteed to not hit network (other than > calling git svn find-rev once to rebuild and maybe once more to find). I > haven't been able to find a way to exactly replicate what git svn find-rev does > before it searches for a sha1 *just* using git-svn's public API, but perhaps > this is the wrong thing to do. Ah I see. I just dislike that a function like that has a side effect that can leave your checkout in a middle of a rebase. I'm fine with hitting the network. > > You need to use svn fetch instead > > Are we OK with git svn fetch possibly requiring a painfully long fetching and > converting of svn revisions linearly until hitting HEAD? Imagine someone > running this the first time with never fetched SVN remote. I'd like to try only > locally pulled stuff first, but there's no --local flag for git svn fetch (and I > didn't want to have to start looking at git config --get > <git-svn-remote>.fetch). > > I guess I'm assuming people have set up git-svn like we mention in our guide, > but maybe they're not using a fetch = refs/remotes/origin/master to convert > already fetched git revs (which is too bad if so...). Is it not common for > people to git fetch && git svn rebase --local to convert and apply changes (or > git pull && git svn fetch)? I see an optimization though; the network hit should be prepended with this check: if Capture("git svn info")['revision'] >= rev: # Don't hit network so that network is not hit unless necessary. Also, find-rev r123 will fail if the commits are 122 and 124. So you need fallback code there.
http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode480 gclient_scm.py:480: # If this doesn't look like a Subversion revision (just numbers), pass it On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > Please add a proper docstring. Done. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode482 gclient_scm.py:482: if not rev.isdigit(): On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > Also look for "or len(rev) > 7" > We'll revision once we have more than 9999999 revisions. Done. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode492 gclient_scm.py:492: return '' On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > No, '01234567' could still be a valid short hash. It would be nice to verify the > hash is valid. Something like > git rev-parse $(rev) should return a valid hash. > > In any case, silently dropping the value is not a valid option. Done. http://codereview.chromium.org/8382030/diff/14003/gclient_scm.py#newcode1008 gclient_scm.py:1008: def FindSafesyncRev(self, rev): On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > Make it a @staticmethod instead of disabling the warning. > > Also, just return rev without touching it. > > It could be a {DATE}, BASE, HEAD, COMMITTED, PREV, or god who knows. > > You could verify that the revision is valid with > svn info -r $(rev) $(url) but I don't think it's useful to do a network request > at this point. Done. http://codereview.chromium.org/8382030/diff/14003/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode391 scm.py:391: return '' On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > return None in case of failure. Done. http://codereview.chromium.org/8382030/diff/14003/scm.py#newcode393 scm.py:393: GIT.Capture(['svn', 'rebase', '--local'], cwd=cwd) On 2011/10/31 19:17:50, Marc-Antoine Ruel wrote: > On 2011/10/31 18:32:28, Dan Beam wrote: > > On 2011/10/31 13:46:51, Marc-Antoine Ruel wrote: > > > rebase (!) > > > > Yeah, I guess this isn't a good idea, but it's the only way I found to convert > > git-svn metadata that seems to be guaranteed to not hit network (other than > > calling git svn find-rev once to rebuild and maybe once more to find). I > > haven't been able to find a way to exactly replicate what git svn find-rev > does > > before it searches for a sha1 *just* using git-svn's public API, but perhaps > > this is the wrong thing to do. > > Ah I see. I just dislike that a function like that has a side effect that can > leave your checkout in a middle of a rebase. I'm fine with hitting the network. > > > > > You need to use svn fetch instead > > > > Are we OK with git svn fetch possibly requiring a painfully long fetching and > > converting of svn revisions linearly until hitting HEAD? Imagine someone > > running this the first time with never fetched SVN remote. I'd like to try > only > > locally pulled stuff first, but there's no --local flag for git svn fetch (and > I > > didn't want to have to start looking at git config --get > > <git-svn-remote>.fetch). > > > > I guess I'm assuming people have set up git-svn like we mention in our guide, > > but maybe they're not using a fetch = refs/remotes/origin/master to convert > > already fetched git revs (which is too bad if so...). Is it not common for > > people to git fetch && git svn rebase --local to convert and apply changes (or > > git pull && git svn fetch)? > > I see an optimization though; the network hit should be prepended with this > check: > if Capture("git svn info")['revision'] >= rev: > # Don't hit network > so that network is not hit unless necessary. > > Also, find-rev r123 will fail if the commits are 122 and 124. So you need > fallback code there. Done.
maurel: I still need to add tests. I'll look at how the other ones work, but I'm just worried about how to mock SVN - it doesn't sound as easy as mocking the safesync_url or git as both can be replicated locally. Maybe SVN can as well? I'd rather not be making network requests in unit tests...
On 2011/11/07 19:02:28, Dan Beam wrote: > maurel: I still need to add tests. I'll look at how the other ones work, but > I'm just worried about how to mock SVN - it doesn't sound as easy as mocking the > safesync_url or git as both can be replicated locally. Maybe SVN can as well? > I'd rather not be making network requests in unit tests... I have a few comments but in general the code looks good. There is two way to test, either with an integration test or with a mocked test. I think in your case a mocking test makes more sense. My take: Test the gclient.py part in gclient_test.py by adjusting the mocks accordingly. Test the edge cases gclient_scm.py can receive with gclient_scm_test.py. Sadly, there is no mocking git test at the moment so you'll have to write a bit more code there. You could test the new functions in scm.py but they are small enough it's probably not worth it IMHO. Feel free to skip these, scm_unittest.py mostly test edge cases like reverting on svn and svn status, which are both an extreme pain. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode480 gclient_scm.py:480: def FindSafesyncRev(cwd, rev, options): Note that this function has nothing to do with 'safe sync' in the first place. It's simply a 'Revision format conversion function'. Try to change the name to what the function actually does. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode481 gclient_scm.py:481: """Find a useful safesync revision for this repository. If SCM is git-svn Finds http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode482 gclient_scm.py:482: and the head revision is less than |rev|, git svn fetch will be style nit: In general we strive for the docstring to have the format: """Single line summary. Long description aligned at +0 compared to the """. Blah blah. """ http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode498 gclient_scm.py:498: # If we didn't find anything, show a warning. Are you sure a warning is a good idea? It'll get lost in the noise, I'd rather abort instead. I don't have a strong opinion, I just don't see an argument to not abort. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode1025 gclient_scm.py:1025: """Find a useful safesync revision for this repository.""" Finds http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode1029 gclient_scm.py:1029: print('WARNING: The content of safesync_url didn\'t look like a valid\n' Same as line 498. Either always return rev silently or abort? http://codereview.chromium.org/8382030/diff/19001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode384 scm.py:384: """Get the most recently pulled git-svn revision.""" "Gets", you see the deal. :) http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode387 scm.py:387: match = re.search('\nRevision: ([0-9]+)\n', output) you should use ^$ instead of \n ? e.g. : re.search(r'^bar$', 'foo\nbar\nbaz\n', re.MULTILINE) http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode399 scm.py:399: ['svn', 'find-rev', 'r' + rev], cwd=cwd).splitlines(True) Why True? What if isinstance(rev, int) ? http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode405 scm.py:405: def VerifyRevisionFormat(cwd, rev): I'd rename the function, you are not verifying the 'format' but the 'validity'. IsRevisionValid is clearer to me about what the function does. http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode968 scm.py:968: SVN.Capture(['info', '-r', rev, url]) If it is an url, it should simply use the format url@revision so this function shouldn't special case. Fix the caller instead. http://codereview.chromium.org/8382030/diff/19001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): http://codereview.chromium.org/8382030/diff/19001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:87: members = [ 'FindSafesyncRev', Align at +4 instead, it's easier to refactor afterward. It's a good idea to do this though, I've been lazy about it.
Still haven't added tests nor really tried this much with SVN. Will do soon. I'm pretty much 20%'ing this, so if there's somebody with more mad Python/depot_tools skillz feel free to let them at it. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode480 gclient_scm.py:480: def FindSafesyncRev(cwd, rev, options): On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Note that this function has nothing to do with 'safe sync' in the first place. > It's simply a 'Revision format conversion function'. Try to change the name to > what the function actually does. Done. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode481 gclient_scm.py:481: """Find a useful safesync revision for this repository. If SCM is git-svn On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Finds Done. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode482 gclient_scm.py:482: and the head revision is less than |rev|, git svn fetch will be On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > style nit: In general we strive for the docstring to have the format: > """Single line summary. > > Long description aligned at +0 compared to the """. > Blah blah. > """ Done. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode498 gclient_scm.py:498: # If we didn't find anything, show a warning. On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Are you sure a warning is a good idea? It'll get lost in the noise, I'd rather > abort instead. > > I don't have a strong opinion, I just don't see an argument to not abort. Done. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode1025 gclient_scm.py:1025: """Find a useful safesync revision for this repository.""" On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Finds Done. http://codereview.chromium.org/8382030/diff/19001/gclient_scm.py#newcode1029 gclient_scm.py:1029: print('WARNING: The content of safesync_url didn\'t look like a valid\n' On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Same as line 498. Either always return rev silently or abort? Done. http://codereview.chromium.org/8382030/diff/19001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode384 scm.py:384: """Get the most recently pulled git-svn revision.""" On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > "Gets", you see the deal. :) Done. http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode387 scm.py:387: match = re.search('\nRevision: ([0-9]+)\n', output) On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > you should use ^$ instead of \n ? e.g. : > re.search(r'^bar$', 'foo\nbar\nbaz\n', re.MULTILINE) Done. (tried this a couple times before and it just wasn't working...) http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode399 scm.py:399: ['svn', 'find-rev', 'r' + rev], cwd=cwd).splitlines(True) > Why True? Oh, guess I didn't need that, you're right. Thought it preserved \n+, but it just leaves a \n on the end of lines. D'oh. > What if isinstance(rev, int) ? Done. (just homogenized to string) http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode405 scm.py:405: def VerifyRevisionFormat(cwd, rev): On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > I'd rename the function, you are not verifying the 'format' but the 'validity'. > > IsRevisionValid is clearer to me about what the function does. Done. http://codereview.chromium.org/8382030/diff/19001/scm.py#newcode968 scm.py:968: SVN.Capture(['info', '-r', rev, url]) On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > If it is an url, it should simply use the format url@revision so this function > shouldn't special case. Fix the caller instead. Done. http://codereview.chromium.org/8382030/diff/19001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): http://codereview.chromium.org/8382030/diff/19001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:87: members = [ 'FindSafesyncRev', On 2011/11/08 14:29:20, Marc-Antoine Ruel wrote: > Align at +4 instead, it's easier to refactor afterward. It's a good idea to do > this though, I've been lazy about it. Done.
Minor nits. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode501 gclient_scm.py:501: help_url = ''.join(['http://code.google.com/p/chromium/wiki/UsingNewGit', Replace lines 498-506 with: if not sha1: raise gclient_utils.Error( ( '%s is not a value hash. Safesync URLs with a git checkout\n' 'currently require a git-svn remote or a safesync_url that\n' 'provides git sha1s. Please add a git-svn remote or change\n' 'your safesync_url. For more info, see:\n' 'http://code.google.com/p/chromium/wiki/UsingNewGit' '#Initial_checkout') % rev) return sha1 http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1025 gclient_scm.py:1025: """Finds a useful revision for this repository.""" Verifies the validity of the revision for this repository. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1026 gclient_scm.py:1026: if scm.SVN.IsValidRevision(cwd=cwd, url=url, rev=rev): I think you should do the url@revision formatting here instead of inside IsValidRevision(). http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1029 gclient_scm.py:1029: 'The content of safesync_url doesn\'t look like a valid Subversion ' Prepend '%s isn't a valid revision' http://codereview.chromium.org/8382030/diff/28001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode389 scm.py:389: print('Match found! ' + match.group(1)) That was for debugging? http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode404 scm.py:404: return lines[-1].strip() if len(lines) else None if lines len() is unnecessary here. http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode409 scm.py:409: def IsValidRevision(cwd, url, rev): Why url here? http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode968 scm.py:968: def IsValidRevision(cwd, url, rev): Remove url from here, have the caller reformat what it needs. You don't use cwd?
I'll do the other stuff tonight (been only really doing this from home), just wanted to respond now guidance on other things you've asked me. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1026 gclient_scm.py:1026: if scm.SVN.IsValidRevision(cwd=cwd, url=url, rev=rev): On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > I think you should do the url@revision formatting here instead of inside > IsValidRevision(). I guess I could, but the git version doesn't use this so that's why I'm just passing cwd, url, and rev to keep the API between each version uniform. SVN does svn info url@rev, git uses git parse-rev rev in the cwd. http://codereview.chromium.org/8382030/diff/28001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode389 scm.py:389: print('Match found! ' + match.group(1)) On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > That was for debugging? Ah, yeah, sorry. Will remove. http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode409 scm.py:409: def IsValidRevision(cwd, url, rev): On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > Why url here? To let scm.{GIT,SVN}.IsValidRevision() take the same args. Maybe it doesn't matter? http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode968 scm.py:968: def IsValidRevision(cwd, url, rev): On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > Remove url from here, have the caller reformat what it needs. > > You don't use cwd? Same reason as above, make both methods take same params. Let me know if this is unnecessary.
http://codereview.chromium.org/8382030/diff/28001/gclient.py File gclient.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient.py#newcode928 gclient.py:928: cwd=scm.checkout_path, url=scm.url, rev=rev, Why pass scm.checkout_path and scm.url when you are calling a member function on scm? http://codereview.chromium.org/8382030/diff/28001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode409 scm.py:409: def IsValidRevision(cwd, url, rev): On 2011/11/09 19:21:59, Dan Beam wrote: > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > Why url here? > > To let scm.{GIT,SVN}.IsValidRevision() take the same args. Maybe it doesn't > matter? I prefer not. It's fine to have different interface.
http://codereview.chromium.org/8382030/diff/28001/gclient.py File gclient.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient.py#newcode928 gclient.py:928: cwd=scm.checkout_path, url=scm.url, rev=rev, On 2011/11/09 19:25:51, Marc-Antoine Ruel wrote: > Why pass scm.checkout_path and scm.url when you are calling a member function on > scm? I think you told me to make them @staticmethods, remember? "Make it a @staticmethod instead of disabling the warning." I can change back if you want?
http://codereview.chromium.org/8382030/diff/28001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode409 scm.py:409: def IsValidRevision(cwd, url, rev): On 2011/11/09 19:25:51, Marc-Antoine Ruel wrote: > On 2011/11/09 19:21:59, Dan Beam wrote: > > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > > Why url here? > > > > To let scm.{GIT,SVN}.IsValidRevision() take the same args. Maybe it doesn't > > matter? > > I prefer not. It's fine to have different interface. OK, great, will change!
http://codereview.chromium.org/8382030/diff/28001/gclient.py File gclient.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient.py#newcode928 gclient.py:928: cwd=scm.checkout_path, url=scm.url, rev=rev, On 2011/11/09 19:36:52, Dan Beam wrote: > On 2011/11/09 19:25:51, Marc-Antoine Ruel wrote: > > Why pass scm.checkout_path and scm.url when you are calling a member function > on > > scm? > > I think you told me to make them @staticmethods, remember? "Make it a > @staticmethod instead of disabling the warning." I can change back if you want? Yes, but I told you that because you were precisely not using self. Convert it back to a normal function member and use self.checkout_path and self.url. Don't always listen to what I write. :)
http://codereview.chromium.org/8382030/diff/28001/gclient.py File gclient.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient.py#newcode928 gclient.py:928: cwd=scm.checkout_path, url=scm.url, rev=rev, On 2011/11/09 19:38:50, Marc-Antoine Ruel wrote: > On 2011/11/09 19:36:52, Dan Beam wrote: > > On 2011/11/09 19:25:51, Marc-Antoine Ruel wrote: > > > Why pass scm.checkout_path and scm.url when you are calling a member > function > > on > > > scm? > > > > I think you told me to make them @staticmethods, remember? "Make it a > > @staticmethod instead of disabling the warning." I can change back if you > want? > > Yes, but I told you that because you were precisely not using self. Convert it > back to a normal function member and use self.checkout_path and self.url. Don't > always listen to what I write. :) Done. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py File gclient_scm.py (right): http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode501 gclient_scm.py:501: help_url = ''.join(['http://code.google.com/p/chromium/wiki/UsingNewGit', On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > Replace lines 498-506 with: > > if not sha1: > raise gclient_utils.Error( > ( '%s is not a value hash. Safesync URLs with a git checkout\n' > 'currently require a git-svn remote or a safesync_url that\n' > 'provides git sha1s. Please add a git-svn remote or change\n' > 'your safesync_url. For more info, see:\n' > 'http://code.google.com/p/chromium/wiki/UsingNewGit' > '#Initial_checkout') % rev) > return sha1 Done. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1025 gclient_scm.py:1025: """Finds a useful revision for this repository.""" On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > Verifies the validity of the revision for this repository. Done. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1026 gclient_scm.py:1026: if scm.SVN.IsValidRevision(cwd=cwd, url=url, rev=rev): On 2011/11/09 19:21:59, Dan Beam wrote: > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > I think you should do the url@revision formatting here instead of inside > > IsValidRevision(). > > I guess I could, but the git version doesn't use this so that's why I'm just > passing cwd, url, and rev to keep the API between each version uniform. SVN > does svn info url@rev, git uses git parse-rev rev in the cwd. Done. http://codereview.chromium.org/8382030/diff/28001/gclient_scm.py#newcode1029 gclient_scm.py:1029: 'The content of safesync_url doesn\'t look like a valid Subversion ' On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > Prepend '%s isn't a valid revision' Done. http://codereview.chromium.org/8382030/diff/28001/scm.py File scm.py (right): http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode389 scm.py:389: print('Match found! ' + match.group(1)) On 2011/11/09 19:21:59, Dan Beam wrote: > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > That was for debugging? > > Ah, yeah, sorry. Will remove. Done. http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode404 scm.py:404: return lines[-1].strip() if len(lines) else None On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > if lines > > len() is unnecessary here. Done. http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode409 scm.py:409: def IsValidRevision(cwd, url, rev): On 2011/11/09 19:37:22, Dan Beam wrote: > On 2011/11/09 19:25:51, Marc-Antoine Ruel wrote: > > On 2011/11/09 19:21:59, Dan Beam wrote: > > > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > > > Why url here? > > > > > > To let scm.{GIT,SVN}.IsValidRevision() take the same args. Maybe it doesn't > > > matter? > > > > I prefer not. It's fine to have different interface. > > OK, great, will change! Done. http://codereview.chromium.org/8382030/diff/28001/scm.py#newcode968 scm.py:968: def IsValidRevision(cwd, url, rev): On 2011/11/09 19:21:59, Dan Beam wrote: > On 2011/11/09 14:16:08, Marc-Antoine Ruel wrote: > > Remove url from here, have the caller reformat what it needs. > > > > You don't use cwd? > > Same reason as above, make both methods take same params. Let me know if this > is unnecessary. Done.
Code looks good.
On 2011/11/10 14:51:36, Marc-Antoine Ruel wrote: > Code looks good. Cool. On to the tests.
Hey Marc-Antoine, I added some of the tests required (git, git-svn). Still need to add gclient_scm and SVN related ones. If you want to start looking now, go ahead, otherwise I should have the rest of them done tomorrow.
lgtm http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:110: 'url' keep a coma there so it's easier to add another value after. http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:676: 'url' same http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py File tests/scm_unittest.py (right): http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:119: add another line http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:137: add another line here http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:150: hashes = self._capture(['rev-list', 'HEAD']).splitlines() # We insert a null value at 0 to do 1-based indexing, not 0-based, as SVN # revisions are 1-based (i.e. they start at r1, not r0). # git rev-list gives revisions in reverse chronological order. self.git_hashes = ([None] + reversed(self._capture(['rev-list', 'HEAD']).splitlines())) saves you two lines.
Actually, one nit, I would prefer a proper mocked test for GetUsableRev(). Using auto_stub, not pymox.
Cool, on to gclient_scm.GetUsableRev with auto_stub and scm.SVN.IsValidRevision, :D! http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:110: 'url' On 2011/12/16 15:03:31, Marc-Antoine Ruel wrote: > keep a coma there so it's easier to add another value after. Done. http://codereview.chromium.org/8382030/diff/39001/tests/gclient_scm_test.py#n... tests/gclient_scm_test.py:676: 'url' On 2011/12/16 15:03:31, Marc-Antoine Ruel wrote: > same Done. http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py File tests/scm_unittest.py (right): http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:119: On 2011/12/16 15:03:31, Marc-Antoine Ruel wrote: > add another line Done. http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:137: On 2011/12/16 15:03:31, Marc-Antoine Ruel wrote: > add another line here Done. http://codereview.chromium.org/8382030/diff/39001/tests/scm_unittest.py#newco... tests/scm_unittest.py:150: hashes = self._capture(['rev-list', 'HEAD']).splitlines() On 2011/12/16 15:03:31, Marc-Antoine Ruel wrote: > # We insert a null value at 0 to do 1-based indexing, not 0-based, as SVN > # revisions are 1-based (i.e. they start at r1, not r0). > # git rev-list gives revisions in reverse chronological order. > self.git_hashes = ([None] + > reversed(self._capture(['rev-list', 'HEAD']).splitlines())) > > saves you two lines. Done. (yay, knew there had to be a way to do this!)
maurel: Adding the last round of tests (supposedly), please take another look.
lgtm, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/8382030/54002
Change committed as 115011 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
