On Sun, Nov 08, 2015 at 11:59:33AM -0800, Andres Freund wrote:
> On November 8, 2015 11:52:05 AM PST, Noah Misch <noah@leadboat.com> wrote:
> >On Sun, Nov 08, 2015 at 11:11:42AM -0800, Andres Freund wrote:
> >> On November 8, 2015 12:54:07 AM PST, Noah Misch <noah@leadboat.com> wrote:
> >>
> >> >I have pushed a stack of branches to
> >> >https://github.com/nmisch/postgresql.git:
> >> >
> >> >mxt0-revert - reverts commits 4f627f8 and aa29c1c
> >> >mxt1-disk-independent - see below
> >> >mxt2-cosmetic - update already-wrong comments and formatting
> >> >mxt3-main - replaces commit 4f627f8
> >> >mxt4-rm-legacy - replaces commit aa29c1c
> >> >
> >> >The plan is to squash each branch into one PostgreSQL commit. In
> >> >addition to
> >> >examining overall "git diff mxt2-cosmetic mxt3-main", I recommend
> >> >reviewing
> >> >itemized changes and commit log entries in "git log -p --reverse
> >> >--no-merges
> >> >mxt2-cosmetic..mxt3-main". In particular, when a change involves
> >> >something
> >> >you discussed upthread or was otherwise not obvious, I put a
> >statement
> >> >of
> >> >rationale in the commit log.
> >>
> >> I'm not following along right now - in order to make cleanups the
> >plan is to revert a couple commits and then redo them prettyfied?
> >
> >Yes, essentially. Given the volume of updates, this seemed neater than
> >framing those updates as in-tree incremental development.
>
> I don't like that plan. I don't have a problem doing that in some development branch somewhere, but I fail to see any
benefitdoing that in 9.5/master. It'll just make the history more convoluted for no benefit.
>
> I'll obviously still review the changes.
Cleanliness of history is precisely why I did it this way. If I had framed
the changes as in-tree incremental development, no one "git diff" command
would show the truncation rework or a coherent subset. To review the whole,
students of this code might resort to a cherry-pick of the repair commits onto
aa29c1c. That, too, proves dissatisfying; the history would nowhere carry a
finished version of legacy truncation support. A hacker opting to back-patch
in the future, as commit 4f627f8 contemplated, would need to dig through this
thread for the bits added in mxt3-main and removed in mxt4-rm-legacy.
The benefits may become clearer as you continue to review the branches.
> Even for review it's nor particularly convenient, because now the entirety of the large changes essentially needs to
bereviewed anew, given they're not the same.
Agreed; I optimized for future readers, and I don't doubt this is less
convenient for you and for others already familiar with commits 4f627f8 and
aa29c1c. I published branches, not squashed patches, mostly because I think
the individual branch commits will facilitate your study of the changes. I
admit the cost to you remains high.
