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