Using self.GetDescription() uses the description from the web.
Given that the user is purposefully disassociating their current
branch from the review on the web, they may have already changed
the commit message. We shouldn't set it back.
Bug: 742730
Change-Id: I0545cb6288c332fd475d1de7fb302f71ee41a415
Reviewed-on: https://chromium-review.googlesource.com/578229
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
In Rietveld, adding a comment to a change automatically
published it no matter what. In Gerrit, we need to explicitly
mark the change as Ready for Review. This CL adds a new
parameter to the wrapper methods around the SetReview
API so that they can mark changes as Ready.
Bug: 740950
Change-Id: Icb2ad7c5beb03a4760657a761841745f0d75514e
Reviewed-on: https://chromium-review.googlesource.com/572031
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
A regression in git-blame prints an incorrect error message which causes
this test case to fail. Alter the test to only check the start of the
string, until the bug is fixed upstream.
Bug: 737688
Change-Id: I4045cb8792d8abe984215c7198e213b23e9f6f5d
Reviewed-on: https://chromium-review.googlesource.com/567778
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Also adds tests for the bug and for --json output.
R=agable@chromium.org, phajdan@chromium.org
Change-Id: I4e2208fdad8e23d48d27d0a354470336a7b86180
Reviewed-on: https://chromium-review.googlesource.com/570030
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Although git-cl-upload warns when uploading a new patchset
to a change owned by someone else, if the uploader has run
'git cl issue 0', then git-cl believes they'll be uploading
a new change, so it doesn't bother checking. However, once
the upload begins, Gerrit notices the Change-Id in the commit
message, and instead adds a new patchset to someone else's
review (if the uploader is a committer).
This change introduces some logic to git-cl-issue to also
remove any Change-Id from the commit message when a user
tries to clear the metadata about their branch.
Bug: 741648
Change-Id: I6c7c3b24a7fc09c68220c8200b732fbdf9cf1fd3
Reviewed-on: https://chromium-review.googlesource.com/568267
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Remove Windows SVN bootstrapping and some SVN tooling. Since
"depot_tools" is no longer sync'd to SVN, and we have been committed to
Git for years now, this is obsolete. Any transition code will never
reach SVN users, and any remaining code should not be used by Chromium
developers.
BUG=chromium:630904
TEST=unit
Change-Id: Ie984e8400a748702b125eaeed8157719ef4b88cc
Reviewed-on: https://chromium-review.googlesource.com/562748
Commit-Queue: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Due to relaxation of when last paragraph of commit message is
is consider as containing footers, `git cl land` started removing
non-canonic footer lines from last paragraph if Git Numberer is enabled
on a repo. This only manifests in manual lands of Rietveld CLs or
bypassing code review entirely.
R=agable@chromium.org
Bug: 736852
Change-Id: I3972c590c3959974157ada9de9891a3c08bd385a
Reviewed-on: https://chromium-review.googlesource.com/562278
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
R=dpranke@chromium.org, phajdan.jr@chromium.org
Bug: 570091
Change-Id: Ic2aa1a9fe18f3fe8d5aa6fa4c4e9269106b36092
Reviewed-on: https://chromium-review.googlesource.com/553719
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Di Mu <dimu@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
It seems like some folks are confused by additional patchsets
after the first putting the change back into WIP mode. This
confusion is honestly understandable. Maybe we try only setting
it on the very first upload, and just controlling the notify
parameter for future patchsets.
Bug: 721836, 737675
Change-Id: If56e5c71e0c6b3b46c2e30ac0b6d80b878218181
Reviewed-on: https://chromium-review.googlesource.com/552779
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: smut <smut@google.com>
Commit-Queue: Aaron Gable <agable@chromium.org>
I added this when we were moving from gcc to clang on OS X many years
ago. Now that gcc has been deprecated on mac for many years, this
is safe to remove (clang errors on this pattern).
This check took 0.6s during `git cl presubmit` on a recent change of
mine, so it should speed presubmit up a bit.
Bug: none
Change-Id: Ia29b046807582e056115519fb5b34ee8a1b9ff91
Reviewed-on: https://chromium-review.googlesource.com/553238
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
The previous CL forgot that the lack of '%wip' doesn't
mark a change ready-to-review, you have to explicitly pass
'%ready' in the refspec to do that.
TBR=tandrii@chromium.org
Bug: 721836
Change-Id: Iea82222d64edf1b73fefa9bca3feec4188e35ab3
Reviewed-on: https://chromium-review.googlesource.com/551005
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Work-In-Progress is a new change flag that can be set on
Gerrit changes. While a change is in WIP mode, certain things
are different:
* It doesn't send emails except to the change owner
* The "Reply" button becomes "Start Review"
* When a change is moved out of WIP, it sends a special
"ready for review" message to any new reviewers
This is much more similar to the Rietveld model, where users
would "Publish" their changes for the reviewers to look at.
Bug: 721836
Change-Id: I3b9697e311fa176cb679ecefbfead9bb32b6afaf
Reviewed-on: https://chromium-review.googlesource.com/549015
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Bug: 570091
Change-Id: Ia7f81a81d7df75004c5f8b7560dfd50a14f4cddd
Reviewed-on: https://chromium-review.googlesource.com/549355
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Michael Moss <mmoss@chromium.org>
Bug: 570091
Change-Id: Iae9dad68a75d751ceac6379baac588f32c59aa06
Reviewed-on: https://chromium-review.googlesource.com/548935
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
This is a reland of https://chromium-review.googlesource.com/c/541280/
with a fix for https://bugs.chromium.org/p/chromium/issues/detail?id=735418
(patchset 1 is original patch, patchset 2 has the fix).
Keep deps_os entries in dependencies, just with should_process set to False
for entries not applicable to target OS list.
This way gclient flatten has proper access to dependencies e.g. to evaluate
recursedeps referring to deps_os entries other than active OS.
Allow but ignore deps_os overriding a value with None, since that does not
fit the new model. There's no correctness harm in not checking out a repo.
Allow "overrides" setting given dependency to the same value. This seems
fairly common, especially for mac/ios and unix/android, even in chromium/src.
Bug: 570091, 735418
Change-Id: I6eba0e4be202212eb86cb959c18f2b2f0c1452b9
Reviewed-on: https://chromium-review.googlesource.com/543076
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This reverts commit 529d6a4e4a.
Reason for revert: broke developers and CI/Try checkouts.
Bug: 735418
Original change's description:
> gclient: include deps_os entries in dependencies
>
> Keep deps_os entries in dependencies, just with should_process set to False
> for entries not applicable to target OS list.
>
> This way gclient flatten has proper access to dependencies e.g. to evaluate
> recursedeps referring to deps_os entries other than active OS.
>
> Allow but ignore deps_os overriding a value with None, since that does not
> fit the new model. There's no correctness harm in not checking out a repo.
>
> Allow "overrides" setting given dependency to the same value. This seems
> fairly common, especially for mac/ios and unix/android, even in chromium/src.
>
> Bug: 570091
> Change-Id: I2037a1ecc5fd2da6b5f73061548b81fc79ba2e72
> Reviewed-on: https://chromium-review.googlesource.com/541280
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
TBR=phajdan.jr@chromium.org,dpranke@chromium.org
Change-Id: Iaa0c39865908a5b25c15dda54ba61c0e76abcbea
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 570091
Reviewed-on: https://chromium-review.googlesource.com/543138
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Keep deps_os entries in dependencies, just with should_process set to False
for entries not applicable to target OS list.
This way gclient flatten has proper access to dependencies e.g. to evaluate
recursedeps referring to deps_os entries other than active OS.
Allow but ignore deps_os overriding a value with None, since that does not
fit the new model. There's no correctness harm in not checking out a repo.
Allow "overrides" setting given dependency to the same value. This seems
fairly common, especially for mac/ios and unix/android, even in chromium/src.
Bug: 570091
Change-Id: I2037a1ecc5fd2da6b5f73061548b81fc79ba2e72
Reviewed-on: https://chromium-review.googlesource.com/541280
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
This is a partial revert of https://chromium-review.googlesource.com/c/527345/
Turns out more people were confused by the new behavior than
expected, so we're returning to "cherry-pick" being the default,
but supporting the collaboration workflow is important, so we're
adding a warning message and support for "reset --hard" behind a
pre-existing flag.
Bug: 723787
Change-Id: Ib6038a42e3bdcc0db93c1f32d759e9ff0e91a065
Reviewed-on: https://chromium-review.googlesource.com/538137
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This makes it possible to run hooks properly
in flattened DEPS.
Bug: 570091
Change-Id: If8175a57ebe8f607bd4ac83d4a26dcc4cc18165c
Reviewed-on: https://chromium-review.googlesource.com/535476
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
This will be useful e.g. to add cwd support for flatten.
No intended behavior change.
Bug: 570091
Change-Id: I014f97739676d55f6d5b37c10afd9221b1d0978d
Reviewed-on: https://chromium-review.googlesource.com/534193
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Currently, "bot_update" relies on a BuildBot cleanup mechanism and, to a
lesser extent, the standard BuildBot directory layout. Both of these are
problematic when projecting it into other circumstances, notably
"remote_run" and LUCI.
Have "bot_update" handle its own cleanup. It will now choose a cleanup
directory within the hierarchy of its checkout, and explicitly purge it
prior to execution if it exists. This enforces its expected behavior in
all circumstances and removes its expectations of the greater checkout
layout.
Export "cleanup_dir" via "infra_paths" to point to "build.dead" when
running on BuildBot builds. Otherwise, it is a default directory which,
on Kitchen, is ephemeral by design.
BUG=chromium:725631
TEST=expectations
Change-Id: I664434c542a25aaa7ff3eac216208a2425730fde
Reviewed-on: https://chromium-review.googlesource.com/528057
Commit-Queue: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Ryan Tseng <hinoka@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
This CL changes the way that "git cl patch" behaves for Gerrit changes.
Previously, git-cl-patch behaved just like it did for Rietveld:
make sure you're on a branch, download the diff, apply it on top
of your branch. However, this causes problems with Gerrit. Namely,
when you upload a change to Gerrit, git-cl has to make sure that all
parents of your local change have previously been uploaded as well,
either as other changes or as commits already landed on the target
branch. But the method for "applying a patch" from Gerrit was to
cherry-pick it, and that changes the commit hash. So the resulting
commit would *not* have been uploaded to Gerrit. Thus, the
following routine didn't work with Gerrit:
$ git checkout -t origin/master -b your-work
$ git cl patch 123456
$ git checkout -tb my-work
$ #hack and commit
$ git cl upload
This would fail during the upload with a message saying that the
contents of 'your-work' hadn't been uploaded.
This CL fixes the situation by replacing the cherry-pick with
a hard reset. This means that the contents of the 'your-work'
branch will be *exactly* what was downloaded from Gerrit. Uploads
based on top of that commit will work just fine.
Finally, in a concession to some people who want 'git cl patch'
to actually apply a patch instead of performing a hard reset, if
the current branch contains local work, then rather than leaving
that work behind with a hard reset, we fall back to the old
cherry-pick behavior with a confirmation dialog and warning that
uploading will be hard.
Bug: 723787
Change-Id: I3ad164f6d3078bff00139d446bb8ce97738a1344
Reviewed-on: https://chromium-review.googlesource.com/527345
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This prevents TBR (self code-review) permissions from blocking
the CL from being uploaded at all. Instead, it will fully
upload, and then show a better error message after upload is
complete.
Bug: 729967
Change-Id: I55e3e98e200143076afcaab858064d9f5c62f8ef
Reviewed-on: https://chromium-review.googlesource.com/527325
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
This brings the gerrit version of "git cl comments" into line
with the Rietveld implementation by including file- and line-level
comments as well as top-level review comments. It requires an
extra API call to do so, so this may result in some slow-down, but
the result is worth it.
It formats the comments to match the formatting used in the
PolyGerrit UI, with the addition of visible URLs linking to
the comment since we can't hyperlink text in the terminal.
This CL also causes it to ignore messages and comments with
the 'autogenerated' tag, which are generally less interesting
and clutter the output.
Bug: 726514
Change-Id: I1fd939d90259b43886ddc209c0e727eab36cc9c9
Reviewed-on: https://chromium-review.googlesource.com/520722
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This way we can get e.g. ordered dict as needed for conditions.
Only the new logic does it, not the regular python exec logic.
Bug: 570091
Change-Id: Ia5554e5b018085b3b9bd876b7f28a9f8e54a7984
Reviewed-on: https://chromium-review.googlesource.com/522564
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
This is a step towards implementing conditions.
Bug: 570091
Change-Id: I99467033082d96021854c23dcff3fc2b56f995b4
Reviewed-on: https://chromium-review.googlesource.com/517107
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
_CalculateAddedDeps calls into depot tools' scm.py GetOldContents to
compare previous and current versions of the DEPS file. GetOldContents
attempts to 'git show <sha>:<path>'. The problem on Windows is that path
uses '\' but git show seems to want a posix path. Git show therefore
doesn't find the file and returns an empty output, leading presubmit to
think all the deps are new.
Bug:725933
Change-Id: Ifbbfbcba4be466d9be623826818fd191bd2ca525
Reviewed-on: https://chromium-review.googlesource.com/514142
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
For a bit of context, see the TODO in the code --
I think that the original intent of that TODO was that we to make
the way that CQ dry runs are triggered consistent, and also make
the behavior of dry runs consistent across different commands.
Change-Id: I80dfc31ade302a6af7fa84011e2871d416ea9c96
Reviewed-on: https://chromium-review.googlesource.com/518930
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Some recipes interacting with older revisions (e.g. bisect)
will need this.
Bug: 570091
Change-Id: I38e5ffa2db1a9bfae3667f015f00977c32ebe51e
Reviewed-on: https://chromium-review.googlesource.com/519407
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Aaron Gable
Matt Giuca
Andrii Shyshkalov
Dan Jacques
Paweł Hajdan, Jr
Michael Moss
Nico Weber
Dan Jacques
Pierre-Antoine Manzagol
Quinten Yearsley