Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14942012:
Fix 'git cl format' (Closed)
|
Created:
7 years, 7 months ago by digit1 Modified:
7 years, 7 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionFix 'git cl format'
This patch fixes the implementation as follows:
1/ Fix '--full' option to check for all changes in the
current branch relative to upstream. The old code only
considered currently modified files, which made little
sense.
2/ Fix '--full' option to apply Chromium style too.
3/ Use a proper source file filter. The original code used
".*.cc .*.cpp .*.h" as the filter, which didn't catch anything !?
4/ Add --no-ext-diff to make it work with custom "git diff" handlers.
R=agable@chromium.org, maruel@chromium.org, pliard@chromium.org
BUG=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=200832
Patch Set 1 #
Total comments: 2
Patch Set 2 : Implement Marc-Antoine's recommendations #
Total comments: 4
Patch Set 3 : Better way to find base commit. #Patch Set 4 : Use splitlines() #
Total comments: 6
Patch Set 5 : nits #
Total comments: 2
Patch Set 6 : remove trailing \ + add a space. #Patch Set 7 : I clearly didn't understand agable@ sentence :) #Messages
Total messages: 19 (0 generated)
Thanks for the fixes! This LGTM. I'm not sure what's up with the source filter -- it definitely worked for me in my local testing; but this version works for me too so if you think it will work for more people it sounds good.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/14942012/1
Presubmit check for 14942012-1 failed and returned exit status 1. INFO:root:Found 1 file(s). INFO:PRESUBMIT:Running pylint on 60 files Running presubmit commit checks ... Running /b/commit-queue/workdir/tools/depot_tools/PRESUBMIT.py Running tests/gclient_utils_test.py Running tests/watchlists_unittest.py Running tests/checkout_test.py Running tests/scm_unittest.py Running tests/gcl_unittest.py Running tests/patch_test.py Running tests/fix_encoding_test.py Running tests/presubmit_unittest.py Running tests/trychange_unittest.py Running tests/rietveld_test.py Running tests/owners_unittest.py Running tests/breakpad_unittest.py Running tests/gclient_test.py Running tests/gclient_smoketest.py Running tests/subprocess2_test.py Running tests/git_cl_test.py Running tests/gclient_scm_test.py INFO:root:/usr/bin/python /b/google_appengine/dev_appserver.py . --port 8080 --storage /tmp/rietveld_testtzy8U8 --clear_search_indexes --skip_sdk_update_check Setting up test upstream git repo... Setting up test git repo... TESTING: uploading to bogus server test | 1 + 1 file changed, 1 insertion(+) Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit upload checks ... Presubmit checks passed. Upload server: http://bogus.example.com:80 (change with -s/--server) Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Got exception while uploading -- saving description to /home/chrome-bot/.git_cl_description_backup TESTING: description was backed up PASS Setting up test upstream git repo... Setting up test git repo... TESTING: git-cl upload wants a server TESTING: git-cl status has no issue TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 77 0 0 100 77 0 1640 --:--:-- --:--:-- --:--:-- 1673 TESTING: Base URL contains branch name TESTING: git-cl push ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'foo-quux\n\nReview URL: http://localhost:8080/5629499534213120' Closing issue (you may be prompted for your codereview password)... TESTING: committed code has proper description TESTING: issue no longer has a branch TESTING: upstream repo has our commit PASS Setting up test SVN repo... Setting up test git-svn repo... The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to Branch feature_branch set up to track local ref refs/remotes/trunk. TESTING: Guessing upstream branch for refs/remotes/trunk The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to Branch feature_branch set up to track local ref refs/remotes/some_branch. TESTING: Guessing upstream branch for refs/remotes/some_branch PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl dcommits ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'test\n\nBUG=\n\nReview URL: http://localhost:8080/5910974510923776' Closing issue (you may be prompted for your codereview password)... PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. Switched to branch 'master' Deleted branch abandoned (was 047cbdc). TESTING: git-cl status dropped abandoned branch PASS Setting up test SVN repo... Setting up test remote git-svn-submodule repo... Switched to branch 'master' TESTING: dcommitted code Switched to branch 'git-cl-cherry-pick' Using 50% similarity for rename/copy detection. Override with --similarity. Description: 'dcommit' TESTING: svn got new revision TESTING: svn diff is correct TESTING: git svn fetch gets new svn revision PASS Setting up test upstream git repo... Setting up test git repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: git-cl upload hook fails Command "git --no-pager config rietveld.server" failed. Could not find settings file. You must configure your review setup by running "git cl config". TESTING: git-cl dcommit hook fails Command "git --no-pager config rietveld.server" failed. Could not find settings file. You must configure your review setup by running "git cl config". PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: dcommitted code Using 50% similarity for rename/copy detection. Override with --similarity. Description: 'dcommit' TESTING: post-cl-dcommit hook executed PASS Setting up test upstream git repo... Setting up test git repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: description shouldn't contain unrelated commits PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue TESTING: git cl patch 5981343255101440 PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: git-cl upload wants a server TESTING: git-cl status has no issue TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 77 0 0 100 77 0 1678 --:--:-- --:--:-- --:--:-- 1711 TESTING: git-cl dcommits ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'foo-quux\n\nReview URL: http://localhost:8080/5840605766746112' Closing issue (you may be prompted for your codereview password)... TESTING: dcommitted code has proper description TESTING: issue no longer has a branch TESTING: upstream svn has our commit PASS Checking out rietveld... Running save-description-on-failure.sh Running push-basic.sh Running upstream.sh Running submit-from-new-dir.sh Running abandon.sh Running submodule-merge-test.sh Running upload-local-tracking-branch.sh Running hooks.sh Running post-dcommit-hook-test.sh Running upload-stale.sh Running patch.sh Running basic.sh ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: depot_tools/git_cl.py Presubmit checks took 96.2s to calculate.
Damn, I need an owner approval :). Added a few ones as reviewers. Thanks.
https://chromiumcodereview.appspot.com/14942012/diff/1/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/1/git_cl.py#newcode1959 git_cl.py:1959: diff_cmd += ['--name-only'] I highly prefer append() even if it is clunky. It's because it's just too easy to get the wrong result otherwise. https://chromiumcodereview.appspot.com/14942012/diff/1/git_cl.py#newcode1965 git_cl.py:1965: diff_cmd += ['@{u}', '--'] + ['*' + ext for ext in CLANG_EXTS] Note that @{u} may not be exactly what you want. I recommend you to use the cl.GetUpstreamBranch() function like used elsewhere. Also, even that may not get the right diff. For example, the user may have called git fetch origin but hadn't rebased yet. So you want to use the merge-base commit, e.g. git merge-base HEAD cl.GetUpstreamBranch() I understand you are simply moving code around but the previous code was wrong.
Thanks Marc-Antoine, I've implemented your recommendations. PTAL.
https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py#newcode1969 git_cl.py:1969: upstream_commit = RunGit(['merge-base', 'HEAD', cl.GetUpstreamBranch()]) Note that cl.GetUpstreamBranch() could return nothing or upstream_commit could be empty. In either case, just print an error message to explain the situation and exit. With the current code, it'd diff against the last commit, which may be unexpected. https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py#newcode1979 git_cl.py:1979: files = diff_output.split() I'd prefer splitlines() so you don't break files with whitespace in them. In practice we don't support these but still.
https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py#newcode1969 git_cl.py:1969: upstream_commit = RunGit(['merge-base', 'HEAD', cl.GetUpstreamBranch()]) I see, I've updated the code to do that. Thanks, though I don't have a good way to test the new code path, hum... https://chromiumcodereview.appspot.com/14942012/diff/8001/git_cl.py#newcode1979 git_cl.py:1979: files = diff_output.split() On 2013/05/17 14:13:59, Marc-Antoine Ruel wrote: > I'd prefer splitlines() so you don't break files with whitespace in them. In > practice we don't support these but still. Done.
https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1970 git_cl.py:1970: if cl: It's an object so it'll always be not false. https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1972 git_cl.py:1972: upstream_commit = upstream_commit.splitlines() strip(), not splitlines https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1973 git_cl.py:1973: if len(upstream_commit) != 1: if not upstream_commit: DieWithError
https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1970 git_cl.py:1970: if cl: Doh :) https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1972 git_cl.py:1972: upstream_commit = upstream_commit.splitlines() On 2013/05/17 15:13:03, Marc-Antoine Ruel wrote: > strip(), not splitlines Done. https://chromiumcodereview.appspot.com/14942012/diff/7003/git_cl.py#newcode1973 git_cl.py:1973: if len(upstream_commit) != 1: On 2013/05/17 15:13:03, Marc-Antoine Ruel wrote: > if not upstream_commit: > DieWithError Done.
lgtm -- you have my owner approval to land once you get agable and maruel's okay on your CL by addressing their comments
lgtm with nit https://chromiumcodereview.appspot.com/14942012/diff/16001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/16001/git_cl.py#newcode1976 git_cl.py:1976: DieWithError('Could not find base commit for this branch.' \ Remove \ it's not needed. Strings are concatenated automatically, just like in C++.
And make sure you add a space when you remove the \. re-LGTM. maruel, thanks for your help with GetUpstreamBranch etc.
https://chromiumcodereview.appspot.com/14942012/diff/16001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/14942012/diff/16001/git_cl.py#newcode1976 git_cl.py:1976: DieWithError('Could not find base commit for this branch.' \ On 2013/05/17 16:36:38, Marc-Antoine Ruel wrote: > Remove \ it's not needed. Strings are concatenated automatically, just like in > C++. Done, thanks. Clearly we need to add to git_cl.py the ability to reformat itself, what could possibly go wrong ;-p
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/14942012/15003
Message was sent while issue was closed.
Change committed as 200832
Message was sent while issue was closed.
On 2013/05/17 17:01:47, I haz the power (commit-bot) wrote: > Change committed as 200832 FTR, since you didn't add any unit test, you'll be forever on the hook if any breakage ever happens. It's a trade off, I don't consider these utility|optional commands to necessarily require a unit test.
On Fri, May 17, 2013 at 8:32 PM, <maruel@chromium.org> wrote: > On 2013/05/17 17:01:47, I haz the power (commit-bot) wrote: > >> Change committed as 200832 >> > > FTR, since you didn't add any unit test, you'll be forever on the hook if > any > breakage ever happens. It's a trade off, I don't consider these > utility|optional > commands to necessarily require a unit test. > > https://chromiumcodereview.**appspot.com/14942012/<https://chromiumcodereview... > Yes, I understand this fully :). I considered adding a unit test, but frankly the Mocking framework being used is insane for this kind of thing. Would you be ok with a standalone test (git_cl_test_format.py maybe) that doesn't uses it? |
