Revise CONTRIBUTING.md #34
Conversation
thatplguy
commented
Apr 17, 2024
thatplguy
commented
- Adds a getting started section
- Consolidates code contribution steps into actionable steps with inline guidance
- Moves general guidance later in the document
- Adds a getting started section - Consolidates code contribution steps into actionable steps with inline guidance - Moves general guidance later in the document
CONTRIBUTING.md
Outdated
|
|
||
| - Merge regularly from the main branch. | ||
|
|
||
| - Rebasing from the main branch (`git pull --rebase origin main`) is not recommended. See [the "Merging" section in these guidelines](https://gitlab-ext.galois.com/program-analysis/guidance/-/blob/master/BestPractices.org?ref_type=heads#:~:text=updating%20a%20submodule.-,Versioning,-Versioning%20should%20follow) for the reasoning. |
There was a problem hiding this comment.
In the past the most difficult for me was to resolve merge conflicts - perhaps we should add explicit commands to run to do so? I actually don't know what you recommend, if rebasing is discouraged.
There was a problem hiding this comment.
Yes, I agree - I prefer rebasing since it shows you the conflicts in smaller, more manageable chunks, with related conflicts usually grouped together. If your branch has two different commits that both conflict with main, rebase will show you the conflicts from one, then the conflicts from the other, whereas merge gives you all the conflicts from both mixed together.
Somewhat related, I'm also a fan of Gitlab's "semi-linear history" mode, where you always rebase (and then run CI) before merging. It produces a nice readable history, made up of a single linear sequence of commits grouped by PR, and trivially satisfies the "not rocket science" rule for CI. Unfortunately, it seems like Github doesn't support this mode.
There was a problem hiding this comment.
I also have a (mild) preference for rebasing, and I think this project's other development practices are conducive to it: informative, atomic, and bisect-friendly commits on frequent, short-lived branches are made substantially more useful by maintaining a linear history.
Separately, I'd suggest using https://gitlab-ext.galois.com/program-analysis/guidance/-/blob/78d2d6229f027579384c39a76cf28026b03279a7/BestPractices.org as a link here instead, since it is more permanent and doesn't try (and fail, at least on Safari) to highlight an irrelevant-to-this-recommendation portion of that document.
There was a problem hiding this comment.
TBH, I also prefer rebasing. Let's settle on that for this project. Overall, here's the guidance I most care about:
- No commit on the main branch should break the build. If you commit work-in-progress code on your branch, rebase (
git rebase -i) prior to merging or "squash and merge". - Avoid force-pushing to shared branches. If you must, prefer
git push --force-with-leaseovergit push --force(for reasons).
Ideally, structure commits into minimal independent changes. For example,
Add logging util to capture timingAdd new core algorithm, disabled by defaultAdd flag to use new core algorithm
is preferable to
Add new core algorithm with flag to enable
is preferable to
First attempt at core algorithmFix loggingRefactor logging fixFix up algorithmWIPFinish algAdd flag
See What goes into a commit for more details.
Also, thank you @samcowger – I'll update the link.
|
@thatplguy should we just wait until you push updates/changes here as discussed? |
|
Yes, just pushed changes and requested another round of reviews. |
Co-authored-by: Michal Podhradsky <[email protected]>
