git-cl for gerrit broken without --squash |
|||||||
Issue description
What steps will reproduce the problem?
1. git clone clank
2. git new-branch foo
3. make a change
4. git cl upload
5. git cl land
What is the expected result?
6. The cl lands
What happens instead of that?
7. The cl doesn't land:
Using 50% similarity for rename/copy detection. Override with --similarity.
WARNING: some changes from local branch haven't been uploaded
Do you want to submit latest Gerrit patchset and bypass hooks?
WARNING: bypassing hooks and submitting latest uploaded patchset
Traceback (most recent call last):
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 4907, in <module>
sys.exit(main(sys.argv[1:]))
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 4889, in main
return dispatcher.execute(OptionParser(), argv)
File "/work/infra/depot_tools/subcommand.py", line 252, in execute
return command(parser, args[1:])
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 4068, in CMDland
return SendUpstream(parser, args, 'land')
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 3677, in SendUpstream
options.verbose)
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 2238, in CMDLand
self.SubmitIssue(wait_for_merge=True)
File "/usr/local/google/home/agable/local/bin/depot_tools/git_cl.py", line 2198, in SubmitIssue
wait_for_merge=wait_for_merge)
File "/work/infra/depot_tools/gerrit_util.py", line 545, in SubmitChange
return ReadHttpJsonResponse(conn, ignore_404=False)
File "/work/infra/depot_tools/gerrit_util.py", line 356, in ReadHttpJsonResponse
conn, expect_status=expect_status, ignore_404=ignore_404)
File "/work/infra/depot_tools/gerrit_util.py", line 349, in ReadHttpResponse
raise GerritError(response.status, reason)
gerrit_util.GerritError: Not Found: Not found: None
The problem is that git-cl thinks the CL has changes which haven't been uploaded? Because of this code:
last_upload = RunGit(['config',
'branch.%s.gerritsquashhash' % self.GetBranch()],
error_ok=True).strip()
# Note: git diff outputs nothing if there is no diff.
if not last_upload or RunGit(['diff', last_upload]).strip():
print('WARNING: some changes from local branch haven\'t been uploaded')
So last_upload is false-ish. Why? because branch.foo.gerritquashhash hasn't been set. Why? Because of this code:
if options.squash:
regex = re.compile(r'remote:\s+https?://[\w\-\.\/]*/(\d+)\s.*')
change_numbers = [m.group(1)
for m in map(regex.match, push_stdout.splitlines())
if m]
if len(change_numbers) != 1:
DieWithError(
('Created|Updated %d issues on Gerrit, but only 1 expected.\n'
'Change-Id: %s') % (len(change_numbers), change_id))
self.SetIssue(change_numbers[0])
RunGit(['config', 'branch.%s.gerritsquashhash' % self.GetBranch(),
ref_to_push])
So it only sets that config value if --squash is passed. Which I didn't do, because I only created a single commit on my local development branch.
,
May 5 2016
tl;dr see https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/squash/chromium-dev/30PqVAOJdbc/_L-zXusnMAAJ your diangosis is correct. I don't have enough sanity left to maintain both squash and non-squash mode. See the link above. On top of that, "git cl land" for Gerrit is supposed to call "Submit" on Gerrit, and not do what it used to do for Rietveld. The reason I did that was because i assumed that, well, "git cl land" didn't work with Gerrit before, so I can totally implement it as I see most fit. Clearly, I was wrong - it sorta worked in Gerrit, and somebody even relied on it. Now, we can easily revisit this and make "git cl land" instead land whatever is in user's checkout same way as if there was no issue associated with it. But this must ensure that Change-Id and branches of whatever lands matches the issue so that Gerrit automatically marks issue as "merged". And maybe more that I am not aware of. Also, see http://crbug.com/609606 for how TBR is to be handled in Gerrit.
,
May 5 2016
I'd honestly be totally happy to have "git cl land" w/ gerrit bail out and refuse to do anything if what you have doesn't match what's been uploaded. "git cl" is supposed to be about dealing with changelists -- which by definition only exist inside the code review tool -- not for arbitrary interactions with the repo. So I'm definitely not requesting that gerrit-based "git cl land" work like it used to on Rietveld. I am definitely asking that the git-cl code not rely on data which is only created via --squash, while allowing --no-squash to also work. I couldn't care less whether that's fixed by removing no-squash mode, or by not relying on that piece of config state, but it should be fixed somehow :)
,
May 7 2016
I can't deprecate the --no-squash, at least not without some pitchforks from some clank devs and I guess breaking their scripts yet again :(
,
May 17 2016
I'll look at this again in June and will actually ask for feedback from opponents of deprecating this (I have a few already). For now its available if someone wants to tackle this.
,
Aug 2 2016
FTR, git cl upload now uses --squash by default. This bug is still valid though.
,
Aug 17 2016
,
Sep 6 2016
,
Sep 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/73449b0bd49eab1e152f419102123d734896da98 commit 73449b0bd49eab1e152f419102123d734896da98 Author: tandrii <tandrii@chromium.org> Date: Wed Sep 14 13:27:24 2016 Gerrit git cl land: abort if not uploaded. R=agable@chromium.org BUG=642759, 609225 Review-Url: https://codereview.chromium.org/2317253003 [modify] https://crrev.com/73449b0bd49eab1e152f419102123d734896da98/git_cl.py
,
Sep 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/build.git/+/ba9de887ec268453298caa8641de3cc91dbfb818 commit ba9de887ec268453298caa8641de3cc91dbfb818 Author: recipe-roller <recipe-roller@chromium.org> Date: Wed Sep 14 23:34:35 2016 Roll recipe dependencies (trivial). This is an automated CL created by the recipe roller. This CL rolls recipe changes from upstream projects (e.g. depot_tools) into downstream projects (e.g. tools/build). More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug (or complain) depot_tools: https://crrev.com/f46c20fcee6e6a0a7d75788847632cd4ac18e2e9 codereview.settings: add GIT_NUMBER_FOOTER setting. (tandrii@chromium.org) https://crrev.com/5d0a0421ce27046c94177511a05699316ec8097a git_cl: update outdated TODOs. (tandrii@chromium.org) https://crrev.com/73449b0bd49eab1e152f419102123d734896da98 Gerrit git cl land: abort if not uploaded. (tandrii@chromium.org) https://crrev.com/bf42940536f6c0c123a9e6278c20bc38729b3717 git cl land to refs/pending: remove unused arg. (tandrii@chromium.org) https://crrev.com/7475196d4c32d66e1c199bf24945b7ae28255e13 repo: update to v1.12.17-cr1 (vapier@chromium.org) TBR=martiniss@chromium.org,phajdan.jr@chromium.org BUG= 642493 , chromium:632203 , 609225 ,642759 Recipe-Tryjob-Bypass-Reason: Autoroller Bugdroid-Send-Email: False Review-Url: https://codereview.chromium.org/2341613004 [modify] https://crrev.com/ba9de887ec268453298caa8641de3cc91dbfb818/infra/config/recipes.cfg
,
Sep 15 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/tools/build_limited/scripts/slave/+/cf7f08c66dda91de2fc541bad96ff9da7fcf8c51 commit cf7f08c66dda91de2fc541bad96ff9da7fcf8c51 Author: recipe-roller <recipe-roller@chromium.org> Date: Thu Sep 15 00:24:04 2016
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra.git/+/7d6811c4c58a9e280bca7bf65a8dfde001bef42a commit 7d6811c4c58a9e280bca7bf65a8dfde001bef42a Author: recipe-roller <recipe-roller@chromium.org> Date: Fri Sep 16 23:37:07 2016 Roll recipe dependencies (trivial). This is an automated CL created by the recipe roller. This CL rolls recipe changes from upstream projects (e.g. depot_tools) into downstream projects (e.g. tools/build). More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug (or complain) build: https://crrev.com/ba9de887ec268453298caa8641de3cc91dbfb818 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/e1d15cd7795cc1c5465983af8a49669242857107 Add rtc_stats_unittests to the test suite (ehmaldonado@chromium.org) https://crrev.com/37294244d9d54d639a083d382da68c50f8fe14e5 Add linux_chromium_headless_dbg trybot (perezju@chromium.org) https://crrev.com/f0f4c4a27746dee8a53234f03e452dba556deb55 Reland of Enable the ninja up-to-date check for Android builders (agrieve@chromium.org) https://crrev.com/2744734cf3aa1d8a02d0f0b7abf99635e9f2aa48 Revert of Enable the ninja up-to-date check for Android builders (patchset #1 id:1 of https://codereview.chromium.org/2343563003/ ) (agrieve@chromium.org) https://crrev.com/1575589a1d8f61fee45ec53ccbf77f073fd38847 recipe_modules/chromite: Use "build_type". (dnj@chromium.org) https://crrev.com/5799bab975aa3eda6785f36d9f7879549074b28b Reland of Enable the ninja up-to-date check for Android builders (agrieve@chromium.org) https://crrev.com/ecdf065c9d2a6e28d86c8ad333432b497158ee7a chromium.android: Enable swarming on Android arm64 builder (bpastene@chromium.org) https://crrev.com/7a71133c89aa2a770fc2326188306cf6a3a6f1e1 Revert of Enable the ninja up-to-date check for Android builders (patchset #1 id:1 of https://codereview.chromium.org/2343953002/ ) (agrieve@chromium.org) https://crrev.com/c65424c419105e0720cc0de09fb9e7a36c3d4601 Pass ninja -n in the ninja -d explain step to avoid more work (agrieve@chromium.org) https://crrev.com/8091fad9d075ecec116e1740360ff955a7654c4a Remove use_isolate from Marshmallow 64 bit Tester (bpastene@chromium.org) https://crrev.com/2f0a7c064259dc0e0bf375c2d1771544f5647a1c Changed sequence of package_build step to be before package_build_for_bisect (miimnk@google.com) https://crrev.com/194bd0247efb60371dbcae87731f13dce4fa7934 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/d2845f7e87aa167cf7df78003b5768f948803d07 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/eb686c79229ef3cf3b39e6a0807f4c98e3e93cf9 Add BuildBucket manifest scheduling support. (dnj@chromium.org) https://crrev.com/537cdb699a70d5d55f3eefb923eae1dc5104a8eb Add goma to wasm waterfall (sbc@chromium.org) https://crrev.com/b6d326d6f6f9608bba307109cc4a1c5efc5946bb Run the ninja "up-to-date" check for all compiles, but just as an fyi (agrieve@chromium.org) https://crrev.com/07a33586eff8451bbc84366055c3cfa36e7b6fea Roll recipe dependencies (nontrivial). (recipe-roller@chromium.org) https://crrev.com/79d8d758281609aa49b9b9af7e18cc187c2ee814 Disable CompilerInfoCache for a while (shinyak@chromium.org) https://crrev.com/a8f07009101d46553ea5d4bce1353ecb365dd3a5 Revert "Roll recipe dependencies (nontrivial)." (tandrii@chromium.org) https://crrev.com/cbca79967afa4f28df6b0ede337db6138eeb167b Add asan=1 to GYP_DEFINES for Dart asan builds (whesse@google.com) https://crrev.com/25b922d8edc1927c3af4bb4a8f7db6434096dca0 WebRTC: Disable iOS API Framework Builder. (ehmaldonado@chromium.org) https://crrev.com/769dbabae3b7ff11023dd1b393fcd2a12e165c65 Make sure goma_ctl is running during wasm_llvm build (sbc@chromium.org) https://crrev.com/2e89429885f7c2503565cb12d564d0f6f58a42f1 Fix Webkit capitalization for N CTS tests. (mikecase@chromium.org) https://crrev.com/e549894cc4f86f3f2107e3686d561ff83331a267 crashpad/continuous: Use bot_update. (dnj@chromium.org) https://crrev.com/f74386f7e19ff3f32693d222262d1a3ee79799ef Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/1a9a50a459c9e3c081719d3e835cac213c319136 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) depot_tools: https://crrev.com/f46c20fcee6e6a0a7d75788847632cd4ac18e2e9 codereview.settings: add GIT_NUMBER_FOOTER setting. (tandrii@chromium.org) https://crrev.com/5d0a0421ce27046c94177511a05699316ec8097a git_cl: update outdated TODOs. (tandrii@chromium.org) https://crrev.com/73449b0bd49eab1e152f419102123d734896da98 Gerrit git cl land: abort if not uploaded. (tandrii@chromium.org) https://crrev.com/bf42940536f6c0c123a9e6278c20bc38729b3717 git cl land to refs/pending: remove unused arg. (tandrii@chromium.org) https://crrev.com/7475196d4c32d66e1c199bf24945b7ae28255e13 repo: update to v1.12.17-cr1 (vapier@chromium.org) https://crrev.com/adcd4b78d5f35da535ccf82a221afb9fa389f631 presubmit_support: Remove a noisy logging.debug() (thakis@chromium.org) https://crrev.com/18ca30ca804679ee624a52e73017d234a8c0008f Teach bot_update to remove partially deleted git repos. (vadimsh@chromium.org) https://crrev.com/972ac5040176acd90c8a1ce412f75d19f77cc4e8 bot_update: ensure correct depot_tools checkout is used. (tandrii@chromium.org) https://crrev.com/15a248123d9032061486cd2d4b3f64369c93a9a5 Revert of bot_update: ensure correct depot_tools checkout is used. (patchset #2 id:20001 of https://codereview.chromium.org/2346973003/ ) (tandrii@chromium.org) https://crrev.com/7f245d07b2282f9847072fccddf7162a7e632a2d Bump git-on-windows bleeding edge version to 2.10.0. (vadimsh@chromium.org) https://crrev.com/6ac12ffd596e338c43e25dc3889e8ac552c2e885 Make bot_update.py print git version it uses. (vadimsh@chromium.org) https://crrev.com/7e16cf303221bbcf81d632924e19ddc888da9c3b owners.py: partial fix for owners-check perf regression (nick@chromium.org) TBR=martiniss@chromium.org,phajdan.jr@chromium.org BUG=646165,none,646838,632008,webrtc:6372,chromium:647812,642493,647446,chromium:627996,642793,635641,645662,chromium:632203,647046,609225,642759 Recipe-Tryjob-Bypass-Reason: Autoroller Bugdroid-Send-Email: False Review-Url: https://codereview.chromium.org/2345413002 [modify] https://crrev.com/7d6811c4c58a9e280bca7bf65a8dfde001bef42a/infra/config/recipes.cfg
,
Sep 27 2016
,
Jan 6 2017
I'm going to close this issue for a few reasons: * --squash is now the default; people using --no-squash need to know what they're doing * I think that "git cl land" should land what has been uploaded, not whatever is local. If they want to land something that hasn't been uploaded... they should upload it. So I don't think there's actually additional work that is worth doing here. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by gov...@chromium.org
, May 4 2016