Is it better to fold patches?

[Simon King]

Hi!

This is a question to the release manager(s):

Assume that there are five patches on a ticket with a positive review.
Would it make life easier for you if we'd fold the five patches into one? Or
is it alright to keep the five patches separate?

Best regards,
Simon

[Jeroen Demeyer]

For the release manager, one patch is easier than five. However, for
reviewers, the opposite might be true.

[Francois Bissey]

I think generally speaking it is easier to keep patches by "topic".
Unrelated patch should be separate but patch that have something in
common should be merged together.
Think about it this way: If I want to send the patch upstream for them
to commit you will organize the patches in group that make sense one way
or another.

Francois

[David Kirkby]

Also, if one of those patches was found to have a fault, all 5 would
have to be removed.

Dave

[Simon King]

Hi all!

Thanks for all your comments! So, it seems people agree that having
several small patches is good for reviewing. But once it has a positive
review, the patches should be folded.

Cheers,
Simon

[kcrisman]

Making it easy for the reviewer is definitely a bonus of not folding.  Another reason not to fold is if the patches have different authors.  Finally, if some of the patches change things in earlier patches in direct response to reviewer comments, folding them can make it hard to trace whether the problem was actually fixed.

However, in some cases there is a sort of patcherrhea and folding them together is wise, so think about it on a case by case basis.  What would *you* want as a reviewer or release manager for your patch, on specific ticket X?

- kcrisman

[Franco Saliola]

+1

--

[kcrisman]

> > Thanks for all your comments! So, it seems people agree that having
> > several small patches is good for reviewing. But once it has a positive
> > review, the patches should be folded.
>
> +1
>

Again, unless there are different authors. And I'm sure Jeroen would
point out that sometimes errors are discovered in his rigorous testing
process and it would be helpful to be able to easily pinpoint the
problem...

[Julien Puydt]

Le 30/08/2012 15:13, Simon King a �crit :

> Thanks for all your comments! So, it seems people agree that having
> several small patches is good for reviewing. But once it has a positive
> review, the patches should be folded.

Other open source projects do things like this : a patch is for a single
coherent change ; which means:
- don't cut patches by chunk ;
- don't cut patches by file ;
- cut a patch if it correspond to several independent changes.

It's useless to review a patch which doesn't contain all changes related
to something (like in: doesn't even compile separately), and it's too
complex to review if it contains several unrelated things.

The idea to submit several patches for review and a single one for
commit is bad for at least two reasons :
(-) it means you commit something which wasn't reviewed as-is!
(-) it makes it more difficult to find which change introduced a bug,
since the changes become bigger.

Snark on #sagemath

PS: see for example
http://www.kernel.org/doc/Documentation/SubmittingPatches

[Simon King]

Hi!

On 2012-08-30, kcrisman <kcri...@gmail.com> wrote:
> However, in some cases there is a sort of patcherrhea and folding them
> together is wise, so think about it on a case by case basis. What would
> *you* want as a reviewer or release manager for your patch, on specific
> ticket X?

If patches concern different topics, then I'd like to have them on
different tickets. As a reviewer, I see that tickets "grow", in the
sense that there is a patch, then another author submits a patch on top
of the first, then I complain, a third patch addresses the complaint,
and so on.

As a reviewer, it seems easier to me to have some patches that I already
have reviewed and further patches that address my concerns for the first
patches.

And as a release manager? I have absolutely no idea. That's why I ask.

Cheers,
Simon

[Jeroen Demeyer]

On 2012-08-30 17:00, Simon King wrote:
> If patches concern different topics, then I'd like to have them on
> different tickets.

Off topic, but I couldn't agree more.

There are a lot of "failed" tickets (i.e. needs_work forever) which
failed because they were trying to do too much at once. I like to use
#12369 and its dependencies as an example of the success of splitting up
an issue over many tickets.

[Jeroen Demeyer]

On 2012-08-30 13:17, Simon King wrote:

> Assume that there are five patches on a ticket with a positive review.
> Would it make life easier for you if we'd fold the five patches into one? Or
> is it alright to keep the five patches separate?

I should add: even more important than this is the fact that it should
be made very clear *which patches* have to be applied (in the ticket
description, not in comment #157).