I think a single (squashed if required) commit for the start of a PR.
ANY changes due to comments should be reflected as separate commits on
the PR.
Once approved, that PR should squash all commits into 1 commit with a
message that describes what was done,etc...
In the past I've heard developers ask for the following:
1x commit to address the problem/feature
1x commit with refactors.
I must be honest, but I am yet to find 1 developer that keeps a list of
all changes they want to be refactored separate from the bug/feature
code. OR better stated I am yet to find where this was sustainable AND
productive.
It would definitely discourage the community to refactor, because who
keeps track of all the code they would like to refactor and change the
code after completing the task. Sounds like double if not triple work to me.
Also, how would this work if there are comments on the code i.e better
approaches, have you thought about... or that might lead to errors. Are
these changes applied to the core/base commit or against the refactor or
both?
Also, a good practice for all is to commit (and push) code on a daily
basis on their branches. "WIP - completed x,y,z. need to address: a,g,r"
comments are not uncommon and safeguard against loss of equipment,
illness of members,etc...
My personal coding style is, I make changes to code as I pass through
it.. (change variable names when they don't make sense, refactor
methods, etc...) So, I rarely have the time to retrace my steps and
post-factum re-apply my refactored changes.
I think the checklist could be reworded, but as everything in this
project, we take it in the spirit was intended and don't follow it to
the letter. Would the rewording of the checklist enhance/improve/make
the intent clearer or not? I don't care either way, but when someone
merges a PR into a main branch with many intermediate commits, it just
clusters the commit log with "contextually irrelevant" noise. (also
makes it harder to follow all the changes that are in a branch). What if
I want to rollback a commit into develop, how many does one have to
revert, 1 is always simpler than 2-10... Also.. cherry picking becomes
simpler...
--Udo
On 5/31/19 13:43, Nabarun Nag wrote:
In my opinion, I am okay will multiple commits within a PR.
But please do squash them to a single commit when it is pushed to develop.
This helps us a ton if it is single commit.
- bisect operations are easier when it is a single commit during major
failure analysis.
- cherrypick is easier when it is one commit.
I even don’t prefer merge commit messages :
- none of the big ASF projects do it.
- visualizing on tools is bit skewed.
- difficult to analyze failures .
I would like to reiterate Dan’s statement on emphasis on people and empathy
over blanket process and rules.
Regards
Naba
On Fri, May 31, 2019 at 1:32 PM Helena Bales <hba...@pivotal.io> wrote:
+1. I would guess that it is the checklist as part of the PR that is
confusing people.
The other reason that history gets rewritten is when force pushing after a
rebase. While fast-forwarding is necessary on occasion, this can be
accomplished without rewriting history by using a merge.
As part of our document on making PRs, we should include instructions on
how to handle the situation where fast-forwarding is necessary, explicitly
discourage the use of merges and force-pushes once a PR has been opened,
and some guidelines regarding the appropriate number of commits when the PR
is initially opened. Once we have these guidelines, it would be helpful to
link to them from the PR checklist that we currently have, and rework the
checklist so that it is in line with our desired process.
On Fri, May 31, 2019 at 1:20 PM Darrel Schneider <dschnei...@pivotal.io>
wrote:
Something I have noticed is that often when I have requested changes be
made to a pull request is that the changes are force pushed ask a single
commit to the pr. I would actually prefer that the changes show up as a
new
commit on the pr instead of everything being rebased into one commit.
That
makes the history of the pr easier to follow and make it easy to see what
has changed since the previous review. What do others think? Have we done
something that makes contributors think the pull request has to be single
commit? I know the initial pull request is supposed to be but from then
on
I'd prefer that we wait to squash when we merge it to develop.
