I just noticed that squash merging is the only allowed merge strategy in the OpenRefine repository. This seems to nuke quite a lot of the git history at times.
For example in the LESS to CSS migration the actual work got squashed together with renaming as well as formatting.
Any reason we couldn’t support additional merge strategies?
| abbe98
January 7 |
- | - |
I just noticed that squash merging is the only allowed merge strategy in the OpenRefine repository. This seems to nuke quite a lot of the git history at times.
For example in the LESS to CSS migration the actual work got squashed together with renaming as well as formatting.
Any reason we couldn’t support additional merge strategies?
I'm strongly supportive of the merger having the ability to choose which merge strategy they want to use. That's the way it used to be set up and it allows flexibility in choosing the right strategy.
The flip side of this is that it's highly desirable that the developers take responsibility for having the commits represent logical sets of work and that they squash/rebase all their "try an experiment" / "revert experiment" or "WIP" / "More WIP" commits before marking their pull request as ready for review.
Tom
My reasoning for setting this up is that it helps keep the commit history more compact, linking each commit to the corresponding PR. It encourages to do more atomic PRs.
I think reformatting should not normally happen in PRs unless the PR is also setting up the appropriate linter to maintain that linting - otherwise people just override each others’ preference over and over.
One can instead ask PR authors to do meaningful commits and rebase when merging… but I am not a big fan of merge commits, as they make the overall sequence of changes less clear.
The flip side of this is that it’s highly desirable that the developers take responsibility for having the commits represent logical sets of work and that they squash/rebase all their “try an experiment” / “revert experiment” or “WIP” / “More WIP” commits before marking their pull request as ready for review.
I definitively agree and think it should be a part of the review with the merger having the option to squash.
It encourages to do more atomic PRs.
I don't think non-collaborators can se which options that are available?
I think reformatting should not normally happen in PRs unless the PR is also setting up the appropriate linter to maintain that linting - otherwise people just override each others’ preference over and over.
It was just an example of a problematic case, the content isn't that relevant.
One can instead ask PR authors to do meaningful commits and rebase when merging… but I am not a big fan of merge commits, as they make the overall sequence of changes less clear.
Having additional merge options wouldn't rule out squash merging. Merge commits aren't nice but it's so common that Git-tooling(and Git itself) have ways to manage it.
I forgot to write that I have enabled the rebase strategy. Let’s see if it works for us!