3 ms·

Yeah but the other problem in pull requests is that if someone makes changes since the last time I reviewed and then they force push I can’t see the difference

Yeah but the other problem in pull requests is that if someone makes changes since the last time I reviewed and then they force push I can’t see the difference from the last time I reviewed. Also if they rebase and push multiple commits there’s no guarantee they would all pass CI which means you can’t guarantee it will work with bisecting.

fragmede 4y ago
Force push doesn't delete the commit from your computer so you should still be able to diff between the (disjoint) commits using the hash.
- alexrtan 4y ago
  If someone else has done the pull request, I review it on GitHub, then they rebase and force push the original commit wouldn’t be on my machine unless I had pulled it earlier correct? Which is something I never do.
  - fragmede 4y ago
    If you don't have the ref locally but you have it open in a browser tab, you should† still be able to manually hack the URL to get a diff. Eg https://github.com/git/git/compare/95d1613a9fce4bcfc82d8ba48eb3224388f0b7e1...master https://github.com/git/git/compare/95d1613a9fce4bcfc82d8ba48... † My company's not on Github Enterprise and I don't have a repo and a second user handy to double check on public Github.
LaLaLand122 4y ago
> if someone makes changes since the last time I reviewed and then they force push I can’t see the difference from the last time I reviewed You need better tools. Even in GitHub you can click on "force pushed" and see the diff. > there’s no guarantee they would all pass CI There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit.
- alexrtan 4y ago
  > There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit. Guarantee is the wrong word. The point is that if you always squash merge after passing the entire test suite you don’t have a bunch of potential garbage commits in history that you have to wade through when bisecting. As for building every commit that’s probably a tough sell and a poor use of money for what benefit?
  - __MatrixMan__ 4y ago
    But if it's only gonna find monstrous feature-at-a-time commits, what good is a bisect anyway? I want the culprit commit to be 5 minutes of bad ideas, not 5 minutes of bad ideas mixed with three days of good ones.
    - snovv_crash 4y ago
      Make smaller PRs then. It's a lot slower to bisect if only some of the commits even compile.
      - LaLaLand122 4y ago
        If the PR is small enough to naturally fit into a single commit the rebasing vs squashing multiple commits discussion disappears (just please don't merge it!)... there is a single commit.
        sulam 4y ago
        Many developers make commits that are “wip” with messages to match. Maybe you commit before you go to lunch, or when you go home for the day. These are useless commits and should be rebased or squashed, but many of these same devs won’t do that unless you force them or the tooling does it for them automatically. A minority of devs build a clean history of commits that tells a story, carefully crafted over time (usually manufactured after the fact with liberal use of rebase). For these devs squash is awful. Unfortunately they are truly a minority and the majority ruins things for them.
mrkeen 4y ago
That's fine. You're checking the difference between the current state of master and the current state of the feature branch, not the difference between the old state of the feature branch and the current state of the feature branch.

fragmede 4y ago

Force push doesn't delete the commit from your computer so you should still be able to diff between the (disjoint) commits using the hash.

alexrtan 4y ago
If someone else has done the pull request, I review it on GitHub, then they rebase and force push the original commit wouldn’t be on my machine unless I had pulled it earlier correct? Which is something I never do.
- fragmede 4y ago
  If you don't have the ref locally but you have it open in a browser tab, you should† still be able to manually hack the URL to get a diff. Eg https://github.com/git/git/compare/95d1613a9fce4bcfc82d8ba48eb3224388f0b7e1...master https://github.com/git/git/compare/95d1613a9fce4bcfc82d8ba48... † My company's not on Github Enterprise and I don't have a repo and a second user handy to double check on public Github.

LaLaLand122 4y ago

> if someone makes changes since the last time I reviewed and then they force push I can’t see the difference from the last time I reviewed You need better tools. Even in GitHub you can click on "force pushed" and see the diff. > there’s no guarantee they would all pass CI There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit.

alexrtan 4y ago
> There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit. Guarantee is the wrong word. The point is that if you always squash merge after passing the entire test suite you don’t have a bunch of potential garbage commits in history that you have to wade through when bisecting. As for building every commit that’s probably a tough sell and a poor use of money for what benefit?
- __MatrixMan__ 4y ago
  But if it's only gonna find monstrous feature-at-a-time commits, what good is a bisect anyway? I want the culprit commit to be 5 minutes of bad ideas, not 5 minutes of bad ideas mixed with three days of good ones.
  - snovv_crash 4y ago
    Make smaller PRs then. It's a lot slower to bisect if only some of the commits even compile.
    - LaLaLand122 4y ago
      If the PR is small enough to naturally fit into a single commit the rebasing vs squashing multiple commits discussion disappears (just please don't merge it!)... there is a single commit.
      - sulam 4y ago
        Many developers make commits that are “wip” with messages to match. Maybe you commit before you go to lunch, or when you go home for the day. These are useless commits and should be rebased or squashed, but many of these same devs won’t do that unless you force them or the tooling does it for them automatically. A minority of devs build a clean history of commits that tells a story, carefully crafted over time (usually manufactured after the fact with liberal use of rebase). For these devs squash is awful. Unfortunately they are truly a minority and the majority ruins things for them.

mrkeen 4y ago

That's fine. You're checking the difference between the current state of master and the current state of the feature branch, not the difference between the old state of the feature branch and the current state of the feature branch.