Add CONTRIBUTING section on PRs and git

[sholderbach]

Description

I describe the status quo (e.g. squashing) which came up as a FAQ.
Further more I add some opinionated best practices.

Feel free to correct me if I am overzealous or imprecise. As it is
already quite long I don't want to expand the text if possible.

Developer-Facing Changes

Added devdocs to have more guidelines/FAQs addressed

[codecov]

Codecov Report

Merging #8521 (dc147da) into main (f9cf1d9) will increase coverage by 0.36%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8521      +/-   ##
==========================================
+ Coverage   68.16%   68.53%   +0.36%     
==========================================
  Files         622      622              
  Lines      100536   100536              
==========================================
+ Hits        68535    68906     +371     
+ Misses      32001    31630     -371     

see 6 files with indirect coverage changes

[amtoine]

only a bunch of really minor things i came up with, it's really great in the end ❤️

one general thing about CONTRIBUTING.md: as it's becoming a bit larger now, maybe a simple table of content at the top would be great to jump to the section of interest 😋

Note
thanks again, i really find that super clear and great ⭐

[amtoine]

"git" and "GitHub" instead of raw strings?

[amtoine]

would git merge and git rebase result in different conflict resolution? 🤔
i have the intuition a git merge might even be easier to solve conflicts in general, because you only compute a single merge commit whereas, in a git rebase, you have to apply all your commits on the target one after the other, resulting in potentially duplicated conflict resolutions => i've already seen that and it's not fun

git a very large field to master, i might very likely miss something here, please correct me if i'm wrong on this 😌

my very personal opinion: i think, as soon as a branch is made public, i.e. other people might have a look at them by git fetching them, you should NEVER rebase the branch.
as you said that makes most of the git operations you might want to perform on it invalid because of branch divergence 🤔
on the other hand, a merge

• will not introduce additional unrelated changes in a PR => the final diff is computed with respect to the same merge target, so the changes in the merge commit will cancel out
• will allow reviewers to still work and test things on the branch as before

Note
i personnally name all my branches that might change often, including with a git rebase as wip/... to make it extra clear 😋
once i publish a PR or share a branch, i remove the wip/ and never rebase again

Warning
this is not a standard at all, just a convention i have with myself 😉

[sholderbach]

Your making absolutely valid arguments for a no public rebase policy, but I think that is a bridge too far as we also probably have contributors that come from rebase heavy workflows.

I added a sentence that we officially bless merge commits in feature branches.

But there is a place for crafting nicer histories with rebase. Furthermore I like an interactive rebase when facing a semantic merge conflict to see if each commit still works with the upstream changes.

[amtoine]

Your making absolutely valid arguments for a no public rebase policy, but I think that is a bridge too far as we also probably have contributors that come from rebase heavy workflows.

yeah for sure, already seen that and it's 100% fine.
people need to come and contribute, regardless their git background and merge strategies!

I added a sentence that we officially bless merge commits in feature branches.

that is a very good compromise 👌

But there is a place for crafting nicer histories with rebase.

i would say this is the work of the PR merge strategy set to a squash rebase with no merge commit, right?
this is how i see it for now at least 😌

Furthermore I like an interactive rebase when facing a semantic merge conflict to see if each commit still works with the upstream changes.

ohh yeah that's a good point 👍

but i can not forget the fact that i find rebasing a PR feature branch to be a pain to deal with as a reviewer 😭

[amtoine]

can we enforce a PR title / PR branch name matches a particular regex? 🤔

[sholderbach]

I don't think so we can easily have something productive there as the humans need to understand it in the end.
I am personally in the camp of "traditional" git commit message.
So I find the "conventional commits" style of "refactor: refactor..." to add more noise than signal in a lot of cases.

We were discussing having the work area in front of the colon as practiced by the serenity project but no consensus yet.

https://github.com/SerenityOS/serenity/blob/master/CONTRIBUTING.md

[amtoine]

I don't think so we can easily have something productive there as the humans need to understand it in the end.

yeah of course...

I am personally in the camp of "traditional" git commit message.

I am personally in the camp of meaningfull but still complete git commit message 😉
which is not something easy to master at all!!

So I find the "conventional commits" style of "refactor: refactor..." to add more noise than signal in a lot of cases.

yeah refactor: refactor ... is not great... 😅
i tend to write, refactor: move files around or refactor: simplify that function or refactor: compress duplicate logic, ..., that kind of messages 🤔

We were discussing having the work area in front of the colon as practiced by the serenity project but no consensus yet.

https://github.com/SerenityOS/serenity/blob/master/CONTRIBUTING.md

i kinda like their : notation 😮

[fdncred]

+1 Thanks @sholderbach

[sholderbach]

Updated it locally based on your suggestions @amtoine. Will push later when I get back to stable WiFi.

[amtoine]

Updated it locally based on your suggestions @amtoine. Will push later when I get back to stable WiFi.

looking forward to seing the little tweaks 😋

[amtoine]

that's really cool 💪