Apps-android-commons: Version Control: Using squash merge when merging PRs to master

[maskaravivek]

In my opinion, when we merge PRs, we should be squashing the commits before merging it. A PR is usually related to one logical change as:

• during the course of development, developers usually keep creating commits after small changes
• whenever a PR is reviewed, new commits are made to address issues pointed in reviewer's comments

Doing a squash would have the following advantages:

• will keep out git history clean. Viewing the git log will clearly show how features were added/things were fixed.
• will make it easier to check the change done by building a particular feature. Rather than viewing multiple commits git show <commit_id> will show the changes
• will make it easier to revert a change
• cherry picking a change to another branch(say release branch) would be much easier. :)

It would be great if we could reach a consensus on this and start following the practise.

@misaochan @neslihanturan @nicolas-raoul @psh

[psh]

I believe that almost all of the benefits of a squash commit could be accomplished with a simple --no-ff merge to master from a feature branch. You get the ability to easily revert (rolling back the single merge commit) and you can easily track features - git log --merges --oneline drops a clean list of the merge commits to the main branch.

If you want the entire branch in your local development that's what a "merge" is for rather than cherry-pick.

I'm not a fan of squashing a whole branch, only to selectively remove a few of the little "work in progress" commits along the way to bring clarity to the work that's going on.

Given the options, the teams I've lead have all chosen the Gitflow Workflow where work is completed + tested on _feature branches_ and a stable master is preserved at all times.

[nicolas-raoul]

Good idea!
It does not require the _sender_ of a pull request to do anything special, right?

How about this:

• Suggest that PR reviewers squash before merging
• Since the PR reviewers are already overbooked, they are free to continue the way they are used to, and switch to doing it whenever they have time to experiment.
• When all PR reviewers are used to squashing (could be in a year, we are not in a hurry), make it a rule.

[misaochan]

I agree with this, especially after having had to cherry pick through multiple-commit PRs recently. ;)

I think the simplest way to do this is for reviewers to use the web interface and click on the drop-down button then select "squash merge". Given that it's so simple, if we do decide to go with this, I'd personally prefer to request reviewers to all start right away for consistency (so that we don't have some squashed commits along with a whole trail of unsquashed ones).

@nicolas-raoul No, senders don't need to do anything special. :)

[misaochan]

If everyone is OK with this, shall we update our documentation in the wiki, and proceed with this change? Tagging the other collaborators who haven't weighed in @neslihanturan @whym

[neslihanturan]

I also agree reviewers can squash before merging @misaochan

[whym]

Agreed.

Furthermore, is it going too far if I suggest disabling "Allow merge commits" in the repo settings? Perhaps yes - there might be exceptions we want to make, like when a PR accidentally consists of two independent, legitimate commits and it's quicker to merge them than asking to re-submit separately.

[misaochan]

Thanks guys! Let's try to start now, then. Will update our wiki.

@whym Yeah, we probably want to hold off on disabling that for now, haha. :) Maybe we can reevaluate in the future.

[misaochan]

Updated https://github.com/commons-app/apps-android-commons/wiki/Project-maintenance .