Integrating Kallithea with TortoiseHg (plus some other comments/suggestions from my team)

[Angel Ezquerra]

On Tue, Feb 16, 2016 at 9:41 AM, Thomas De Schampheleire
<patrickdepinguin at gmail.com> wrote:
> On Tue, Feb 16, 2016 at 8:36 AM, Angel Ezquerra
> <angel.ezquerra at gmail.com> wrote:
>> On Mon, Feb 15, 2016 at 1:42 PM, Thomas De Schampheleire
>> <patrickdepinguin at gmail.com> wrote:
>>> On Mon, Feb 15, 2016 at 12:04 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> [..]
>>>
>>> I find the request to skip directly to the approve button very
>>> strange, to be honest. Code review is not just about
>>> approving/rejecting, but also on giving code comments (which are most
>>> useful when placed inline in the code, really).
>>>
>>> Why would you want to skip to that button and give the impression that
>>> you cannot/should not comment on the code?
>>
>>
>> Both you and Mads make similar comments, so I´ll answer here to both of you :-)
>>
>> It does not make sense, as you say, to want to directly approve or
>> reject a revision without reviewing it first.
>> However it does make sense to review the revision _outside_ Kallithea
>> (e.g. using TortoiseHg and your favorite editor) and then want to just
>> mark the revision as Approved or Rejected in Kallithea.
>> That is basically the reason for the request. People are used to their
>> tools, and they'd want to use them to review the code (in addition to
>> being able to do it offline). Ideally they'd like to be able to right
>> click on the revision on TortoiseHg and select "Approved" on some sort
>> of Kallithea or Code Review menu. Since that is not possible the
>> second best solution is to just open the page and jump straight to the
>> main form.
>>
>
> But 'doing the review in TortoiseHg' means that you cannot make
> comments there, correct? (you'd have to write a TortoiseHg plugin that
> allows commenting from the UI and send the requests to Kallithea).

Yes, that would be one of the requests made by some of my users :-)

> So this means that either users will not write any comments and just
> respond with approved / rejected; or they are 'forced' to accumulate
> all comments in the main comment form without automatic line
> references.
> I think it would be better to try and convince your users about the
> benefits of real inline code review, than to 'help' them stick too
> non-ideal processes.

I don't disagree with you but I question the usefulness of line-level
comments in many scenarios, ours included.

For example, we are unfortunate enough to use IBM Rhapsody for some of
our code. Rhapsody is a tool that generates C++ code for you. It
stores its information in huge, cryptic xml files that contain a lot
of "useless" fields (such as modification time, block id's and so on)
which change pretty much whenever you hit save in Rhapsody. The actual
"meat" of your C++ code is embedded within those xml files.

This makes it really impractical to review the .xml files as if they
were text files (even though they are, technically text files). The
diffs are huge and only a very small portion of the diff is of any
interest, and it is really hard to find among the sea of useless
changes that Rhapsody does on each save. In fact in most cases
Kallithea does not even show the diffs by default, since they are so
big! Instead, what we do is use Rhapsody's provided diff tool, that
will hide all those dummy changes and just show you the actual
relevant changes.

In that context, the review basically must happen on the developer
machine, and the comments must be done at the "changeset level". This
also happens when you work with binary files (e.g. assests). Even if
all your files are real text files, focusing too much on line-level
details may discourage you from looking into the big picture and
making comments on the changeset level.

So while I agree that it would be best to be able to do line level
comments, it is not always practical. In addition to all this it is
hard to deny that a good text editor or IDE will be better suited to
read the code during a review. This can be a big deal on a big code
base when a change is complex. So I think that there are many valid
reasons why people may prefer to do the review outside of Kallithea,
and only use the Kallithea web interface to write their comments after
the fact.

> Anyway, I am not here to force an approach to others, this is just my
> suggestion. It's probably possible to resolve your request with some
> minimal HTML addition, so I suggest you propose a patch and we can
> then discuss on that technically.

I understand. I really appreciate your comments.
Today I found another reason why I would want to have a way to jump to
the comments (if not the comment form) quickly. I was going through
other people's comments and I just wanted to look why a particular
changeset had been rejected. To do so I had to manually go down to the
end of the page. Instead I would have prefered to have a "Changeset
comments" icon on the toolbar which would have let me jump to those
comments.

Cheers,

Angel