4 ms·

If I've already reviewed a PR and the author makes further changes, I definitely prefer to review an add-on commit. If the history is rewritten/rebased, then IM

If I've already reviewed a PR and the author makes further changes, I definitely prefer to review an add-on commit. If the history is rewritten/rebased, then IME the entire PR needs to be re-reviewed from scratch. If we're talking about a <10 line change, then, by all means, rebase to your heart's content. With anything more complicated than that, rebasing a branch that's already been looked at can be disruptive and I'd strongly recommend against it (though squash-and-merge after review is fantastic).

seba_dos1 4y ago
> then IME the entire PR needs to be re-reviewed from scratch Why? What's the difference? You can still diff the previous version of the PR with the current version and end up with the same thing that an add-on commit would give you, but ready to merge as-is.
Arch-TK 4y ago
Does your project have a policy of compiling and running (and e.g. tests passing) at EVERY commit? I can't imagine being able to easily enforce that without asking people to edit the correct part of their commit. It's maybe more difficult with gitlab/github interfaces where changing the middle of a sequence of commits will not render very well, but in email based workflows it works fine. On the other hand, being able to bisect a project without having to worry about whether an unrelated issue is causing you to traverse the wrong branch of the bisect is an enormous advantage compared to the minimal effort required of keeping track of a modified (rebased) commit in the middle of a set of commits under review.
- rjmunro 4y ago
  You can now use --first-parent when you bisect to ensure bisect doesn't go into branches but stays on the main branch. https://git-scm.com/docs/git-bisect#Documentation/git-bisect.txt---first-parent https://git-scm.com/docs/git-bisect#Documentation/git-bisect...
  - Arch-TK 4y ago
    I don't see how this solves the problem of patches which fix up previous patches. This workflow doesn't require using merges, but it will introduce situations (irrespective of whether you use --first-parent or not) where patches fix previous patches potentially leaving a gap where code doesn't compile, doesn't run, or doesn't pass tests.
- Supermancho 4y ago
  > I can't imagine being able to easily enforce that without asking people to edit the correct part of their commit. I imagine something something githooks. However it might be enforced, it seems like a miserable way to develop.
Shatnerz 4y ago
I didn't actually read the linked article but I see it is from GitLab. GitLab makes it easy to view the diff between versions of an MR even if it includes rebases.
- johnnypangs 4y ago
  How does that feature work in Gitlab? I also use it along with a rebase policy at work and sometimes have that issue.
  - nemetroid 4y ago
    https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...
- matijs 4y ago
  GitHub does the same. There is a compare button that appears after a force (with-lease) push of a rebased branch.
jasonlotito 4y ago
Are there CR tools that don't display the diffs between rebases? Seems like a tooling issue more than anything.
kotborealis 4y ago
Some code review systems, like gerrit, actually encourage this workflow: you can easily view diffs between commit versions ('patchsets').
rubyist5eva 4y ago
In your view if a PR is not rebased but trunk is merged into it, does that warrant a full re-review? The end result is functionally the same as a rebase.
thibran 4y ago
Changes should be added with additional commits. When the review is complet, the code should be rebased and merged (git merge --ff-only my-reviewed-banch). This leads to a clean git commit history and an easy review process.
omegalulw 4y ago
You want to ask the author to change the commit you are interested in and then review the diff to that commit. This is if course mostly valuable if you don't squash commits on merge. Otherwise, the extra rebase work isn't that valuable.

seba_dos1 4y ago

> then IME the entire PR needs to be re-reviewed from scratch Why? What's the difference? You can still diff the previous version of the PR with the current version and end up with the same thing that an add-on commit would give you, but ready to merge as-is.

Arch-TK 4y ago

Does your project have a policy of compiling and running (and e.g. tests passing) at EVERY commit? I can't imagine being able to easily enforce that without asking people to edit the correct part of their commit. It's maybe more difficult with gitlab/github interfaces where changing the middle of a sequence of commits will not render very well, but in email based workflows it works fine. On the other hand, being able to bisect a project without having to worry about whether an unrelated issue is causing you to traverse the wrong branch of the bisect is an enormous advantage compared to the minimal effort required of keeping track of a modified (rebased) commit in the middle of a set of commits under review.

rjmunro 4y ago
You can now use --first-parent when you bisect to ensure bisect doesn't go into branches but stays on the main branch. https://git-scm.com/docs/git-bisect#Documentation/git-bisect.txt---first-parent https://git-scm.com/docs/git-bisect#Documentation/git-bisect...
- Arch-TK 4y ago
  I don't see how this solves the problem of patches which fix up previous patches. This workflow doesn't require using merges, but it will introduce situations (irrespective of whether you use --first-parent or not) where patches fix previous patches potentially leaving a gap where code doesn't compile, doesn't run, or doesn't pass tests.
Supermancho 4y ago
> I can't imagine being able to easily enforce that without asking people to edit the correct part of their commit. I imagine something something githooks. However it might be enforced, it seems like a miserable way to develop.

Shatnerz 4y ago

I didn't actually read the linked article but I see it is from GitLab. GitLab makes it easy to view the diff between versions of an MR even if it includes rebases.

johnnypangs 4y ago
How does that feature work in Gitlab? I also use it along with a rebase policy at work and sometimes have that issue.
- nemetroid 4y ago
  https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...
matijs 4y ago
GitHub does the same. There is a compare button that appears after a force (with-lease) push of a rebased branch.

jasonlotito 4y ago

Are there CR tools that don't display the diffs between rebases? Seems like a tooling issue more than anything.

kotborealis 4y ago

Some code review systems, like gerrit, actually encourage this workflow: you can easily view diffs between commit versions ('patchsets').

rubyist5eva 4y ago

In your view if a PR is not rebased but trunk is merged into it, does that warrant a full re-review? The end result is functionally the same as a rebase.

thibran 4y ago

Changes should be added with additional commits. When the review is complet, the code should be rebased and merged (git merge --ff-only my-reviewed-banch). This leads to a clean git commit history and an easy review process.

omegalulw 4y ago

You want to ask the author to change the commit you are interested in and then review the diff to that commit. This is if course mostly valuable if you don't squash commits on merge. Otherwise, the extra rebase work isn't that valuable.