Hacker News Reader

Comment by jbergknoff | Hacker News Reader

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).

View on HN

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.

How does that feature work in Gitlab? I also use it along with a rebase policy at work and sometimes have that issue.

https://docs.gitlab.com/ee/user/project/merge_requests/versi...

GitHub does the same. There is a compare button that appears after a force (with-lease) push of a rebased branch.

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

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

> 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.

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.

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...

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.

> 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.

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.

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.

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.

Reply on news.ycombinator.com