Skip to content
Snippets Groups Projects
New look: Off

set up a code review tool

    • Closed (moved) Issue created by Sathyanarayanan Gunasekaran @gsathya

    We need a code review tool, copy pasting diffs into trac does not work well at all. ReviewBoard +1

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items 0

    • No child items are currently assigned. Use child items to break down this issue into smaller parts.

    Activity

    • Sathyanarayanan Gunasekaran added component::internal services/tor sysadmin team priority::medium resolution::wontfix status::closed type::task labels

      added component::internal services/tor sysadmin team priority::medium resolution::wontfix status::closed type::task labels

    • G
      Sathyanarayanan Gunasekaran @gsathya ·
      Author

      I spoke to weasel about this at the Tor dev meet and he said he'd be ok with giving me a Debian VM if I would set up this tool and maintain it - I'm OK with this, but I promised him that I'd test it on a personal server first, but it turns out my server is too shitty to test anything. So, I'm kinda blocked on this.

    • A
      Damian Johnson @atagar ·

      Didn't spot that we now had a ticket for this...

      11:11 < atagar> Sebastian, ioerror_: I have some experience with ReviewBoard if we end up going
with that (submitted them five patches so far this month) and I wouldn't mind setting us up an
instance. That said, I don't think that we have the people vs project ratio to make code reviews
possible.
    • Nick Mathewson
      Nick Mathewson @nickm ·

      For posterity, let me document my current methodology.

      I take your patch in whatever format it is, and I download it. If you've got a git branch, I do "git remote add yourhandle yourrepo" and then I do "git fetch yourhandle". If it's one or more "git format-patch" files, I download it. If it's a unified diff, I download it grudgingly. If it's something else, I download it extra-grudgingly.

      Then if I downloaded files, I review them with less. If I downloaded a branch, I review it commit-by-commit with "git log -p --stat --reverse master..yourhandle/yourbranch". I add -b if you seem to have messed with lots of whitespace. I might also do "git diff master...yourhandle/yourbranch" (note the three dots) to get all of your changes in the branch in one big diff. Then I write up comments, referencing commit IDs and function names, and put them in trac.

      Note that I always review a local copy, so you can't change the code out from under me.

      When I'm done, I do "git checkout master" to get to master, then "git merge yourhandle/yourbranch." Then I try it out for a while, then "git push" it.

      For merging stuff not into master but into maint-0.2.3 or something, I adjust accordingly.

    • Nick Mathewson
      Nick Mathewson @nickm ·

      FWIW, my current stance on review tools is: let's figure out what we can set up and try with minimal effort, and try it for a while.

    • I
      Jacob Appelbaum @ioerror ·

      If we can use gerrit, I'd be really happy - I think it is a fine tool.

      Here is an example: https://gerrit.chromium.org/gerrit/#/c/30029/

    • F
      Tom Lowenthal @flamsmark ·

      I'm marking this as "wontfix" for now. I'm not convinced that picking or setting up a tool is going to fix things. I'd rather work on the actual process, and if we find that our process really needs gerrit, let's set up gerrit. Process first, tools later.

      Trac:
      Status: new to closed
      Resolution: N/A to wontfix
      Cc: gsathya to gsathya, strangecharm

    • G
      Sathyanarayanan Gunasekaran @gsathya ·
      Author

      Replying to StrangeCharm:

      I'm marking this as "wontfix" for now. I'm not convinced that picking or setting up a tool is going to fix things. I'd rather work on the actual process, and if we find that our process really needs gerrit, let's set up gerrit. Process first, tools later.

      I agree. I'm reopening this ticket to discuss the process. (Or feel free to open a new ticket for this)

      The process to contribute is -

      1. clone the git repo
      2. create a feature branch
      3. hack
      4. push the branch
      5. link the branch in a ticket and change status to 'needs_review'

      The review process is -

      1. pull the branch
      2. view the diff in your favorite editor
      3. copy paste the diffs on the ticket with comments inline
      4. change status to 'needs_revision'

      I don't think this "copy paste the diffs on the ticket with comments inline" works at all, the rest is ok. I hate the whole copy pasting of diffs -- this should all be automated. Also, for the contributor It's weirdly hard to figure out which part of code the diffs refer to. I've started using github because of this and link to the compare view instead of the branch, so the reviewer can quickly see what changes I've done and see how much time it's going to take to review that.

      I'd like to know your thoughts on improving this process :)

      Trac:
      Resolution: wontfix to N/A
      Status: closed to reopened

    • Nick Mathewson
      Nick Mathewson @nickm ·

      Could we please have a new ticket for that then? This is a "set up a code review" ticket. Repurposing tickets to mean new stuff is generally a recipe for confusion.

      Trac:
      Status: reopened to closed
      Resolution: N/A to wontfix

    • Trac closed

      closed

    Please register or sign in to reply