On Sat, Jan 4, 2014 at 6:04 AM, John Cremona <[email protected]> wrote: > On 4 January 2014 13:24, Nathann Cohen <[email protected]> wrote: >> Hellooooooooooo !! >> >>> Btw, I'm still not convinced by Volker's explanations, >> >> Well, by the look of that thread I don't think anybody but Volker was >> actually convinced :-P >> >>> and the policy I >>> was suggesting at the beginning of that thread still makes more sense to >>> me. In summary, here's what I was suggesting: >>> >>> 1. The default diff/commits view of a ticket T linked from its trac page >>> should be something like >>> >>> git log --graph commit ^trac/develop ^dep1 ^dep2 ... >> >> Yes. A big "YES" >> >> >>> where commit is the contents of T's Commit field and dep1, dep2... >>> are the contents of the Commit fields of the tickets D1, D2... listed >>> in T's Dependencies field. >>> >>> Setting T to positive_review means that the set of commits described >>> by the above "git log" command has been reviewed. >> >> Indeed. Actually, now that I understand GIt better, it really feels like we >> should not be reviewing branches but rewieving *COMMITS*. This way we would >> know which commits we can use. Reviewing branches does not seem to make any >> sense in a git workflow. I mean, it's the source of all problems we have >> right now. > > This is quite logical, since a branch is a pointer to a commit and not > some fixed thing, so if I say "I have reviewed branch u/x/y/z then it > tells you absolutely nothing, while if I say that I am happy with > commit a1b2c3d4... then it does refer to a specific snapshot of the > code. With the hg model we were reviewing patches, or sequences of > patches considered as a single unit, and "positive review" meant > something like "I approve of the combined effect of applying these > patches". > > One difficulty with reviewing individual commits is that many of them > are snapshots along the way to a finished piece of work. For example, > I am about to make a commit of some work I was in the middle of > yesterday, so that I can come back to it later, while in the meantime > I want to be able to change branches so that I can review some other > tickets. It would not make sense for that half-way commit to ever be > reviewed in isolation.
I don't think commits need to be reviewed in isolation; if the ticket is A+B+C I can just look at the diff master..C and give my +1 to the set. This is actually more flexible than patches because I can look at master...B and then B..C if that makes it easier. > Of course, the half-way commit only exists on > my own computer so I could rewrite local history later, but why should > I bother? Of course it's nice for history to be somewhat clean, but that's not a requirement. > Does it make sense for us to mark a specific commit as "ready for > review" rather than a branch? Working back through that commit's > history one might pass by what I called half-way commits above, which > could be ignored, until reaching either (1) a commit which has already > been merged -- in which case the changes to be reviewed are the ones > between that one and the current one; or (2) a commit which is also > tagged "ready for review"; or (3) a commit which has a positive > review, which could be treated as in case (0). Ok, I know that the > number of cases here is more than the ones described; but I have > already found that when reviewing a ticket it has been necessary to > say in the comment on trac exactly which commit I was looking at, and > also when marking a ticket as ready for review I intend to always say > which commit it is which I would like reviewed. It would not be hard > for trac to have fields showing that information? You might think it > redundant, but yesterday someone with initials JD made a new commit to > a branch after I had started reviewing a ticket tagged "ready for > review"! Whenever the commit changes, that should result in a message on trac. I suppose in the (rare?) case that two people are working on a ticket at the same time like this, you could refer to the commit manually in your comment, otherwise it is assumed that you're referring to the commit that was active at that time. (Even better of course is in-line comments.) -- 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 [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/groups/opt_out.
