well, I always use squash-and-merge so I'm not against the primary
motivation for this thread.
On 12/20/19 10:07 AM, Owen Nichols wrote:
The proposed action manifests as a commit to the Geode git repository, so a PR
is an acceptable vehicle for voting in this case.
On Dec 20, 2019, at 9:38 AM, Bruce Schuchardt <bschucha...@pivotal.io> wrote:
I see a lot of plus-ones and a "voting deadline" on this DISCUSS thread and a request to
"vote" using a PR. This all seems out of order to me. Our votes are supposed to be on
the email list, aren't they? and I haven't seen a VOTE request.
On 12/20/19 9:33 AM, Nabarun Nag wrote:
+1
On Fri, Dec 20, 2019 at 9:25 AM Owen Nichols <onich...@pivotal.io> wrote:
Based on the feedback so far, I will amend the proposal to drop item 2).
Therefore, the current ability to create merge commits using command-line
git will remain available.
The proposal as amended is now:
Change GitHub settings to make "Squash and merge" the default
(by removing “Create a merge commit” option).
Update the PR template to change the text "Is your initial contribution
a single, squashed commit” to “Is your initial contribution squashed for
ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a
‘fix’ commit)"
As Naba suggested, we can try it, and if we don’t like it, it’s simple to
revert.
I’ve create a PR for the proposed change here:
https://github.com/apache/geode/pull/4513
Please use the PR to vote for against this proposal. It will not be
merged before the VOTING DEADLINE of DEC 31 (if no -1’s at that time)
On Dec 20, 2019, at 8:31 AM, Ju@N <jujora...@gmail.com> wrote:
+1
On Fri 20 Dec 2019 at 16:18, Owen Nichols <onich...@pivotal.io> wrote:
Hi Bruce, this proposal will not waste a single second of your time. It
just prevents people from accidentally pressing a button that we have
already agreed should never be pressed, but because we never configured
our
GitHub to match our stated policy, is currently the default.
However, it will save a lot of time and frustration for anyone needing
to
bisect failures, revert, or cherry-pick changes, which has merit even if
you personally never do any of those three things.
Please start a separate thread if you would like to revisit the
community
decision to require passing PR checks.
On Dec 20, 2019, at 7:49 AM, Bruce Schuchardt <bschucha...@pivotal.io>
wrote:
I agree with Jake. I would go further by saying that I see very little
merit in this proposal. I think we're getting more and more
bureaucratic
in our process and that it stifles productivity. I was recently forced
to
spend three days fixing tests in which I had changed an import statement
before they would pass stress testing. I'm glad the tests now pass
reliably but I was very frustrated by the process.
On 12/19/19 4:49 PM, Jacob Barrett wrote:
I’m in agreement with Dan. Changes to the infrastructure to flat out
prevent things that should be self policing is annoying. This PR review
lock we have had already cost us valuable time waiting for PR pipelines
to
pass that have no relevance to the commit, like CI work: I’d hat to see
yet
another process enforced that Kees us from getting work done when
necessary.
-Jake
On Dec 19, 2019, at 4:43 PM, Dan Smith <dsm...@pivotal.io> wrote:
-1 to (1) and (2).
I think merge commits are appropriate in certain circumstances, so I
don't
think we should make a blanket restriction. In fact I think our
release
process involves some merges.
I think setting standards on what is reasonable to be an individual
commit
will do a lot more to clean up our history than blocking merges. We'd
rather not see commits like "Spotless Apply" in the history, but if
reasonably separate and well written commits come in as part of a
merge, I
think that's fine.
-Dan
On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao <jil...@pivotal.io>
wrote:
+1
On Thu, Dec 19, 2019, 4:05 PM Owen Nichols <onich...@pivotal.io>
wrote:
I’d like to advance this topic from an informal request/discussion
to a
discussion of a concrete proposal.
To recap, it sounds like there is general agreement that commit
history
on
develop should be linear (no merge commits), and that the biggest
obstacle
to this is that the PR merge button in GitHub creates a merge
commit
by
default.
I propose the following changes:
1) Change GitHub settings to remove the ability to create a merge
commit.
This will make Squash & Merge the default.
2) Change GitHub settings to require linear history on develop.
This
prevents merge commits via command-line (not recommended, but wiki
still
has instructions for merging PRs this way).
3) Update the PR template to change the text "Is your initial
contribution
a single, squashed commit” to “Is your initial contribution
squashed
for
ease of review (e.g. a single commit, or a ‘refactoring’ commit
plus
a
‘fix’ commit)"
For clarity, I am proposing no-change in the following areas:
i) Leave Rebase & Merge as an option for PRs that have been
structured to
benefit from it (this can make it easier in a bisect to see whether
the
refactoring or the “fix” broke something).
ii) Leave existing wording in the wiki as-is [stating that
squashing
is
preferred].
Please comment via this email thread.
-Owen
On Dec 16, 2019, at 10:49 AM, Kirk Lund <kl...@apache.org> wrote:
I think it's already resolved Udo ;)
Here's the problem, if I fixup a dunit test by removing all uses
of
"this."
and I rename the dunit test, then git doesn't remember that the
file
is a
rename -- it forever afterwards interprets it as a new file that I
created
if I touch more than 50% of the lines (which "this." can easily
do). If
we
squash two commits: the rename and the cleanup of that dunit test
--
then
we effectively lose the history of that file and it shows that I
created
a
new file.
Also for the record, I've been working on Geode since the
beginning
and I
was never made aware of this change in our process. I never voted
on
it.
I'm not a big fan of changing various details in our process every
single
week. It's very easy to miss these discussions unless someone
points it
out
to me.
On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer <
ukohlme...@pivotal.io>
wrote:
I'm not sure what this discussion is about... WE, as a community,
have
agreed in common practices, in two place no less...
1) Quoting our PR template
For all changes:
*
Is there a JIRA ticket associated with this PR? Is it referenced
in
the commit message?
*
Has your PR been rebased against the latest commit within the
target
branch (typically|develop|)?
*
***Is your initial contribution a single, squashed commit?*
*
Does|gradlew build|run cleanly?
*
Have you written or updated unit tests to verify your changes?
*
If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion underASF 2.0
<http://www.apache.org/legal/resolved.html#category-a>?
On our PR template we call out that the initial PR commit should
be
squashed.
2)
https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
<
https://cwiki.apache.org/confluence/display/GEODE/Code+contributions
-- See "Accepting a PR Using the Command Line" - Point #3.
@Kirk, if each of your commits "stands alone", I commend you on
the
diligence, but in reality, they should either then be stand alone
PR's
or just extra work created for yourself.
If we want to change the way we have agreed upon we
submit/commit/merge
changes back into develop... Then this is another discussion
thread,
until then, I think we should all remind ourselves on our agreed
contributions code of conduct.
--Udo
On 12/16/19 9:59 AM, Nabarun Nag wrote:
Kirk, I believe that creating a Pull Request with multiple
commits is
ok.
It's just in the end that when it's being pushed to develop
branch,
it
needs to be squash merged. I believe that is what you have
mentioned
in
the
first paragraph, and I am more than happy with that.
If you can see in the first screenshot comparison that I had
attached
in
the first email in this thread is what I want to avoid.
Thank you for your feedback.
Regards
Naba
On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund <kl...@apache.org>
wrote:
Whenever I submit a PR with multiple commits that I intend to
rebase
to
develop, I always try to ensure that each commit stands alone
as
is
(compiles and passes tests). Separating file renames and
refactoring
from
behavior changes into different commits seems very valuable to
me,
and
I've
had trouble getting people to review PRs without this
separation
(but
it
could be squashed as it's merged to develop).
It sounds to me like the real problem is (a) some PRs have
multiple
commits
that don't compile or don't pass tests, and (b) some PRs that
should
be
merged with squash are not (by accident most likely).
I can submit multiple PRs instead of one PR with multiple
commits.
So
I'll
change my response to -0 if that helps prevent commits to
develop
that
don't compile or pass tests. Without preventing rebase or merge
commits
from github, I'm not sure how we can really enforce this or
prevent
(b)
above.
On Fri, Dec 13, 2019 at 3:38 PM Alexander Murmann <
amurm...@pivotal.io>
wrote:
I wonder if Kirk's and Naba's statements are necessarily at
odds.
Make the change easy (warning: this may be hard), then make
the
easy
change."
-Kent Beck
Following Kent Beck's advise might reasonably split into two
commits.
One
refactor commit and a separate commit that introduces the
actual
change.
They should be individually revertible and might be easier
understood
if
split out. I vividly remember a change on our code base where
someone
did a
huge amount of refactoring that resulted than in one parameter
changing
in
order to get the desired functionality change. If that was in
one
commit,
it would be hard to see the actual change. If split out, it's
beautiful
and
crystal clear.
I am unsure how that would be reflected in terms of JIRA
ticket
references.
Usually we assume that if there is a commit with the ticket
number,
the
issue is resolved. Maybe the key here is to create a separate
refactoring
ticket.
Would that allow us to have our cake and eat it too?
On Fri, Dec 13, 2019 at 3:16 PM Nabarun Nag <n...@pivotal.io>
wrote:
It is to help with bisect operations when things start
failing
...
helps
us
it revert and build faster.
also with cherry picking features / fixes to previous
versions
.
And keeping the git history clean with no unnecessary “merge
commits”.
Regards
Naba
On Fri, Dec 13, 2019 at 2:25 PM Kirk Lund <kl...@apache.org>
wrote:
-1 I really like to sometimes have more than 1 commit in a
PR
and
keep
them
separate when they merge to develop
On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag <
n...@pivotal.io>
wrote:
Hi Geode Committers,
A kind request for using squash commit instead of using
merge.
This will really help us in our bisect operations when a
regression/flakiness in the product is introduced. We can
automate
and
go
through fewer commits faster, avoiding commits like
"spotless
fix"
and
"re-trigger precheck-in" or other minor commits in the
merged
branch.
Also, please use the commit format : (helps us to know who
worked
on
it,
what is the history)
* GEODE-xxxx: <brief intro >
* explanation line 1 *
explanation
line
2*
This is not a rule or anything, but a request to help out
your
fellow
committers in quickly detecting a problem.
For inspiration, we can look into Apache Kafka / Spark
where
they
have
a
complete linear graph for their main branch HEAD [see
attachment]
Regards
Naba.
--
Ju@N
