On 20 October 2015 at 15:14, Christopher Orr <[email protected]> wrote: > 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.
Yes so that fixes one issue but complicates post-hoc analysis of PRs. For instance, suppose I wanted to determine which areas of Jenkins core are the most tricky for contributors to help out with... My initial guess would be to look at the master branch for merge commits, trace each one and count how long the commits being merged were. With the above scheme I no longer have GIT tooling to help and I have to cobble together something involving the GitHub APi as well as GIT... frankenstein's history analyser if you like. If we avoid squashing then this analysis is much easier and just an exercise in command line scripting > > 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. -- 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/CA%2BnPnMzKL2MnJG0bN8nS0m_xO0TySF%3DSyEu%2BYAeveG61bTcvyA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
