New patchwork state: Invalid

New patchwork state: Invalid

[Siddhesh Poyarekar]

Hi,

Adhemerval, Florian and I noticed a couple of manpage patches during 
review that managed to apply to the glibc tree because they only added 
new files and as a result, didn't fail CI.  I've added a new terminal 
state 'Invalid' to patchwork to mark such patches from now on.  It 
should be used for any patches that don't belong to glibc, e.g. because 
they've been cross-posted.

This is now separate from the 'Fails to apply' state, which was 
previously overloaded to indicate non-glibc patches.

Thanks,
Sid

Re: New patchwork state: Invalid

[Carlos O'Donell]

On Mon, Sep 19, 2022 at 05:49:30PM -0400, Siddhesh Poyarekar wrote:
> Adhemerval, Florian and I noticed a couple of manpage patches during review
> that managed to apply to the glibc tree because they only added new files
> and as a result, didn't fail CI.  I've added a new terminal state 'Invalid'
> to patchwork to mark such patches from now on.  It should be used for any
> patches that don't belong to glibc, e.g. because they've been cross-posted.

I noticed this too! I asked DJ aobut his and he said it was common.

I'm OK with "Invalid" but I would rather go with "Not applicable"
because the patch is *valid* but not applicable. Total bikeshed.

> This is now separate from the 'Fails to apply' state, which was previously
> overloaded to indicate non-glibc patches.

Our maxim should be: As few states as possible.

My opinion is that we should not have "Fails to apply."

Instead the state should always be "Failed CI."

Applying a patch is a lower level operation that is related to
pre-commit CI.

The flow could be like this:
- A patchwork bot attempts to re-apply the patch, and if it fails it
  creates a "fail" entry in the CI which reads "Rebase bot failed".
- A patchwork bot notices the fail and marks the patch or whole
  series as "Failed CI"

My suggestion is as follows:

- Remove "Fails to apply." from the workflow. As few states as possible
  should be our goal.
- Suggest renaming "Invalid" to "Not applicable" only because it sounds
  kinder (complete bikeshed).

Cheers,
Carlos.

Re: New patchwork state: Invalid

[Siddhesh Poyarekar]

On 2022-09-20 06:33, Carlos O'Donell wrote:
> On Mon, Sep 19, 2022 at 05:49:30PM -0400, Siddhesh Poyarekar wrote:
>> Adhemerval, Florian and I noticed a couple of manpage patches during review
>> that managed to apply to the glibc tree because they only added new files
>> and as a result, didn't fail CI.  I've added a new terminal state 'Invalid'
>> to patchwork to mark such patches from now on.  It should be used for any
>> patches that don't belong to glibc, e.g. because they've been cross-posted.
> 
> I noticed this too! I asked DJ aobut his and he said it was common.
> 
> I'm OK with "Invalid" but I would rather go with "Not applicable"
> because the patch is *valid* but not applicable. Total bikeshed.
> 
>> This is now separate from the 'Fails to apply' state, which was previously
>> overloaded to indicate non-glibc patches.
> 
> Our maxim should be: As few states as possible.
> 
> My opinion is that we should not have "Fails to apply."
> 
> Instead the state should always be "Failed CI."
> 
> Applying a patch is a lower level operation that is related to
> pre-commit CI.
> 
> The flow could be like this:
> - A patchwork bot attempts to re-apply the patch, and if it fails it
>    creates a "fail" entry in the CI which reads "Rebase bot failed".
> - A patchwork bot notices the fail and marks the patch or whole
>    series as "Failed CI"
> 
> My suggestion is as follows:
> 
> - Remove "Fails to apply." from the workflow. As few states as possible
>    should be our goal.
> - Suggest renaming "Invalid" to "Not applicable" only because it sounds
>    kinder (complete bikeshed).

lol, so we had a "Not applicable" before, which indicated what I added 
as "Invalid".  This is because you and I changed "Not applicable" to 
"Fails to apply" a couple of weeks ago because I had misremembered "Not 
applicable" as being "Does not apply" ;)

I'm fine with reinstating what we had before (i.e. "Not applicable") and 
merge "Fails to apply" with "Fails CI".  I'll make that change in the 
wiki and patchwork today.

Sid