9 ms·

Who is creating a separate PR for each commit on their feature/fix branch? sounds like crazy town. I just dont understand why someone would operate like this.

Who is creating a separate PR for each commit on their feature/fix branch? sounds like crazy town. I just dont understand why someone would operate like this. Lets assume you're squash merging your feature branchs to your local main, then you're raising the them as prs. why would you do this?

steveklabnik 22d ago
This is standard practice in the "stacked diffs" world: one review, one commit.
jsphweid 22d ago
1 commit == 1 reviewable unit == 1 PR == 1 CL == 1 feature == 1 fix is a perfectly reasonable way of working. I used to work at companies where no one squashed their commits and the entire git logs were filled with 80% non-sense like "temp" or "bad" or "working" with the other 20% being coherent changes. What's the point of doing this I ask?
- cobalt 22d ago
  it lets you maintain version history when working, then most workflows auto squash on merge
- datsci_est_2015 22d ago
  Well are we talking about commits pre- or post-merge? I don’t care how many commits you put into the PR / MR as long as they squash down to a single commit upon merge.
  - steveklabnik 22d ago
    When you work this way, each commit is expected to be able to land independently.
    - datsci_est_2015 22d ago
      Doesn’t sound like it leaves much room for error. How do you address PR / MR comments? Force push?
      - steveklabnik 21d ago
        The reason people like to work this way is that it lets you very effectively respond to review. You address them by amending the commit to incorporate the feedback. This enables good interdiff review, so you can re-review just the new stuff in the new version of the commit and not the entire thing all over again.
- zmmmmm 22d ago
  there is an inbetween .... i insist people interactively rebase those commits out. In some contexts it is actually important to have traceability of iterative proof of work towards the final result.
- adrian_m 22d ago
  I don’t quite get the situation. After merging a PR/MR (with the squash feature of Gitlab or GitHub) the equality you describe holds and there’s a nice sequential git log. I quite like that I can tell my colleagues they can commit whenever they feel like it (and should probably commit more often than they feel like) because it becomes one clean commit in the end anyways.
  - boomlinde 22d ago
    In my experience the situation with GitHub is either that - you apply the commits as-is on the target branch and end up with a ton of pointless "fix this" and "fix that" commits that were created during the review process, - you squash everything and lose what might otherwise have been meaningful, useful commit boundaries for the purpose of bisecting, reverting or just investigating the history of changes or - you rework your commits and force push changes to the branch you want to pull and create confusion as to what changed because that's not usually how review is conducted with GitHub. I think the pull request is a bad abstraction. Not terrible, and certainly easy to grasp, but bad enough that everything you do with it is a compromise. At the very best, if you apply with the squash+rebase strategy and limit the scope of your PRs to what makes perfect sense as just one commit in the target branch, it's just a convoluted way of working with individual commits.
- l72 21d ago
  I wish git worked like some of the other dvcs (bazaar/breezy). The default merge type from a branch should be a merge commit and git log should only show the first parent (commits directly on the current branch). This gives you a very clean log on main (it only shows commits directly on this branch). There is no need to squash, rebase, or anything else. And if you want to dig down into individual commits that happened on a branch, you can! This can be a bit replicated by forcing a merge type of merge and setting an alias of log to "log --first-parent", but since that isn't the default, that isn't what you see when you look at the commit logs on platforms like github. I never understood why git decided to show a flat list of every commit that happened, even if the commit originally happened on a separate branch.
what 22d ago
Why would you have more than one commit for a PR? That sounds like crazy town.
- chrisweekly 22d ago
  IMHO equating commits and PRs puts undue pressure on the scope and quality of a given commit, adding potential for unnecessary stress and eliminating the benefits of an additional buffer / layer for aggregation of changes. A PR representing a sizable feature or refactor might naturally contain a dozen commits, each dedicated to a logical area or a requisite subset of the whole. Assuming on principle a goal of keeping main in a known-good state, such intermediate and incomplete changes (fine in an unstable feature branch) would wreak havoc. It's equivalent to asking, "Why would you have more than one story in an epic (or task in a story)?".
  - what 22d ago
    If your PR has more than one commit, each one should be deployable in isolation. Which means you can split your giant PR into smaller ones that can be reviewed independently.
    - tclancy 22d ago
      I’ve worked under both systems, but isn’t the purity you’re describing a bit of a dodge in that you wind up force pushing amended commits when you find you forgot something?
      - steveklabnik 22d ago
        Why is that a dodge? that's the expected way to work in this system, and it should be able to show you the interdiff between those amends.
        tclancy 21d ago
        Sorry, “dodge” was a loaded word. I suppose what I mean is, what’s the difference between a PR with multiple commits and a commit with multiple commits?
verall 22d ago
On large teams I think the "cherry pick" workflow (Gerrit style) beats the "pull request" workflow (GitHub/gitlab style). On smaller teams it's the other way around. I think it's somewhere around 10-20 people actively committing that the cherry pick workflow comes out ahead.
- danpalmer 22d ago
  This is exactly it. When I worked in a ~10 person team I just didn't get it, PRs worked quite well (with some basic discipline, they're not perfect). When I moved to a... much larger company... I don't know how PRs would work here, it would be way too unwieldy. The Gerrit style works fantastically here.
shubhamjain 22d ago
I haven’t used this project but I have used Gerrit. It has its drawbacks (like terrible UX) but its style of code reviews were the most sensible and commit of every PR might not be as bad as it sounds. GitHub’s PR reviews are atrocious and it’s unfortunate they have become the gold standard. In Gerrit, you commit every review, and the author has to edit individual commits to address them (using git rebase). This may sound PITA but it makes the history absolutely clean, and makes it easier for both reviewers and authors to review and address suggestions. On Github, on the other hand, reviewing a large PR is just insanely hard. Making sure comment was addressed properly is hard as well, they can get lost in a sea of suggestions. They also become separate commits instead of being part of the commit itself. The commit should always been treated as unit of work rather than the branch.
IshKebab 22d ago
You wouldn't. Imagine you have more than one of what you are calling a "feature/fix branch" and they depend on each other.
barrkel 21d ago
It makes every commit small, so they can be reviewed quickly and easily. Small commits can often be tested faster, since irrelevant tests don't need to run. Small commits are less risky. The smaller the delta of change, the lower the probability that something breaks. Small commits get merged sooner; big commits take time to build up. Merging early front-loads your integration risk; merging later puts integration risk just before delivery. Breaking a big feature into small commits means using feature flags to control whether a feature is enabled or not (since control paths will generally be incomplete). This means you separate the delivery of the code from the delivery of the feature, and has the added benefit that you can turn off a feature that has a problematic rollout without needing to redeploy code.

steveklabnik 22d ago

This is standard practice in the "stacked diffs" world: one review, one commit.

jsphweid 22d ago

1 commit == 1 reviewable unit == 1 PR == 1 CL == 1 feature == 1 fix is a perfectly reasonable way of working. I used to work at companies where no one squashed their commits and the entire git logs were filled with 80% non-sense like "temp" or "bad" or "working" with the other 20% being coherent changes. What's the point of doing this I ask?

cobalt 22d ago
it lets you maintain version history when working, then most workflows auto squash on merge
datsci_est_2015 22d ago
Well are we talking about commits pre- or post-merge? I don’t care how many commits you put into the PR / MR as long as they squash down to a single commit upon merge.
- steveklabnik 22d ago
  When you work this way, each commit is expected to be able to land independently.
  - datsci_est_2015 22d ago
    Doesn’t sound like it leaves much room for error. How do you address PR / MR comments? Force push?
    - steveklabnik 21d ago
      The reason people like to work this way is that it lets you very effectively respond to review. You address them by amending the commit to incorporate the feedback. This enables good interdiff review, so you can re-review just the new stuff in the new version of the commit and not the entire thing all over again.
zmmmmm 22d ago
there is an inbetween .... i insist people interactively rebase those commits out. In some contexts it is actually important to have traceability of iterative proof of work towards the final result.
adrian_m 22d ago
I don’t quite get the situation. After merging a PR/MR (with the squash feature of Gitlab or GitHub) the equality you describe holds and there’s a nice sequential git log. I quite like that I can tell my colleagues they can commit whenever they feel like it (and should probably commit more often than they feel like) because it becomes one clean commit in the end anyways.
- boomlinde 22d ago
  In my experience the situation with GitHub is either that - you apply the commits as-is on the target branch and end up with a ton of pointless "fix this" and "fix that" commits that were created during the review process, - you squash everything and lose what might otherwise have been meaningful, useful commit boundaries for the purpose of bisecting, reverting or just investigating the history of changes or - you rework your commits and force push changes to the branch you want to pull and create confusion as to what changed because that's not usually how review is conducted with GitHub. I think the pull request is a bad abstraction. Not terrible, and certainly easy to grasp, but bad enough that everything you do with it is a compromise. At the very best, if you apply with the squash+rebase strategy and limit the scope of your PRs to what makes perfect sense as just one commit in the target branch, it's just a convoluted way of working with individual commits.
l72 21d ago
I wish git worked like some of the other dvcs (bazaar/breezy). The default merge type from a branch should be a merge commit and git log should only show the first parent (commits directly on the current branch). This gives you a very clean log on main (it only shows commits directly on this branch). There is no need to squash, rebase, or anything else. And if you want to dig down into individual commits that happened on a branch, you can! This can be a bit replicated by forcing a merge type of merge and setting an alias of log to "log --first-parent", but since that isn't the default, that isn't what you see when you look at the commit logs on platforms like github. I never understood why git decided to show a flat list of every commit that happened, even if the commit originally happened on a separate branch.

what 22d ago

Why would you have more than one commit for a PR? That sounds like crazy town.

chrisweekly 22d ago
IMHO equating commits and PRs puts undue pressure on the scope and quality of a given commit, adding potential for unnecessary stress and eliminating the benefits of an additional buffer / layer for aggregation of changes. A PR representing a sizable feature or refactor might naturally contain a dozen commits, each dedicated to a logical area or a requisite subset of the whole. Assuming on principle a goal of keeping main in a known-good state, such intermediate and incomplete changes (fine in an unstable feature branch) would wreak havoc. It's equivalent to asking, "Why would you have more than one story in an epic (or task in a story)?".
- what 22d ago
  If your PR has more than one commit, each one should be deployable in isolation. Which means you can split your giant PR into smaller ones that can be reviewed independently.
  - tclancy 22d ago
    I’ve worked under both systems, but isn’t the purity you’re describing a bit of a dodge in that you wind up force pushing amended commits when you find you forgot something?
    - steveklabnik 22d ago
      Why is that a dodge? that's the expected way to work in this system, and it should be able to show you the interdiff between those amends.
      - tclancy 21d ago
        Sorry, “dodge” was a loaded word. I suppose what I mean is, what’s the difference between a PR with multiple commits and a commit with multiple commits?

verall 22d ago

On large teams I think the "cherry pick" workflow (Gerrit style) beats the "pull request" workflow (GitHub/gitlab style). On smaller teams it's the other way around. I think it's somewhere around 10-20 people actively committing that the cherry pick workflow comes out ahead.

danpalmer 22d ago
This is exactly it. When I worked in a ~10 person team I just didn't get it, PRs worked quite well (with some basic discipline, they're not perfect). When I moved to a... much larger company... I don't know how PRs would work here, it would be way too unwieldy. The Gerrit style works fantastically here.

shubhamjain 22d ago

I haven’t used this project but I have used Gerrit. It has its drawbacks (like terrible UX) but its style of code reviews were the most sensible and commit of every PR might not be as bad as it sounds. GitHub’s PR reviews are atrocious and it’s unfortunate they have become the gold standard. In Gerrit, you commit every review, and the author has to edit individual commits to address them (using git rebase). This may sound PITA but it makes the history absolutely clean, and makes it easier for both reviewers and authors to review and address suggestions. On Github, on the other hand, reviewing a large PR is just insanely hard. Making sure comment was addressed properly is hard as well, they can get lost in a sea of suggestions. They also become separate commits instead of being part of the commit itself. The commit should always been treated as unit of work rather than the branch.

IshKebab 22d ago

You wouldn't. Imagine you have more than one of what you are calling a "feature/fix branch" and they depend on each other.

barrkel 21d ago

It makes every commit small, so they can be reviewed quickly and easily. Small commits can often be tested faster, since irrelevant tests don't need to run. Small commits are less risky. The smaller the delta of change, the lower the probability that something breaks. Small commits get merged sooner; big commits take time to build up. Merging early front-loads your integration risk; merging later puts integration risk just before delivery. Breaking a big feature into small commits means using feature flags to control whether a feature is enabled or not (since control paths will generally be incomplete). This means you separate the delivery of the code from the delivery of the feature, and has the added benefit that you can turn off a feature that has a problematic rollout without needing to redeploy code.