+1
> On 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>
>>
>>
