Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
240203005:
Implement git-drover. (Closed)
|
Created:
6 years, 8 months ago by iannucci Modified:
6 years, 5 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Visibility:
Public. |
DescriptionImplement git-drover.
Only supports chromium/src.git. Have tested cherry-picks and reverts in
--prep_only mode.
Have tested multi-repo logic by uncommenting lines in OK_REPOS.
R=agable@chromium.org, hinoka@chromium.org
BUG=
Patch Set 1 #
Total comments: 67
Patch Set 2 : No fancy stuff #Patch Set 3 : address feedback #
Total comments: 2
Messages
Total messages: 7 (0 generated)
PTAL. Needs tests (at the very least for git_common, probably for the whole thing).
https://codereview.chromium.org/240203005/diff/1/git_cache.py File git_cache.py (right): https://codereview.chromium.org/240203005/diff/1/git_cache.py#newcode270 git_cache.py:270: if not verbose: print if *not* verbose?? https://codereview.chromium.org/240203005/diff/1/git_cache.py#newcode300 git_cache.py:300: verbose=False, fetch_specs=()): None https://codereview.chromium.org/240203005/diff/1/git_common.py File git_common.py (right): https://codereview.chromium.org/240203005/diff/1/git_common.py#newcode282 git_common.py:282: print commits remove print https://codereview.chromium.org/240203005/diff/1/git_common.py#newcode291 git_common.py:291: def check(*args, **kwargs): between 'cached_fetch' and 'config' seems like a weird place for this. Would only make sense if this file were alphabetical, which it isn't. https://codereview.chromium.org/240203005/diff/1/git_common.py#newcode692 git_common.py:692: now newlines https://codereview.chromium.org/240203005/diff/1/git_drover.py File git_drover.py (right): https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode7 git_drover.py:7: __doc__ = """ wat why is this necessary. __doc__ is already correctly assigned by the time we hit line 63. $ cat mything.py #!/usr/bin/env python """This is a thing. It is capable of doing things. And stuff. And it has a docstring. """ import sys print __doc__ __doc__ += (lambda: '\n'.join( ['thing %d' % num for num in xrange(5)]))() def main(): print 'Hello, there.' print 'I already printed my docstring at module load time.' print 'But I modified it, so here it is again:' print __doc__ if __name__ == '__main__': sys.exit(main()) https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode8 git_drover.py:8: Merge/Revert changes to chromium release branches. Chromium https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode26 git_drover.py:26: import urllib2 dammit depot_tools y u no have requests https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode90 git_drover.py:90: ################################################################################ I see you like to live dangerously with your comment separator lengths. Mine are usually 72 because fortran... https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode98 git_drover.py:98: print msg textwrap.dedent this one too https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode137 git_drover.py:137: Inside a git repo, but origin's remote URL doesn't match one of the I suppose technically these should be indented +4? https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode153 git_drover.py:153: Commits specified appear to be from a different repo than the repo s/Commits specified/The specified commits/ https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode167 git_drover.py:167: sample_path = "/path/to/cache" single quotes https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode169 git_drover.py:169: sample_path = r"X:\path\to\cache" here too https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode196 git_drover.py:196: def find_hash_urls(commits, presumed_url=None): It's a 70-line function, with three embedded functions... it should have a docstring. This whole function is actually really hard to read. Not sure what to do to make it clearer; async is just hard. But still. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode199 git_drover.py:199: def process_async_results(asr, results): Yes, it processes async results... still the least descriptive name ever. Should be something about lost_commits. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode251 git_drover.py:251: while commits and not presumed_url: guessed_url? presumed means you're guessing without reason. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode321 git_drover.py:321: if parsed.scheme == 'https': See if you can reorganize this whole conditional. The whole if for if break else return elif if return else return return 'yes this code is still reachable' is really unreadable. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode322 git_drover.py:322: for host in OK_REPOS: Turn this into if not any(...): return False https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode324 git_drover.py:324: break relying on the fact that |host| will remain set to the last value upon the break is poor form; we pretend that loop iterator values are scoped to the loop. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode380 git_drover.py:380: elif isinstance(ref, Channel): This is all getting removed. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode415 git_drover.py:415: mDD - a release milestone (will consult omahaproxy, aborts if ambiguous) remove? https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode416 git_drover.py:416: os,channel - consults omahaproxy for the current branch-head remove https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode477 git_drover.py:477: parser.error("?confusion? must specify either revert_from or merge_to") s/\?confusion\?/You/ https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode503 git_drover.py:503: git.check('branch', '-D', '__drover_change') why branch -D here but update-ref -d for __drover_base above? https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode516 git_drover.py:516: else: # merge revert https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode520 git_drover.py:520: die("""\ This doesn't require a multiline string. https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode529 git_drover.py:529: announce('Success! Uploading to codereview.chromium.org') Not true for src-internal https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode552 git_drover.py:552: Missing the actual git push call.
https://codereview.chromium.org/240203005/diff/1/download_from_google_storage.py File download_from_google_storage.py (right): https://codereview.chromium.org/240203005/diff/1/download_from_google_storage... download_from_google_storage.py:69: env.pop('AWS_CREDENTIAL_FILE', None) ??? https://codereview.chromium.org/240203005/diff/1/download_from_google_storage... download_from_google_storage.py:91: def _inner(line): "data" or "char" is probably more appropriate vs "line", since subprocess2 will only ever call this with a single char https://codereview.chromium.org/240203005/diff/1/download_from_google_storage... download_from_google_storage.py:102: err = [] why not a string or cStringIO? I'd probably prefer cStringIO.
https://codereview.chromium.org/240203005/diff/1/download_from_google_storage.py File download_from_google_storage.py (right): https://codereview.chromium.org/240203005/diff/1/download_from_google_storage... download_from_google_storage.py:102: err = [] On 2014/04/18 00:28:53, Ryan T. wrote: > why not a string or cStringIO? > > I'd probably prefer cStringIO. Disregard that, I ran a test and your list implementation is faster. I don't understand python.
better now https://chromiumcodereview.appspot.com/240203005/diff/1/download_from_google_... File download_from_google_storage.py (right): https://chromiumcodereview.appspot.com/240203005/diff/1/download_from_google_... download_from_google_storage.py:69: env.pop('AWS_CREDENTIAL_FILE', None) On 2014/04/18 00:28:53, Ryan T. wrote: > ??? Otherwise gsutil freaks out because the env var is set but not a valid path. This is more-better. https://chromiumcodereview.appspot.com/240203005/diff/1/download_from_google_... download_from_google_storage.py:91: def _inner(line): On 2014/04/18 00:28:53, Ryan T. wrote: > "data" or "char" is probably more appropriate vs "line", since subprocess2 will > only ever call this with a single char What? That's not true... /me reads subprocess2 implementation What the... who... why.... arg! :( https://chromiumcodereview.appspot.com/240203005/diff/1/download_from_google_... download_from_google_storage.py:102: err = [] On 2014/04/18 00:37:36, Ryan T. wrote: > On 2014/04/18 00:28:53, Ryan T. wrote: > > why not a string or cStringIO? > > > > I'd probably prefer cStringIO. > > Disregard that, I ran a test and your list implementation is faster. > string allocation n' stuff > I don't understand python. https://chromiumcodereview.appspot.com/240203005/diff/1/git_cache.py File git_cache.py (right): https://chromiumcodereview.appspot.com/240203005/diff/1/git_cache.py#newcode270 git_cache.py:270: if not verbose: On 2014/04/18 00:17:08, agable wrote: > print if *not* verbose?? Yeah, otherwise this doubles up with the gsutil output which says rougly the same thing. https://chromiumcodereview.appspot.com/240203005/diff/1/git_cache.py#newcode300 git_cache.py:300: verbose=False, fetch_specs=()): On 2014/04/18 00:17:08, agable wrote: > None Er... why? it's iterable and immutable... that's strictly better than None? https://chromiumcodereview.appspot.com/240203005/diff/1/git_common.py File git_common.py (right): https://chromiumcodereview.appspot.com/240203005/diff/1/git_common.py#newcode282 git_common.py:282: print commits On 2014/04/18 00:17:08, agable wrote: > remove print Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_common.py#newcode291 git_common.py:291: def check(*args, **kwargs): On 2014/04/18 00:17:08, agable wrote: > between 'cached_fetch' and 'config' seems like a weird place for this. Would > only make sense if this file were alphabetical, which it isn't. Oh, but it is :p https://chromiumcodereview.appspot.com/240203005/diff/1/git_common.py#newcode692 git_common.py:692: On 2014/04/18 00:17:08, agable wrote: > now newlines Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py File git_drover.py (right): https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode7 git_drover.py:7: __doc__ = """ On 2014/04/18 00:17:08, agable wrote: > wat why is this necessary. __doc__ is already correctly assigned by the time we > hit line 63. > > $ cat mything.py > #!/usr/bin/env python > > """This is a thing. > > It is capable of doing things. And stuff. > And it has a docstring. > """ > > import sys > > print __doc__ > > __doc__ += (lambda: '\n'.join( > ['thing %d' % num for num in xrange(5)]))() > > def main(): > print 'Hello, there.' > print 'I already printed my docstring at module load time.' > print 'But I modified it, so here it is again:' > print __doc__ > > if __name__ == '__main__': > sys.exit(main()) Hmmmmmmmmmmm...... Not sure why I did that... Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode8 git_drover.py:8: Merge/Revert changes to chromium release branches. On 2014/04/18 00:17:08, agable wrote: > Chromium Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode26 git_drover.py:26: import urllib2 On 2014/04/18 00:17:08, agable wrote: > dammit depot_tools y u no have requests IKR https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode90 git_drover.py:90: ################################################################################ On 2014/04/18 00:17:08, agable wrote: > I see you like to live dangerously with your comment separator lengths. Mine are > usually 72 because fortran... https://www.youtube.com/watch?v=Dpjl4XJ91xY https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode98 git_drover.py:98: print msg On 2014/04/18 00:17:08, agable wrote: > textwrap.dedent this one too Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode137 git_drover.py:137: Inside a git repo, but origin's remote URL doesn't match one of the On 2014/04/18 00:17:08, agable wrote: > I suppose technically these should be indented +4? meh... makes 80 col awkward https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode153 git_drover.py:153: Commits specified appear to be from a different repo than the repo On 2014/04/18 00:17:08, agable wrote: > s/Commits specified/The specified commits/ Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode167 git_drover.py:167: sample_path = "/path/to/cache" On 2014/04/18 00:17:08, agable wrote: > single quotes Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode169 git_drover.py:169: sample_path = r"X:\path\to\cache" On 2014/04/18 00:17:08, agable wrote: > here too Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode196 git_drover.py:196: def find_hash_urls(commits, presumed_url=None): On 2014/04/18 00:17:08, agable wrote: > It's a 70-line function, with three embedded functions... it should have a > docstring. This whole function is actually really hard to read. Not sure what to > do to make it clearer; async is just hard. But still. rewrite it in go? :p https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode199 git_drover.py:199: def process_async_results(asr, results): On 2014/04/18 00:17:08, agable wrote: > Yes, it processes async results... still the least descriptive name ever. Should > be something about lost_commits. PTAL https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode251 git_drover.py:251: while commits and not presumed_url: On 2014/04/18 00:17:08, agable wrote: > guessed_url? presumed means you're guessing without reason. We ARE guessing without reason. In the event that they have a git repo locally, we presume that all commits that they specify are in the repo which corresponds to that local repo (but this may be totally false). https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode321 git_drover.py:321: if parsed.scheme == 'https': On 2014/04/18 00:17:08, agable wrote: > See if you can reorganize this whole conditional. The whole > > if > for > if > break > else > return > elif > if > return > else > return > return 'yes this code is still reachable' > > is really unreadable. Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode322 git_drover.py:322: for host in OK_REPOS: On 2014/04/18 00:17:08, agable wrote: > Turn this into > if not any(...): > return False Not needed https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode324 git_drover.py:324: break On 2014/04/18 00:17:08, agable wrote: > relying on the fact that |host| will remain set to the last value upon the break > is poor form; we pretend that loop iterator values are scoped to the loop. Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode380 git_drover.py:380: elif isinstance(ref, Channel): On 2014/04/18 00:17:08, agable wrote: > This is all getting removed. Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode415 git_drover.py:415: mDD - a release milestone (will consult omahaproxy, aborts if ambiguous) On 2014/04/18 00:17:08, agable wrote: > remove? Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode416 git_drover.py:416: os,channel - consults omahaproxy for the current branch-head On 2014/04/18 00:17:08, agable wrote: > remove Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode477 git_drover.py:477: parser.error("?confusion? must specify either revert_from or merge_to") On 2014/04/18 00:17:08, agable wrote: > s/\?confusion\?/You/ yeah, but argparse should have caught this already. I just have it for completeness in this if-tree. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode503 git_drover.py:503: git.check('branch', '-D', '__drover_change') On 2014/04/18 00:17:08, agable wrote: > why branch -D here but update-ref -d for __drover_base above? Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode516 git_drover.py:516: else: # merge On 2014/04/18 00:17:08, agable wrote: > revert Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode520 git_drover.py:520: die("""\ On 2014/04/18 00:17:08, agable wrote: > This doesn't require a multiline string. Says you. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode529 git_drover.py:529: announce('Success! Uploading to codereview.chromium.org') On 2014/04/18 00:17:08, agable wrote: > Not true for src-internal Done. https://chromiumcodereview.appspot.com/240203005/diff/1/git_drover.py#newcode552 git_drover.py:552: On 2014/04/18 00:17:08, agable wrote: > Missing the actual git push call. ^_^
https://codereview.chromium.org/240203005/diff/1/git_common.py File git_common.py (right): https://codereview.chromium.org/240203005/diff/1/git_common.py#newcode291 git_common.py:291: def check(*args, **kwargs): On 2014/04/28 21:05:28, iannucci wrote: > On 2014/04/18 00:17:08, agable wrote: > > between 'cached_fetch' and 'config' seems like a weird place for this. Would > > only make sense if this file were alphabetical, which it isn't. > > Oh, but it is :p "run_with_retcode" comes before "cached_fetch" in what alphabet? https://codereview.chromium.org/240203005/diff/1/git_drover.py File git_drover.py (right): https://codereview.chromium.org/240203005/diff/1/git_drover.py#newcode251 git_drover.py:251: while commits and not presumed_url: On 2014/04/28 21:05:28, iannucci wrote: > On 2014/04/18 00:17:08, agable wrote: > > guessed_url? presumed means you're guessing without reason. > > We ARE guessing without reason. In the event that they have a git repo locally, > we presume that all commits that they specify are in the repo which corresponds > to that local repo (but this may be totally false). That's actually a really good reason, not no reason. educatedly_guessed_url https://codereview.chromium.org/240203005/diff/40001/git_drover.py File git_drover.py (right): https://codereview.chromium.org/240203005/diff/40001/git_drover.py#newcode214 git_drover.py:214: Returns a list of commits which did not have any non-MISSING result. "...which had only MISSING results." https://codereview.chromium.org/240203005/diff/40001/git_drover.py#newcode389 git_drover.py:389: if not s.isdigit(): Doesn't handle 1780_21 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
