Developer guide: Do not change tickets with status "positive_review"

[cheuberg]

As discussed at sage-devel, pushing new commits to a ticket with status positive_review might lead to a race condition resulting in merging the previous commit.

Thus change the developer guide to reflect this.

Component: documentation

Author: Clemens Heuberger

Branch/Commit: u/cheuberg/doc/do-not-change-positive-review-tickets @ e18c886

Issue created by migration from https://trac.sagemath.org/ticket/18228

[cheuberg]

Branch: u/cheuberg/doc/do-not-change-positive-review-tickets

[cheuberg]

Commit: e18c886

[cheuberg]

New commits:

e18c886

Trac #18228: Developer guide: Do not change tickets with status "positive_review"

[nathanncohen]

comment:3

Hello Clemens,

I have a problem with what you want to add to the manual: adding commits to tickets in positive_review state is something that people do all the time, and there is not much danger in doing so. It happens when you notice that you forgot a typo, it happens when you forgot a bug, it happens when you find a better way to rewrite something.. Well, it happens all the time and it is almost always harmless. I do not think that it should be forbidden by the manual.

[cheuberg]

comment:4

Hello Nathann,

I also thought that until that discussion at sage-devel where no other consensus was reached and until I found 7 tickets within the last year which were not completely merged.

I think that the danger of loosing commits due to the current workflow of the release manager is higher than simply doing this change here.

Kind regards, Clemens

[cheuberg]

comment:6

Here is a list of the tickets I am aware of where pushes after positive_review led to incomplete merges:

Original ticket	Resolution
#14880	unclear
#15017	#18218
#15599	only merge commit missing, does not matter
#15963	#16128
#16847	#18219
#17221	#18206
#17307	no longer fixable (author of commit should have been changed)

[jdemeyer]

comment:7

One thing I don't really like about this ticket is that it refers to the preferences of the current release manager. This is unlike everything else in the Sage manual, which is general Sage policy.

(this doesn't mean that I'm against this ticket, but I just wanted to point this out)

[nathanncohen]

comment:8

Hello Clemens,

I totally agree with you on the fact that this is both tricky and dangerous. On the other hand adding commits to positively reviewed tickets is something that we do, and pretty often. Not only when we have forgotten a typo/bug, but also in many cases when somebody sees a positively reviewed ticket and objects with what is being done inside.

In this case that person will usually set the ticket back to needs_work or needs_info to talk about the problem (s)he thinks the ticket contains.

What I want to say is that having a rule like this would change radically the way we do things in Sage. Thus, I am not saying that the problem should not be addressed but rather that this is not a good way to solve it.

From the point of view of the release manager, however, this is very easy to detect: when he is about to merge a ticket, he can compare what he merges with the current head of the branch. I it not very complicated to implement, whatever script he uses.

This however, would still mean that he would run tests of a branch before noticing that the branch has changed. What about this second way out: when he starts the tests on a branch before merging it, he could 'freeze' the ticket (either by changing its status to 'closed: branch testing' or even by only adding an (automatic) message on the ticket saying "this branch is being tested, don't change its content".

Those two solutions (the second especially) are in my opinion much better than changing the way we work, in particular because they don't add new procedures and things to keep in mind when you contribute. The process is less than straightforward and having an automatic message posted on a ticket saying 'the release manager is running tests on its ticket before inclusion' is both educative and a solution to the problem.

Nathann

[cheuberg]

comment:9

Hello Nathann,

Replying to @nathanncohen:

From the point of view of the release manager, however, this is very easy to detect: when he is about to merge a ticket, he can compare what he merges with the current head of the branch. I it not very complicated to implement, whatever script he uses.

This however, would still mean that he would run tests of a branch before noticing that the branch has changed.

As outlined by Volker in the sage-devel thread, this might lead to an infinite loop.

Those two solutions (the second especially) are in my opinion much better than changing the way we work

If you convince the release manager to do that, I'll happily set modify this branch to include the proposed semantics.

Kind regards,

Clemens

[nathanncohen]

comment:10

Hello,

This however, would still mean that he would run tests of a branch before noticing that the branch has changed.

As outlined by Volker in the sage-devel thread, this might lead to an infinite loop.

O_o

I do not know what you have in mind, for here is the procedure I mentionned in the post to which you answer

---

From the point of view of the release manager, however, this is very easy to detect: when he is about to merge a ticket, he can compare what he merges with the current head of the branch. I it not very complicated to implement, whatever script he uses.

I see no infinite loop there.

If you convince the release manager to do that, I'll happily set modify this branch to include the proposed semantics.

If you insist on changing the rules, please open a sage-devel thread explaining its consequences: no last-minute typo/bugfix, and no way to set to needs_work a ticket in positive_review. I am personally against it.

Nathann

[nathanncohen]

comment:12

Yet another alternative mentionned by Volker: a two-weeks period between a merge and the latest commit. I know that Thierry would be interested by something like that.

[cheuberg]

comment:13

Hello Nathann,

I had troubles with trac once more, I did not accept my answers, so I shortened them until I could press the submit button. So I try not to use the reply function, perhaps then trac will accept longer answers.

You are right, I am not sure whether I understand your first suggestion.

My understanding from the sage-devel thread is that the release manager merges a certain number of positive_review tickets into his private develop branch, runs some tests, and if those pass, he then closes the tickets without looking at them again. This may take several hours.

If he'd look at the ticket again and would see a mismatch, he'd have to repeat the whole procedure, and this might lead to an infinite loop. And in his words, "Having the release manager chase after branches as people keep adding stuff after review is not a sustainable workflow".

Concerning your second point: I do not insist on changing the rules. I have opened a sage-devel thread, linked above. Several suggestions, some of them similar to yours, have been made by several people, but the release manager did not show any indication of changing his point of view that a ticket with positive review must not be changed.

I think that the issue has to be resolved as the 7 tickets show. Therefore, as the easiest solution, I propose this ticket.

[cheuberg]

comment:14

Replying to @nathanncohen:

Yet another alternative mentionned by Volker: a two-weeks period between a merge and the latest commit. I know that Thierry would be interested by something like that.

I never understood that proposal: a two-weeks cooling down period might reduce the probability of another push to a ticket, but does not make them impossible.

For those who rely on the diff view of trac, a two weeks interval would mean that reviewing chains of dependent tickets would take months.

[nathanncohen]

comment:15

Hello Clemens,

Concerning your second point: I do not insist on changing the rules. I have opened a sage-devel thread, linked above. Several suggestions, some of them similar to yours, have been made by several people, but the release manager did not show any indication of changing his point of view that a ticket with positive review must not be changed.

I think that the issue has to be resolved as the 7 tickets show. Therefore, as the easiest solution, I propose this ticket.

I believe that a branch like yours shouldn't be accepted without a poll on sage-devel. You should explain the change that you want to make, your reasons for doing so, and the consequences that you are aware of.

Thanks,

Nathann