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

[Christophe Fergeau]

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"


> > 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. You can use -v1, -v2 suffixes, but then you are keeping
obsolete stuff around. 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). And a cover letter usually contains more
than just history summary, so still useful ;)


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

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

> 
> > 
> >>> 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 ;) 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? 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"
:)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170725/8660257a/attachment.sig>