6 ms·

Relatively common practice where I work. What do you do instead?

shric 3y ago
Not the GP but we tend to use decently small and meaningful commits to make PRs easier to review. They then get squashed, so you can only (easily) revert the whole PR.
- williamdclt 3y ago
  Personally I’m happy with that. If the thing merged caused an issue, I’d want to revert the entire thing that was merged, not a subpart of it which would result in a state of the codebase that nobody reviewed. If you’re very diligent about making good atomic commits that always pass all test that can work, but I find that squashing PRs (and PRs being relatively small) is a very good trade off
  - matrss 3y ago
    Sounds reasonable. Also, when squashing a PR would conflate unrelated commits then maybe those commits should have been separate PRs instead. I've recently started to like squashing after usually preferring rebase merges, because with squashes I have an easy option to rewrite the commit message and edit out all of those meaningless "fix typo", "improve" and "now actually working" lines...
    - palata 3y ago
      > I've recently started to like squashing after usually preferring rebase merges, because with squashes I have an easy option to rewrite the commit message And why couldn't you rewrite the commit messages while rebasing? EDIT: oh, you mean from the GitHub webUI, I presume. Got it.
      - matrss 3y ago
        > EDIT: oh, you mean from the GitHub webUI, I presume. Got it. Yes, exactly, while reviewing and merging other peoples PRs. Technically I could still rewrite in other ways, but while squashing in the web UI was the most comfortable workflow.
- lubutu 3y ago
  You're not the only ones, but I can't understand this approach. Do people never then read the version history? It must be impossible to understand commits' diffs with the changes all squashed together.
  - palata 3y ago
    Not the OP, but I think that the point of squashing every PR is that the reviewers/PR run the whole PR, not the individual commits. If you have a PR with 5 commits, 4 of which break the build and the last one fixes it, then merging that will be a problem if you need to git bisect later. So the idea is really "what's the point of having a history full of broken state?". > It must be impossible to understand commits' diffs with the changes all squashed together. This would be a hint that your PR was too big and addressing more than one thing.
    - bonzini 3y ago
      > If you have a PR with 5 commits, 4 of which break the build and the last one fixes it, then merging that will be a problem if you need to git bisect later. And the answer is that you don't; each commit is individually testable and reviewable. Changes requested by reviewers are squashed into the commits and then merged into the project. Unfortunately, while the git command line has "range-diff" to ease review with this workflow, neither GitHub not Gitlab have an equivalent in their UI.
      - palata 3y ago
        Well, I was obviously meaning that "workflows that squash the commits in a PR are workflows where each individual commit is not tested/reviewed separately". Of course, if your workflow is different, then... well it is different. Doesn't make the "squash workflows" irrational. Disclaimer: I don't squash PRs.
      - syndicatedjelly 3y ago
        > And the answer is that you don't; each commit is individually testable and reviewable. How does this work in practice? Is every single atomic commit reviewed by someone? When do they review each of those commits? How many commits typically go into a PR? > Changes requested by reviewers are squashed into the commits and then merged into the project. So a reviewer finds the appropriate commit that their comment applies to, and then changes the actual commit itself? Who is the author of the commit at that point? I'm trying to understand what you're talking about, because you seem to have something figured out, for a problem that every team I've worked on struggles with.
mewpmewp2 3y ago
Work around it.
deleted 3y ago
[deleted]
from-nibly 3y ago
throw spaghetti at the wall till the phone app stays quiet!
quectophoton 3y ago
What I've seen the most is rolling back the deployment (if any) and writing a new commit with the fix.

Not the GP but we tend to use decently small and meaningful commits to make PRs easier to review. They then get squashed, so you can only (easily) revert the whole PR.

williamdclt 3y ago
Personally I’m happy with that. If the thing merged caused an issue, I’d want to revert the entire thing that was merged, not a subpart of it which would result in a state of the codebase that nobody reviewed. If you’re very diligent about making good atomic commits that always pass all test that can work, but I find that squashing PRs (and PRs being relatively small) is a very good trade off
- matrss 3y ago
  Sounds reasonable. Also, when squashing a PR would conflate unrelated commits then maybe those commits should have been separate PRs instead. I've recently started to like squashing after usually preferring rebase merges, because with squashes I have an easy option to rewrite the commit message and edit out all of those meaningless "fix typo", "improve" and "now actually working" lines...
  - palata 3y ago
    > I've recently started to like squashing after usually preferring rebase merges, because with squashes I have an easy option to rewrite the commit message And why couldn't you rewrite the commit messages while rebasing? EDIT: oh, you mean from the GitHub webUI, I presume. Got it.
    - matrss 3y ago
      > EDIT: oh, you mean from the GitHub webUI, I presume. Got it. Yes, exactly, while reviewing and merging other peoples PRs. Technically I could still rewrite in other ways, but while squashing in the web UI was the most comfortable workflow.
lubutu 3y ago
You're not the only ones, but I can't understand this approach. Do people never then read the version history? It must be impossible to understand commits' diffs with the changes all squashed together.
- palata 3y ago
  Not the OP, but I think that the point of squashing every PR is that the reviewers/PR run the whole PR, not the individual commits. If you have a PR with 5 commits, 4 of which break the build and the last one fixes it, then merging that will be a problem if you need to git bisect later. So the idea is really "what's the point of having a history full of broken state?". > It must be impossible to understand commits' diffs with the changes all squashed together. This would be a hint that your PR was too big and addressing more than one thing.
  - bonzini 3y ago
    > If you have a PR with 5 commits, 4 of which break the build and the last one fixes it, then merging that will be a problem if you need to git bisect later. And the answer is that you don't; each commit is individually testable and reviewable. Changes requested by reviewers are squashed into the commits and then merged into the project. Unfortunately, while the git command line has "range-diff" to ease review with this workflow, neither GitHub not Gitlab have an equivalent in their UI.
    - palata 3y ago
      Well, I was obviously meaning that "workflows that squash the commits in a PR are workflows where each individual commit is not tested/reviewed separately". Of course, if your workflow is different, then... well it is different. Doesn't make the "squash workflows" irrational. Disclaimer: I don't squash PRs.
    - syndicatedjelly 3y ago
      > And the answer is that you don't; each commit is individually testable and reviewable. How does this work in practice? Is every single atomic commit reviewed by someone? When do they review each of those commits? How many commits typically go into a PR? > Changes requested by reviewers are squashed into the commits and then merged into the project. So a reviewer finds the appropriate commit that their comment applies to, and then changes the actual commit itself? Who is the author of the commit at that point? I'm trying to understand what you're talking about, because you seem to have something figured out, for a problem that every team I've worked on struggles with.

mewpmewp2 3y ago

Work around it.

deleted 3y ago

[deleted]

from-nibly 3y ago

throw spaghetti at the wall till the phone app stays quiet!

quectophoton 3y ago

What I've seen the most is rolling back the deployment (if any) and writing a new commit with the fix.