Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10388020:
Add a revert script. (Closed)
|
Created:
8 years, 7 months ago by Emily Fortuna Modified:
8 years, 7 months ago Reviewers:
Siggi Cherem (dart-lang) CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd a revert script.
Committed: https://code.google.com/p/dart/source/detail?r=7434
Patch Set 1 #
Total comments: 34
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Messages
Total messages: 5 (0 generated)
check out the git version, especially. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py File tools/revert.py (right): https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode12 tools/revert.py:12: parser.add_option('--range', '-r', dest='rev_range', action='store', Yes, this could/should have been a positional argument, but I wanted to provide a better -h message to explain how to use this script. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:138: reverts.reverse() these two lines (137-138) could certainly be done more efficiently, but I decided to write it this way to clearly show what was going on.
https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py File tools/revert.py (right): https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode1 tools/revert.py:1: import optparse + #!/usr/bin/env python + # Copyright ... https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode8 tools/revert.py:8: git-svn users.""" wrap """ to next line (not sure if this is in the styleguide, but I tend to do this for multi-line documentation) https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode12 tools/revert.py:12: parser.add_option('--range', '-r', dest='rev_range', action='store', maybe rename it to --revisions instead of range? https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode14 tools/revert.py:14: 'you wish to undo. An individual number, or a range 8-10.') in git it's common to specify a range as A..B, maybe we can use that syntax? https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode19 tools/revert.py:19: if revision_range.find('-') > -1 or revision_range.find(':') > -1: oh - I guess I'm suggesting a third syntax :). Either way is ok, but if we have more than one, we should include an example in the help message. (e.g. 8:10, or 8..10) https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode22 tools/revert.py:22: if len(revision_range.split('-')) == 1: len(revision_range.split('-')) => len(split) https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode30 tools/revert.py:30: fail('Warning: are you sure you want to revert a range of revisions? If ' maybe check if start != end (if they are equal, we can skip this error) https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode37 tools/revert.py:37: def fail(error_msg, user_input=False): fail => maybe_fail? or prompt_user? https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode42 tools/revert.py:42: - user_input: True if we require user confirmation to continue. We assume in this case, the message is not necessarily an error. Maybe rename error_msg to simply 'msg' https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode58 tools/revert.py:58: os.chdir(os.path.join(os.path.dirname(os.path.dirname(os.path.abspath( join seems unnecessary? https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode71 tools/revert.py:71: p = subprocess.Popen(['git', 'log'], stdout=subprocess.PIPE) should we add here '-1' to this git log command? It seems that find_git_info will find a revision number in older commits, and we'll return False here, even when there are local commits. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:107: git commit_id that corresponds to a particular svn revision number.""" wrap """ to next line https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:143: run_cmd(['git', 'commit', '-m', commit_msg]) this command shouldn't be necessary (there shouldn't be any open files). https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:151: 'changes! Are you **SURE** you want to continue? ', user_input=True) In the case of git, I feel that we should just not let people continue and fail (since all local changes will be part of the revert CL that will get submitted). https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:155: revert(revisions[0], revisions[1], git_user) after this, should we print a message saying: now create a CL and submit it!?
PTAL! https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py File tools/revert.py (right): https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode1 tools/revert.py:1: import optparse On 2012/05/08 01:16:07, sigmund wrote: > + #!/usr/bin/env python > + # Copyright ... Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode8 tools/revert.py:8: git-svn users.""" On 2012/05/08 01:16:07, sigmund wrote: > wrap """ to next line (not sure if this is in the styleguide, but I tend to do > this for multi-line documentation) Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode12 tools/revert.py:12: parser.add_option('--range', '-r', dest='rev_range', action='store', On 2012/05/08 01:16:07, sigmund wrote: > maybe rename it to --revisions instead of range? Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode14 tools/revert.py:14: 'you wish to undo. An individual number, or a range 8-10.') On 2012/05/08 01:16:07, sigmund wrote: > in git it's common to specify a range as A..B, maybe we can use that syntax? Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode19 tools/revert.py:19: if revision_range.find('-') > -1 or revision_range.find(':') > -1: On 2012/05/08 01:16:07, sigmund wrote: > oh - I guess I'm suggesting a third syntax :). > > Either way is ok, but if we have more than one, we should include an example in > the help message. (e.g. 8:10, or 8..10) Done. Yes, the colon version is svn-like, so I'll add the git version, too. :-) https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode22 tools/revert.py:22: if len(revision_range.split('-')) == 1: On 2012/05/08 01:16:07, sigmund wrote: > len(revision_range.split('-')) => len(split) Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode30 tools/revert.py:30: fail('Warning: are you sure you want to revert a range of revisions? If ' On 2012/05/08 01:16:07, sigmund wrote: > maybe check if start != end (if they are equal, we can skip this error) Done, though it will be strange if someone specifies a range from 100..100, but I should definitely deal with this case. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode37 tools/revert.py:37: def fail(error_msg, user_input=False): On 2012/05/08 01:16:07, sigmund wrote: > fail => maybe_fail? or prompt_user? Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode37 tools/revert.py:37: def fail(error_msg, user_input=False): On 2012/05/08 01:16:07, sigmund wrote: > fail => maybe_fail? or prompt_user? Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode42 tools/revert.py:42: - user_input: True if we require user confirmation to continue. We assume On 2012/05/08 01:16:07, sigmund wrote: > in this case, the message is not necessarily an error. Maybe rename error_msg to > simply 'msg' Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode58 tools/revert.py:58: os.chdir(os.path.join(os.path.dirname(os.path.dirname(os.path.abspath( On 2012/05/08 01:16:07, sigmund wrote: > join seems unnecessary? Oops, that was a refactoring gone wrong that I missed. Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcode71 tools/revert.py:71: p = subprocess.Popen(['git', 'log'], stdout=subprocess.PIPE) On 2012/05/08 01:16:07, sigmund wrote: > should we add here '-1' to this git log command? It seems that find_git_info > will find a revision number in older commits, and we'll return False here, even > when there are local commits. When there are local commits, a commit name is listed, but a commit_id is not, so find_git_info returns None. But you're right -1 is only what's really needed to determine that. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:107: git commit_id that corresponds to a particular svn revision number.""" On 2012/05/08 01:16:07, sigmund wrote: > wrap """ to next line Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:143: run_cmd(['git', 'commit', '-m', commit_msg]) On 2012/05/08 01:16:07, sigmund wrote: > this command shouldn't be necessary (there shouldn't be any open files). This is done because when I run git revert, I pass the -n (no commit) flag, because I want all of the reverts (potentially multiple in a sequence) to be one big commit together. Is this a bad strategy? https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:155: revert(revisions[0], revisions[1], git_user) On 2012/05/08 01:16:07, sigmund wrote: > after this, should we print a message saying: now create a CL and submit it!? Done. https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:155: revert(revisions[0], revisions[1], git_user) On 2012/05/08 01:16:07, sigmund wrote: > after this, should we print a message saying: now create a CL and submit it!? Done.
https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py File tools/revert.py (right): https://chromiumcodereview.appspot.com/10388020/diff/1/tools/revert.py#newcod... tools/revert.py:143: run_cmd(['git', 'commit', '-m', commit_msg]) On 2012/05/08 17:33:27, Emily Fortuna wrote: > On 2012/05/08 01:16:07, sigmund wrote: > > this command shouldn't be necessary (there shouldn't be any open files). > > This is done because when I run git revert, I pass the -n (no commit) flag, > because I want all of the reverts (potentially multiple in a sequence) to be one > big commit together. Is this a bad strategy? Ah - got it. It might be better to do it without the -n, I'm not sure how the -n deals with subsequent changes in the same file. We can test it out. https://chromiumcodereview.appspot.com/10388020/diff/8001/tools/revert.py File tools/revert.py (right): https://chromiumcodereview.appspot.com/10388020/diff/8001/tools/revert.py#new... tools/revert.py:173: print ('Reverting success! The buildbots and your teammates thank you! Now ' :) nice message. One suggestion, maybe reword the 'reverting success', it leads me to think that this script also submitted it (if I'm lazy and decide not to read the full sentence, believe me, people do)
and lgtm! |
