Handling problems with patches after the patch is pushed

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Handling problems with patches after the patch is pushed

Carl Sorensen
Dear team,

I've been verifying issues from 2.21.1.

This has raised a need for clarification about how we handle issues
once they have been pushed.

Consider issue 5890: https://gitlab.com/lilypond/lilypond/-/issues/5890

The issue was fixed and a solution was pushed.

Then, Dan recognized that there was another warning that either showed
up in the patch or was not fixed by the patch.  So he posted an
excellent comment pointing out the problem.

So now, we have a situation where there is a closed issue with status
fixed, and a request for a change simultaneously.  I don't know how to
resolve this.

It seems to me that there are at least two possibilities for how this
should be handled.

1) Once an issue is accepted and pushed, if there are problems
resulting from the issue, a new issue should be created.  This lets
the original issue stand as fixed.

2) Once an issue is accepted and pushed, if there are problems
resulting from the issue, the patch should be reverted, the issue
should be reopened, and the comments should be added to the issue
discussion.

It seems to be unworkable to have a patch pushed to master but still
have comments on that issue saying the patch is bad, even if the issue
is reopened.  Because we now have a half-baked patch committed to
master, and we'll either have to add another patch (two patches to
resolve the issue) or modify the current patch (which has the
potential for merge conflicts if it takes a long time to get back to
it.

Every problematic commit I've seen as I've worked on verifying issues
for 2.20, 2.21, and 2.19 has resulted from adding comments after an
issue is closed.  I think we should have a policy asking that we
implement either 1 or 2 above.

Thanks,

Carl

Reply | Threaded
Open this post in threaded view
|

Re: Handling problems with patches after the patch is pushed

David Kastrup
Carl Sorensen <[hidden email]> writes:

> Dear team,
>
> I've been verifying issues from 2.21.1.
>
> This has raised a need for clarification about how we handle issues
> once they have been pushed.
>
> Consider issue 5890: https://gitlab.com/lilypond/lilypond/-/issues/5890
>
> The issue was fixed and a solution was pushed.
>
> Then, Dan recognized that there was another warning that either showed
> up in the patch or was not fixed by the patch.  So he posted an
> excellent comment pointing out the problem.
>
> So now, we have a situation where there is a closed issue with status
> fixed, and a request for a change simultaneously.  I don't know how to
> resolve this.
>
> It seems to me that there are at least two possibilities for how this
> should be handled.
>
> 1) Once an issue is accepted and pushed, if there are problems
> resulting from the issue, a new issue should be created.  This lets
> the original issue stand as fixed.

Seems appropriate here.

> 2) Once an issue is accepted and pushed, if there are problems
> resulting from the issue, the patch should be reverted, the issue
> should be reopened, and the comments should be added to the issue
> discussion.

I don't think reopening makes a lot of sense: I'd also open a new issue
here and post a reference to the new issue, possibly with a note which
version is affected from the revert.  Ongoing information then in the
new issue.

> Every problematic commit I've seen as I've worked on verifying issues
> for 2.20, 2.21, and 2.19 has resulted from adding comments after an
> issue is closed.  I think we should have a policy asking that we
> implement either 1 or 2 above.

New issue + crossreference would be my suggestion.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Handling problems with patches after the patch is pushed

James Lowe-9
On 17/05/2020 18:43, David Kastrup wrote:
> New issue + crossreference would be my suggestion.

and mine.

James


Reply | Threaded
Open this post in threaded view
|

Re: Handling problems with patches after the patch is pushed

Dev mailing list
In reply to this post by David Kastrup
Am Sonntag, den 17.05.2020, 19:43 +0200 schrieb David Kastrup:

> Carl Sorensen <[hidden email]> writes:
>
> > Dear team,
> >
> > I've been verifying issues from 2.21.1.
> >
> > This has raised a need for clarification about how we handle issues
> > once they have been pushed.
> >
> > Consider issue 5890:
> > https://gitlab.com/lilypond/lilypond/-/issues/5890
> >
> >
> > The issue was fixed and a solution was pushed.
> >
> > Then, Dan recognized that there was another warning that either showed
> > up in the patch or was not fixed by the patch.  So he posted an
> > excellent comment pointing out the problem.
> >
> > So now, we have a situation where there is a closed issue with status
> > fixed, and a request for a change simultaneously.  I don't know how to
> > resolve this.
> >
> > It seems to me that there are at least two possibilities for how this
> > should be handled.
> >
> > 1) Once an issue is accepted and pushed, if there are problems
> > resulting from the issue, a new issue should be created.  This lets
> > the original issue stand as fixed.
>
> Seems appropriate here.
>
> > 2) Once an issue is accepted and pushed, if there are problems
> > resulting from the issue, the patch should be reverted, the issue
> > should be reopened, and the comments should be added to the issue
> > discussion.
>
> I don't think reopening makes a lot of sense: I'd also open a new issue
> here and post a reference to the new issue, possibly with a note which
> version is affected from the revert.  Ongoing information then in the
> new issue.
>
> > Every problematic commit I've seen as I've worked on verifying issues
> > for 2.20, 2.21, and 2.19 has resulted from adding comments after an
> > issue is closed.  I think we should have a policy asking that we
> > implement either 1 or 2 above.
>
> New issue + crossreference would be my suggestion.
+1 to all three replies.

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Handling problems with patches after the patch is pushed

Trevor Daniels
In reply to this post by Carl Sorensen
Carl Sorensen wrote 17/05/2020 18:32:27

>
>I've been verifying issues from 2.21.1.
>
>This has raised a need for clarification about how we handle issues
>once they have been pushed.
>
>It seems to me that there are at least two possibilities for how this
>should be handled.
>
>1) Once an issue is accepted and pushed, if there are problems
>resulting from the issue, a new issue should be created.  This lets
>the original issue stand as fixed.
>
>2) Once an issue is accepted and pushed, if there are problems
>resulting from the issue, the patch should be reverted, the issue
>should be reopened, and the comments should be added to the issue
>discussion.
I definitely prefer (1). I think (2) would potentially run into a
variety of
further problems, and would make for a confusing trail if it needed
to be followed later. Maybe a comment and pointer could be added
to the first issue to indicate the continuation of the fix.

Trevor
Reply | Threaded
Open this post in threaded view
|

Re: Handling problems with patches after the patch is pushed

Carl Sorensen
On Sun, May 17, 2020 at 3:10 PM Trevor <[hidden email]> wrote:
>
> Carl Sorensen wrote 17/05/2020 18:32:27
<snip>
>> It seems to me that there are at least two possibilities for how this
>> should be handled.
>>
>> 1) Once an issue is accepted and pushed, if there are problems
>> resulting from the issue, a new issue should be created. This lets
>> the original issue stand as fixed.
>>
<snip>
> I definitely prefer (1). I think (2) would potentially run into a variety of
> further problems, and would make for a confusing trail if it needed
> to be followed later. Maybe a comment and pointer could be added
> to the first issue to indicate the continuation of the fix.
>

There have been enough people responding positively to my preferred
solution of (1) that I have gone ahead and done that in this issue.

Issue #5890 is now verified, and all of 2.21.1 has now been verified.

Thanks,

Carl