On Tue, Dec 17, 2013 at 1:36 PM, Georg S. Weber <georgswe...@googlemail.com> wrote: > Hi all, > > I've been reading docs (good job done, BTW!) and experimenting a bit with > git in the past days to get acquaintance with the new git workflow for Sage > trac tickets. > There's a common situation I didn't find well described yet, and I wanted to > ask whether my thoughts/my intuition is right. > (Hopefully it is OK do ask here on sage-devel and not on sage-git, where I > possibly could have added the 35th or so post to that other "diff" thread : > https://groups.google.com/forum/#!topic/sage-git/mbAH7xAia3Y .) > Assume the following resp., is it then OK to use the desribed recipe: > > 1. The current Sage version is (would be), say, "6.2". > > 2. I've got that in my local git Sage repo, especially the tag "6.2" is > there listed (by "git tag --list"). > > 3. I want to review some ticket #abcde from trac, the corresponding trac > branch is u/johndoe/tracabcde > > 4. Now comes the important twist: that branch "u/johndoe/tracabcde" was/is > based on some previous Sage version, say "6.1" instead of "6.2"! > > 5. According to the development docs ("Git the Hard Way / Checking Out > Tickets") I would do: > > git fetch trac u/johndoe/tracabcde > > > 6. And then create a new branch, say "my_branch", to work (do the review) > on, if I choose not to work "detached": > > git checkout -b my_branch FETCH_HEAD > > > 7. But since the "u/johndoe/tracabcde" branch misses everything that went > into Sage in between 6.1 and 6.2, I suppose the first thing I now should do > is: > > git merge --no-edit 6.2
Yep. > (Note: As integrator, I would be "on" the master branch (e.g. "vanilla > 6.2"), and merge into this branch that other ticket u/johndoe/tracabcde > branch. But merging in git is symmetric, so to check whether merging works, > we can do it the other way around, "merging in" 6.2 into the ticket branch.) > > 8a. Assume the (automated in git) merge went fine, This should hopefully be verified by the patchbot as well, so the author(s) and potential reviewers of a ticket will know sooner rather than later that the merge has (syntactic) issues. > then one of the next > things to do (among "sage -b", "sage -t", checking doctest coverage of > canged/new files etc.) This should all be done by the patchbot--unless you have a non-standard system I really don't want to be wasting people's time reviewing tickets that don't build or pass tests or have insufficient coverage. Of course building the code and trying out a few novel examples manually should be encouraged. Hopefully in the future there'll be a "single cell server" like box on each ticket to make this even easier--maybe even with a "add these examples to the docstring" button if we're going really far. > is to visually inspect the exact diff output for the > code that is added/changed/deleted by ticket #abcde: > > git diff 6.2.. > > > (or equivalently: > > git diff 6.2..my_branch > ) > > and if all is fine, I can give a positive review. Yep, this is the meat of the review. You should be able to inspect this online too. > Of course, in the review > comment, I should comment clearly that the review was done *against Sage > v6.2*. I don't think this is critical. I'm hoping that versions become more of a continuum, with certain ones being blessed for official binary/sdist release. > 8b. Or else, assume the merge attempt does not succeed automatically. Then > I might as a first step do the manual work necessary to complete the merge. > As a result, the local branch "my_branch" contains a rebased version (from > 6.1 to 6.2) of the changes that the trac ticket #abcde is about. > 9. Now I would amend the commit message if necessary, and push the re-based > changes back to trac (also changing the branch that ticket points to). Yes, this would be greatly appreciated. > 10. Ultimately, I could still give the original author (John Doe) a positive > review for his part of the changes (i.e. the bulk of it), and possibly he > (or a third person, of course) gives me a positive review for the 6.1 -> 6.2 > rebasing part (non-trivial by assumption). > So the ticket might reach a "positive review" status nevertheless, although > the original author (John Doe) did not himself "run behind and catch up" > with every new Sage version that appeared after the original code was > published to ticket #abcde. I do not think all rebases require a third reviewer. There are often obvious (to a human) merge resolutions that can't be done mechanically. Of course sometimes there are non-trivial conflicts, in which case the rebaser would also merit the title of author and need the changes checked off by someone else. > And again, the ticket comments should say clearly that the work was done > (re-)based to Sage version 6.2. Yes, the fact that it was rebased should be given as the reason for the new branch. > Let me emphasize that I consciously did not use the "master" branch directly > in any way in the above description, because e.g. "sage -dev vanilla" drops > me currently where I want, i.e. at tag "6.0.rc0", but "git branch --list" > tells me that's detached from master (which is at tag 5.13 I suppose) ... > > > Thoughts? > > I would be especially interested in a kind of "moving tag" named "CURRENT" > or so, which is always identical to the latest official Sage release tag. > (So I could have written "git merge --no-edit CURRENT" and "git diff > CURRENT.." in the above.) > > Or is this already somewhere encoded in the dev scripts and described their > documentation, and it was just me to not find it? remotes/trac/master? - Robert -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/groups/opt_out.
