[Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Christophe de Dinechin]

> On 25 Jul 2017, at 12:36, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Tue, Jul 25, 2017 at 12:23:34PM +0200, Christophe de Dinechin wrote:
>>> and there are plenty of public places you can push your work.
>> 
>> So plenty of places, but by no means a shared one? The point is that
>> we need a shared one, to be able to view pending reviews at a glance.
> 
> Note that "we need a way to easily view pending reviews" does not imply
> "this has to be done through review/ branches in a shared repository”

No, it does not. I’m just pointing out one possible way to do it while maintaining
the mail-based process that some people here seem to love.

> 
> 
>>> If necessary, a cover-letter should say on which commit the series applies. There are tools like patchew that may also help. 
>> 
>> My point is that 1) this is not the case today, and 2) a git link is
>> the best way to summarize the history (faster and more reliable than
>> a cover letter)
> 
> A git branch won't tell you the changes between different versions of
> the same series.

Just like a single mail won’t tell you the changes between this patch and
the previous one. You have to look at the history to find it. As an aside,
git-publish uses tags to preserve the previous versions of a patch in the
git history.


> You can use -v1, -v2 suffixes, but then you are keeping
> obsolete stuff around.

You are not keeping obsolete stuff around, you are keeping a history.
A mailing list is not a better way to record the history of changes than git itself.
But at least, you can cleanup branches once the patch is merged (knowing
that the actual history of the patch is recorded in the mail should we need it).
With patches, it’s much harder to look at an e-mail and say “that was merged”.

So no, I don’t think you’d keep obsolete stuff around. You’d keep
“not yet reviewed stuff” around, which is a feature, not a bug.


> And then, a diff between 2 such branches is not
> the best way of summarizing what changed (we would not need commit logs
> if a diff was expressive enough).

I don’t understand what you are saying here.


> And a cover letter usually contains more than just history summary, so still useful ;)

I do not advocate we don’t send patch or a cover letter, I advocate that we send
a git URL along with the patch (or any other practical way to track if that thing
was reviewed and actioned upon).


> 
> 
>>>>> I see several benefits to doing this:
>>>>> 
>>>>> 1. We always know exactly which component and branch is being patched
>>>>> 
>>> 
>>> As long as contributor keep pinging or resending his series, this is already the case.
>> 
>> As Frediano said at the beginning of the series, “I’m tired of hearing this reply”.
> 
> And this is not an actionable answer... My perception is that there
> rarely are 'ping' on old series. Does this mean we are doing a good job
> at reviews?

I think that we (but not I) are doing an OK job at reviews, but we apparently drop
some reviews, e.g. because they were too complex, or did not represent the
priority of the time.

That being said, I observe that there are better ways to track WIP than pure mail.
Redmine, JIRA, pull requests, whatever. All well known solutions to the problems
we complain about.

As an aside, these tools typically solve many other problems too, like being able to
record things to be done *before* there is a patch for them, or CI, or priorities, etc.
Frankly, Bugzilla + Mail brings me back a good 15 years ago or something.

I don’t care much about which tool we use. I do mind that we have none.


> (I doubt it or we would not have this conversation) Does
> this mean patch senders do not want to do that? Why? Does this mean it's
> done a lot, but to no avail? All I'm reading is "I'm not happy with how
> things work", with nothing specific.

It’s funny, because
a) I never said I was unhappy, and
b) I gave a very simple, very specific suggestion for action, which was to add a
URL to a branch with the name review/<author>/<topic> on freedesktop.org to the
cover letter or patch description.

So how you turn that into “I’m not happy with how things work with nothing specific”
is a bit beyond my understanding.

As a new team member, I’m find it unnecessarily difficult to know what a
given patch applies to, or to reconstruct code that actually compiles for testing,
or to see how it was tested before I look at the code, etc. I noticed that the patch
URL would *also* solve that problem. That’s about it.

As an aside, I find it strange to have private repos of half a dozen team members
in my history already, instead of a shared repo for things we are working on.


> 
>>> Eventually, the contributor can send a WIP series for some input without detailed review.
>> 
>> I think you meant “eventually” to mean the same thing as
>> “eventuellement” in French (if necessary or possibly), not in the
>> english sense of “at the end”?
>> 
>> I did that with my early recorder WIP. You apparently did not like
>> that way of proceeding.
> 
> Iirc you sent a link to a git branch without the accompanying patch
> series, and this was what Marc-André asked you not to do.

Yes, I had pointed that out earlier in my mail. Which is why I am only suggesting
here we add a link to the existing mails, not that we skip the mail.

> 
>>> 
>>>>> 3. If you want to test, a git checkout is enough to test (assuming you did
>>>>> the git fetch above). Simpler IMO than git am (even if I assume most of us
>>>>> have scripts to process incoming mail)
>>> 
>>> qemu uses patchew, I think it would be worth to consider it for
>>> spice as well. It applies the patches on a mailing list, run some
>>> tests, gives you a stable URL, tracks the review (and the various
>>> iteration recently iirc)…
>> 
>> Good tool, but different problem.
> 
> I personally don't know which exact problem we are tackling ;)

This started with old patches not being tracked. But as most of the industry
knows, tools that deal with this tend to integrate solutions for a whole catalog
of problems we also have, like lack of CI for the WIP branches.

> I see some people having issues keeping track of pending series, others saying
> that CI would be nice, ... Are we trying to solve one single very specific
> problem?

As written above, the state of the art in the industry for solving these problems
typically solves them with integrated packages, not one at a time. Because it’s
much better that way, as people who worked with such tools know.

> If yes, what is it? Patch reviews not being done in a timely
> manner? Patch series being forgotten? Patch series status hard to know
> by email? Something else? (note that you said "problem", not "problems"
> :)

Starting with the first in your list, but with the full knowledge of the state of the art
for tools solving this problem solves other problems that we discussed earlier at the
same time, and listing these problems as well.


Cheers,
Christophe