[Bf-committers] Code Review

[Dalai Felinto]

Hello there,
after 3 days of real-beta-testing, a lot of comments, 3 reviewers and
3 patches here is my personal feedback so far. The system is really
good. It sometimes feels even better than the available tools I have
offline (*).

Regardless of the points presented next I think this system can
definitively help. For new coders - e.g. GSOCs, where the mentor (and
others) can assist the student directly in any commit; to regular
patch review; to daily commits (although I wonder what you, Brecht,
have in mind for this logistic-wise).Even if nothing changes I found
it already to be quite a resource for us to use. Thanks again for
putting that together.

Positive points:
===========
1) in-code comments - it's definitively a joy to explain why you are
taking some decisions when you can pinpoint that directly in the code
line.

2) FUN - it's fun to comment the code, and I believe it's fun to
review it as well.

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.

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

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).

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.

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).

Cheers,
Dalai

(*) this may seem as a bit of overstatement. And the fact that I'm yet
to make 'meld' to work in OSX does contribute to that. Nevertheless it
bits the slow svn tortoise worlflow in windows. And even in Linux I
haven't seen a tool that mixes "svn diff" with "meld" (or svn base in
tortoise) like this.

2011/3/11 Brecht Van Lommel <brechtvanlommel at pandora.be>:
> Hi Raul,
>
> On Fri, Mar 11, 2011 at 8:59 PM, Tom M <letterrip at gmail.com> wrote:
>>> So can I stick to my old way of send my patches? many times I don't have
>>> the possibility to choose :(
>>
>> Obviously you will have to, although others can likely upload the
>> patch for you if you like.
>
> It's not the intention to make this the required way to submit patches
> just yet, it's only an experiment. So you should be fine sending
> patches as usual.
>
> I'll try to find out if this is a bug or if we can solve it somehow,
> in any case this tool should not get in the way of submitting patches.
>
> Brecht.
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>