Re: [Jflex-devel] Branch protection

[Gerwin K. <ge...@do...>]

> On 7 Oct 2018, at 9:02 pm, Régis Décamps <re...@de...> wrote:
> 
> Hello Gerwin,
> 
> Le jeu. 9 nov. 2017 à 23:10, Gerwin Klein <ge...@do... <mailto:ge...@do...>> a écrit :
>> On 10.11.2017, at 00:46, Régis Décamps via Jflex-devel <jfl...@li... <mailto:jfl...@li...>> wrote:
>> 
>> Hello all,
>> 
>> I've accidentally submitted to master a couple of times.
>> 
>> Github has a nice feature to protect branches. How do you feel about protecting master <https://github.com/jflex-de/jflex/settings/branches/master>?
>> 
>> I propose to restrict master to PR only ; and we can even enforce CI status check as well <https://help.github.com/articles/enabling-required-status-checks/>.
> 
> I’d be happy with that. 
> 
> Just to close the loop, I think I feel very happy with the protection of the branches.

Yes, I agree, this has worked quite nicely.


> Also the status check from Travis comes fast enough and provides a nice safety bet. I feel almost in the same environment than at work.

:-)


> I tentatively enabled a more restrictive "Require branches to be up to date before merging". It was annoying. You have to press the merge button and wait again. If you submit multiple things, it's really annoying that you need to press "Merge". I think it would be nice if github had auto-merge, but until then,I dislike this protection.

I’m happy either way. It’s Ok to leave that one off, I don’t think we’ve had a test failure yet that was caused by a merge itself. We will probably get one at some point, but I don’t think that’s a big problem.


>  Now, as you can read in #410 <https://github.com/jflex-de/jflex/pull/410> I'm experimenting with a few code quality services. I really like Semmle lgtm.com <lgtm.comhttps://lgtm.com/projects/g/jflex-de/jflex> the most so far.

Yes, this one looks fairly nice. 



>> 
>> Also, I personally feel that the cleanest way to integrate the branch is to "Squash and merge", but that's a slightly different topic.
> 
> The merge pattern depends for me. I like topical commits that pertain to one specific thing. A pull request is often just one of these, then squash and merge is good, but it’s also reasonably common that there are multiple things on one theme that each make sense on their own in a PR, and then I’d usually prefer rebase and merge. I do tend towards smaller commits myself, but I don’t think everyone has to do that.
> 
> The advantage of smaller topical commits is that you can isolate features and still revert/reorder things afterwards if necessary, e.g. when you later figure out that some feature doesn’t quite interact so well with the rest and you have to remove or change it.
> 
> I still prefer the squash ; I believe a PR should be topical in the first place.

I think commits should also be topical (potentially smaller units than a PR), but I’m not really opposed to squashing. My usual process is it have potentially messy commits on a feature branch first until I’m reasonably happy with the result, and then clean up the commits into more logical units for a PR, then rebase for the merge. It mostly depends on the size of the PR if that makes sense or not. I’m also quite fine with work-in-progress commits on the PR and then squash at the end if the entire thing is small enough for a single commit.

We could just go with both, depending what the person who submits and merges the PR prefers.


> I really dislike the standard merge commit. It creates a complex history that shows the temporary feature branch.<Screen Shot 2018-10-07 at 11.57.50.png>
> (I think merge makes more sense in case you merge a maintenance branch into master/devel branch).

I completely agree with that. The recent merge commit that sneaked in was just me clicking the wrong button on GitHub.

Cheers,
Gerwin