Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit history and merging Pull Requests #6

Open
briederer opened this issue May 23, 2021 · 9 comments
Open

Commit history and merging Pull Requests #6

briederer opened this issue May 23, 2021 · 9 comments
Labels
workflow For internal workflow and CI

Comments

@briederer
Copy link
Owner

We also need to come up with a clean way of pushing pull requests to master to keep the history clean.
Seems like that two (of three) options on the Github-App doesn't work as I expect them to 🤔
Will try the last one and otherwise I won't merge anymore in the app.

@briederer briederer added the workflow For internal workflow and CI label May 23, 2021
@fzierler
Copy link
Collaborator

I am not fully sure I understand the problem you are experiencing. We could try to rebase and squash commits before merging any pull requests so that every pull request is linked to a single commit in the correct chronological order. I am using GitKraken which provides a nice GUI for this.

@briederer
Copy link
Owner Author

briederer commented May 23, 2021

Yes I think too that rebase/squash is the way to go (also since with rebase one could combine multiple PRs if needed).
Also this is no problem to do at the PC but the GitHub-Smartphone App provides only three possibilities to merge:

  • Simple merge
  • Squash and merge
  • Rebase and merge

So far I've tried the first (CompatHelper PR #1) and the third (for #4) and the merges were a bit messy.
So basically I just want to figure out if there is a possibility to have nice merges using the Smartphone app 😉

@fzierler
Copy link
Collaborator

fzierler commented May 23, 2021

Ah, I did not think about the mobile app. I could still squash the latest three commits of #4 on master although I see why it is preferable to do this before merging.

Edit: Also I think we should try to continue not merging our own pull requests. This should make for a good code review workflow.

@briederer
Copy link
Owner Author

Re squashing: this would be nice of you thanks.

Re self-merging: makes sense. If there is something very urgent to be added immediately we can just push to master. Otherwise file a PR and wait for the other(s) to approve and merge.

@fzierler
Copy link
Collaborator

Yes, I don't think self-merging is bad in itself but we can try and avoid it where possible. Also I have now squashed the last commits

@fzierler
Copy link
Collaborator

fzierler commented May 24, 2021

When merging #7 through the Website I encountered the same issue that you had. After merging the pull request both your commit and a merge commit showed up in the history. I am pretty sure that there is a better way of doing this than manually squashing the commits afterwards.

Edit. I had the same issue as you when trying to merge multiple CompatHelper pull requests. It seems the proper way of doing this is to first resolve the conflicts and then to squash&merge.

@briederer
Copy link
Owner Author

The following ways seem to be the best:

  • Merging one PR (with/out multiple commits):
    • Website/Mobile app: Squash and merge
    • CLI/GUI: Squash and merge
  • Merging multiple PRs at once:
    • Website/Mobile app: Don't do it
    • CLI/GUI: Follow this guide and squash/merge the new branch with all PRs to master

@briederer
Copy link
Owner Author

I've cleaned up the full commit history now following this (guide)[https://blog.nona.digital/cleaning-up-commit-history-with-git-rebase/amp/] and tried to get some structure into our commit-naming scheme.

My suggestion would be to stick to the following conventions for commits:

  • Add: or Fix: for direct pushes to master
    • optional: more infos listed with + and * markers
  • PR: for usual Pull requests that are merged
    • PR reference in parantheses
    • squashed commits listed with + marker
  • CompatHelper: Added new compat entry/entries for merges from CompatHelper pull requests
    • All PR references in parantheses
    • State changed Project.toml
    • Added Packages listed at * Package 'Packagename' v'versionnumber'

@briederer briederer changed the title Merging Pull Request Commit history and merging Pull Requests May 25, 2021
@fzierler
Copy link
Collaborator

Great. Let's keep this issue open for now until I manage to submit commits/PR using this convention. ;) It seems that the best way to deal with multiple ComaptHelper PRs is to create a combined one from scratch. This is probably more straightforward than merging multiple PRs and then squashing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow For internal workflow and CI
Projects
None yet
Development

No branches or pull requests

2 participants