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

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

Hi Renato,
> On May 17, 2016, at 10:14 AM, Renato Golin <renato.golin at
linaro.org> wrote:
> 
> 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. :)
Fair enough.
> 
> 
>> 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.
I am fine keeping PR#, I just find it convenient for them to be clickable in
mail.
Also, I do not see any problem supporting two styles. People have their
preferences and the tools should adapt, not the other way around.
> 
> 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.
What is missing form the suggested approach to work with the current policy?

As far as I can tell, what I explicitly called out is already interesting
information that people more or less put.
The idea is to have a tool (hook) doing the boring things on top of our current
practices. If this is not the case, then, yes, this is not going to work.
> 
> 
>> 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.
If they want :).
I tend to believe that if that makes people's job easier (or equally
painful) they would use it.
> 
> Second, what if this is a series of fixes, how to you connect this fix
> with others?
Good point. Thanks for bringing that up.
This use case actually goes beyond the conveniency I wanted to provide (i.e.,
not having to file a PR if this is a simple fix).
Indeed, if we need more than one commit to fix the problem I was thinking that
we may want to create a PR first anyway.
Anyhow, if we decide to support that use case, we could choose to leave the PR
open or have different keyword and stuff. Though, it seems to become
complicated.
Like we discuss with Jim, I want the path of less resistance to do the right
thing and adding a bunch of keywords seem against that.
> 
> Finally, and more importantly, who do we CC?
Why is this different from the current process of filing a PR or fixing a bug?
> To which component do we
> associate the bug?
We could use the default component or take advantage of the TAG in the title
line.
> Which fix version?
I did not get that.
> Is this a candidate for
> back-port?
That should be answered by the answer of your next question.
> Is this a conformance or performance regression, or just a
> fix to a new bug?
This should be answered in the commit message already, i.e., people should
document when the bug was introduced and what is fixed (performance,
correctness, etc.)

I do not plan to make a commit hook smart enough to give/extract that
information.

The commit hook is about documenting that a commit is a fix and the commit
message is about answering all the interesting questions. But I do not plan to
enforce any rules on them. People will need to use their own judgment to know
what is best for the project.
If that means reminding contributors on their commit message what is helpful to
release managers when fixing a bug, I do not see the harm.
> 
> All those questions require answers, and they either go blank (thus
> making the bug practically useless),
Yes, those are required answers, but I disagree that marking the bug is useless.
Marking that a commit is a fix is already an improvement compared to the current
situation where it takes time to figure out if something is a fix, a new
feature, a refactoring. Then, we can work with the contributor to get more
information about the commit if that is missing.
> 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.
Again, if the commit hook does not work on top of our current practice this is
bad, and I do not want it.
> 
> 
>> 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.
Indeed.
> 
> 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!
Well, if I need to change my workflow (e.g., using a different tool than git for
the commit) for llvm compared to the other project I work on, that path is
already dead for me. If we do not achieve transparency, we shouldn’t work into
it.

> We still don't need to add to the
> commit policy.
Sorry for nitpicking your comment, but this is *not* a policy, this is a
recommendation. We will not revert commit or whatever because of that. I am
trying to instil what information is required by release managers to help them
do their job. Contributors already know that information and most likely already
put it in the commit message.
> If no one end up using it, equally great, we didn't
> change the policy for nothing.
I do not see the suggested patch as a change but as something explicitly stating
what makes a commit message useful. If you disagree, that is fine you do not
have to follow those suggestions. If nobody wants them to appear in our commit
policy, that is fine as well.
> 
> 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.
Sounds fair, so I do not think this is actually changing anything, but I may
miss something.
> 
> 
>> 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.
Why?
> 
> 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.
Same here. I am trying to bring awareness of this problem. Even if this thread
ends up without any action taken, I believe some people would have gain that
awareness and that is already good (I am kind of an optimistic :)).

Cheers,
-Quentin
> 
> 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

On 17 May 2016 at 21:54, Quentin Colombet <qcolombet at apple.com>
wrote:
> I am fine keeping PR#, I just find it convenient for them to be clickable
in mail.
> Also, I do not see any problem supporting two styles. People have their
preferences and the tools should adapt, not the other way around.
The current policy already allows you to use URLs instead of just the
bug numbers, as long as PR# is on there somewhere.

I certainly won't object to that. :)

> As far as I can tell, what I explicitly called out is already interesting
information that people more or less put.
Indeed. But trying to codify makes people less appealed to use it. Our
approach to policies have been always "codify the least, cover the
most".

> The idea is to have a tool (hook) doing the boring things on top of our
current practices. If this is not the case, then, yes, this is not going to
work.
The tool is orthogonal, and there has been some pretty good ideas on
this thread already that can work without any change to the policy.

> I tend to believe that if that makes people's job easier (or equally
painful) they would use it.
Long time ago I realised that what makes my job easier is rarely what
makes other people's job easier, by being encouraged to use the tools
that other people built. :)

Having the tools is great, encouraging their usage is ok, encoding it
into a vast community's working, not so much.

> This use case actually goes beyond the conveniency I wanted to provide
(i.e., not having to file a PR if this is a simple fix).
> Indeed, if we need more than one commit to fix the problem I was thinking
that we may want to create a PR first anyway.
> Anyhow, if we decide to support that use case, we could choose to leave the
PR open or have different keyword and stuff. Though, it seems to become
complicated.
> Like we discuss with Jim, I want the path of less resistance to do the
right thing and adding a bunch of keywords seem against that.
The current policy is aligned with you adding as much as you think
it's necessary to the commit message. That includes metadata for your
bug-creating script.

Personally, I'd prefer that you have the script running on your local
tree, and changing your commit messages *before* commit. Once the
script is stable and more people are using it, it could even migrate
to an official SVN hook.

But I still strongly advise against encoding that on the policy.

> Why is this different from the current process of filing a PR or fixing a
bug?
Because when filling a bug manually we actually have to think about
those things. I don't think there's value in creating dozens of bugs
under  "newbugs" that only you see. The additional metadata can make a
back-port bug-scraping script much more efficient.

> We could use the default component or take advantage of the TAG in the
title line.
People use different tags, sometimes not at all. Also, Bugzilla's
selection of components doesn't really reflect current distribution,
but that's a separate issue. :)

>> Which fix version?
>
> I did not get that.
If we want to back-port stuff, we should say it somewhere (preferably
metadata, not free text) where the fix should apply to.

> The commit hook is about documenting that a commit is a fix and the commit
message is about answering all the interesting questions. But I do not plan to
enforce any rules on them. People will need to use their own judgment to know
what is best for the project.
> If that means reminding contributors on their commit message what is
helpful to release managers when fixing a bug, I do not see the harm.
I foresee confusion. An SVN hook can create bugs in the wrong place,
with the wrong content and be actually more trouble than it's worth.

If this is an optional feature, it should not be an SVN hook that will
run on *every* commit.

The commit message policy are things that we really encourage everyone
to do every time, where relevant. If we include optional directives,
people will take it even less serious than they do now. :(


> Yes, those are required answers, but I disagree that marking the bug is
useless.
I just meant "creating an empty bug, without any info or metadata" is
useless. Or, as useful as saying "fixes bug" on the commit message,
but noisier.

> Well, if I need to change my workflow (e.g., using a different tool than
git for the commit) for llvm compared to the other project I work on, that path
is already dead for me. If we do not achieve transparency, we shouldn’t work
into it.
Well, it's hard to get everyone working in the same way on different
projects. I surely don't want to force the kernel's way of working
into LLVM just because I may work on both. And if I have to use two
sets of tools for both of them, than that's a cost I alone should be
prepared to pay for.

> Sorry for nitpicking your comment, but this is *not* a policy, this is a
recommendation. We will not revert commit or whatever because of that. I am
trying to instil what information is required by release managers to help them
do their job. Contributors already know that information and most likely already
put it in the commit message.
I replied to this above: it *is* a policy, not a hard rule, not a
guideline. We seriously recommend those practices, and sometimes frown
upon if you don't follow it, but we won't slap your hand or revert
your commit.

As it stands, that policy is a the right strength level. I don't want
it being more strict, not less. From the discussions on the policies
(including the commit message), most people in the community don't
want that either.

>> People out of the system will never care. People in the system that
>> get things slightly wrong will be very annoyed.
>
> Why?
I said above: people trying to do one thing and the script
interpreting as another will create more extra work for those that
wanted none.

The way you describe your tool seems like a very personal way of
working. I imagine you find it very helpful (we have our own set of
scripts at Linaro), but making a personal style into the policy & an
SVN hook is pushing a bit too far, IMHO.

cheers,
--renato