On 20/10/15 13:26, Stephen Connolly wrote: > On 20 October 2015 at 12:06, James Nord <[email protected]> wrote: >>> I have some concerns about mandating a tidy-up (I note that Jesse is >>> against rewriting the history of a PR branch as that means that GitHub >>> hides the code review comments) >> >> >> Can you explain this. If I pushed a rebased commit the comments are still >> there with "commented on an outdated diff" (and if the comment is still >> applicable to the LOC is still shows) [1] >> This is no different to comments on a commit being addressed in a future >> commit and as such are still visible? > > Well I will not speak for Jesse, so you would want to check with him > as to his logic. > > I have seen that the GitHub comment tracking feature can be > unreliable... while it does the right thing and keeps the comments > with the new rewritten history *most* (say 8 out of 10 times) of the > time, there are times when it doesn't... so if you rewrite the history > I cannot trust that all my comments will have been tracked correctly > (especially if I have more than 10 of them... then there's a good > chance that 2 of them were mis-tracked... and it's oh so fun trying to > manually track comments, esp if they get "deleted" and you have to > switch back to email threads) and I have to re-review all the changes > to ensure that in rewriting the history you didn't inadvertently > introduce some other side change
FWIW (I'm not involved in core dev), what I usually do with GitHub pull requests is to squash them after the review is complete — I see no need to pollute git history with ["wip", "cleanup", "cleanup", "address review comments", "fix nitpicks"] — but I do this entirely locally. i.e. I squash commits but don't then force-push that feature branch to the remote, so I avoid wiping out the "who-committed-what-when" notifications in the PR, and I avoid screwing up the GitHub comment tracking. That is, I do something like: 1. git checkout feature/whatever 2. git rebase -i HEAD~n 3. <squash to one or more logical commits> 4. git checkout master 5. git merge feature/whatever 6. git push 7. Enter "Merged" and click "Comment & close" on the GitHub PR (if the PR hasn't auto-closed) In this case, the commit history and PR comments on GitHub should remain as they were while the review was in progress. Regards, Chris -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/56264C35.9070705%40orr.me.uk. For more options, visit https://groups.google.com/d/optout.
