Opened 5 years ago

Last modified 5 years ago

#18228 needs_info defect

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

Reported by: cheuberg Owned by:
Priority: major Milestone: sage-6.7
Component: documentation Keywords:
Cc: Merged in:
Authors: Clemens Heuberger Reviewers:
Report Upstream: N/A Work issues:
Branch: u/cheuberg/doc/do-not-change-positive-review-tickets (Commits) Commit: e18c88688732d8d26ff3fc4a0723d14e8a37cf16
Dependencies: Stopgaps:

Description (last modified by 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.

Change History (15)

comment:1 Changed 5 years ago by cheuberg

  • Branch set to u/cheuberg/doc/do-not-change-positive-review-tickets

comment:2 Changed 5 years ago by cheuberg

  • Commit set to e18c88688732d8d26ff3fc4a0723d14e8a37cf16
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

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

comment:3 Changed 5 years ago by ncohen

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.

comment:4 Changed 5 years ago by cheuberg

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

comment:5 Changed 5 years ago by cheuberg

  • Description modified (diff)

comment:6 Changed 5 years ago by cheuberg

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

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

comment:7 Changed 5 years ago by jdemeyer

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)

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:8 follow-up: Changed 5 years ago by ncohen

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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by cheuberg

Hello Nathann,

Replying to ncohen:

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

comment:10 in reply to: ↑ 9 Changed 5 years ago by ncohen

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

Last edited 5 years ago by ncohen (previous) (diff)

comment:11 Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_info

comment:12 follow-up: Changed 5 years ago by ncohen

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.

comment:13 follow-up: Changed 5 years ago by cheuberg

  • Status changed from needs_info to needs_review

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.

comment:14 in reply to: ↑ 12 Changed 5 years ago by cheuberg

Replying to ncohen:

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.

comment:15 in reply to: ↑ 13 Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_info

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

Note: See TracTickets for help on using tickets.