Giter Club home page Giter Club logo

review-o-matic's Introduction

review-o-matic and friends

This projects includes a few scripts to help review and submit gerrit patches.

troll-o-matic

This script trolls gerrit for BACKPORT/UPSTREAM/FROMGIT/FROMLIST prefixed changes and compares the downstream backport with the upstream source. If a difference between upstream and downstream is detected, the script will print the difference between patches for the human reviewer to validate. The script also checks that the required fields are present in the commit message and checks upstream git logs for "Fixes:" references in case the backport has a bug which is already fixed upstream.

A sample config is available in the repository under config.sample.ini.

Usage

usage: troll-o-matic.py [-h] [--verbose] [--chatty] [--daemon] [--dry-run]
                        [--force-cl FORCE_CL] [--force-rev FORCE_REV]
                        [--force-all] [--force-prefix FORCE_PREFIX]
                        [--force-project FORCE_PROJECT] [--config CONFIG]

Troll gerrit reviews

optional arguments:
  -h, --help            show this help message and exit
  --verbose             print commits
  --chatty              print diffs
  --daemon              Run in daemon mode, for continuous trolling
  --dry-run             skip the review step
  --force-cl FORCE_CL   Force review a CL
  --force-rev FORCE_REV
                        Specify a specific revision of the force-cl to review
                        (ignored if force-cl is not true)
  --force-all           Force review all (implies dry-run)
  --force-prefix FORCE_PREFIX
                        Only search for the provided prefix
  --force-project FORCE_PROJECT
                        Only search for changes in the provided project
  --config CONFIG       Path to config file

Example Invocations

Daemon mode:

troll-o-matic.py --config config.ini --daemon

Target a CL for testing:

troll-o-matic.py --config config.ini --force-cl 1487385 --dry-run

submit-o-matic

This script monitors a set of patches on gerrit, flipping the CodeReview/Verify/CQReady bits on the review until it is merged.

Usage

usage: submit-o-matic.py [-h] --last_cid LAST_CID [--daemon] [--review]
                         [--verify] [--ready] [--dry-run]

Auto review/submit gerrit cls

optional arguments:
  -h, --help           show this help message and exit
  --last_cid LAST_CID  Gerrit change-id of last patch in set
  --daemon             Run in daemon mode, continuously update changes until
                       merged
  --review             Mark changes as reviewed
  --verify             Mark changes as verified
  --ready              Mark changes as ready
  --dry-run	       Practice makes perfect

Example Invocations

Flip the Verify and CQReady bits on the entire gerrit series ending in change 1439600. Continue to flip those bits until it has been marked as merged.

submit-o-matic.py --last_cid 1439600 --daemon --verify --ready

backport-o-matic

Adds the required fields to a backport commit message. Should be used in conjunction with git filter-branch to alter a series of git commits.

Usage

usage: backport-o-matic.py [-h] [--prefix PREFIX] [--tree TREE] [--bug BUG]
                           [--test TEST] [--sob SOB] [--no-preserve-tags]

    Add CrOS goo to commit range
 
    Usage:
      git filter-branch --msg-filter "backport-o-matic.py --prefix='UPSTREAM'         --bug='b:12345' --test='by hand' --sob='Real Name <email>'"
  

optional arguments:
  -h, --help          show this help message and exit
  --prefix PREFIX     subject prefix
  --tree TREE         location of git-tree
  --bug BUG           BUG= value
  --test TEST         TEST= value
  --sob SOB           "Name <email>" for SoB
  --no-preserve-tags  Overwrite existing CrOS tags

Example Invocations

git filter-branch -f --msg-filter "backport-o-matic.py --prefix='FROMGIT' --bug='None' --test='Tested, trust me' --sob='Sean Paul <[email protected]>' --tree='git://anongit.freedesktop.org/drm/drm'" 031ae70a3329..

review-o-matic

Does what troll-o-matic does above, but for your local git tree instead of gerrit.

Usage

usage: review-o-matic.py [-h] --start START [--prefix PREFIX] [--verbose]
                         [--chatty]

Auto review UPSTREAM patches

optional arguments:
  -h, --help       show this help message and exit
  --start START    commit hash to start from
  --prefix PREFIX  subject prefix
  --verbose        print commits
  --chatty         print diffs

Example Invocations

review-o-matic.py --start "$( git log --pretty=format:%H cros/chromeos-4.19.. | tail -n1 )"

relate-o-matic

Given a commit hash, this script will find other patches in the same series.

This requires that the given commit has a Link: tag in the commit message pointing to a allowed patchwork location.

Usage

usage: relate-o-matic.py [-h] [--git-dir GIT_DIR] [--verbose] [--chatty]
                         --commit COMMIT

Get related patches

optional arguments:
  -h, --help         show this help message and exit
  --git-dir GIT_DIR  Path to git directory
  --verbose          print commits
  --chatty           print diffs
  --commit COMMIT    commit hash to find related patches

Example Invocations

Find all commits in the series that contains commit d9facae6afe1

relate-o-matic.py --git-dir ~/src/kernel --commit d9facae6afe1

review-o-matic's People

Contributors

arowa-e-suliman avatar atseanpaul avatar computersforpeace avatar ddavenport-chromium avatar drinkcat avatar evangreen avatar gwendalcr avatar hsuantingchen avatar ribalda avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar

review-o-matic's Issues

Check for presence of more recent revisions for FROMLIST

For reference: https://crrev.com/c/3503330

Patchset 1 was an old version of the patch on patchwork; the most recent version of the patch had addressed feedback from that version and had a Reviewed-By.

Feature request: Is it possible to query whether there are more recent revisions of a patch available and have review-o-matic either call this out in its auto review, or even give a CR-1?

Bot validates UPSTREAM patch with reference to maintainer repo

Example of validated patch:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1644481/2

Despite being an UPSTREAM CL, the cherry-picked line is as follows:

(cherry picked from commit c27bb30e7b6d385c5bff26406089377d678f1a1d
 git://linuxtv.org/media_tree.git)

The reference does exist in Linus' tree, so ultimately this is not a big issue, but we probably don't need the reference to the maintainer tree in that case. Maybe the bot should reject such patches?

Upgrading FROMGIT to UPSTREAM uses maintainer tree

From tfiga

I just noticed that it if one changes a patch from FROMGIT to UPSTREAM
and forgets to remove the maintainer's tree URL, Seanbot uses that URL
as the upstream link for the patch, rather than a proper link to
Linus' tree. I think it could also be useful to have it warn about the
issue.

An example CL where it happened:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2379934/9#message-a516047bca792f32076c39ba9689f0aab623ccfc

Make "Signed-off-by" check more forgiving

This is a very common CR-1 vote, especially for cherry-picks (to release branches, etc.):

Your commit message is missing the following required field(s):
    Signed-off-by: xxx

I don't believe it provides much value. In fact, Signed-off-by is totally unnecessary on chromium-review:
https://chromium.googlesource.com/chromiumos/docs/+/HEAD/contributing.md#Signed_off_by
but it's useful for training people on the upstream flow (where it is required), and for interacting between the two.

Can we improve this, at least for some limited cases?

Possible cases, in no particular order:

  1. using Gerrit's Cherry Pick feature (for release branches, etc.)
  2. hitting the Gerrit "rebase" button (e.g., to break excess dependencies)
  3. adding Disallow-Recycled-Builds (to help bots submit the change properly)
  4. adding additional BUG=
  5. fixing Cq-Depend
  6. stacks of CLs collated from different uploaders

These are all things that might be done by non-original-uploaders, and yet have no real bearing on any S-o-b chain. Gerrit can easily-enough tell you that there were no changes to the code contents in many of these cases; they are just being moved around by essentially automated tools.

This appears to be the most-ignored CR-1 reason for merged CLs:
https://chromium-review.googlesource.com/q/label:code-review%253D-1%252Cseanpaul%2540chromium.org+project:chromiumos/third_party/kernel+is:merged

Examples pulled from that search:

Leaving CR-1's that people ignore causes divergence in expectations: some reviewers start to complain that there are CR-1's, and some just ignore it, because it doesn't make much sense, and people want to move on with their life.

Puzzling FROMGIT verification error

In https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1702621:

"""
...
Signed-off-by: Jonathan Cameron [email protected]
(cherry picked from commit eb7ecb5192b2dcf2992f39156aacf72f6e78eae3
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg)
...
""""

I get the error message:

"""
The commit hash(es) you've provided in your commit message could not be found
upstream. The following hash/remote/branch tuple(s) were tried:

eb7ecb5192b2dcf2992f39156aacf72f6e78eae3
from remote https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git branch togreg

Please double check your commit hash is valid in the upstream tree, and please
fully specify the remote tree and branch for FROMGIT changes (see below):

(cherry picked from commit <commit SHA>
 <remote git url> <remote git branch>)

"""

But the path does exist:

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=eb7ecb5192b2dcf2992f39156aacf72f6e78eae3

Handle TO-UPSTREAM

So here I was planning to upstream a CL, but I uploaded it to gerrit as well for staging purposes. Added "TO-UPSTREAM:" tag so I don't forget and my intentions were clear:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2309331/1#message-b37fa7764bbc2ca7e5d0962e46784ea47d6402ec

Seanbot quickly -1d it, which I suppose it's not exactly wrong (I don't want it to land as is either). But it -1d it for the wrong reasons (It assumed it was already sent somewhere upstream and downstreamed wrong).

Let me know what you think.

No support for LKML FROMLIST (i.e., non-patchwork; using lore.kernel.org/lkml / public-inbox)

Some important mailing lists don't have patchwork instances, because patchwork doesn't scale well for them. Notably, LKML doesn't have one. Instead, people use lore.kernel.org.

It'd be nice if we could get FROMLIST support for public-inbox.

I'm mostly logging this for the record, so it's easier to point at the missing feature rather than wonder why the review-o-matic skips over such CLs (e.g., https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3591058/comments/f8266795_f0d4cd01).

And even if we don't get non-patchwork support, it might be nice to post a comment about why review-o-matic couldn't handle the review.

Diff does not contain file names

When uploading a BACKPORT patch, the bot gives a diff against the upstream patch, which is super convenient. However the diff is missing the file names, which can make it difficult to analyze. Example from https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1644482/4#message-3d1c6692e0e48871695b5416ab8c58a9da8293bb:

This patch differs from the source commit.

This is expected, and this message is posted to make reviewing backports easier.
  --- 
  +++ 
  @@ -123 +122,0 @@
  --	__u8	pad;
  @@ -142 +140,0 @@
  --	__u8	pad;
  @@ -157 +154,0 @@
  --	__u8	pad;
------------------

Maybe this is because the diff is made against the patches themselves?

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.