small request regarding commits in binutils-gdb.git

small request regarding commits in binutils-gdb.git

[Joel Brobecker]

Hello,

Please forgive this message for all (most) of you who already know
this, but I happen to notice a few commits here and there, where
the revision log contains a copy/paste of the ChangeLog entry.

This was a pretty typical and accepted practice when were using CVS.
But, the convention with git tools is that the first line should
be the subject of the commit, and this subject should be followed
by an empty line before the rest of the revision log starts (if
there is text afterwards).

Below are examples of commit revision logs which follow that convention:

> daily update

or

> PR symtab/16426
>
>     * dwarf2read.c (dwarf2_get_dwz_file): Call gdb_bfd_record_inclusion.
>     (try_open_dwop_file): Ditto.

Here is one, however, that does not follow the convention:

>       PR gas/16434
>       * config/tc-z80.c (wrong_match): Provide format string to
>       as_warn.

In the example above, making the "PR gas/16434" by stripping
the leading spaces, and adding an empty line after it would
be sufficient.

However, I'd like to take this opportunity to suggest a practice
that some of us have started adopting on the GDB project, which
is to make the revision log something close to the email we'd send
to submit patch for including in binutils-gdb.git.

It would look like this:

> psymtab cleanup patch 3/3
>
> This last patch removes "partial" from the names of
> expand_partial_symbol_names and map_partial_symbol_filenames.
> It also renames expand_partial_symbol_names to match the
> struct quick_symbol_functions "method" that it wraps:
> expand_symtabs_matching.
>
> This patch also adds two parameters to expand_symtabs_matching
> so that it can fully wrap the underlying quick_symbol_functions method.
> This makes it usable in more places.
> I thought of having a cover function that still had the same
> signature as the old expand_partial_symbol_names function,
> but I couldn't think of a good name, and it wasn't clear it was
> worth it anyway.
>
> gdb/ChangeLog:
>
>     * symfile.h (expand_symtabs_matching): Renamed from
>     expand_partial_symbol_names.  Update prototype.
>     (map_symbol_filenames): Renamed from map_partial_symbol_filenames.
>     * symfile.c (expand_symtabs_matching): Renamed from

The upside is that it makes it easier for you to submit your patch,
even if it submitted a long time after you actually wrote the patch.
Just use "git send-email", and voila.

For the rest of the community, using a meaningful subject, one that
actually gives a hint at what it does, is something that I have
found to be really helpful. For instance, "Provide format string
to as_warn" can be more helpful when scanning through a list
of commits than "PR gas/16434".

I am also considering the idea of writing a small hook that
would reject new commits introducing commits where the "empty line
after subject" rule is not followed. Would that be an acceptable
restriction?

-- 
Joel

Re: small request regarding commits in binutils-gdb.git

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 329 bytes --]

On Wednesday 15 January 2014 07:12:51 Joel Brobecker wrote:
> However, I'd like to take this opportunity to suggest a practice
> that some of us have started adopting on the GDB project, which
> is to make the revision log something close to the email we'd send
> to submit patch for including in binutils-gdb.git.

+10000
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: small request regarding commits in binutils-gdb.git

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I am also considering the idea of writing a small hook that
Joel> would reject new commits introducing commits where the "empty line
Joel> after subject" rule is not followed. Would that be an acceptable
Joel> restriction?

It would be fine by me.

Tom

Re: small request regarding commits in binutils-gdb.git

[Eli Zaretskii]

> Date: Wed, 15 Jan 2014 16:12:51 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> It would look like this:
> 
> > psymtab cleanup patch 3/3
> >
> > This last patch removes "partial" from the names of
> > expand_partial_symbol_names and map_partial_symbol_filenames.
> > It also renames expand_partial_symbol_names to match the
> > struct quick_symbol_functions "method" that it wraps:
> > expand_symtabs_matching.
> >
> > This patch also adds two parameters to expand_symtabs_matching
> > so that it can fully wrap the underlying quick_symbol_functions method.
> > This makes it usable in more places.
> > I thought of having a cover function that still had the same
> > signature as the old expand_partial_symbol_names function,
> > but I couldn't think of a good name, and it wasn't clear it was
> > worth it anyway.
> >
> > gdb/ChangeLog:
> >
> >     * symfile.h (expand_symtabs_matching): Renamed from
> >     expand_partial_symbol_names.  Update prototype.
> >     (map_symbol_filenames): Renamed from map_partial_symbol_filenames.
> >     * symfile.c (expand_symtabs_matching): Renamed from

The text before the log entry is just a long way of saying the same
thing as the log entry, isn't it?  It repeats the info that is already
in the log entry.  Why does it make sense to repeat all that?

> I am also considering the idea of writing a small hook that
> would reject new commits introducing commits where the "empty line
> after subject" rule is not followed. Would that be an acceptable
> restriction?

No objections from me.

Re: small request regarding commits in binutils-gdb.git

[Joel Brobecker]

> > > This last patch removes "partial" from the names of
> > > expand_partial_symbol_names and map_partial_symbol_filenames.
> > > It also renames expand_partial_symbol_names to match the
> > > struct quick_symbol_functions "method" that it wraps:
> > > expand_symtabs_matching.
> > >
> > > This patch also adds two parameters to expand_symtabs_matching
> > > so that it can fully wrap the underlying quick_symbol_functions method.
> > > This makes it usable in more places.
> > > I thought of having a cover function that still had the same
> > > signature as the old expand_partial_symbol_names function,
> > > but I couldn't think of a good name, and it wasn't clear it was
> > > worth it anyway.
> > >
> > > gdb/ChangeLog:
> > >
> > >     * symfile.h (expand_symtabs_matching): Renamed from
> > >     expand_partial_symbol_names.  Update prototype.
> > >     (map_symbol_filenames): Renamed from map_partial_symbol_filenames.
> > >     * symfile.c (expand_symtabs_matching): Renamed from
> 
> The text before the log entry is just a long way of saying the same
> thing as the log entry, isn't it?  It repeats the info that is already
> in the log entry.  Why does it make sense to repeat all that?

I agree I could have chosen a more demonstrative example...

But even with this example, it isn't just a repeat. The text before
the ChangeLog says what the intent of the patch is, and provides extra
information that usually doesn't go into the ChangeLog entry. For example,
it says "This makes it usable in more places".

-- 
Joel

Re: small request regarding commits in binutils-gdb.git

[Eli Zaretskii]

> Date: Wed, 15 Jan 2014 20:25:18 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: binutils@sourceware.org, gdb-patches@sourceware.org
> 
> But even with this example, it isn't just a repeat.

I didn't say it was just a repeat, I said most of it just repeats.

> The text before the ChangeLog says what the intent of the patch is,
> and provides extra information that usually doesn't go into the
> ChangeLog entry. For example, it says "This makes it usable in more
> places".

Why not have this as part of the log entry.  Here's an example (from
Emacs):

	* window.h (struct window): Replace hchild, vchild and buffer slots
	with a single contents slot.  This is possible because each valid
	window may have either the child window (in vertical or horizontal
	combination) or buffer to display (for the leaf window).  Using that,
	a lot of operations to traverse and/or change window hierarchies may
	be simplified.  New member horizontal is used to distinguish between
	horizontal and vertical combinations of internal windows.

Re: small request regarding commits in binutils-gdb.git

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 746 bytes --]

On Wednesday 15 January 2014 11:57:45 Eli Zaretskii wrote:
> > Date: Wed, 15 Jan 2014 20:25:18 +0400
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: binutils@sourceware.org, gdb-patches@sourceware.org
> > 
> > But even with this example, it isn't just a repeat.
> 
> I didn't say it was just a repeat, I said most of it just repeats.
> 
> > The text before the ChangeLog says what the intent of the patch is,
> > and provides extra information that usually doesn't go into the
> > ChangeLog entry. For example, it says "This makes it usable in more
> > places".
> 
> Why not have this as part of the log entry.  Here's an example (from
> Emacs):

i'd lobby for just stopping the ChangeLog practice altogether :)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: small request regarding commits in binutils-gdb.git

[Fred Cooke]

I'd be with you, Mike, but these packages are regularly distributed
without source control, as tarballs, and as such it's useful. For my
own stuff it's not possible to build without git present, it's a
prerequisite.

The first line of a git commit should say enough about the why/what to
stand alone. The rest should add detail. If the first line can not
achieve this, it makes log view forms with only the first line pretty
much useless. Line length restrictions are archaic IMO, however the
standard tooling makes long lines look bad on purpose, so...

In any case, I totally agree that pasting a "what changed" entry is
not OK, and adding the full "I did it because" email content is a VERY
good thing.

eg

> Removed duplicated code in bad style from XY arch.
>
> This code was added for ABC reason, however it duplicates LMNOP code and is in bad taste. Removing it lightens the maintenance burden, while retaining the functionality in the other superior form.

What: Code was removed from XY arch
Why: Because it was poor style and duplicated
Detail: More human friendly verbose version of the same

On Wed, Jan 15, 2014 at 6:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 15 January 2014 11:57:45 Eli Zaretskii wrote:
>> > Date: Wed, 15 Jan 2014 20:25:18 +0400
>> > From: Joel Brobecker <brobecker@adacore.com>
>> > Cc: binutils@sourceware.org, gdb-patches@sourceware.org
>> >
>> > But even with this example, it isn't just a repeat.
>>
>> I didn't say it was just a repeat, I said most of it just repeats.
>>
>> > The text before the ChangeLog says what the intent of the patch is,
>> > and provides extra information that usually doesn't go into the
>> > ChangeLog entry. For example, it says "This makes it usable in more
>> > places".
>>
>> Why not have this as part of the log entry.  Here's an example (from
>> Emacs):
>
> i'd lobby for just stopping the ChangeLog practice altogether :)
> -mike

Re: small request regarding commits in binutils-gdb.git

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 441 bytes --]

On Wednesday 15 January 2014 14:48:51 Fred Cooke wrote:
> I'd be with you, Mike, but these packages are regularly distributed
> without source control, as tarballs, and as such it's useful.

i don't think it is, but let's assume you're right.  there's no reason the 
dist steps can't autogenerate a full ChangeLog from git history and throw it 
into the tarball.  it's what other GNU projects are doing now (like 
coreutils).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: small request regarding commits in binutils-gdb.git

[Eli Zaretskii]

> From: Mike Frysinger <vapier@gentoo.org>
> Date: Wed, 15 Jan 2014 15:02:07 -0500
> Cc: gdb-patches@sourceware.org,
>  Eli Zaretskii <eliz@gnu.org>,
>  Joel Brobecker <brobecker@adacore.com>,
>  binutils <binutils@sourceware.org>
> 
> there's no reason the dist steps can't autogenerate a full ChangeLog
> from git history and throw it into the tarball.  it's what other GNU
> projects are doing now (like coreutils).

If we are going to generate ChangeLog files, this discussion is still
relevant, since the text needs to be formatted according to ChangeLog
rules.

Re: small request regarding commits in binutils-gdb.git

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1079 bytes --]

On Wednesday 15 January 2014 15:38:50 Eli Zaretskii wrote:
> > From: Mike Frysinger <vapier@gentoo.org>
> > Date: Wed, 15 Jan 2014 15:02:07 -0500
> > Cc: gdb-patches@sourceware.org,
> > 
> >  Eli Zaretskii <eliz@gnu.org>,
> >  Joel Brobecker <brobecker@adacore.com>,
> >  binutils <binutils@sourceware.org>
> > 
> > there's no reason the dist steps can't autogenerate a full ChangeLog
> > from git history and throw it into the tarball.  it's what other GNU
> > projects are doing now (like coreutils).
> 
> If we are going to generate ChangeLog files, this discussion is still
> relevant, since the text needs to be formatted according to ChangeLog
> rules.

not necessarily.  there's two options:
	- just use `git log --stat` and pipe it into a ChangeLog.  this isn't a 
"GNU ChangeLog", but it is a ChangeLog
	- use the abbreviated commit messages that the tools rely on to create a 
GNU ChangeLog format

obviously i'd prefer the first, but i can live with the second.  it's still way 
better than hand maintaining ChangeLog files themselves.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: small request regarding commits in binutils-gdb.git

[Eli Zaretskii]

> From: Mike Frysinger <vapier@gentoo.org>
> Date: Wed, 15 Jan 2014 15:57:47 -0500
> Cc: fred.cooke@gmail.com,
>  gdb-patches@sourceware.org,
>  brobecker@adacore.com,
>  binutils@sourceware.org
> 
> > If we are going to generate ChangeLog files, this discussion is still
> > relevant, since the text needs to be formatted according to ChangeLog
> > rules.
> 
> not necessarily.  there's two options:
> 	- just use `git log --stat` and pipe it into a ChangeLog.  this isn't a 
> "GNU ChangeLog", but it is a ChangeLog

This is still a GNU project, isn't it?  We still adhere to GNU coding
standards, right?  So I don't think we can avoid producing GNU-style
ChangeLog files.

Re: small request regarding commits in binutils-gdb.git

[Alan Modra]

On Wed, Jan 15, 2014 at 11:09:08PM +0200, Eli Zaretskii wrote:
> This is still a GNU project, isn't it?  We still adhere to GNU coding
> standards, right?  So I don't think we can avoid producing GNU-style
> ChangeLog files.

People have been putting the changelog entry in the git log.  If that
continues, it ought to be easy to generate a GNU ChangeLog by "git log
| awk".  I don't see any need for a ChangeLog in the source
repository if we can generate one to be distributed along with the
sources.

-- 
Alan Modra
Australia Development Lab, IBM

Re: small request regarding commits in binutils-gdb.git

[Joel Brobecker]

> > not necessarily.  there's two options:
> > 	- just use `git log --stat` and pipe it into a ChangeLog.  this isn't a 
> > "GNU ChangeLog", but it is a ChangeLog
> 
> This is still a GNU project, isn't it?  We still adhere to GNU coding
> standards, right?  So I don't think we can avoid producing GNU-style
> ChangeLog files.

JIC we go further in that direction, the scripts I use to produce
the release make some commits, and one of the things they do is
extracting ChangeLog entries from commit text. It assumes the format
I have been using, which is

| path/to/ChangeLog:
| 
|        [ChangeLog text]

The code as is works well in the context of the controlled commits
the scripts are creating. They would probably need to be improved
for general usage, and the convention could also be adapted.

I would probably also add another "server"-side check to reject
improperly formatted commit logs prior to switching over to automatic
generation.

-- 
Joel

Re: small request regarding commits in binutils-gdb.git

[Fred Cooke]

Fair call, though the commit message format requirements might become
a bit onerous if done this way, OR, generation might generate quite a
few errors/misprints. I guess the rate of commits is pretty low, and
commit size pretty big, on average, for binutils, though.

On Wed, Jan 15, 2014 at 9:02 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 15 January 2014 14:48:51 Fred Cooke wrote:
>> I'd be with you, Mike, but these packages are regularly distributed
>> without source control, as tarballs, and as such it's useful.
>
> i don't think it is, but let's assume you're right.  there's no reason the
> dist steps can't autogenerate a full ChangeLog from git history and throw it
> into the tarball.  it's what other GNU projects are doing now (like
> coreutils).
> -mike

Re: small request regarding commits in binutils-gdb.git

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 616 bytes --]

On Wednesday 15 January 2014 20:41:16 Fred Cooke wrote:
> Fair call, though the commit message format requirements might become
> a bit onerous if done this way, OR, generation might generate quite a
> few errors/misprints. I guess the rate of commits is pretty low, and
> commit size pretty big, on average, for binutils, though.

commit message format isn't really any more onerous than maintaining a 
ChangeLog file by hand

i don't know how other projects handle typos/updates.  maybe they just live 
with them.  i haven't looked too closely at the scripts in use, but should be 
easy to ask.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: small request regarding commits in binutils-gdb.git

[Pedro Alves]

On 01/15/2014 04:03 PM, Eli Zaretskii wrote:

> The text before the log entry is just a long way of saying the same
> thing as the log entry, isn't it?  It repeats the info that is already
> in the log entry.  Why does it make sense to repeat all that?

Comments in the code should describe anything non-obvious about
the _present_ state of the code.  ("We used to do this, and now
do that" comments are not appropriate in the code.)  The ChangeLog
entry says _what_ changed.  The rationale for the change, the _why_
the change was made, the bridge between old and new, is what is
often lost in the mailing list archives, often times spread over
a discussion crossing several emails (and if it crosses the month
border, it's usually harder even to retrieve), if in any at all.
The commit log is the perfect place to store that info.

-- 
Pedro Alves

Re: small request regarding commits in binutils-gdb.git

[Tom Tromey]

Pedro> (and if it crosses the month border, it's usually harder even to
Pedro> retrieve)

This is a pet peeve of mine.  It makes archaeology non-obvious
sometimes, like when there is a multi-month gap in the discussion.  I
wish we had better software support for this, and for other common
archaeological tasks.

Tom

Re: small request regarding commits in binutils-gdb.git

[Gary Benson]

Eli Zaretskii wrote:
> > Date: Wed, 15 Jan 2014 16:12:51 +0400
> > From: Joel Brobecker <brobecker@adacore.com>
> > 
> > It would look like this:
> > 
> > > psymtab cleanup patch 3/3
> > >
> > > This last patch removes "partial" from the names of
> > > expand_partial_symbol_names and map_partial_symbol_filenames.
> > > It also renames expand_partial_symbol_names to match the
> > > struct quick_symbol_functions "method" that it wraps:
> > > expand_symtabs_matching.
> > >
> > > This patch also adds two parameters to expand_symtabs_matching
> > > so that it can fully wrap the underlying quick_symbol_functions method.
> > > This makes it usable in more places.
> > > I thought of having a cover function that still had the same
> > > signature as the old expand_partial_symbol_names function,
> > > but I couldn't think of a good name, and it wasn't clear it was
> > > worth it anyway.
> > >
> > > gdb/ChangeLog:
> > >
> > >     * symfile.h (expand_symtabs_matching): Renamed from
> > >     expand_partial_symbol_names.  Update prototype.
> > >     (map_symbol_filenames): Renamed from map_partial_symbol_filenames.
> > >     * symfile.c (expand_symtabs_matching): Renamed from
> 
> The text before the log entry is just a long way of saying the same
> thing as the log entry, isn't it?  It repeats the info that is
> already in the log entry.  Why does it make sense to repeat all
> that?

The ChangeLog says what.  The text before it says why.

Cheers,
Gary

-- 
http://gbenson.net/

Re: small request regarding commits in binutils-gdb.git

[Jose E. Marchesi]

    
    > I am also considering the idea of writing a small hook that
    > would reject new commits introducing commits where the "empty line
    > after subject" rule is not followed. Would that be an acceptable
    > restriction?
    
    No objections from me.

Same here.

Re: small request regarding commits in binutils-gdb.git

[Cary Coutant]

> For the rest of the community, using a meaningful subject, one that
> actually gives a hint at what it does, is something that I have
> found to be really helpful. For instance, "Provide format string
> to as_warn" can be more helpful when scanning through a list
> of commits than "PR gas/16434".

This is all good, but could you clarify what the conventions are now
for where the "PR gas/16434" needs to go in order for the patch to get
sent to bugzilla?

-cary

Re: small request regarding commits in binutils-gdb.git

[Tom Tromey]

>>>>> "Cary" == Cary Coutant <ccoutant@google.com> writes:

>> For the rest of the community, using a meaningful subject, one that
>> actually gives a hint at what it does, is something that I have
>> found to be really helpful. For instance, "Provide format string
>> to as_warn" can be more helpful when scanning through a list
>> of commits than "PR gas/16434".

Cary> This is all good, but could you clarify what the conventions are now
Cary> for where the "PR gas/16434" needs to go in order for the patch to get
Cary> sent to bugzilla?

It will work as long as it appears anywhere in the commit message.

Tom