[Bf-committers] Code Review

[Brecht Van Lommel]

Hi,

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.

Brecht.