Same. Do whatever you want in your feature branch, what matters is the Files list and the description in the PR. The whole thing gets squashed into a single commit anyway (which also makes reverting much easier).
2. have not very disjunct PRs (sometimes for e.g. legacy maintenance projects you mostly have disjunct PRs normaly you do not)
then you need stacked PRs for productivity, i.e. you need to be able to continue working on a new PR based on the old PR before that is fully merged (or reviwed).
In this case in my experience three workflow work:
1. you (may) squash commits, and rebase stacked PRs once the previous PR has been merged (or sometimes majorly modified, but that quite advanced rebase usage). This works but has some major pain points: 1) rebased during reviews are terrible bad handled by github, 2) git doesn't keep track of the original start of a branch, this can lead to issues if you squash the commits when merging, 3) no good build in tooling for it
2. All forms of history manipulation are forbidden including rebasing and squashing. It's merge only because of this git doesn't get confused when merging squashed commits and everything seems fine... Until you now realize that follow up changes from reviews of a parent PR happen in the git history chronologically after your follow up PR and that can be a total pain depending on what changes. (Through you are allowed to fully rebase your history before marking a PR as ready for review so as long as the "stack" of PRs isn't too deep it's fine).
3. you agree with Linus that github PRs have major issues and go with a patch based approach for merging, now you need completely different tooling which often less nice modern UI but id doesn't have any of the issues of point 1 or 2
It's was quite a wtf are you doing industry moment when I realized that the most widely used contributions flows (weather in open source or in companies) are either quite flawed (1&2), productivity nightmares (no stacked commits) or quite inconvenient (3).
don't know why, but recent teams around me have always made strict rules about number of commits in PRs. I just wanted to tell them the same thing you said: "Why don't you just look at the diffs?" curious for other opinions. (sorry not really about this particular topic)
I prefer to have clear commits that tell a tidy story. For example:
* Refactor function `foo` to accept a second parameter
* Add function `bar`
* Use `bar` and `foo` in component `Baz` to implement feature #X
If you give me a commit history like this, I can easily validate that each step in your claimed process does what you describe.
If you instead give me a messy history and ask me to read the diff, you might know that the change to file `Something.ts` on line 125 was conceptually part of the refactor to `foo`, but I'll have to piece that together myself. It's not obvious to the person who didn't write the code what the purpose of any given change was supposed to be.
This isn't a huge deal if your team's process is such that each step above is a PR on its own, but if your PRs are at the coarseness of a full feature, it's helpful to break down the sub-steps into smaller (but sane and readable) diffs.
A good practice is to rebase your commits before creating a PR into a single commit. You are free to commit as many times as you want to while doing your work. This minimizes the noise in the log.
You do you but, at the point of publishing a branch for review, I'd insist the changes are presented as a story, with well-written commit messages that helps the reader/reviewer orient themselves and presents a coherent narrative.
Anthing else, I call it a landfill site, not a maintained repository.
In fact, I'd go as far as using their commit habit as a measure of a candidate's consideration for their colleagues.
But then when you're done, turn it into a series of patches for a reviewer to read. In the words of Greg Kroah-Hartman, "pretend I'm your math teacher and show your working".
In a maths assignment, you spend ages making a big mess on a scrap of paper. Then when you've got the solution, you list the steps nice and clearly for the teacher as if you got it right first time. In software development, if you're not a dick, you do the same. You make a big old mess with loads of commits, then when you're done and it's review time, you turn it into a series of tidy commits that are easy for someone to review one-by-one.
Commit and commit often, but then clean up the history into discrete, readable chunks.
If your PRs are tiny it's not a big deal, but with 190 files changed in this one, it absolutely should have been rebased into a more reasonable commit history.
MenhirMike|2 years ago
eitland|2 years ago
I almost never look at them, but once in a while it is really great to see the thought process that led to something.
dathinab|2 years ago
1. require reviews before merging
2. have not very disjunct PRs (sometimes for e.g. legacy maintenance projects you mostly have disjunct PRs normaly you do not)
then you need stacked PRs for productivity, i.e. you need to be able to continue working on a new PR based on the old PR before that is fully merged (or reviwed).
In this case in my experience three workflow work:
1. you (may) squash commits, and rebase stacked PRs once the previous PR has been merged (or sometimes majorly modified, but that quite advanced rebase usage). This works but has some major pain points: 1) rebased during reviews are terrible bad handled by github, 2) git doesn't keep track of the original start of a branch, this can lead to issues if you squash the commits when merging, 3) no good build in tooling for it
2. All forms of history manipulation are forbidden including rebasing and squashing. It's merge only because of this git doesn't get confused when merging squashed commits and everything seems fine... Until you now realize that follow up changes from reviews of a parent PR happen in the git history chronologically after your follow up PR and that can be a total pain depending on what changes. (Through you are allowed to fully rebase your history before marking a PR as ready for review so as long as the "stack" of PRs isn't too deep it's fine).
3. you agree with Linus that github PRs have major issues and go with a patch based approach for merging, now you need completely different tooling which often less nice modern UI but id doesn't have any of the issues of point 1 or 2
It's was quite a wtf are you doing industry moment when I realized that the most widely used contributions flows (weather in open source or in companies) are either quite flawed (1&2), productivity nightmares (no stacked commits) or quite inconvenient (3).
readline_prompt|2 years ago
lolinder|2 years ago
* Refactor function `foo` to accept a second parameter
* Add function `bar`
* Use `bar` and `foo` in component `Baz` to implement feature #X
If you give me a commit history like this, I can easily validate that each step in your claimed process does what you describe.
If you instead give me a messy history and ask me to read the diff, you might know that the change to file `Something.ts` on line 125 was conceptually part of the refactor to `foo`, but I'll have to piece that together myself. It's not obvious to the person who didn't write the code what the purpose of any given change was supposed to be.
This isn't a huge deal if your team's process is such that each step above is a PR on its own, but if your PRs are at the coarseness of a full feature, it's helpful to break down the sub-steps into smaller (but sane and readable) diffs.
ukzwc0|2 years ago
FPGAhacker|2 years ago
gonzo41|2 years ago
dathinab|2 years ago
I.e. with an idea like:
- if we try to commit so that each commit does a singular change
- then by limiting the number of commits we limit the number of "logical" changes in a PR
- and in turn make reviews and similar easier
recursive|2 years ago
1. Branch f-prime from master. 2. Squash merge f to f-prime. 3. Pull request f-prime to master. 4. Profit.
penguin_booze|2 years ago
Anthing else, I call it a landfill site, not a maintained repository.
In fact, I'd go as far as using their commit habit as a measure of a candidate's consideration for their colleagues.
_heimdall|2 years ago
nkozyra|2 years ago
Diffs are great but sometimes they're just as overwhelming in a huge PR. It's nice to first follow 5-10 commits in chunks of logical change.
JammyDodge|2 years ago
But then when you're done, turn it into a series of patches for a reviewer to read. In the words of Greg Kroah-Hartman, "pretend I'm your math teacher and show your working".
In a maths assignment, you spend ages making a big mess on a scrap of paper. Then when you've got the solution, you list the steps nice and clearly for the teacher as if you got it right first time. In software development, if you're not a dick, you do the same. You make a big old mess with loads of commits, then when you're done and it's review time, you turn it into a series of tidy commits that are easy for someone to review one-by-one.
lolinder|2 years ago
If your PRs are tiny it's not a big deal, but with 190 files changed in this one, it absolutely should have been rebased into a more reasonable commit history.
aantix|2 years ago
I don’t want to interrupt my flow with intermediary commits.
h0l0cube|2 years ago
throwaway892238|2 years ago
tehlike|2 years ago