Automattic / sensei

Sensei LMS - Online Courses, Quizzes, & Learning
https://senseilms.com
GNU General Public License v2.0
542 stars 197 forks source link

Development process upgrade: rebase instead of merge? #1688

Closed lkraav closed 7 years ago

lkraav commented 7 years ago

The criss-cross merging of master with feature or developer branches creates a very confusing picture of what happens where and why. Example below (mainly the bottom part):

95da8b1 2016-12-20 10:14 Panos Kountanis        o {origin/master} {origin/HEAD} <version/1.9.10> Update Changelog for 1.9.10
0896ab0 2016-12-20 10:13 Panos Kountanis        o Bump version number
1e88407 2016-12-07 12:20 Panos Kountanis        o Remove deprecated function wp_kses_js_entities
90d3666 2016-12-14 13:59 Panos Kountanis        o Fix Module updates for courses in wp4.7 closes #1665
b1c6ef8 2016-12-12 14:46 Panos Kountanis        o Fix fatal error caused by get_terms filters. Closes #1665
4ea64d0 2016-10-19 10:38 Panos Kountanis        o Bump Version to 1.9.9, update changelog
100bc17 2016-10-19 10:33 Panos Kountanis        M─┐ Merge pull request #1598 from Automattic/fix/1594-Admin-Fatal-Error
2d93f43 2016-10-19 10:31 Panos Kountanis        │ o {imknight/fix/1594-Admin-Fatal-Error} {origin/fix/1594-Admin-Fatal-Error} Use array() insted of newer literal syntax. Closes #1594
3845e66 2016-10-18 16:23 Panos Kountanis        M─┘ <version/1.9.8> Merge pull request #1593 from Automattic/fix/1591-Next-Lesson-button-HTML
9cff2c9 2016-10-18 13:56 Panos Kountanis        │ o Use wp_kses on Next Lesson button HTML
2616574 2016-10-18 14:20 Dan Johnson            o │ 1.9.8 version bump
18bd1f8 2016-10-18 14:19 Dan Johnson            o │ 1.9.8 changelog update
2dbbc73 2016-10-14 14:14 Panos Kountanis        o─┘ Remove deleted posts from course order list refs #1586
d61cb51 2016-10-14 12:47 Panos Kountanis        M─┐ Merge pull request #1589 from Automattic/fix/1588-whitelist-all-video-html-tag-attributes
6507cbb 2016-10-13 18:51 Panos Kountanis        │ o Whitelist video html tag attributes. Closes #1588 refs #1587
6335a50 2016-10-13 12:12 Panos Kountanis        M─┘ <version/1.9.8-beta> Merge pull request #1587 from Automattic/fix/1560-kses-without-hooks
d8e2e74 2016-10-13 11:28 Panos Kountanis        │ o {imknight/fix/1560-kses-without-hooks} {origin/fix/1560-kses-without-hooks} Use `Sensei_Utils::wp_kses` in lesson and course video embed metaboxes #1560
c1e52fe 2016-10-12 17:35 Panos Kountanis        │ o Introduce Sensei_Utils::wp_kses that uses a custom hook instead of `pre_kses` fixes #1560
2419353 2016-10-12 16:42 Panos Kountanis        M─┘ Merge pull request #1585 from Automattic/fix/1582-Deleted-courses-in-Order-Courses-list
38d347f 2016-10-12 15:51 Panos Kountanis        │ o Remove falsy course_ids from order courses list before save
b5a206a 2016-10-12 14:23 Panos Kountanis        │ o When a course is deleted, remove it from Courses -> Order
6c19710 2016-10-12 13:18 Panos Kountanis        │ o Remove trashed courses from Courses -> Order
f634e88 2016-10-11 16:48 Panos Kountanis        M─┘ Merge pull request #1581 from Automattic/fix/login-link-1574
a708ca1 2016-10-11 16:31 Panos Kountanis        │ o Allow link html on Sensei notices refs #1574
31c3aa6 2016-10-11 15:32 Panos Kountanis        M─┘ {pgk/master} Merge pull request #1580 from Automattic/fix/allow-course-archive-front-page
d77276c 2016-10-11 15:29 Panos Kountanis        │ o {imknight/fix/allow-course-archive-front-page} {origin/fix/allow-course-archive-front-page} Remove the is_home() check from Sensei_Course::is_fron_page. refs #14
5821373 2016-10-11 14:26 Panos Kountanis        M─┘ Merge pull request #1576 from pgk/1491-allow_course_archive_on_front_page-on-front-page
e82e4a7 2016-10-10 10:25 Panos Kountanis        │ o Do not use `is_front_page` as it produces notifications
fe7ccd9 2016-10-11 14:23 Panos Kountanis        M─│─┐ Merge pull request #1569 from Automattic/revert-wrong-commit
6af2b97 2016-10-11 13:45 Panos Kountanis        │ │ M─┐ {imknight/revert-wrong-commit} {origin/revert-wrong-commit} Merge remote-tracking branch 'a8c/master' into pgk-revert-commit
9279503 2016-10-11 12:22 Panos Kountanis        M─│─│─┘ Merge pull request #1570 from Automattic/hotfix-#1568-escape-link
b7e256a 2016-10-06 10:28 Nicola Mustone         │ │ │ o {origin/hotfix-#1568-escape-link} use wp_kses instead of esc_html
f2cce7a 2016-10-10 17:38 Panos Kountanis        M─│─│─│─┐ Merge pull request #1575 from pgk/1573-Conflict-Heroic-Knowledge-Base
94b63da 2016-10-06 19:44 Panos Kountanis        │ │ │ │ o {pgk/1573-Conflict-Heroic-Knowledge-Base} Limit archive content on `lesson` post types only. Closes #1573
7bc4b5b 2016-10-10 17:37 Panos Kountanis        M─│─│─│─│─┐ Merge pull request #1572 from pgk/load-template-functions
d868acc 2016-10-06 16:45 Panos Kountanis        │ │ │ │ │ o {pgk/load-template-functions} Load template functions on `after_setup_theme`
2687300 2016-10-06 17:41 Panos Kountanis        M─┴─│─│─┘ │ Merge pull request #1552 from pgk/1491-allow_course_archive_on_front_page-on-front-page
d005d35 2016-09-28 16:40 Panos Kountanis        │ o │ │ ┌─┘ allow_course_archive_on_front_page should only run on front page refs #1491
9bfeedc 2016-10-06 17:10 Panos Kountanis        M─│─│─│─│─┐ Merge pull request #1541 from ivantedja/1537-fix-course-results-title
5004886 2016-09-08 21:44 Ivan Sebastian         │ │ │ │ │ o Fix title filter for Course Results in newer theme
c28a1a2 2016-10-06 17:08 Panos Kountanis        M─│─│─│─│─│─┐ Merge pull request #1566 from pgk/1523-Undefined-Method-Sensei-Prereq
f0e3682 2016-10-03 14:51 Panos Kountanis        │ │ │ │ │ │ o {pgk/1523-Undefined-Method-Sensei-Prereq} Fix method typo in sensei_has_user_completed_prerequisite_lesson fixes #1523
39e7f8a 2016-10-06 17:04 Panos Kountanis        M─│─│─│─│─│─│─┐ Merge pull request #1561 from pgk/1559-PHP-Compatibility
1d15724 2016-09-30 21:24 Panos Kountanis        │ │ │ │ │ │ │ o {pgk/1559-PHP-Compatibility} Keep PHP 5.3 compatibility by not using anonymous functions refs #1559
c0faf04 2016-10-05 18:06 Nicola Mustone         │ │ o │ │ │ │ │ more escaping
1891815 2016-10-05 17:58 Nicola Mustone         │ │ o │ │ │ │ │ escape translations
2cafa39 2016-10-05 17:48 Nicola Mustone         │ │ o │ │ │ │ │ escape all the things
40eafd5 2016-10-05 17:19 Nicola Mustone         │ │ o │ │ │ │ │ revert wrong commit on lessons
7aa8d8e 2016-09-29 11:35 Panos Kountanis        M─│─┴─┴─┴─│─┴─┘ <version/1.9.7> Merge pull request #1558 from pgk/master

I propose the official policy to be adjusted and start requesting rebase use over merge. Before merging any feature or developer branch, it should be cleanly rebased on top of latest everbranch master, whenever possible (occasional work sync timelapses need to be adjusted for, i.e. PR review is delayed but master advances). Master should really never need to be merged into anything.

Hypothesis is that the resulting linear cleanliness should produce increased understanding for all participants, therefore resulting in higher quality code inputs already in short-term perspective.

Your thoughts?

lkraav commented 7 years ago

PS one of the triggers here was the massive sudden rollback in master, where a whole bunch of commits suddenly just disappeared to get the 1.9.10 release out for fixing WP 4.7 crashes.

pgk commented 7 years ago

Essentially, for Sensei, we are following https://www.atlassian.com/git/tutorials/comparing-workflows/feature-branch-workflow, with tags denoting stable release snapshots

Agree, rebasing with master before opening a pull request is recommended for most development work. I would not go as far as to enforce it though

Reason is, there are drawbacks to rebasing, mostly related to the commit sha changing and using git pull --rebase.

If a feature is being developed by more than one person on a branch, it is easy to mess things with rebases, especially because if you rebase, you need to git push -f back to the branch.

Fixing conflicts when rebasing is harder for new folks, as it might end up requiring more advanced git commands (cherry-picking, resetting spefific blobs, reflog etc). Merging might be easier for them in the beginning.

lkraav commented 7 years ago

Sure, there is no magic bullet, since people's skill level at this varies so widely. http://endoflineblog.com/gitflow-considered-harmful was an interesting recent read + comments discussion.

Maybe a useful tip for someone, stgit has helped me a lot with intra-branch patch management. Makes rebase-principle operations a ton quicker to handle.

The difficult case still remains where you need >1 wip feature branches sitting underneath your own...

I guess the main value filing this issue might have been to just draw periodic attention to "this stuff does matter, care should be taken". Perhaps some quick incremental wins could be found by group-thinking about it.

donnapep commented 7 years ago

Closing as answered.