[bug#48696,2/3] doc: Add "Addressing Mistakes" section.

* doc/contributing.texi (Addressing Mistakes): New section.
---
 doc/contributing.texi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Ludovic Courtès <ludo@gnu.org> writes:

> * doc/contributing.texi (Addressing Mistakes): New section.
> ---
>  doc/contributing.texi | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/doc/contributing.texi b/doc/contributing.texi
> index 7dc912b4de..8308551261 100644
> --- a/doc/contributing.texi
> +++ b/doc/contributing.texi
> @@ -1402,6 +1402,40 @@ you're confident, it's OK to commit.
>  That last part is subject to being adjusted, allowing individuals to commit
>  directly on non-controversial changes on parts they’re familiar with.
>  
> +@subsection Addressing Mistakes
> +
> +We all make mistakes.  We expect peer review (@pxref{Submitting
> +Patches}) and tools such as @command{guix lint} (@pxref{Invoking guix
> +lint}) and the test suite (@pxref{Running the Test Suite}) to catch
> +issues before they are pushed; yet, mistakes might go through---that
> +happens to both newcomers and old-timers, and there is nothing to be
> +ashamed of when it happens.  As a community, we expect committers to
> +recognize and address mistakes as soon as possible.
> +
> +Some mistakes can directly affect all users---for instance because they
> +make @command{guix pull} fail or break core functionality, because they
> +break major packages (at build time or run time), or because they
> +introduce known security vulnerabilities.
> +
> +@cindex reverting commits
> +The person who pushed the faulty commit(s) should be at the forefront to
> +address such an issue in a timely fashion: by pushing a followup commit
> +to fix it (if possible), or by reverting it to leave time to come up
> +with a proper fix, and by communicating with other developers about the
> +problem.
> +
> +If the committer is unavailable to address the issue in time, other
> +committers are entitled to revert the offending commit(s), explaining in
> +the commit log and on the mailing list what the problem was, with the
> +goal of leaving time to the original committer and author(s) to propose
> +a way forward.
> +
> +The Guix project values friendly cooperation and a constant effort to
> +focus on the way forward when issues arise.  Committers should lead by
> +example, notably as a way to encourage contributors and contributors to
> +be.  Blame as well as defensiveness do not have their place in Guix when
> +addressing genuine mistakes.

I too would like to see less blame, but one factor is how things are
framed and the language used.

On the language here, "mistake" is a word I would generally avoid if the
aim is avoid blaming someone, since mistakes are made by a person or set
of people. I'd prefer a term like "problem", since I don't perceieve
that as directly linked to a person or set of people.

On the bit about the "person who pushed the faulty commits" (so, person
to blame...) I'd much prefer an emphisis on group responsibility to
mitigate the impact of problems quickly, and understand the factors that
led to that problems in the first place. That avoids assigning blame,
rather than the process pushing responsibility to the person to blame
("person who pushed the faulty commit(s)").

On this same thread, I'd like to see less blaming in the form of asking
people to "explain". When there's a problem, and you ask someone to
explain, I would interpret that as "I'm blaming you for this, please
give your account of how the mistake was made", to which the person can
either answer explaining the details as to why they are to blame, or can
disagree with the implicit assertion that they are to blame. To avoid
assigning blame, one can just ask someone to "describe" what happened,
which I wouldn't interpret as being loaded with the same implicit
assertion.

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> +The Guix project values friendly cooperation and a constant effort to
>> +focus on the way forward when issues arise.  Committers should lead by
>> +example, notably as a way to encourage contributors and contributors to
>> +be.  Blame as well as defensiveness do not have their place in Guix when
>> +addressing genuine mistakes.
>
> I too would like to see less blame, but one factor is how things are
> framed and the language used.

Point taken!

> On the language here, "mistake" is a word I would generally avoid if the
> aim is avoid blaming someone, since mistakes are made by a person or set
> of people. I'd prefer a term like "problem", since I don't perceieve
> that as directly linked to a person or set of people.
>
> On the bit about the "person who pushed the faulty commits" (so, person
> to blame...) I'd much prefer an emphisis on group responsibility to
> mitigate the impact of problems quickly, and understand the factors that
> led to that problems in the first place. That avoids assigning blame,
> rather than the process pushing responsibility to the person to blame
> ("person who pushed the faulty commit(s)").

I get what you say and very much like the idea of focusing on group
responsibility.

There’s blame, and there’s accountability.  I see group responsibility
in setting up processes and carrying out proper peer review to avoid
problems.  I see accountability when it comes to commits actually
pushed—in the end, it’s one person running ‘git push’.  In my view,
“mistake” can be a way to name a “problem” that someone created and is
accountable for (Jelle wrote a nice message on this topic a while back).
This is getting a bit philosophical though, and I’m not sure my
understanding of English is good enough to go any further.  :-)

I think you have a point though.  Could you propose different wording
for this section?

(My goal for this section was to (1) spell out circumstances that may
lead to reverts, (2) explain the implications of committer
accountability, and (3) define our community standards in terms of
focusing on addressing issues and not on blaming individuals.)

> On this same thread, I'd like to see less blaming in the form of asking
> people to "explain". When there's a problem, and you ask someone to
> explain, I would interpret that as "I'm blaming you for this, please
> give your account of how the mistake was made", to which the person can
> either answer explaining the details as to why they are to blame, or can
> disagree with the implicit assertion that they are to blame. To avoid
> assigning blame, one can just ask someone to "describe" what happened,
> which I wouldn't interpret as being loaded with the same implicit
> assertion.

I agree with what you write in general, though my understanding is that
you’re not referring to the text in this patch, right?

Thanks for your feedback,
Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Chris,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> +The Guix project values friendly cooperation and a constant effort to
>>> +focus on the way forward when issues arise.  Committers should lead by
>>> +example, notably as a way to encourage contributors and contributors to
>>> +be.  Blame as well as defensiveness do not have their place in Guix when
>>> +addressing genuine mistakes.
>>
>> I too would like to see less blame, but one factor is how things are
>> framed and the language used.
>
> Point taken!
>
>> On the language here, "mistake" is a word I would generally avoid if the
>> aim is avoid blaming someone, since mistakes are made by a person or set
>> of people. I'd prefer a term like "problem", since I don't perceieve
>> that as directly linked to a person or set of people.
>>
>> On the bit about the "person who pushed the faulty commits" (so, person
>> to blame...) I'd much prefer an emphisis on group responsibility to
>> mitigate the impact of problems quickly, and understand the factors that
>> led to that problems in the first place. That avoids assigning blame,
>> rather than the process pushing responsibility to the person to blame
>> ("person who pushed the faulty commit(s)").
>
> I get what you say and very much like the idea of focusing on group
> responsibility.
>
> There’s blame, and there’s accountability.  I see group responsibility
> in setting up processes and carrying out proper peer review to avoid
> problems.  I see accountability when it comes to commits actually
> pushed—in the end, it’s one person running ‘git push’.  In my view,
> “mistake” can be a way to name a “problem” that someone created and is
> accountable for (Jelle wrote a nice message on this topic a while back).
> This is getting a bit philosophical though, and I’m not sure my
> understanding of English is good enough to go any further.  :-)

I guess part of what I'm getting at here is using language and the
perspective to try and delay that inference about individual
accountability, until the discussion around a problem has reached a
clear conclusion about what happened.

In doing so, opportunity is left open to actually consider the full
situation, rather than immediately narrowing it down to what a
particular individual or group did or didn't do.

> I think you have a point though.  Could you propose different wording
> for this section?
>
> (My goal for this section was to (1) spell out circumstances that may
> lead to reverts, (2) explain the implications of committer
> accountability, and (3) define our community standards in terms of
> focusing on addressing issues and not on blaming individuals.)

What I would like to see is more like this:

  Problems happen, while minimising there occurrence is important, it's
  also important to respond to problems in a useful way. There are two
  priorities, mitigating the impact and understanding what happened in
  order to reduce the chance of similar incidents in the future. The
  responsibility for both these things primarily lies with those
  involved, but like everything this is a group effort.

  When working to mitigate the impact of a problem, obviously the
  response is very much dependent on the situation. If it's possible to
  fix things that are broken, that's preferable. If that's infeasible,
  then promptly reverting changes to return to a working state is
  justified (as with any commit, note why the change is being made in
  the commit message).

  Once the problem has been dealt with to some extent, then it's the
  responsibility of those involved to make sure the situation is
  understood. If you are working to understand what happened, focus on
  gathering information and avoid assigning any blame. Do ask those
  involved to describe what has happened, don't ask them to explain the
  situation, even if you think they have something to explain, as this
  implicitly blames them, which is unhelpful. Accountability comes from
  a consensus about the problem, learning from it and improving
  processes so that it's less likely to reoccur.

I'm not sure how much needs saying about reverts, but I did include
something.

For committer accountability, that's where I'm talking about the
"responsibilities of those involved". I guess that's a little vague, but
what I'm trying to do there is trying to capture the group of relevant
people, for example, the person who proposed the breaking change, the
committer who pushed it, and the other person that reverted it.

In terms of trying to focus on addressing issues and not blaming
individuals, I think just avoiding language that implicitly blames
people would be a big step forward. Whether that's enough, I'm unsure.

>> On this same thread, I'd like to see less blaming in the form of asking
>> people to "explain". When there's a problem, and you ask someone to
>> explain, I would interpret that as "I'm blaming you for this, please
>> give your account of how the mistake was made", to which the person can
>> either answer explaining the details as to why they are to blame, or can
>> disagree with the implicit assertion that they are to blame. To avoid
>> assigning blame, one can just ask someone to "describe" what happened,
>> which I wouldn't interpret as being loaded with the same implicit
>> assertion.
>
> I agree with what you write in general, though my understanding is that
> you’re not referring to the text in this patch, right?

Yeah, although I think something to this effect might be worth
including.

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> I think you have a point though.  Could you propose different wording
>> for this section?
>>
>> (My goal for this section was to (1) spell out circumstances that may
>> lead to reverts, (2) explain the implications of committer
>> accountability, and (3) define our community standards in terms of
>> focusing on addressing issues and not on blaming individuals.)
>
> What I would like to see is more like this:
>
>   Problems happen, while minimising there occurrence is important, it's
>   also important to respond to problems in a useful way. There are two
>   priorities, mitigating the impact and understanding what happened in
>   order to reduce the chance of similar incidents in the future. The
>   responsibility for both these things primarily lies with those
>   involved, but like everything this is a group effort.
>
>   When working to mitigate the impact of a problem, obviously the
>   response is very much dependent on the situation. If it's possible to
>   fix things that are broken, that's preferable. If that's infeasible,
>   then promptly reverting changes to return to a working state is
>   justified (as with any commit, note why the change is being made in
>   the commit message).
>
>   Once the problem has been dealt with to some extent, then it's the
>   responsibility of those involved to make sure the situation is
>   understood. If you are working to understand what happened, focus on
>   gathering information and avoid assigning any blame. Do ask those
>   involved to describe what has happened, don't ask them to explain the
>   situation, even if you think they have something to explain, as this
>   implicitly blames them, which is unhelpful. Accountability comes from
>   a consensus about the problem, learning from it and improving
>   processes so that it's less likely to reoccur.
>
> I'm not sure how much needs saying about reverts, but I did include
> something.
>
> For committer accountability, that's where I'm talking about the
> "responsibilities of those involved". I guess that's a little vague, but
> what I'm trying to do there is trying to capture the group of relevant
> people, for example, the person who proposed the breaking change, the
> committer who pushed it, and the other person that reverted it.
>
> In terms of trying to focus on addressing issues and not blaming
> individuals, I think just avoiding language that implicitly blames
> people would be a big step forward. Whether that's enough, I'm unsure.

OK.

I like what you wrote; I think it addresses #3 and a bit of #2 above,
but I find a bit too abstract, not sufficiently hands-on (when can
commits be reverted? what’s the timeframe? who’s involved?), and lacking
examples.  “Problems happen” sounds unspecific to me (it reminds me of
Forest Gump :-)) and I’m uncomfortable with the passive voice that tends
to erase individuals.

  @subsection Addressing Issues
  
  Peer review (@pxref{Submitting Patches}) and tools such as
  @command{guix lint} (@pxref{Invoking guix lint}) and the test suite
  (@pxref{Running the Test Suite}) should catch issues before they are
  pushed.  Yet, commits that ``break'' functionality might occasionally
  go through.  When that happens, there are two priorities: mitigating
  the impact, and understanding what happened to reduce the chance of
  similar incidents in the future.  The responsibility for both these
  things primarily lies with those involved, but like everything this is
  a group effort.
  
  Some issues can directly affect all users---for instance because they
  make @command{guix pull} fail or break core functionality, because they
  break major packages (at build time or run time), or because they
  introduce known security vulnerabilities.
  
  @cindex reverting commits
  The people involved in authoring, reviewing, and pushing such
  commit(s) should be at the forefront to mitigate their impact in a
  timely fashion: by pushing a followup commit to fix it (if possible),
  or by reverting it to leave time to come up with a proper fix, and by
  communicating with other developers about the problem.
  
  If these persons are unavailable to address the issue in time, other
  committers are entitled to revert the commit(s), explaining in the
  commit log and on the mailing list what the problem was, with the goal
  of leaving time to the original committer, reviewer(s), and author(s)
  to propose a way forward.
  
  Once the problem has been dealt with, it is the responsibility of
  those involved to make sure the situation is understood.  If you are
  working to understand what happened, focus on gathering information
  and avoid assigning any blame.  Do ask those involved to describe what
  happened, do not ask them to explain the situation---this would
  implicitly blame them, which is unhelpful.  Accountability comes from
  a consensus about the problem, learning from it and improving
  processes so that it's less likely to reoccur.

There’s still “the people involved”, “these persons”, and “such commits”
(I removed “faulty”), because I couldn’t think of a way of avoiding
these without making the text too abstract or dismissing the idea
entirely (the idea that if I push a breaking change, others can expect
me to be spend time “mitigating the effort”).

Also, I think it’s useful to distinguish between “I revert my commit”
and “someone reverts my commit” due to their different social and
emotional implications (our goal is precisely to suggest that these
implications are out of place in this group effort that Guix is, but we
can’t deny that they preexist).

WDYT?

Thanks for taking the time to work on it!

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Chris,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> I think you have a point though.  Could you propose different wording
>>> for this section?
>>>
>>> (My goal for this section was to (1) spell out circumstances that may
>>> lead to reverts, (2) explain the implications of committer
>>> accountability, and (3) define our community standards in terms of
>>> focusing on addressing issues and not on blaming individuals.)
>>
>> What I would like to see is more like this:
>>
>>   Problems happen, while minimising there occurrence is important, it's
>>   also important to respond to problems in a useful way. There are two
>>   priorities, mitigating the impact and understanding what happened in
>>   order to reduce the chance of similar incidents in the future. The
>>   responsibility for both these things primarily lies with those
>>   involved, but like everything this is a group effort.
>>
>>   When working to mitigate the impact of a problem, obviously the
>>   response is very much dependent on the situation. If it's possible to
>>   fix things that are broken, that's preferable. If that's infeasible,
>>   then promptly reverting changes to return to a working state is
>>   justified (as with any commit, note why the change is being made in
>>   the commit message).
>>
>>   Once the problem has been dealt with to some extent, then it's the
>>   responsibility of those involved to make sure the situation is
>>   understood. If you are working to understand what happened, focus on
>>   gathering information and avoid assigning any blame. Do ask those
>>   involved to describe what has happened, don't ask them to explain the
>>   situation, even if you think they have something to explain, as this
>>   implicitly blames them, which is unhelpful. Accountability comes from
>>   a consensus about the problem, learning from it and improving
>>   processes so that it's less likely to reoccur.
>>
>> I'm not sure how much needs saying about reverts, but I did include
>> something.
>>
>> For committer accountability, that's where I'm talking about the
>> "responsibilities of those involved". I guess that's a little vague, but
>> what I'm trying to do there is trying to capture the group of relevant
>> people, for example, the person who proposed the breaking change, the
>> committer who pushed it, and the other person that reverted it.
>>
>> In terms of trying to focus on addressing issues and not blaming
>> individuals, I think just avoiding language that implicitly blames
>> people would be a big step forward. Whether that's enough, I'm unsure.
>
> OK.
>
> I like what you wrote; I think it addresses #3 and a bit of #2 above,
> but I find a bit too abstract, not sufficiently hands-on (when can
> commits be reverted? what’s the timeframe? who’s involved?), and lacking
> examples.  “Problems happen” sounds unspecific to me (it reminds me of
> Forest Gump :-)) and I’m uncomfortable with the passive voice that tends
> to erase individuals.
>
>   @subsection Addressing Issues
>
>   Peer review (@pxref{Submitting Patches}) and tools such as
>   @command{guix lint} (@pxref{Invoking guix lint}) and the test suite
>   (@pxref{Running the Test Suite}) should catch issues before they are
>   pushed.  Yet, commits that ``break'' functionality might occasionally
>   go through.  When that happens, there are two priorities: mitigating
>   the impact, and understanding what happened to reduce the chance of
>   similar incidents in the future.  The responsibility for both these
>   things primarily lies with those involved, but like everything this is
>   a group effort.
>   
>   Some issues can directly affect all users---for instance because they
>   make @command{guix pull} fail or break core functionality, because they
>   break major packages (at build time or run time), or because they
>   introduce known security vulnerabilities.

I'm not sure what this paragraph is getting at?

In any case, for security vulnerabilities, to affect all users they
would also have to occur in major packages.

>   @cindex reverting commits
>   The people involved in authoring, reviewing, and pushing such
>   commit(s) should be at the forefront to mitigate their impact in a
>   timely fashion: by pushing a followup commit to fix it (if possible),
>   or by reverting it to leave time to come up with a proper fix, and by
>   communicating with other developers about the problem.
>   
>   If these persons are unavailable to address the issue in time, other
>   committers are entitled to revert the commit(s), explaining in the
>   commit log and on the mailing list what the problem was, with the goal
>   of leaving time to the original committer, reviewer(s), and author(s)
>   to propose a way forward.
>   
>   Once the problem has been dealt with, it is the responsibility of
>   those involved to make sure the situation is understood.  If you are
>   working to understand what happened, focus on gathering information
>   and avoid assigning any blame.  Do ask those involved to describe what
>   happened, do not ask them to explain the situation---this would
>   implicitly blame them, which is unhelpful.  Accountability comes from
>   a consensus about the problem, learning from it and improving
>   processes so that it's less likely to reoccur.
>
> There’s still “the people involved”, “these persons”, and “such commits”
> (I removed “faulty”), because I couldn’t think of a way of avoiding
> these without making the text too abstract or dismissing the idea
> entirely (the idea that if I push a breaking change, others can expect
> me to be spend time “mitigating the effort”).
>
> Also, I think it’s useful to distinguish between “I revert my commit”
> and “someone reverts my commit” due to their different social and
> emotional implications (our goal is precisely to suggest that these
> implications are out of place in this group effort that Guix is, but we
> can’t deny that they preexist).
>
> WDYT?

I think the above text looks good. As noted above, I'm unsure about the
second paragraph, but that's not a big issue.

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>>   @subsection Addressing Issues
>>
>>   Peer review (@pxref{Submitting Patches}) and tools such as
>>   @command{guix lint} (@pxref{Invoking guix lint}) and the test suite
>>   (@pxref{Running the Test Suite}) should catch issues before they are
>>   pushed.  Yet, commits that ``break'' functionality might occasionally
>>   go through.  When that happens, there are two priorities: mitigating
>>   the impact, and understanding what happened to reduce the chance of
>>   similar incidents in the future.  The responsibility for both these
>>   things primarily lies with those involved, but like everything this is
>>   a group effort.
>>   
>>   Some issues can directly affect all users---for instance because they
>>   make @command{guix pull} fail or break core functionality, because they
>>   break major packages (at build time or run time), or because they
>>   introduce known security vulnerabilities.
>
> I'm not sure what this paragraph is getting at?

It’s supposed to be provide concrete guidance to a committer wondering
whether they can/should/are entitled to revert a given commit.

> In any case, for security vulnerabilities, to affect all users they
> would also have to occur in major packages.

Agreed.  The word “known” is important here: if I remove *-CVE-*.patch,
or if I downgrade a package, I’m likely introducing a “known”
vulnerability; if I’m adding a new package that later happens to be
vulnerable, it’s not a “known” vulnerability (it’s just routine ;-)).

> I think the above text looks good. As noted above, I'm unsure about the
> second paragraph, but that's not a big issue.

OK, thanks for taking the time to discuss it.  I’ll send a v2 so
everyone gets a chance to chime in.

Ludo’.

Hi!

Changes since the initial version:

  1. “Addressing Mistakes” section replaced by “Addressing Issues”
     following suggestions by Chris Baines at
     <https://issues.guix.gnu.org/48696#17>.

  2. Fix typo and clarify Git commit signing instructions as
     reported by Julien and Maxime (these were in pre-existing
     text).

If there are no objections, I’d like to push within a couple of
days.

Thanks,
Ludo’.

Ludovic Courtès (4):
  doc: Structure the "Commit Access" section.
  doc: Add "Addressing Issues" section.
  doc: Explain more reasons for commit revocation.
  doc: Clarify Git commit signing; fix typo.

 doc/contributing.texi | 121 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 100 insertions(+), 21 deletions(-)

Hi!

Ludovic Courtès <ludo@gnu.org> skribis:

> If there are no objections, I’d like to push within a couple of
> days.

[...]

>   doc: Structure the "Commit Access" section.
>   doc: Add "Addressing Issues" section.
>   doc: Explain more reasons for commit revocation.
>   doc: Clarify Git commit signing; fix typo.

Pushed as b5b9266e409abb18970bd34557cf087eb29fa713.

Thanks Chris and everyone for your feedback!

Ludo’.