Re: [pgadmin-hackers] Patch submissions - Mailing list pgadmin-hackers

Hi

On Thu, Mar 16, 2017 at 5:58 PM, Atira Odhner <aodhner@pivotal.io> wrote:
> Hi Dave,
>
> I'm wondering what pain you are feeling around having multiple patches.
>
> From my perspective it is much easier to deal with smaller commits as it
> gives us a quicker way to understand each change if we want to look back at
> history. I agree that each patch should work standalone (tests should be
> passing, the application should run.) But, I think aiming for a patch size
> that encompasses an entire feature is often too large.
>
> When we were squashing patches and then having to go back and modify them,
> we were spending a large portion of our time managing patches and branches
> rather than working on code. With smaller patches, rebasing is much simpler
> and easier to understand. So I'd really like to be able to continue to do
> things this way. I do understand the desire not to have a 'bad' commit on
> master.
>
> Can you explain how having a large number of patches impacts your process
> and maybe we can together come up with a process that works better for all
> of us?
> E.g. if you just wanted to review all the changes at once, you could do `git
> apply *.patch` to review, and then `git am *.patch` when you are ready to
> commit the changes.

We aim to commit only the final version of the code, which is also
what I'm reviewing. If a patch set contains multiple patches
correcting review comments, and there are also other patches produced
in the middle, it becomes very difficult to separate the unrelated
changes, and review the final version of each individual change.

I was trying not to single out any particular work, but really the
Jasmine changes are a great example of this:

Patch 1 added Jasmine (feature 1)
Patch 2 added centralised translation rendering (feature 2)
Patch 3 refactored clipboard code into it's own file (feature 3)
Patch 4 (original version) removed some dead code in sqleditor.js (feature 4)
Patch 4 fixed quotes in sqleditor.js (arguably part of feature 2)
Patch 5 was rejected early on as it was unnecessary, but was resubmitted
Patch 6 fixed an issue in patch 3 (feature 3)
Patch 7 cleaned up patch 1 (feature 1)
Patch 8 updated packages to exclude unnecessary files introduced in
patch 1 (feature 1)
Patch 9 fixed patch 7 (feature 1)

I can tell you that had those been patches to PostgreSQL, they would
have been rejected out of hand.

What I want to see submitted, is 4 separate patches that evolve with
the feedback given:

Add Jasmine (feature 1)
Centralised translation rendering (feature 2)
Refactor clipboard code (feature 3)
Remove dead code (feature 4)

That enables reviewers to independently review and test each new block
of functionality, and commit the change as one completed unit of work.
With the 10 patches above, that wasn't possible because the
dependencies are intermixed.

I appreciate this puts more work on developers, however, there are far
fewer reviewers than developers, and patch review is not simply a case
of applying the patches and committing; it can take a significant
amount of time.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company