Re: Commitfest problems - Mailing list pgsql-hackers

On 14/12/14 17:30, Andrew Dunstan wrote:

> On 12/14/2014 12:05 PM, Tom Lane wrote:
>> Craig Ringer <craig@2ndquadrant.com> writes:
>>> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>>>> Compare this to say, for example, huge patches such as RLS.
>>> I specifically objected to that being flattened into a single monster
>>> patch when I saw that'd been done. If you look at my part in the work on
>>> the row security patch, while I was ultimately unsuccessful in getting
>>> the patch mergeable I spent quite a bit of time splitting it up into a
>>> logical patch-series for sane review and development. I am quite annoyed
>>> that it was simply flattened back into an unreviewable, hard-to-follow
>>> blob and committed in that form.
>> TBH, I'm not really on board with this line of argument.  I don't find
>> broken-down patches to be particularly useful for review purposes.  An
>> example I was just fooling with this week is the GROUPING SETS patch,
>> which was broken into three sections for no good reason at all.  (The
>> fourth and fifth subpatches, being alternative solutions to one problem,
>> are in a different category of course.)  Too often, decisions made in
>> one subpatch don't make any sense until you see the larger picture.
>>
>> Also, speaking of the larger picture: the current Postgres revision
>> history amounts to 37578 commits (as of sometime yesterday) --- and
>> that's
>> just in the HEAD branch.  If we'd made an effort to break feature patches
>> into bite-size chunks like you're recommending here, we'd probably have
>> easily half a million commits in the mainline history.  That would not be
>> convenient to work with, and I really doubt that it would be more useful
>> for "git bisect" purposes, and I'll bet a large amount of money that most
>> of them would not have had commit messages composed with any care at all.
> 
> I have tried to stay away from this thread, but ...
> 
> I'm also quite dubious about this suggested workflow, partly for the
> reasons Tom gives, and partly because it would constrain the way I work.
> I tend to commit with little notes to myself in the commit logs, notes
> that are never intended to become part of the public project history. I
> should be quite sad to lose that.

Interestingly enough, I tend to work in a very similar way to this. When
submitting patches upstream, I tend to rebase on a new branch and then
squash/rework as required.

One thing I do tend to find is that once I start rebasing the patches
for upstream, I find that many of my personal notes can be rewritten as
part of the patch comment (I would like to think that comments useful to
myself will be useful to other developers some day). Once the rebase is
complete, often I find that I have no non-public comments left so that
problem becomes almost non-existent.

> As for using git bisect, usually when I do this each iteration is quite
> expensive. Multiplying the number of commits by a factor between 10 and
> 100, which is what I think this would involve, would just make git
> bisect have to do between 3 and 7 more iterations, ISTM. That's not a win.

I find that this isn't too bad in practice. If you think about
monolithic patches during a commitfest, you can imagine that most of
them will touch one of the core .h files which will require most things
to be rebuilt once applied during bisection.

With smaller iterative patchsets, each patch tends to only touch a
handful of files and so "make install" generally only needs to rebuild
and link a very small number of files which is reasonably quick. Since
all of the changes for global header files are contained in 1-2 patches
then the number of complete rebuilds isn't as many as you might expect.

> On the larger issue, let me just note that I don't believe we have what
> is fundamentally a technological problem, and while technological
> changes can of course sometimes make things easier, they can also blind
> us to the more basic problems we are facing.

Absolutely. I firmly believe that the right tools for the job are
available, it's really trying to find a workflow solution that works
well for everyone involved in the project.


ATB,

Mark.