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

Thu Jul 27 08:15:42 UTC 2017

> On 26 Jul 2017, at 12:19, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> On 25 Jul 2017, at 19:37, Christophe Fergeau <cfergeau at redhat.com> wrote:
>>> 
>>> On Tue, Jul 25, 2017 at 02:26:36PM +0200, Christophe de Dinechin wrote:
>>>>>>> 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.
>>> 
>>> The patch submitter's mind who sends ping when the series gets too old
>>> can be seen as such a tool, with the added benefit that they know if the
>>> series is still relevant, they can solve complex conflicts when
>>> rebasing, ... :)
>>> 
>>>>> (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.
>>> 
>>> The quote (from you) on top of this part of my answer was
>>> « As Frediano said at the beginning of the series, “I’m tired of hearing
>>> this reply”. »
>>> I was specifically referring to that, which is non-specific, and which I
>>> interpret as unhappiness. I'll blame written media for any
>>> misinterpretation here :)
>> 
>> The good thing about written media is that we can go back for the full
>> context, namely:
>> 
>>> 
>>>>>> 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”.
>> 
>> I was simply quoting Frediano to indicate that he had already responded.
>> 
>> Ping is not a solution, it is a symptom of the problem. The problem being
>> forgotten reviews.
>> 
>> Also, ping is a response for forgotten reviews, how does it help figuring out
>> the
>> origin repository? Read again what I wrote, that’s clearly what I am talking
>> about.
>> 
>> So I’m sorry, but it looks like you are giving a canned response, “ping”,
>> that not
>> only ignores Frediano’s frustration with having to ping, but also ignores the
>> problem
>> I’m bringing up as being also solved by git URLs, namely the difficulty to
>> find
>> which repository a patch applies to. Hence my reusing the “I’m tired of”
>> formulation :-)
>> 
>> 
>>>>> 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.
>>> 
>>> Yes, but it's good to know what is the main issue people are having with
>>> the current workflow, so that we make sure this really is fixed by any
>>> change
>>> in tools and processes. The other features would then just be additional
>>> niceties.
>> 
>> I agree.
>> 
>> Now, any objection to
>> 
>> 1. Recommending that we use git URLs in patches?
> 
> No, I used in the past. Simply "You can find the series at...".
> One of the advantage of this is that if people update the branch
> (for instance just to keep in sync with master) people can
> find the updated patches.
> 
>> 2. Having a shared location for branches under review?
>> 
> 
> Not all people have the right to update these branches, to be
> fair with these people you'll have to make it for them which
> is expensive and should be the work of a tool. PRs solve this
> issue using multiple repositories. Maybe tools like patchwork
> or patchew could be extended but this could take a bit of effort.

So I hear that there is more support for PR/MR than manual shared repositories.

Can we configure gitlab to make sure any comment on a MR is also
sent over mail?

> 
>> 
>> Thanks,
>> Christophe
>> 
>>> 
>>> Christophe
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170727/e3251e2d/attachment-0001.html>