[Qemu-devel] Re: commit rules for common git tree

[Michael S. Tsirkin]

On Tue, Dec 29, 2009 at 09:03:18PM +0100, Aurelien Jarno wrote:
> On Tue, Dec 29, 2009 at 08:12:57PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 29, 2009 at 06:40:31PM +0100, Aurelien Jarno wrote:
> > > On Tue, Dec 29, 2009 at 07:23:28PM +0200, Michael S. Tsirkin wrote:
> > > > On Sun, Dec 27, 2009 at 05:01:38PM -0600, Anthony Liguori wrote:
> > > > > Likewise, if you see a patch go in that you think would have 
> > > > > benefited  
> > > > > from being on the list, point it out.
> > > > 
> > > > How *would* I see it? I guess I could write scripts that correlated git
> > > > logs (or qemu commit list if it is ever resurrected) with mailing list
> > > > archive.  If patch is not there, look in mailing list around the time
> > > > for a hint: could have explanation in the discussion. Or maybe it is
> > > > part of a series someone pushed ... you get the point.
> > > > 
> > > 
> > > All those commit have a single Signed-of-by: line. That decreases the
> > > number of patches to take.
> > > 
> > > Anyway I still don't really understand you are trying to achieve here,
> > > and without pointing to real problems, I am not sure we are going to
> > > understand.
> > 
> > What would you call real problems?
> > - You listed complains 3 times as an issue for committers.
> >   Is this a problem worth solving? I think my suggestions
> >   will help solve it.
> 
> This has nothing to do with "all patches should be submitted to the
> mailing list first"

Let's start with "should be submitted to the mailing list" without
"first". Makes sense?

> > - Even codewise: it's not uncommon for commit X to revert commit Y by
> >   another person, at least partially. I can point them out
> >   privately, prefer not to do it publically. This hurts git bisect and
> >   generally makes following history harder.  Unavoidable but seems less
> >   common with reviewed patches.
> 
> Then those are the real problem, and should be avoided.

Everyone did this at some point. It's human to err.

> Pointing the
> people that they should not commit patches like that is the way to go
> IMHO.

Yes but one does not *know* one is making a mistake beforehand.  So I
think "like that" is "without giving anyone a chance to review".

> > > The life of committers is not really easy, and the patch submitters do
> > > not really help to improve it:
> > > - a lot of patches are only reviewed by the committers, not so many
> > >   persons send Acked-by: mails.
> > > - you have to deal with code you don't understand
> > > - you have to track continuously respined series (with patches moving
> > >   from one to another, being merged or changing title)
> > > - you get complains when you review a patch
> > > - you get complains when you don't review a patch fast enough
> > > - you get complains after you applied a patch
> > > - etc...
> > 
> > Most complaints are result of somewhat opaque nature of the commit
> > process.  For example, people would complain "where's my patch" often to
> > be said: it is applied, not yet pushed. Sending mail "applied thanks"
> > would solve this.  I am suggesting making it more transparent and reduce
> > number of complaints. Thus life of committers will become easier :).
> > 
> 
> I don't get this kind of complains very often.
> 
> I get complains because:
> - I don't review patches fast enough. Here other people can help by
>   reviewing the patches and sending an Acked-by:
> - I have applied a patch that breaks something. It get even worse when
>   the patch has been committed sometimes ago and the patch submitter has
>   disappear. Again other people can help reviewing patches to catch
>   common mistakes.
> 
> If others make efforts sharing the load a bit, I am fine making efforts.
> Otherwise no.

Well, commits without post are not making sharing the load easy:
- If patches are applied without notifying the list others waste time
  reviewing a patch that was already reviewed and applied.
- If code changes without notifying the list, others must consider
  and merge multiple channels when trying to understand a patch
  (e.g. it might not even apply to your tree, you must pull
   to understand it).

-- 
MST