Re: Code review tool? (and [PATCH] fix for stencil rotating)

[Han-Wen Nienhuys]

[cc-ing Evan who wrote git-cl]

Hi Reinhold,

thanks for trying this out.  - I agree that it's not perfect, but from
my perspective as reviewer, it's actually much better than using mail
directly.  Perhaps you guys should try the reverse too,  ie. do a code
review through the tool.

On Wed, Sep 10, 2008 at 6:48 AM, Reinhold Kainhofer
<address@hidden> wrote:
> Am Dienstag, 9. September 2008 schrieb Han-Wen Nienhuys:
>> What do you think of using codereview.appspot for code reviews?
>
> While I like the idea of using something like that, I find google's service
> too limited and too svn-centric. In particular, it works on a file-basis
> rather than on patch sets.

Having written a similar tool for Google internal use (that is:
interfacing git with P4 review infrastructure), I can understand some
of these limitations: a change in both SVN and P4 can have only one
revision, which is what the patch would look like if you submit.

The per-file rather than per patch-set comment confuses me, as SVN
uses changesets too. Do you mean that you want a view that includes
all diffs in a single page?

As far as I can see, writing a review tool that deals with git
correctly would imply that you need to create a partial clone of
git-web too (you'd have to be able to navigate the commit tree).    It
would be an interesting excercise to write such a beast.

> It also automatically includes all changes and does not let you select which
> changes you want to submit. If you work on a basic issue and something else
> that is based on it, you can't upload e.g. a patch that you committed locally
> separately already from some uncommited changes.

Are you sure that git-cl sends uncommittted changes?  I would qualify
that as a bug.

> Also, if you committed
> something locally, git-cl will completely ignore this and ask you again for a
> summary (and doesn't ask for a description).
> Another issue is that it relies on your gmail.com Address (I suppose using it
> as the sender's address for the CCs), so mails to lilypond-devel will not
> automatically go throught, because these addresses are not subscribed (unless
> your are already using gmail like hanwen).

We could add an exception rule for gmail, but we should check how much
spam we got from gmail over the last months.  David?

> If that tool were tailored to git, it would take one local commit (identified
> by the hash) and upload it as a git patch, taking the summary/description
> from the patch. Patches, which are dependent on each other would then also be
> no problem.
>
> Anyway, I also did a test run for my fix to stencil rotation:
> http://codereview.appspot.com/5060
>
> Please ignore the file flags-in-scheme.ly, which just contains some debug code
> to check whether the bounding boxes are now rotated correctly. I never meant
> to submit it (and I also didn't add it to my local git commit, but git-cl
> does not look at git commits and does not let you select files/changes
> individually, but can only upload all local changes).
> Both patch sets are identical up to a small change in flags-in-scheme.ly,
> which should be ignored anyway.

-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen