RFC: put commit messages in *-commits subject lines?

Hi, everyone. I was comparing our commit messages with another commit list I'm on, and I realized (not for the first time) that our subject lines are less than useful. Why don't we have the first line of our commit messages in the subject lines? Compare:

[cfe-commits] r167788 - in /cfe/trunk: lib/Analysis/CFG.cpp test/Analysis/dead-stores.cpp

with

[cfe-commits] r167788 - Fix bad CFG construction bug when handling C++ 'try' statements.

The first gives me just enough information (in my abbreviated mesage list window) to see that there was a change to Clang's Analysis library; if I look at the whole subject, I can see what files were changed (or the first few, at least). But the second actually tells me what changed, and whether or not it affects me.

Having this form will also encourage people to put a useful summary in the first line of their commit, which is useful for anyone viewing history (especially git users, where the one-line summary is very common).

Is there a reason we currently prefer files to log messages?
Jordan

I suspect not. I would much prefer your suggestion. If you can
implement the fix to our scripts (and get Tanya or Chris or Anton to
give you access), I would be thrilled.

Hi, everyone. I was comparing our commit messages with another commit list I'm on, and I realized (not for the first time) that our subject lines are less than useful. Why don't we have the first line of our commit messages in the subject lines? Compare:

[cfe-commits] r167788 - in /cfe/trunk: lib/Analysis/CFG.cpp test/Analysis/dead-stores.cpp

with

[cfe-commits] r167788 - Fix bad CFG construction bug when handling C++ 'try' statements.

The first gives me just enough information (in my abbreviated mesage list window) to see that there was a change to Clang's Analysis library; if I look at the whole subject, I can see what files were changed (or the first few, at least). But the second actually tells me what changed, and whether or not it affects me.

Having this form will also encourage people to put a useful summary in the first line of their commit, which is useful for anyone viewing history (especially git users, where the one-line summary is very common).

Also, having this format would mean we would be less likely to go over
the subject character limit that causes gmail to truncate subjects &
break threading...

So for both/all those reasons, I'd love this.

Also, having this format would mean we would be less likely to go over
the subject character limit that causes gmail to truncate subjects &
break threading...

yes.please. This is so annoying. Fixing that alone would be worth it.

-- Sean Silva

I'm in favor of it. Of course, the truly awesomest thing would be something like:

    [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.

Even if it required some heuristics and horrible hackery in the commit script, it would be worth it.

-Chris

Yeah, crossed my mind too. First problem being the matching changes in
test/include/lib - but, as you say, some heuristics might catch the
common cases.

+1, awesome. Regardless of putting [path/to/dir], I prefer a summary line.

I know a few person tends to put blank line in 1st line, to see blank
line with git --oneline, though.
It could be improved if he saw blank line from *-commits. :slight_smile:

...Takumi

Moving the important information first may also be worth considering. Subject lines are often cut off.

I prefer

[lib/Analysis] Fix bad CFG construction bug when handing C++ 'try' ...

rather than

[cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction ...

"[cfe-commits]" can be removed from the subject as the amount of mails forces people anyway to filter/tag their emails

We can leave "r167788" in the subject line as people probably search for it. However, the commit message seems to be better to actually recognize a certain commit. So we do not need the revision at the beginning of the line.

Cheers
Tobi

I agree that the list name is redundant and should be dropped, but the revision number is compact and very useful...

-Chris

I agree that the list name is redundant and should be dropped, but the revision number is compact and very useful...

It's the only part of the message that says "r123456", including the
'r', which in my experience makes it *the* go-to for copypasting.

-- Sean Silva

Hi,

Chris Lattner wrote:

>> [cfe-commits] r167788 - in /cfe/trunk: lib/Analysis/CFG.cpp test/Analysis/dead-stores.cpp
>>
>> with
>>
>> [cfe-commits] r167788 - Fix bad CFG construction bug when handling C++ 'try' statements.
>>
>> The first gives me just enough information (in my abbreviated mesage list window) to see that there was a change to Clang's Analysis library; if I look at the whole subject, I can see what files were changed (or the first few, at least). But the second actually tells me what changed, and whether or not it affects me.
>>
>> Having this form will also encourage people to put a useful summary in the first line of their commit, which is useful for anyone viewing history (especially git users, where the one-line summary is very common).
>>
>> Is there a reason we currently prefer files to log messages?
>
> I suspect not. I would much prefer your suggestion. If you can
> implement the fix to our scripts (and get Tanya or Chris or Anton to
> give you access), I would be thrilled.

I'm in favor of it. Of course, the truly awesomest thing would be something like:

    [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.

Even if it required some heuristics and horrible hackery in the commit script, it would be worth it.

I see that currently the commit emails contain this marker in the header:
X-Mailer: svnmailer-1.0.9
So the following fixes assume that we are running svnmailer-1.0.9.

To get the first line of the log message as a subject line, please apply this
one line patch: http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=17;bug=379534
Note that you can directly patch the installed script: on my machine it is
located in /usr/local/lib/python2.7/dist-packages/svnmailer/notifier/_mail.py
and then replace or add the following line to the config file /etc/svn-mailer.conf

commit_subject_template = %(prefix)s r%(revision)s - %(log)s

Also as David Blaikie has pointed out, we are missing an upper bound on the
subject line length, so you may want to consider adding this:

max_subject_length = 120

While we are at it, let's also fix the missing context, by using a custom diff
command with the -p flag. Here is the config line:

diff_command = /usr/bin/diff -up -L %(label_from)s -L %(label_to)s %(from)s %(to)s

Can somebody do these modifications to llvm.org's /etc/svn-mailer.conf?

Thanks,
Sebastian

Hi,

Sebastian Pop wrote:

Chris Lattner wrote:
> I'm in favor of it. Of course, the truly awesomest thing would be something like:
>
> [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.

commit_subject_template = %(prefix)s r%(revision)s - %(log)s

I just realized that this line does not match Chris' subject line as it doesn't
print the directories touched by the patch. Please use the following instead:

commit_subject_template = %(prefix)s r%(revision)s - [%(dirs)s] %(log)s

Thanks,
Sebastian

Hi,

Sebastian Pop wrote:

Chris Lattner wrote:
> I'm in favor of it. Of course, the truly awesomest thing would be something like:
>
> [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.

commit_subject_template = %(prefix)s r%(revision)s - %(log)s

I just realized that this line does not match Chris' subject line as it doesn't
print the directories touched by the patch. Please use the following instead:

commit_subject_template = %(prefix)s r%(revision)s - [%(dirs)s] %(log)s

For what it's worth, (I think) Chris' suggestion of including the
directories was about including them "smart"ly by removing conceptual
duplicates (lib/foo + include/foo + test/foo) and generally giving a
brief sense of what a change is touching. If the change you have adds
the full (repo-relative) path all the directories without any smart
deduplication then I suspect it's going to easily push the log
description off most mail readers (especially mobile) & reduce the
value of this change.

I suspect we'll want to just stick with revision + log for now.

What's the prefix? There was some discussion of dropping the
[cfe-commits] & similar prefixes. Can that be done? Do we need to get
more buy-in from mailing list users to make sure this won't break
their mail rules (there are other ways to identify mailing list mail
other than the prefix, but I'm just checking).

Do you have some examples of what the format you're suggesting would
look like for various real-world commits?

I would love some magic to filter out test/XXX from the subject line if anything in lib/ has been changed...maybe something like this?

[clang]
for_paths = tools/clang
exclude_paths = tools/clang/test
show_nonmatching_paths = yes

[clang_test]
for_paths = tools/clang/test
ignore_if_other_matches = yes

Jordan

David Blaikie wrote:

> Hi,
>
> Sebastian Pop wrote:
>> Chris Lattner wrote:
>> > I'm in favor of it. Of course, the truly awesomest thing would be something like:
>> >
>> > [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.
>>
>> commit_subject_template = %(prefix)s r%(revision)s - %(log)s
>
> I just realized that this line does not match Chris' subject line as it doesn't
> print the directories touched by the patch. Please use the following instead:
>
> commit_subject_template = %(prefix)s r%(revision)s - [%(dirs)s] %(log)s

For what it's worth, (I think) Chris' suggestion of including the
directories was about including them "smart"ly by removing conceptual
duplicates (lib/foo + include/foo + test/foo) and generally giving a
brief sense of what a change is touching. If the change you have adds
the full (repo-relative) path all the directories without any smart
deduplication then I suspect it's going to easily push the log
description off most mail readers (especially mobile) & reduce the
value of this change.

I suspect we'll want to just stick with revision + log for now.

That's fine. Alternatively we can provide a list of all the dirs that we want
to see appearing and match them like this:

for_paths = .*(?P<SelectDirs>(lib/Analysis|lib/Transforms/Vectorize|lib/Transforms/Scalar)).*

commit_subject_template = %(prefix)s r%(revision)s - [%(SelectDirs)s] %(log)s

What's the prefix? There was some discussion of dropping the
[cfe-commits] & similar prefixes. Can that be done? Do we need to get
more buy-in from mailing list users to make sure this won't break
their mail rules (there are other ways to identify mailing list mail
other than the prefix, but I'm just checking).

Do you have some examples of what the format you're suggesting would
look like for various real-world commits?

I suppose that the current subject format is the default of svn-mailer that is
from the documentation:

   %(prefix)s r%(revision)s %(part)s - %(files/dirs)s

I think that this matches exactly what I can see on the commits mailing lists,
except for the %(part)s part that I haven't seen so far: probably we do not
split large emails.

I did some tests on my local machine, and with the patched version of svnmailer,
it does extract correctly the first line of the log message for all the commits
that I have looked at.

Sebastian

Bump for something that would be good to do.

David Blaikie wrote:

> Hi,
>
> Sebastian Pop wrote:
>> Chris Lattner wrote:
>> > I'm in favor of it. Of course, the truly awesomest thing would be something like:
>> >
>> > [cfe-commits] r167788 - [lib/Analysis] Fix bad CFG construction bug when handling C++ 'try' statements.
>>
>> commit_subject_template = %(prefix)s r%(revision)s - %(log)s

Using r172358 as an example (a moderately large commit chosen
otherwise at random), the current commit message subject is:

[llvm-commits] [llvm] r172358 - in /llvm/trunk:
include/llvm/ADT/StringRef.h
include/llvm/Analysis/DOTGraphTraitsPass.h
include/llvm/Object/RelocVisitor.h include/llvm/Support/YAMLTraits.h
lib/Analysis/ValueTracking.cpp lib/MC/ELFObjectWriter.cpp
lib/MC/WinCOFFObjectWriter.cpp lib/Support/APFloat.cpp
lib/Support/DynamicLibrary.cpp lib/Target/R600/AMDGPUSubtarget.h
lib/Target/R600/AMDILDevice.h
lib/Target/R600/AMDILPeepholeOptimizer.cpp
lib/Transforms/IPO/DeadArgumentElimination.cpp (and on and on)

So I assume the new message would be:

[llvm-commits] [llvm] r172358 - Remove redundant 'llvm::' qualifications

Which seems reasonable. (I think the repo prefix is helpful given that
both the Clang and LLVM lists handle mail for multiple repositories
(llvm + compiler-rt + zorg at least on the llvm-commits list, cfe +
libc++ on the cfe-commits list). Unless we could drop the prefix for
the common repo (llvm and cfe respectively) & add it in only for the
other repos)

I wouldn't mind some way to drop the mailing list prefix too, given
how I organize my email - but I don't suppose there's a nice way to do
that that wouldn't interfere with other people's workflows.

>
> I just realized that this line does not match Chris' subject line as it doesn't
> print the directories touched by the patch. Please use the following instead:
>
> commit_subject_template = %(prefix)s r%(revision)s - [%(dirs)s] %(log)s

[llvm-commits] [llvm] r172358 - [include/llvm/ADT
include/llvm/Analysis include/llvm/Object include/llvm/Support
lib/Analysis lib/MC lib/Target lib/Transforms ...] Remove redundant
'llvm::' qualifications

I suppose this isn't the best example - of course it'll be across lots
of directories. But even for more common commits (working in one area
- changing headers, library, and tests all together) would contain a
lot of redundant information in the "dirs" list.

For what it's worth, (I think) Chris' suggestion of including the
directories was about including them "smart"ly by removing conceptual
duplicates (lib/foo + include/foo + test/foo) and generally giving a
brief sense of what a change is touching. If the change you have adds
the full (repo-relative) path all the directories without any smart
deduplication then I suspect it's going to easily push the log
description off most mail readers (especially mobile) & reduce the
value of this change.

I suspect we'll want to just stick with revision + log for now.

That's fine. Alternatively we can provide a list of all the dirs that we want
to see appearing and match them like this:

for_paths = .*(?P<SelectDirs>(lib/Analysis|lib/Transforms/Vectorize|lib/Transforms/Scalar)).*

commit_subject_template = %(prefix)s r%(revision)s - [%(SelectDirs)s] %(log)s

Could we go one step further and map these paths to names (eg: map
"lib/Analysis", "include/llvm/Analysis", "test/Analysis" -> "Analysis"
and unique the results)? Also, I'd prefer to have the dirs after the
log message I think, though perhaps if we get them short enough it'd
work.

This solution does seem like it could involve a bit of manual work to
update the list of blessed dirs all the time. If we could express it
generically (lib/*, test/*, include/llvm/*) it might be OK, but as it
stands I'm still not sure it's worth the hassle.

Is anybody working on this? If not, where might one start looking to fix this?

+John C and Daniel, who have access to the servers.

Way back when this thread was first active I asked around off-list where this needs to happen, and Daniel and Tanya said this (respectively):

I'd prefer to stick with using either svn-mailer or svnnotify, as those are the ones we use in other places. svnnotify in particular is very full featured, although we would need to install it on the server but that is straightforward.

Maybe. The server is very out of date, and there are no packages anymore for it.. so if its just one file then that might work :wink:

I need to get John C. to update the OS again. He didn't do it and then Zac came early.. so it never got done. But its time and needs to be done. Once thats done, it will allow us to upgrade llvm.org much easier.

Sebastian, maybe you can psas your full config file on to John and Daniel?

Jordan

+John C and Daniel, who have access to the servers.

Way back when this thread was first active I asked around off-list where this needs to happen, and Daniel and Tanya said this (respectively):

I'd prefer to stick with using either svn-mailer or svnnotify, as those are the ones we use in other places. svnnotify in particular is very full featured, although we would need to install it on the server but that is straightforward.

Maybe. The server is very out of date, and there are no packages anymore for it.. so if its just one file then that might work :wink:

I need to get John C. to update the OS again. He didn't do it and then Zac came early.. so it never got done. But its time and needs to be done. Once thats done, it will allow us to upgrade llvm.org much easier.

Hrm. It sounds like we had a "You're waiting for me; I'm waiting for you" thing happening back in 2012. Tanya disappeared mid-discussion in early 2012 (I assume because Zac arrived), and since llvm.org was running smoothly, I just decided to wait until Tanya contacted me about it. It sounds like she might have been waiting on me to bring the subject up again.

In any event, for the current upgrade plans, Tanya asked me about dates for the upgrade, and I responded yesterday with a list of potential dates. All LLVM admins should be in the loop as I CC'ed that email to the llvm-admin list. Admins, if you haven't received that email, please let me know. I am currently waiting for a response on a few questions.

-- John T.

Jordan Rose wrote:

+John C and Daniel, who have access to the servers.

Way back when this thread was first active I asked around off-list where this needs to happen, and Daniel and Tanya said this (respectively):

>> I'd prefer to stick with using either svn-mailer or svnnotify, as those are the ones we use in other places. svnnotify in particular is very full featured, although we would need to install it on the server but that is straightforward.
>>
> Maybe. The server is very out of date, and there are no packages anymore for it.. so if its just one file then that might work :wink:
>
> I need to get John C. to update the OS again. He didn't do it and then Zac came early.. so it never got done. But its time and needs to be done. Once thats done, it will allow us to upgrade llvm.org much easier.

Sebastian, maybe you can psas your full config file on to John and Daniel?

I started looking at this problem in a blind way:

I see that currently the commit emails contain this marker in the header:
X-Mailer: svnmailer-1.0.9
So the following fixes assume that we are running svnmailer-1.0.9.

If somebody can send me the current config file from the server, I will modify
that, test locally, and send back a patch. If you decide to switch to svnnotify,
you wouldn't need my fixes, but I would still be happy to help if needed.

Let's get this bug fixed :wink:

Thanks,
Sebastian