Yes I agree, if your history is so heinous, and bothers you so much, and you really really want to rebase, just make a new PR for pure merging after the review is done (or discuss with you reviewers about it). But otherwise it doesn't really matter. For example, if git didn't have a rebase command, then everything would work just fine and we would not even care about this issue. Git works perfectly fine without the rebase command, it is a complete functioning and useful tool without it. Cleaning the history is 99% of the time an unnecessary step. The arguments for cleaning the history are very superficial and really amount to "I don't like that the history is messy.". There is very little gain from rebasing but the overhead is high as we've shown. The rebase command does not actually solve any problems, it just introduces them. The main goal here is that reviewers should not recommend rebasing so I'm not fond of Matthew's suggestion as it is written because it removes that guideline. We've also said that you can rebase your own PR if you discuss that with your reviewers. So any rebasing purists can work that out on their PRs.
I suggest that we let the proposal stand and that Jo can document all of the cases over the next several months where rebasing was necessary and where this guideline caused more problems that it solved. Once Jo builds his case with real data he can propose a new guideline and then we can discuss. Is that sufficient to move forward here? Jason moorepants.info +01 530-601-9791 On Wed, Jul 15, 2015 at 11:23 AM, Ondřej Čertík <ondrej.cer...@gmail.com> wrote: > On Sun, Jul 12, 2015 at 2:53 AM, Joachim Durchholz <j...@durchholz.org> > wrote: > > Am 12.07.2015 um 00:17 schrieb Jason Moore: > >> > >> FYI: We had a discussion at SymPy on how to synchronize with master and > >> decided that we no longer want to allow or encourage rebasing after a > pull > >> request is made to the main SymPy repo. > > > > > > I think that's overstated. > > > > I see some reasons against merging: > > > > A) A history with merges is harder to read. You need to track multiple > > parallel developments, and that's particularly hard if merge conflicts > were > > resolved (because some of the code you see didn't get into master, at > least > > not in the form it's committed initially). > > > > B) The results from `git bisect` become harder to read if a bug stems > from > > the combination of two changes that were made in parallel branches: the > > bisect will point at the merge commit, but the actual problem happened in > > some earlier commit. > > The more parallel branches in existence, the more prominent this will > > become. > > I agree with this --- many times there is just a few lines change, > that took 50 commits to get done, because the author was learning how > to make it work. > > For these cases I recommend to send a new PR with a polished (=rebased) > history. > > Then there is no problem. > > Ondrej > > > > >> The main reason is that we stall too many new contributors by forcing > them > >> to do too much git kung fu. > > > > > > This problem can be mitigated via interactive rebasing, but I agree it's > an > > extra hoop to jump through and cannot be asked of everyone. > > > >> But there are additional important reasons too: > >> > >> > >> - Once a pull request is "public" the history should not change because > >> other people may have pull it. > > > > > > Does anybody ever pull from a pull request? > > I can see that happening when collaborating on a fix, but it's not > something > > affecting newbies. > > > >> - Rebasing with merge conflicts is much more difficult > > > > > > Only if you try to rebase everything in a single go. You may end up > > resolving the same conflict over and over for each commit, and that can > get > > confusing very quickly, and it's also annoying and a lot of needless > work. > > > > However, there is a process that's actually quite easy: > > 1) `git rebase --interactive` first; this is a rebase *within your > working > > branch*. > > I also squash commits that were a continuation of previous work. I.e. if > I > > have a sequence of 1.code - 2.reformat - 3.code - 4.bugfix - 5.reformat - > > 6.code - 7.bugfix - 8.reformat, I like to rearrange this to 1.code - > 3.code > > - 4. bugfix - 6.code - 7.bugfix - 5.reformat - 7.reformat, and then I > squash > > this into two commits, one with 13467 for the coding and 57 for > > reformatting. > > If this still is confusing, this can be broken down into a series of > > primitive steps: (a) swap two commits, leaving all others alone, (b) > merge > > two adjacent commits, leaving all others alone; repeat until the history > is > > rewritten in a way that's easy to read and review. > > 2) `git fetch master && git rebase master`. This will expose the merge > > conflicts, but now it's far less commits, and it's also giving you much > less > > pain because most conflicts will apply just in one commit. > > > >> - Reviewers are no longer able to see commit by commit changes in a > review > >> process. > > > > > > Only if you squash everything into a single commit. > > Reordering and squashing commits makes sense if some work jumped between > > multiple things, such as reformatting, fixing docstrings, and actual code > > changes (and if the code changes touched multiple loosely-related > points, it > > may make sense to keep these in separate commits anyway). > > > >> - Github does not manage the diffs in a PR review if a rebase happens. > > > > > > You mean the "click here to see outdated diffs" thing? > > Sure, part of the discussion are removed one more click, which means that > > people won't know to look there and usually won't find them. > > However, if a discussion is relevant for posteriority, it should have > been > > merged into the commit in some way, either into the commit comment or in > a > > code comment or docstring. After all, people who're looking at `git > > annotate` or `git log` aren't going to see these discussions either. > > > >> If you want to rebase then you should do it before you make a pull > >> request, > >> either locally or on your fork's branch. > > > > > > Oh, now I see what's the concern - indeed, rebasing on the main repo is > bad, > > very bad, and a real no-go. But that's a non-issue! > > > > For pull requests, they're on separate branches, and people don't work on > > the PR branch of anybody else (unless they explicitly have agreed to > > cooperate in that way, in which case the work branch should indeed not be > > rebased anymore unless all downstream collaborators agree). > > > > Newbies don't even have the permissions to modify the main repo. > > > > The one point where I see strong language against rebasing justified is > when > > merging into master on the main repo. This is merge, and merge only, and > > should be written in bold letters into the project maintenance > guidelines. > > For people who contribute pull requests, I do not think that applies. > > > > YMMV, but I'd be interested to hear why. > > > > Regards, > > Jo > > > > -- > > You received this message because you are subscribed to the Google Groups > > "sympy" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to sympy+unsubscr...@googlegroups.com. > > To post to this group, send email to sympy@googlegroups.com. > > Visit this group at http://groups.google.com/group/sympy. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/sympy/55A22B0B.20405%40durchholz.org. > > > > For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups > "sympy" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to sympy+unsubscr...@googlegroups.com. > To post to this group, send email to sympy@googlegroups.com. > Visit this group at http://groups.google.com/group/sympy. > To view this discussion on the web visit > https://groups.google.com/d/msgid/sympy/CADDwiVAMf%2BPnH2vu_Kk4q_-LqrQXG%3DnEX91PrQhidqbtSxJN4A%40mail.gmail.com > . > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "sympy" group. To unsubscribe from this group and stop receiving emails from it, send an email to sympy+unsubscr...@googlegroups.com. To post to this group, send email to sympy@googlegroups.com. Visit this group at http://groups.google.com/group/sympy. To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/CAP7f1AjgHH6h35wBQia8W3QHk6C9McBA3nc8%3D7rM_R7JFoiqpg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
