llvm dev - May 2016 - [RFC] Helping release management

On 17 May 2016 at 02:07, Quentin Colombet via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Basically, the high-level status is:
> 1. Commits should state when they are fixes.
> 2. Bugs should be tracked in a PR.
Yup.

> None of these is a hard requirement but instead, best practices that we
> should remind to the contributors.
> For #1, I propose the attached patch for the commit message policy.
Your proposal goes against another policy, which is to leave the
discussion to the bugzilla, since it's either already there, or should
be there.

Also, links can rot, PR numbers cannot. (hey, it rhymes!)

> For A, the idea is to have an automatic way to update the PRs (like
resolved
> and fixed with a revision number) when some keywords are set (like fixes
> PR#). The idea with Diffusion is to have a field that marks the commit has
> been a fix and that we could manually set if we forgot to set the keyword
at
> commit time.
A commit that "Fixes PR123" does not necessarily close it. There may
be multiple commits, or we may wait until the buildbots are happy to
close it (I certainly do).

I think that the update here is simpler than that: Whenever a commit
refers a PR, update the bug saying "commit rNNNN was marked as a fix
to this bug".

People CC on the bug can then verify and act appropriately.

> For B, assuming the PR or Diffusion becomes the way to keep trace of every
> fixes, it is a matter of making easy to document that we are fixing a bug
> even if a PR does not exist. Maybe it is possible to have some commit hooks
> that given a keyword (e.g., fixes PR#New) file a PR with the commit message
> and marks the PR as fixed (also patch the actual commit message to put the
> PR number).
If you add some hyper-text markings to make it explicit, then creating
a bug will be unequivocal. But if you try to guess by the description,
then we'll end up with a flood of bugs (been there, done that).

Right now, "fixes PR" is our hyper-text marking, but it's only
injective, you want something bijective. I can't think of anything
that would have the same semantics whether you have a PR number or
not.

I actually see this as an attempt to overcome the problem that people
don't create enough PRs, by forcing them to write more on the commit
message. For me, this is trying to solve the wrong problem.

I'm also strongly against any more regulation of the commit message,
as this can create a culture of backlash when people don't, which can
lead to a sour community.

With my autistic hat on, I'd *love* a very strong set of rules and
regulations. But when I wear my community hat, I realise that I'm
probably the only one that would like my own rules, as are we all.

> Now, regarding how this information could be used by release managers to
> track what has been pulled in their release, I believe we would need to
> build additional tools on top of that information, but having this
> information is the priority IMHO.
This is the problem we were trying to solve in the first place, and I
think we changed the priorities with the illusion that it would get us
closer to the real problem. I don't think it will.

By writing a script that will *only* track PRs and the commits that
refers to them, we can easily *drive* people to use the proper path
with a carrot, not a stick.

If you file a bug (as per current community practices), and link your
commits to it with a "fixes PR" message (as per current commit
policy), we'll track your commit and make it a candidate for
back-ports.

If you don't, you'll be summarily ignored. Have a nice day. After all,
we can't baby sit everyone that doesn't want to follow the rules that
already exist.

We could then use Bugzilla to mark to which release we'd like
back-ported. We can use the "Version" field, or "Keywords".
All of
that easily automated.

We can also only grab the list of bugs that have been closed as
"Delivered", check the list of patches against it, add to the list,
send to the release manager with the names of all the people that
"fixed" that bug, and ask them for more information on possible fix
ups and reverts.

With time, people will get on track, and less effort will be needed by
release managers, because people will begin to add "PR" tags on
reverts, fixups, etc. and all will fall into place without effort.

My tuppence.

--renato

Hi Renato,

Thanks for your feedbacks.
> On May 17, 2016, at 5:02 AM, Renato Golin <renato.golin at
linaro.org> wrote:
> 
> On 17 May 2016 at 02:07, Quentin Colombet via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> Basically, the high-level status is:
>> 1. Commits should state when they are fixes.
>> 2. Bugs should be tracked in a PR.
> 
> Yup.
> 
> 
>> None of these is a hard requirement but instead, best practices that we
>> should remind to the contributors.
>> For #1, I propose the attached patch for the commit message policy.
> 
> Your proposal goes against another policy, which is to leave the
> discussion to the bugzilla, since it's either already there, or should
> be there.
I do not see how the proposal goes against that policy. I certainly believe that
the discussion should stay in the bugzilla, but I do not see why this is
incompatible with the fact of summarizing some information in the commit
message.
Basically what I tend to push for is self contained information for quick
reference in the log. Then, if people want more information, they are encouraged
to check the bugzilla.
> 
> Also, links can rot, PR numbers cannot. (hey, it rhymes!)
Yes, links can rot, though http:/llvm.org/PR# is relatively stable (at least it
did not break with the last migration), plus the PR number is still here.
In other words, I believe that link adds a convenient way to access bugzilla
while still providing the default information if the link rots.
> 
> 
>> For A, the idea is to have an automatic way to update the PRs (like
resolved
>> and fixed with a revision number) when some keywords are set (like
fixes
>> PR#). The idea with Diffusion is to have a field that marks the commit
has
>> been a fix and that we could manually set if we forgot to set the
keyword at
>> commit time.
> 
> A commit that "Fixes PR123" does not necessarily close it. There
may
> be multiple commits, or we may wait until the buildbots are happy to
> close it (I certainly do).
I am fine with that definition :).
> 
> I think that the update here is simpler than that: Whenever a commit
> refers a PR, update the bug saying "commit rNNNN was marked as a fix
> to this bug".
> 
> People CC on the bug can then verify and act appropriately.
Works for me.
> 
> 
>> For B, assuming the PR or Diffusion becomes the way to keep trace of
every
>> fixes, it is a matter of making easy to document that we are fixing a
bug
>> even if a PR does not exist. Maybe it is possible to have some commit
hooks
>> that given a keyword (e.g., fixes PR#New) file a PR with the commit
message
>> and marks the PR as fixed (also patch the actual commit message to put
the
>> PR number).
> 
> If you add some hyper-text markings to make it explicit, then creating
> a bug will be unequivocal. But if you try to guess by the description,
> then we'll end up with a flood of bugs (been there, done that).
> 
> Right now, "fixes PR" is our hyper-text marking, but it's
only
> injective, you want something bijective. I can't think of anything
> that would have the same semantics whether you have a PR number or
> not.
I miss the point.
For existing PR we would have: fixes PR1234.
For new PR we would have: fixes PRNew, where PRNew is a keyword that means
create a PR and put the actual number in place of PRNew.
> 
> I actually see this as an attempt to overcome the problem that people
> don't create enough PRs, by forcing them to write more on the commit
> message. For me, this is trying to solve the wrong problem.
Well, what I am trying to do is to make that automatic. I would be the first one
annoyed if I have to file a PR before fixing any thing. If when I am fixing
something, I just have to add “fixes PRNew”, that sounds easy enough to me :).

That being said, is your concern the fact that if we make that commit+PR
creation easy, people would not file PR before hand?
> 
> I'm also strongly against any more regulation of the commit message,
> as this can create a culture of backlash when people don't, which can
> lead to a sour community.
You may be right, though I feel like people in the community were responding
well when we bring to their attention that their commit message would have been
more useful if they added this or that (like the link to the buildbot that broke
when reverting).

I see this as an education step to bring all the contributions to the high
standard we all love. Maybe I am naive and people would actually be against that
and I would love to hear other opinions.
> 
> With my autistic hat on, I'd *love* a very strong set of rules and
> regulations. But when I wear my community hat, I realise that I'm
> probably the only one that would like my own rules, as are we all.
> 
> 
>> Now, regarding how this information could be used by release managers
to
>> track what has been pulled in their release, I believe we would need to
>> build additional tools on top of that information, but having this
>> information is the priority IMHO.
> 
> This is the problem we were trying to solve in the first place, and I
> think we changed the priorities with the illusion that it would get us
> closer to the real problem. I don't think it will.
> 
> By writing a script that will *only* track PRs and the commits that
> refers to them, we can easily *drive* people to use the proper path
> with a carrot, not a stick.
> 
> If you file a bug (as per current community practices), and link your
> commits to it with a "fixes PR" message (as per current commit
> policy), we'll track your commit and make it a candidate for
> back-ports.
> 
> If you don't, you'll be summarily ignored. Have a nice day. After
all,
> we can't baby sit everyone that doesn't want to follow the rules
that
> already exist.
Sure, but I don’t see why you are opposing this two approaches: file a PR
followed by a fix and file a PR with a fix.
Of course, people that don’t use the marking keywords will be out of the system,
that I agree, but it seems to me that having automatic process like this will
lower the bar for most people (assuming this is possible).
> 
> We could then use Bugzilla to mark to which release we'd like
> back-ported. We can use the "Version" field, or
"Keywords". All of
> that easily automated.
Agree. Though this goes beyond what I am trying to solve: document fixes.
> 
> We can also only grab the list of bugs that have been closed as
> "Delivered", check the list of patches against it, add to the
list,
> send to the release manager with the names of all the people that
> "fixed" that bug, and ask them for more information on possible
fix
> ups and reverts.
> 
> With time, people will get on track, and less effort will be needed by
> release managers, because people will begin to add "PR" tags on
> reverts, fixups, etc. and all will fall into place without effort.
> 
> My tuppence.
> 
> --renato
Thanks again for your input, I think it helps clarifying what is at stake and
how we plan to address it.

Cheers,
-Quentin

On 17 May 2016 at 17:25, Quentin Colombet <qcolombet at apple.com>
wrote:
> I do not see how the proposal goes against that policy. I certainly believe
that the discussion should stay in the bugzilla, but I do not see why this is
incompatible with the fact of summarizing some information in the commit
message.
> Basically what I tend to push for is self contained information for quick
reference in the log. Then, if people want more information, they are encouraged
to check the bugzilla.
That is already clear on the current policy, though...

"The body should be concise, but explanatory, including a complete
reasoning.
 Unless it is required to understand the change, examples, code snippets and
 gory details should be left to bug comments, web review or the mailing
list."

But I got it that you want "some" things more specific. I'm just
sceptical as to how much that will benefit the overall scheme of
things.

I think this should be a proper review, separated from this thread,
with the reasoning well explained. Let's focus on explaining it. :)

> Yes, links can rot, though http:/llvm.org/PR# is relatively stable (at
least it did not break with the last migration), plus the PR number is still
here.
> In other words, I believe that link adds a convenient way to access
bugzilla while still providing the default information if the link rots.
I'm not fussy about the URL, but changing the style now will mean all
commits so far will behave differently, people will continue using the
old style for a long time, and we'll have to support both ways
forever.

It should be hard to auto-link on any tool / web-page from the PR. I
just don't see the benefit in this change.

Remember, this is about changing how people write commit messages and
it's not a trivial matter. We already got used to doing this, unless
there's a strong argument against the current model, I don't think we
should change it.

> I miss the point.
> For existing PR we would have: fixes PR1234.
> For new PR we would have: fixes PRNew, where PRNew is a keyword that means
create a PR and put the actual number in place of PRNew.
But that's not all there is to it...

The PRNew tag will mean: create a bug, add the text of this commit to
it, mark it fixed by this commit, close it.

First, it'll require people actually remembering to put that, which in
my view is as easy/hard to make them remember to open a bug.

Second, what if this is a series of fixes, how to you connect this fix
with others?

Finally, and more importantly, who do we CC? To which component do we
associate the bug? Which fix version? Is this a candidate for
back-port? Is this a conformance or performance regression, or just a
fix to a new bug?

All those questions require answers, and they either go blank (thus
making the bug practically useless), or they require further
free-text-based encoding on what they are in the commit message.

I have had the unfortunate chance to work on free-text encodings [1]
where some of the encoded part was well defined, others not so much.
They were all equally horrid to work with. I'm really glad I don't
ever have to look back.

Moreover, the experience was equally appalling to the users of our
tools (people manually encoding the knowledge), so I can confidently
say that using such a scheme, on any depth, will bring us misery.

> Well, what I am trying to do is to make that automatic. I would be the
first one annoyed if I have to file a PR before fixing any thing. If when I am
fixing something, I just have to add “fixes PRNew”, that sounds easy enough to
me :).
It would be as easy to write a script to automatically create a bug
[2] for you from your current git branch, update your local PRNew
tags, then commit.

The difference being that this will not get into the commit message
policy, which was already regarded as too long and prone to be ignored
by most people that contributed to that discussion...

If everyone uses your tool, great! We still don't need to add to the
commit policy. If no one end up using it, equally great, we didn't
change the policy for nothing.

My whole point is, KISS. It's a lot easier to add things to a policy
than it is to remove from them. I'd rather err on the side of caution.

> Sure, but I don’t see why you are opposing this two approaches: file a PR
followed by a fix and file a PR with a fix.
> Of course, people that don’t use the marking keywords will be out of the
system, that I agree, but it seems to me that having automatic process like this
will lower the bar for most people (assuming this is possible).
People out of the system will never care. People in the system that
get things slightly wrong will be very annoyed.

Unless we're willing to reject any commit message that is malformed
(and you can't tell if it's just malformed or ignored, so you have to
reject *all* non-conforming), people with good intention will be the
ones hurt the most.

I don't think we want to start rejecting commits because of the
messages... or we'd have done by now.

cheers,
--renato

[1] http://arep.med.harvard.edu/labgc/jong/Fetch/SwissProtAll.html
[2] https://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html