Ok, I'll need to stop using Gmail. In the meantime, sorry for the
inconvenience, retrying, hoping that Gmail won't randomly screw
up the formatting this time.
So, I do don't like history that look like the following:
> commit eeee: last nits from reviewer
> commit dddd: oops, typo that prevented compilation
> commit cccc: some more fix found during review
> commit bbbb: refactor half of preceding patch following reviewer comments
> commit aaaa: Do something awesome - patch for #666
To be sure we talk of the same thing, let it be clear that I'm talking about
multiple commits due to the review process, i.e. changeset that is separated
into multiple commits only because that is the history of the mistakes made
by the author and remarked by the reviewer (or even that the author see
after having made it's branch public).
Now I could sum the reason why I think this is a regression compared to the
rather clean history we've had so can be summed up by two things:
1) I cannot see any case where having those details once the ticket is
committed to the project (i.e, has been +1'ed) would be useful, and I
certainly never ever wanted to know those when looking at our history.
2) I do think that keeping those details would be counter-productive:
- It makes reading the history harder by the sheer fact of the added
volume of commits. And since those commits are noise once the feature is
committed (again, when knowing that the initial author had forgot to
check the tests were not compiling and had to commit another one liner,
or had tried another approach that has been heavily refactored in the
next patch because the first attempt was ugly has ever be useful?).
- It potentially makes bisect harder. Again, I'm talking about commits
coming from the review process. Those, more or less by definition, come
to fix mistakes made initially. They will more often than not not be
compiling, or maybe just have the tests that do not compile, or
introduce bugs that are fixed in the very next commit. When you bisect,
those kind of commit are a pain in the ass.
- It makes it more annoying to find what commits refer to what ticket. Not
impossible, just a little harder.
Let it be clear that I'm not saying keeping the 'review commits' make
anything *impossible*, but it does make very common tasks more complicated
(and potentially very frustrating in the case of bisect), while adding only
noise.
Of course, we could debate whether those 'review commits' are noise or not,
i.e. if they can have a concrete use ever, but my personal experience on
that is that it is noise.
> The other side of the equation is that there is tremendous power to be had
> from distributed versioning, and this proposed workflow discourages taking
> advantage of that
Would you mind elaborating, maybe be a little more concrete on that one.
Because just to be such we agree, I did not propose to rebase public
branches or something like that.
I proposed basically that once a changeset has been finalized on a branch
xxx of the author repro, instead of merging the branch xxx, the committer
would (locally) extract the content of that changeset and commit is a one
commit to the project repository (say trunk). Given that git is content
based, it "should" work without much problem for other branch based on xxx
to merge trunk in their branch.
Sure that may not be "beautiful" or something, and I you have better to
propose, be my guest.
Last thing, I did check the kernel source git history (which as far as I
know is considered as a model of git usage and distribution), and there is
not a single commit that looks like some nits issue from the review of a
preceding commit.
On Mon, Jan 9, 2012 at 4:38 PM, Sylvain Lebresne <sylv...@datastax.com> wrote:
> So, I do don't like history that look like the following:
>> commit eeee: last nits from reviewer> commit dddd: oops, typo that prevented
>> compilation> commit cccc: some more fix found during review> commit bbbb:
>> refactor half of preceding patch following reviewer comments> commit aaaa:
>> Do something awesome - patch for #666
> To be sure we talk of the same thing, let it be clear that I'm talking
> aboutmultiple commits due to the review process, i.e. changeset that
> is separatedinto multiple commits only because that is the history of
> the mistakes madeby the author and remarked by the reviewer (or even
> that the author seeafter having made it's branch public).
> Now I could sum the reason why I think this is a regression compared
> to therather clean history we've had so can be summed up by two
> things:
> 1) I cannot see any case where having those details once the ticket
> iscommitted to the project (i.e, has been +1'ed) would be useful, and
> Icertainly never ever wanted to know those when looking at our
> history.
> 2) I do think that keeping those details would be counter-productive:
> - It makes reading the history harder by the sheer fact of the added
> volume of commits. And since those commits are noise once the feature
> is committed (again, when knowing that the initial author had
> forgot to check the tests were not compiling and had to commit
> another one liner, or had tried another approach that has been
> heavily refactored in the next patch because the first attempt was
> ugly has ever be useful?). - It potentially makes bisect harder.
> Again, I'm talking about commits coming from the review process.
> Those, more or less by definition, come to fix mistakes made
> initially. They will more often than not not be compiling, or maybe
> just have the tests that do not compile, or introduce bugs that are
> fixed in the very next commit. When you bisect, those kind of
> commit are a pain in the ass. - It makes it more annoying to find
> what commits refer to what ticket. Not impossible, just a little
> harder.
> Let it be clear that I'm not saying keeping the 'review commits'
> makeanything *impossible*, but it does make very common tasks more
> complicated(and potentially very frustrating in the case of bisect),
> while adding onlynoise.
>> The other side of the equation is that there is tremendous power to be had>
>> from distributed versioning, and this proposed workflow discourages taking>
>> advantage of that
> Would you mind elaborating, maybe be a little more concrete on that
> one.Because just to be such we agree, I did not propose to rebase
> publicbranches or something like that.
> I proposed basically that once a changeset has been finalized on a
> branchxxx of the author repo, instead of merging the branch xxx, the
> committerwould (locally) extract the content of that changeset and
> commit is a onecommit to the project repository (say trunk). Given
> that git is contentbased, it "should" work without much problem for
> other branch based on xxxto merge trunk in their branch.
> Sure that may not be "beautiful" or something, and I you have better
> topropose, be my guest.
> Last thing, I did check the kernel source git history (which as far as
> Iknow is considered as a model of git usage and distribution), and
> there isnot a single commit that looks like some nits issue from the
> review of apreceding commit.
> --Sylvain
