[Bf-committers] Code Review

Brecht Van Lommel brechtvanlommel at pandora.be
Tue Mar 15 13:24:13 CET 2011


Thanks for the comments, I just sent a fairly small patch for review
to the list. I would like to see even changes like this reviewed, even
if most if the time the reply is just LGTM.

On Tue, Mar 15, 2011 at 9:44 AM, Dalai Felinto <dfelinto at gmail.com> wrote:
> Specific things would be nice to have:
> ===========================
> 1) raw diff between patches - this is not a big of a deal. But
> somethings it's handy to quickly look at a raw diff file to see what
> changed between patches. The tool does support side-by-side views of
> patches with highlighting of changes, so it's not a big deal for small
> patches.

I think stepping through the files that changed between two patches is
pretty quick, with j or J key, and it does show you which files
changed on the front page, it's just that you happened to change all
of them in each patch I think.

> 2) upload files - a good patch comes with a good army of test files.
> Of course one can upload a file somewhere else (I used the original
> tracker ;). But

Right, this is lacking. We could mail the list with attachments, but
that's not ideal either.

> 3) merge system - this is probably only a bug, but: if any of the
> files you are patching changed after you created the file the upload
> may fail (it failed in my first try) with no warning ! (so it indeed
> created a new 'ticket'/entry but the diffs were all invalid).

Perhaps we should report the lack of warning for this as a bug. Still
need to make a wiki page, so will note it there too.

> 4) more space to comment when you add a new patch - not a big deal
> since you can add a limitless comment when publishing the
> comment-snippets, but I was surprised to be constrained by a
> twitter-like limit.

You can make the text area bigger, either dragging or using the +
button depending on the browser.

> 5) merge of comments:
> A new system will require a new dynamic. That goes without saying.
> Trying a "workflow" on the spot I noticed that (for the current
> system) it may be interesting to merge comments. Otherwise a system of
> round of review may need to be respected. And this can harm one of the
> nicest things about this system - how anyone can review the patch at
> any time.
> For the current system, if one sends a new patch too early (e.g. that
> doesn't address all the comments in the 1st patch and still need
> review from new devs) some information can get lost. For example, (1)
> I sent a patch (2) inline commented my own code (3) people
> review/comment some points (4) I send a new patch (5) inline commented
> the changes in new patch (6) a new reviewer comes in - s/he will not
> be able to read the comments in (2) even if that part of the code is
> still present in the new patch (s/he can read it, but only if s/he
> goes through the individual batch of comments or patches).

Fair enough, I think it's good to skim through the existing comments
before reviewing anyway, but it would be good if they got preserved
between patches in the code.


More information about the Bf-committers mailing list