|
|
Created:
8 years, 1 month ago by Jói Modified:
8 years, 1 month ago Reviewers:
Nico CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConsolidate mass-rename.sh and move_source_file.py
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168926
Patch Set 1 #Patch Set 2 : Reupload with similarity=30 to see the move diff. #
Total comments: 2
Patch Set 3 : Add support for comments. #
Total comments: 2
Patch Set 4 : Address review comments. #
Total comments: 8
Patch Set 5 : Respond to review comments. #
Messages
Total messages: 13 (0 generated)
Nico, I was thinking about doing this as a follow-up: Adding an import of sort-headers.py and using its DiffAndConfirm (with should_confirm=False) after we rewrite a file for includes. I've been using sort-headers.py a lot in the last year or so and for several months I've never had to correct anything it's done, so I think this is safe enough. Opinions? As an aside, to import sort-headers.py it will need to be renamed sort_headers.py, and this is a naming convention we should start following for all Python files under src/tools. Cheers, Jói
thakis: friendly ping
Do you have numbers of how fast the old version of the sh script was and how fast the new one is? On 2012/11/13 16:24:38, Jói wrote: > Nico, > > I was thinking about doing this as a follow-up: Adding an import of > sort-headers.py and using its DiffAndConfirm (with should_confirm=False) after > we rewrite a file for includes. > > I've been using sort-headers.py a lot in the last year or so and for several > months I've never had to correct anything it's done, so I think this is safe > enough. > > Opinions? If it's fast enough, sure, why not. > As an aside, to import sort-headers.py it will need to be renamed > sort_headers.py, and this is a naming convention we should start following for > all Python files under src/tools. You can do sort_headers = import('sort-headers'), then you don't need to rename the script. > > Cheers, > Jói
> You can do sort_headers = import('sort-headers'), then you don't need to > rename > the script. Agreed, and I will just do this for now, but we should probably follow standard Python naming conventions for Python scripts, to make them importable in the standard fashion. > Do you have numbers of how fast the old version of the sh script was and how > fast the new one is? The new script is considerably faster, at around 2.5s real and user time for the new script, vs. around 4.5s real, 11s user for the older script. This is on my Z600, not an SSD, just an HDD. See methodology at bottom of email. Doing the timing test, I noticed two things: a) The new script is safer; for instance, when moving chrome/browser/ui/browser.h, the old script could cause bad include guard updates like this diff in a file that was not being moved: -#ifndef CHROME_BROWSER_UI_BROWSER_TAB_STRIP_MODEL_DELEGATE_H_ -#define CHROME_BROWSER_UI_BROWSER_TAB_STRIP_MODEL_DELEGATE_H_ +#ifndef CHROME_BROWSER_DIAGNOSTICS_BROWSER_TAB_STRIP_MODEL_DELEGATE_H_ +#define CHROME_BROWSER_DIAGNOSTICS_BROWSER_TAB_STRIP_MODEL_DELEGATE_H_ b) The old script would update path references in comments, which is nice. I'll fix this for // comments and upload a new version. Methodology of timing: I saved the following as /tmp/test.sh, first result below that is the old script, second is the new script, third is the old script again (to make sure I wasn't seeing bias due to warm cache or some such): === snip === git mv chrome/browser/defaults.h chrome/browser/diagnostics/ git mv chrome/browser/defaults.cc chrome/browser/diagnostics/ git mv chrome/browser/icon_manager.h chrome/browser/diagnostics/ git mv chrome/browser/icon_manager.cc chrome/browser/diagnostics/ git mv chrome/browser/ui/browser.h chrome/browser/diagnostics/ git mv chrome/browser/ui/browser.cc chrome/browser/diagnostics/ ./tools/git/mass-rename.sh === snip === joi@bunt:~/c/chrome/src (lk1114)$ time bash /tmp/test.sh Processing: chrome/browser/ui/browser.cc -> chrome/browser/diagnostics/browser.cc Processing: browser/ui/browser.cc -> browser/diagnostics/browser.cc Processing: chrome/browser/ui/browser.h -> chrome/browser/diagnostics/browser.h Processing: CHROME_BROWSER_UI_BROWSER -> CHROME_BROWSER_DIAGNOSTICS_BROWSER Processing: browser/ui/browser.h -> browser/diagnostics/browser.h Processing: chrome/browser/defaults.cc -> chrome/browser/diagnostics/defaults.cc sed: no input files Processing: browser/defaults.cc -> browser/diagnostics/defaults.cc Processing: chrome/browser/defaults.h -> chrome/browser/diagnostics/defaults.h Processing: CHROME_BROWSER_DEFAULTS -> CHROME_BROWSER_DIAGNOSTICS_DEFAULTS Processing: browser/defaults.h -> browser/diagnostics/defaults.h Processing: chrome/browser/icon_manager.cc -> chrome/browser/diagnostics/icon_manager.cc sed: no input files Processing: browser/icon_manager.cc -> browser/diagnostics/icon_manager.cc Processing: chrome/browser/icon_manager.h -> chrome/browser/diagnostics/icon_manager.h Processing: CHROME_BROWSER_ICON_MANAGER -> CHROME_BROWSER_DIAGNOSTICS_ICON_MANAGER Processing: browser/icon_manager.h -> browser/diagnostics/icon_manager.h real 0m4.740s user 0m11.345s sys 0m4.620s joi@bunt:~/c/chrome/src (c0-consolidate)$ time bash /tmp/test.sh real 0m2.647s user 0m2.432s sys 0m1.184s joi@bunt:~/c/chrome/src (lk1114)$ time bash /tmp/test.sh Processing: chrome/browser/ui/browser.cc -> chrome/browser/diagnostics/browser.cc Processing: browser/ui/browser.cc -> browser/diagnostics/browser.cc Processing: chrome/browser/ui/browser.h -> chrome/browser/diagnostics/browser.h Processing: CHROME_BROWSER_UI_BROWSER -> CHROME_BROWSER_DIAGNOSTICS_BROWSER Processing: browser/ui/browser.h -> browser/diagnostics/browser.h Processing: chrome/browser/defaults.cc -> chrome/browser/diagnostics/defaults.cc sed: no input files Processing: browser/defaults.cc -> browser/diagnostics/defaults.cc Processing: chrome/browser/defaults.h -> chrome/browser/diagnostics/defaults.h Processing: CHROME_BROWSER_DEFAULTS -> CHROME_BROWSER_DIAGNOSTICS_DEFAULTS Processing: browser/defaults.h -> browser/diagnostics/defaults.h Processing: chrome/browser/icon_manager.cc -> chrome/browser/diagnostics/icon_manager.cc sed: no input files Processing: browser/icon_manager.cc -> browser/diagnostics/icon_manager.cc Processing: chrome/browser/icon_manager.h -> chrome/browser/diagnostics/icon_manager.h Processing: CHROME_BROWSER_ICON_MANAGER -> CHROME_BROWSER_DIAGNOSTICS_ICON_MANAGER Processing: browser/icon_manager.h -> browser/diagnostics/icon_manager.h real 0m4.843s user 0m11.413s sys 0m4.768s On Wed, Nov 14, 2012 at 7:03 PM, <thakis@chromium.org> wrote: > Do you have numbers of how fast the old version of the sh script was and how > fast the new one is? > > > On 2012/11/13 16:24:38, Jói wrote: >> >> Nico, > > >> I was thinking about doing this as a follow-up: Adding an import of >> sort-headers.py and using its DiffAndConfirm (with should_confirm=False) >> after >> we rewrite a file for includes. > > >> I've been using sort-headers.py a lot in the last year or so and for >> several >> months I've never had to correct anything it's done, so I think this is >> safe >> enough. > > >> Opinions? > > > If it's fast enough, sure, why not. > > >> As an aside, to import sort-headers.py it will need to be renamed >> sort_headers.py, and this is a naming convention we should start following >> for >> all Python files under src/tools. > > > You can do sort_headers = import('sort-headers'), then you don't need to > rename > the script. > > >> Cheers, >> Jói > > > > > http://codereview.chromium.org/11358216/
Nice! http://codereview.chromium.org/11358216/diff/1004/tools/git/move_source_file.py File tools/git/move_source_file.py (right): http://codereview.chromium.org/11358216/diff/1004/tools/git/move_source_file.... tools/git/move_source_file.py:31: def MoveFile(from_path, to_path, already_moved): It's a bit confusing that this is called MoveFile and has a dont_move parameter. Can you split this into two functions, one for moving, and one for updating stuff, and then call only the update function if --already-moved is passed?
Added support for updating path references in comments. This made the code a bit messier so I extracted a MultiFileFindReplace method instead of the multiple paths through the previous UpdateReferences method. Now that re.sub is being used to update the comments we find, the tool is a little slower to run the test case, but still faster than the original. It now takes about 3s real time, 8s user time to run the test case. Cheers, Jói http://codereview.chromium.org/11358216/diff/1004/tools/git/move_source_file.py File tools/git/move_source_file.py (right): http://codereview.chromium.org/11358216/diff/1004/tools/git/move_source_file.... tools/git/move_source_file.py:31: def MoveFile(from_path, to_path, already_moved): On 2012/11/14 19:51:36, Nico wrote: > It's a bit confusing that this is called MoveFile and has a dont_move parameter. > Can you split this into two functions, one for moving, and one for updating > stuff, and then call only the update function if --already-moved is passed? Done.
Argh, rietveld somehow ate what I just typed. In short: Is the comment processing really worth it? It's a lot more code (enough to suggest this script should have a test), takes 20% longer, isn't perfect (doesn't find directory-relative references) etc. If you want to do it, can you do it in a follow-up, as it's unrelated to what this CL is about? http://codereview.chromium.org/11358216/diff/6001/tools/git/move_source_file.py File tools/git/move_source_file.py (right): http://codereview.chromium.org/11358216/diff/6001/tools/git/move_source_file.... tools/git/move_source_file.py:89: grep_pattern % original, '--'] + file_globs, If you pass -E, then the called doesn't have to escape '(' and so on, which hides the fact that this shells out to something non-python a bit better. Do you need to regex-escape `original`?
(the rest looks good)
Removed the comment-finding stuff from this change, as requested. Will move it to the follow-up change. Backing out the extraction of MultiFileFindAndReplace from UpdatePostMove (previously UpdateUsages) altogether wouldn't make sense though, it is much more nicely factored this way and is the same amount of code (it's 18 lines more, but that includes 19 more lines of comments than in patch set 3's UpdateUsages). Cheers, Jói http://codereview.chromium.org/11358216/diff/6001/tools/git/move_source_file.py File tools/git/move_source_file.py (right): http://codereview.chromium.org/11358216/diff/6001/tools/git/move_source_file.... tools/git/move_source_file.py:89: grep_pattern % original, '--'] + file_globs, On 2012/11/15 18:19:02, Nico wrote: > If you pass -E, then the called doesn't have to escape '(' and so on, which > hides the fact that this shells out to something non-python a bit better. Do you > need to regex-escape `original`? Done.
LGTM, some optional comments below. (In particular, no changes required for the first comment) https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... File tools/git/move_source_file.py (right): https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:59: guard_formats): For what it's worth, this interface looks more complicated than necessary to me. I'd just have original, replacement, file_globs. I'd git grep for original, and use re.sub(original, replacement) for the replacement (then the replacement can use capture group references). https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:63: them by formatting |grep_pattern| with |original| and running git nit: Don't linebreak in the middle of `git grep` https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:105: root, or the .gyp(i) standard full path from root minus first path The gyp standard is not full path from root minus first path, it's "relative to gyp file", and most gyp files happen to be one dir in. The code is fine and this is a reasonable heuristic, but the comment should say that this is a heuristic. I'd just omit the last three lines and instead say "tries to update all gyp files" https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:124: def PathMinusFirstComponent(path): nit: Add "# foo/bar/baz -> bar/baz", this makes it easier to understand what this is doing (for me at least)
Thanks, responded to the nits, will think about the other for the follow-up change. Committing now. Cheers, Jói https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... File tools/git/move_source_file.py (right): https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:59: guard_formats): On 2012/11/20 18:32:39, Nico wrote: > For what it's worth, this interface looks more complicated than necessary to me. > I'd just have original, replacement, file_globs. I'd git grep for original, and > use re.sub(original, replacement) for the replacement (then the replacement can > use capture group references). That's a good point. Using re.sub seems like it may be a little slower (I think it was the reason the comment updates were slower) but I plan to start using it anyway in the follow-up. I won't change anything in this change, but will think about the interface before sending the follow-up change for review again. https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:63: them by formatting |grep_pattern| with |original| and running git On 2012/11/20 18:32:39, Nico wrote: > nit: Don't linebreak in the middle of `git grep` Done. https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:105: root, or the .gyp(i) standard full path from root minus first path On 2012/11/20 18:32:39, Nico wrote: > The gyp standard is not full path from root minus first path, it's "relative to > gyp file", and most gyp files happen to be one dir in. The code is fine and this > is a reasonable heuristic, but the comment should say that this is a heuristic. > I'd just omit the last three lines and instead say "tries to update all gyp > files" Done. https://codereview.chromium.org/11358216/diff/11002/tools/git/move_source_fil... tools/git/move_source_file.py:124: def PathMinusFirstComponent(path): On 2012/11/20 18:32:39, Nico wrote: > nit: Add "# foo/bar/baz -> bar/baz", this makes it easier to understand what > this is doing (for me at least) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/11358216/14001
Change committed as 168926 |