ChangeLogs in commit messages

ChangeLogs in commit messages

[Gary Benson]

Hi all,

I recently added an extra section to the contributions checklist on
the wiki documenting the conversation on commit messages from when
we switched to git:

  https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

Andreas pointed out that different people are including the ChangeLog
entries in different ways,

  1. With paths and with the date-and-author header:

    gdb/
    2014-07-30  Gary Benson  <gbenson@redhat.com>

            * btrace.c: Include defs.h.
            * common/ptid.c: Include defs.h or server.h as appropriate.
            * nat/mips-linux-watch.c: Likewise.

  2. With date-and-author headers but no paths:

    2014-08-04  Tom Tromey  <tromey@redhat.com>
      
            * gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
            "target_resume".

  3. With paths but no date-and-author headers:

    gdb/ChangeLog:
      
            * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
              Add debug trace.

  4. With no preamble at all:

            * chew.c (print_stack_level, main): Cast result of pointer
            difference to match format string.

What are people's preferences here?  My preference is #1, but I could
live with #3.  If we come to some kind of concensus on this I'll
update the wiki to reflect this.

Thanks,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Joel Brobecker]

>   1. With paths and with the date-and-author header:
> 
>     gdb/
>     2014-07-30  Gary Benson  <gbenson@redhat.com>
> 
>             * btrace.c: Include defs.h.
>             * common/ptid.c: Include defs.h or server.h as appropriate.
>             * nat/mips-linux-watch.c: Likewise.
> 
>   2. With date-and-author headers but no paths:
> 
>     2014-08-04  Tom Tromey  <tromey@redhat.com>
>       
>             * gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
>             "target_resume".
> 
>   3. With paths but no date-and-author headers:
> 
>     gdb/ChangeLog:
>       
>             * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
>               Add debug trace.
> 
>   4. With no preamble at all:
> 
>             * chew.c (print_stack_level, main): Cast result of pointer
>             difference to match format string.
> 
> What are people's preferences here?  My preference is #1, but I could
> live with #3.  If we come to some kind of concensus on this I'll
> update the wiki to reflect this.

#3, since date and author are often redundant with the commit's
author. And even if not in the same, it's in the ChangeLog entry
that should be checked in as part of the commit. Also, I feel
like having those in the CL is an extra source of potential
issue (eg: if forgot to update the date), and revision logs
cannot be fixed once the commit has been pushed, whereas dates
in ChangeLog entries can.

But I can also live with #1. Not super keen on #2 and #4 in the sense
that it seems important to me to say which ChangeLogs are being
updated, since filenames are relative to those.

I do think we can be a little flexible without much downside,
and therefore have a set of accepted practices.

-- 
Joel

Re: ChangeLogs in commit messages

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Thu 14 Aug 2014 09:32:31 Gary Benson wrote:
> What are people's preferences here?  My preference is #1, but I could
> live with #3.  If we come to some kind of concensus on this I'll
> update the wiki to reflect this.

(4) -- none of the above.  the ChangeLog files contain the same data, so the 
commit messages should contain useful details.  i.e. the same content you used 
when you posted to the mailing list.
-mike

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

Re: ChangeLogs in commit messages

[Gary Benson]

Mike Frysinger wrote:
> On Thu 14 Aug 2014 09:32:31 Gary Benson wrote:
> > What are people's preferences here?  My preference is #1, but I
> > could live with #3.  If we come to some kind of concensus on this
> > I'll update the wiki to reflect this.
> 
> (4) -- none of the above.  the ChangeLog files contain the same
> data, so the commit messages should contain useful details.
> i.e. the same content you used when you posted to the mailing list.

Do you mean #4 (changelog entries with no path/author-date) or are you
proposing new option #5 (no changelog in the commit message at all)?
#5 would suit me too.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> >   1. With paths and with the date-and-author header:
> > 
> >     gdb/
> >     2014-07-30  Gary Benson  <gbenson@redhat.com>
> > 
> >             * btrace.c: Include defs.h.
> >             * common/ptid.c: Include defs.h or server.h as appropriate.
> >             * nat/mips-linux-watch.c: Likewise.
> > 
> >   2. With date-and-author headers but no paths:
> > 
> >     2014-08-04  Tom Tromey  <tromey@redhat.com>
> >       
> >             * gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
> >             "target_resume".
> > 
> >   3. With paths but no date-and-author headers:
> > 
> >     gdb/ChangeLog:
> >       
> >             * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
> >               Add debug trace.
> > 
> >   4. With no preamble at all:
> > 
> >             * chew.c (print_stack_level, main): Cast result of pointer
> >             difference to match format string.
> > 
> > What are people's preferences here?  My preference is #1, but I
> > could live with #3.  If we come to some kind of concensus on this
> > I'll update the wiki to reflect this.
> 
> #3, since date and author are often redundant with the commit's
> author. And even if not in the same, it's in the ChangeLog entry
> that should be checked in as part of the commit. Also, I feel like
> having those in the CL is an extra source of potential issue (eg:
> if forgot to update the date), and revision logs cannot be fixed
> once the commit has been pushed, whereas dates in ChangeLog entries
> can.

My concern with omitting the author-and-date is for commits with
multiple authors, and/or multiple commits that have been squashed
with git-rebase (which uses the date and author of the commit that
was "pick"ed rather than that of other commits that were "squash"ed
or "fixup"ed into it.  For example:

  commit 314c6a3559393741f22fdd9836f83d9f364fbd2a
  Author: Tom Tromey <tromey@redhat.com>
  Date:   Fri Jun 13 09:22:09 2014 -0600

      Make gdbserver CORE_ADDR unsigned
    
      gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
      me; and furthermore likely to cause problems in common/, as it is
      different from gdb's definition.
    
      gdb/gdbserver/
      2014-07-24  Tom Tromey  <tromey@redhat.com>
                  Gary Benson  <gbenson@redhat.com>
    
              * server.h (CORE_ADDR): Now unsigned.

This commit came from a half-finished branch Tom started back in
January.  I picked it up June or so, updated/tidied it a bit and
submitted it for review.  It was pushed on July 24 but the date on
the commit is June 13.  I think most people use rebase eventually,
so this kind of thing is pretty common.  The multiple authors
things is less common but not unique.

Thanks,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Thu 14 Aug 2014 08:56:57 Mike Frysinger wrote:
> On Thu 14 Aug 2014 09:32:31 Gary Benson wrote:
> > What are people's preferences here?  My preference is #1, but I could
> > live with #3.  If we come to some kind of concensus on this I'll
> > update the wiki to reflect this.
> 
> (4) -- none of the above.  the ChangeLog files contain the same data, so the
> commit messages should contain useful details.  i.e. the same content you
> used when you posted to the mailing list.

erp, i thought you only had 3 options.  so i mean (5) here :).
-mike

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

Re: ChangeLogs in commit messages

[Joel Brobecker]

> My concern with omitting the author-and-date is for commits with
> multiple authors, and/or multiple commits that have been squashed
> with git-rebase (which uses the date and author of the commit that
> was "pick"ed rather than that of other commits that were "squash"ed
> or "fixup"ed into it.  For example:

I had that in mind, but since the author'ing info is in the ChangeLog
file, I don't see any advantage of having to maintain it in the revision
log as well. But this is again an area where I think having multiple
options isn't going to hurt us, so I think we would be fine if some
prefer it this way, while others use the author-less version.

-- 
Joel

Re: ChangeLogs in commit messages

[Joel Brobecker]

> Do you mean #4 (changelog entries with no path/author-date) or are you
> proposing new option #5 (no changelog in the commit message at all)?
> #5 would suit me too.

No, I strongly object to #5. The ChangeLog entries are part of
the email to be sent along with the patch to be reviewed, We had
that discussion many many years ago, and at the time, people
wanted the CL entry in the email, rather than as a diff.

Also, having the CL entry as a diff means maintaining that diff
over time, which is guaranteed to be a source of conflicts.
I know there are tools out there to deal with this specific source
of conflict, but I don't think we should require everyone to set
that up.

And finally, I find the CL entry to be useful to have when I review
revision logs.

-- 
Joel

Re: ChangeLogs in commit messages

[Siva Chandra]

On Thu, Aug 14, 2014 at 1:32 AM, Gary Benson <gbenson@redhat.com> wrote:
>   3. With paths but no date-and-author headers:
>
>     gdb/ChangeLog:
>
>             * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
>               Add debug trace.

I vote for this. However, IMHO, the suffix of "ChangeLog" in the
directory name is also redundant.

Re: ChangeLogs in commit messages

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

> No, I strongly object to #5. The ChangeLog entries are part of
> the email to be sent along with the patch to be reviewed, We had
> that discussion many many years ago, and at the time, people
> wanted the CL entry in the email, rather than as a diff.

How does this have anything to do with the commit message?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > No, I strongly object to #5. The ChangeLog entries are part of
> > the email to be sent along with the patch to be reviewed, We had
> > that discussion many many years ago, and at the time, people
> > wanted the CL entry in the email, rather than as a diff.
> 
> How does this have anything to do with the commit message?

The email can be sent nearly verbatim using "git send-email".
This is an objective that can be achieved with zero extra work
if you consider the other reasons I gave why I think the CL entry
should be in the revision log.

-- 
Joel

Re: ChangeLogs in commit messages

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> > No, I strongly object to #5. The ChangeLog entries are part of
>> > the email to be sent along with the patch to be reviewed, We had
>> > that discussion many many years ago, and at the time, people
>> > wanted the CL entry in the email, rather than as a diff.
>> 
>> How does this have anything to do with the commit message?
>
> The email can be sent nearly verbatim using "git send-email".

The use of send-email is not a requirement.

> This is an objective that can be achieved with zero extra work
> if you consider the other reasons I gave why I think the CL entry
> should be in the revision log.

That can also be achieved without the duplication in the commit message.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > This is an objective that can be achieved with zero extra work
> > if you consider the other reasons I gave why I think the CL entry
> > should be in the revision log.
> 
> That can also be achieved without the duplication in the commit message.

Again, I do think that having the CL entry in the commit message is
useful, and I disagree with the idea of removing it from there.
Imagine you are scanning the past changes, looking for what might
have caused a behavior change. When using "git log", having the CL
in there often helps.

-- 
Joel

Re: ChangeLogs in commit messages

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

> Again, I do think that having the CL entry in the commit message is
> useful, and I disagree with the idea of removing it from there.
> Imagine you are scanning the past changes, looking for what might
> have caused a behavior change. When using "git log", having the CL
> in there often helps.

It's already included.  You can use git log -p to show it.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > Again, I do think that having the CL entry in the commit message is
> > useful, and I disagree with the idea of removing it from there.
> > Imagine you are scanning the past changes, looking for what might
> > have caused a behavior change. When using "git log", having the CL
> > in there often helps.
> 
> It's already included.  You can use git log -p to show it.

But -p shows the _entire_ patch. This is a lot of clutter just
to get the ChangeLog entries, don't you think?

-- 
Joel

Re: ChangeLogs in commit messages

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> > Again, I do think that having the CL entry in the commit message is
>> > useful, and I disagree with the idea of removing it from there.
>> > Imagine you are scanning the past changes, looking for what might
>> > have caused a behavior change. When using "git log", having the CL
>> > in there often helps.
>> 
>> It's already included.  You can use git log -p to show it.
>
> But -p shows the _entire_ patch.

You can limit it to the ChangeLogs.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: ChangeLogs in commit messages

[Eli Zaretskii]

> Date: Thu, 14 Aug 2014 14:15:30 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb@sourceware.org, Andreas Arnez <arnez@linux.vnet.ibm.com>
> 
> My concern with omitting the author-and-date is for commits with
> multiple authors

Doesn't the --author switch provide a solution for that?

Re: ChangeLogs in commit messages

[Joel Brobecker]

> >> It's already included.  You can use git log -p to show it.
> >
> > But -p shows the _entire_ patch.
> 
> You can limit it to the ChangeLogs.

Why am I on the bench spending all this time definding a currently
accepted position while you just attack all my explanations with
one-liners? Since you are trying to get us to change, why do _you_
make a proposal, and answer precisely how to fill the gaps that
we might find with it?

-- 
Joel

Re: ChangeLogs in commit messages

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> >> It's already included.  You can use git log -p to show it.
>> >
>> > But -p shows the _entire_ patch.
>> 
>> You can limit it to the ChangeLogs.
>
> Why am I on the bench spending all this time definding a currently
> accepted position while you just attack all my explanations with

Where did I attack anyone?

> one-liners? Since you are trying to get us to change, why do _you_

I don't.  You were claiming that the duplication is unavoidable, I only
show you the working alternatives.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > one-liners? Since you are trying to get us to change, why do _you_
> 
> I don't.  You were claiming that the duplication is unavoidable, I only
> show you the working alternatives.

The way you do it just absolutely fustrating: 1. I don't say it is
unavoidable, I say it is useful. And 2. you counter it by saying you can,
but you don't show how. If you show me how, I can then tell you whether
I agree with you or not.

-- 
Joel

Re: ChangeLogs in commit messages

[Andreas Arnez]

On Thu, Aug 14 2014, Gary Benson wrote:

> Mike Frysinger wrote:
>> On Thu 14 Aug 2014 09:32:31 Gary Benson wrote:
>> > What are people's preferences here?  My preference is #1, but I
>> > could live with #3.  If we come to some kind of concensus on this
>> > I'll update the wiki to reflect this.
>> 
>> (4) -- none of the above.  the ChangeLog files contain the same
>> data, so the commit messages should contain useful details.
>> i.e. the same content you used when you posted to the mailing list.
>
> Do you mean #4 (changelog entries with no path/author-date) or are you
> proposing new option #5 (no changelog in the commit message at all)?
> #5 would suit me too.

Since Mike obviously meant #5, I tend to agree.  At least as long as we
keep ChangeLog files in git, rather than extracting them with some
automatism from the git commit messages, e.g., as suggested here:

  https://sourceware.org/ml/gdb-patches/2014-01/msg00578.html

Let's assume we consistently do #5, so no ChangeLog entries in the
commit messages, but just in the ChangeLog files.  Then, to see them
again in a "git log", we can still do something like this:

  git log -U0 gdb/ChangeLog --since='1 week ago'

On the other hand, it's more difficult to *suppress* the existing
ChangeLogs in a "git log".

Re: ChangeLogs in commit messages

[Sergio Durigan Junior]

On Thursday, August 14 2014, Joel Brobecker wrote:

> No, I strongly object to #5. The ChangeLog entries are part of
> the email to be sent along with the patch to be reviewed, We had
> that discussion many many years ago, and at the time, people
> wanted the CL entry in the email, rather than as a diff.

On Thursday, August 14 2014, Joel Brobecker wrote:

> Again, I do think that having the CL entry in the commit message is
> useful, and I disagree with the idea of removing it from there.
> Imagine you are scanning the past changes, looking for what might
> have caused a behavior change. When using "git log", having the CL
> in there often helps.

Being the maintainer of GDB for RHEL, and having to deal with many
patches to backport, I second what Joel said.

I prefer CL messages to be included in the commit message (a la option
#1 in Gary's message), and I strongly prefer to have a good explanation
of what the patch does in the message too.

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: ChangeLogs in commit messages

[Gary Benson]

Eli Zaretskii wrote:
> > Date: Thu, 14 Aug 2014 14:15:30 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb@sourceware.org, Andreas Arnez <arnez@linux.vnet.ibm.com>
> > 
> > My concern with omitting the author-and-date is for commits with
> > multiple authors
> 
> Doesn't the --author switch provide a solution for that?

It only allows for two people though, author and committer.
As far as I'm aware anyway!  I'm sure there are commits in
there with more than two authors.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Eli Zaretskii]

> Date: Fri, 15 Aug 2014 09:05:10 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: brobecker@adacore.com, gdb@sourceware.org, arnez@linux.vnet.ibm.com
> 
> Eli Zaretskii wrote:
> > > Date: Thu, 14 Aug 2014 14:15:30 +0100
> > > From: Gary Benson <gbenson@redhat.com>
> > > Cc: gdb@sourceware.org, Andreas Arnez <arnez@linux.vnet.ibm.com>
> > > 
> > > My concern with omitting the author-and-date is for commits with
> > > multiple authors
> > 
> > Doesn't the --author switch provide a solution for that?
> 
> It only allows for two people though, author and committer.

Can't --author be used more than once, or mention more than 1 author
in the same --author switch?

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > Do you mean #4 (changelog entries with no path/author-date) or are
> > you proposing new option #5 (no changelog in the commit message at
> > all)?  #5 would suit me too.
> 
> No, I strongly object to #5. The ChangeLog entries are part of the
> email to be sent along with the patch to be reviewed, We had that
> discussion many many years ago, and at the time, people wanted the
> CL entry in the email, rather than as a diff.

This discussion is starting to blur the lines between "what is in the
commit message" and "what is in the gdb-patches email".  Right now,
the two are the same, especially if you use git-send-email, but I
don't think git-send-email convenience is a valid reason to say that
ChangeLog entries need to be in the commit message.  If we wanted a
situation where gdb-patches emails have ChangeLog entries but commit
messages do not then we could surely script that.

> And finally, I find the CL entry to be useful to have when I review
> revision logs.

Noted.

My work on GDB mostly involves _generating_ commits, so I have a
natural bias towards removing things that make that difficult (and
ChangeLogs are an easy target).  I do recognise, however, that some
people's work also involves _consuming_ commits (for creating release
branches, or for distro integration) so when I hear Joel and Sergio
stating the cases for a) ChangeLog files to exist, and b) ChangeLog
entries to be included in commit messages I have to admit that their
arguments carry more weight than my "everything would be so much more
convenient for me if ChangeLogs went away".  So I think we should
stop discussing the removal of ChangeLogs, in this thread anyway.

Andreas Arnez pointed out that Joel previously mentioned generating
the actual ChangeLog files from the commit messages, here:

  https://sourceware.org/ml/gdb-patches/2014-01/msg00578.html

This sounds like an interesting idea, but we really would have to
standardize on a particular format, and I think #1 (path and author-
date headers) is the only option that could realistically work.
If we standardize on this now (and put some checks on the server to
weed out bad messages) then come December we'll have four months of
commit messages we can use to check whether we can correctly replicate
the ChangeLog files.  And, if we can, we can consider omitting the
files from the repo and generating them as needed for tarballs.

How does this sound?

Cheers,
Gary

--
http://gbenson.net/

Re: ChangeLogs in commit messages

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> Can't --author be used more than once, or mention more than 1 author
> in the same --author switch?

Git supports only a single author per commit.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ChangeLogs in commit messages

[Joel Brobecker]

> Andreas Arnez pointed out that Joel previously mentioned generating
> the actual ChangeLog files from the commit messages, here:
> 
>   https://sourceware.org/ml/gdb-patches/2014-01/msg00578.html
> 
> This sounds like an interesting idea, but we really would have to
> standardize on a particular format, and I think #1 (path and author-
> date headers) is the only option that could realistically work.
> If we standardize on this now (and put some checks on the server to
> weed out bad messages) then come December we'll have four months of
> commit messages we can use to check whether we can correctly replicate
> the ChangeLog files.  And, if we can, we can consider omitting the
> files from the repo and generating them as needed for tarballs.
> 
> How does this sound?

That sounds good to me. I do agree that a server-side "update"
check is needed to verify the CL entry is present in the revision
log and in the correct format.

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > Andreas Arnez pointed out that Joel previously mentioned
> > generating the actual ChangeLog files from the commit messages,
> > here:
> > 
> >   https://sourceware.org/ml/gdb-patches/2014-01/msg00578.html
> > 
> > This sounds like an interesting idea, but we really would have to
> > standardize on a particular format, and I think #1 (path and
> > author- date headers) is the only option that could realistically
> > work.  If we standardize on this now (and put some checks on the
> > server to weed out bad messages) then come December we'll have
> > four months of commit messages we can use to check whether we can
> > correctly replicate the ChangeLog files.  And, if we can, we can
> > consider omitting the files from the repo and generating them as
> > needed for tarballs.
> > 
> > How does this sound?
> 
> That sounds good to me. I do agree that a server-side "update" check
> is needed to verify the CL entry is present in the revision log and
> in the correct format.

Does anybody have any experience writing such checks?  Or, does
anybody know of any project that already uses such checks?  I can
look into doing it myself (it's a pre-receive hook, right?)

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Joel Brobecker]

> Does anybody have any experience writing such checks?  Or, does
> anybody know of any project that already uses such checks?  I can
> look into doing it myself (it's a pre-receive hook, right?)

I have loads of experience writing git hooks, but none with
this repository's implementation.

I would typically adjust the "update" hook for that, but it looks
like the "pre-receive" hook would also work.

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > Does anybody have any experience writing such checks?  Or, does
> > anybody know of any project that already uses such checks?  I can
> > look into doing it myself (it's a pre-receive hook, right?)
> 
> I have loads of experience writing git hooks, but none with
> this repository's implementation.
> 
> I would typically adjust the "update" hook for that, but it looks
> like the "pre-receive" hook would also work.

I've put together a quick pre-receive hook (inlined below).  Each
received commit on the "master" branch that touches the "gdb"
subdirectory gets its message checked.  The check itself is fairly
cursory: it splits the message using the "YYYY-MM-DD  NAME  <EMAIL>"
headers, checks each is preceeded by a path starting with "gdb/" and
ending with "/", and checks each is followed by more "NAME  <EMAIL>"
lines, blank lines, or lines starting with tab.  I don't know how
comprehensive we want to be here as the message should already have
been checked over by the reviewer.

I've never done anything server-side with git before, so there may
well be things I'm missing here.  I was mainly experimenting to see
how difficult this all was :)

Cheers,
Gary

-- 
#!/usr/bin/env python

import re
import subprocess
import sys

GIT = "/usr/bin/git"
SHA1HASH = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE)

name_mail = r".*  <[^@]+@[^>]+>$"
CL_HDR_A = re.compile(r"^\d{4}-\d{2}-\d{2}  " + name_mail)
CL_HDR_B = re.compile(r"^\s+" + name_mail)

class MessageChecker:
    def __init__(self, rev):
        self.rev = rev

    def check(self, condition, msg):
        if not condition:
            print "error: %s: bad commit message" % self.rev
            print "error: %s" % msg
            sys.exit(1)

    def check_message(self, lines):
        self.check(len(lines) > 2, "message is too short")
        self.check(lines[1] == "", "second line should be blank")
        splits = [index
                  for index in xrange(2, len(lines))
                  if CL_HDR_A.match(lines[index]) is not None]
        self.check(splits, "no ChangeLog entries found")
        for start, limit in zip(splits, splits[1:] + [0]):
            start -= 1
            limit -= 1
            self.check_changelog(lines[start:limit])

    def check_changelog(self, lines):
        self.check(len(lines) > 2, "ChangeLog entry is too short")
        path = lines.pop(0)
        self.check(path.startswith("gdb/") and path.endswith("/"),
                   "each ChangeLog entry should be preceeded by its path")
        lines.pop(0) # lines[1] has already been checked
        while lines and CL_HDR_B.match(lines[0]) is not None:
            lines.pop(0)
        self.check(lines, "only ChangeLog headers found (no data)")
        for line in lines:
            self.check(not line or line.startswith("\t"),
                       "bad ChangeLog line %s" % repr(line))

def check_hash(hash):
    assert SHA1HASH.match(hash) is not None

def git(*command):
    fp = subprocess.Popen((GIT,) + command, stdout=subprocess.PIPE)
    output = fp.communicate()[0]
    if fp.returncode:
        sys.exit(1)
    return output

def git_rev_list(oldrev, newrev):
    check_hash(oldrev)
    check_hash(newrev)
    return git("rev-list", oldrev + ".." + newrev)

def git_diff_tree(rev):
    check_hash(rev)
    return git("diff-tree", rev)

def git_cat_file_commit(rev):
    check_hash(rev)
    return git("cat-file", "commit", rev)

def commit_touches_gdb(rev):
    lines = git_diff_tree(rev).rstrip().split("\n")
    check = lines.pop(0)
    assert check == rev
    for line in lines:
        if line.split()[-1] == "gdb":
            return True
    return False

def check_commit(rev):
    lines = git_cat_file_commit(rev).rstrip().split("\n")
    while lines and lines[0]:
        lines.pop(0)
    assert lines
    lines.pop(0)
    MessageChecker(rev).check_message(lines)

def process_pack(oldrev, newrev, refname):
    if refname == "refs/heads/master":
        revs = git_rev_list(oldrev, newrev).rstrip().split("\n")
        revs.reverse()
        for rev in revs:
            if commit_touches_gdb(rev):
                check_commit(rev)
            print "%s: ok" % rev

def main():
    for line in sys.stdin.xreadlines():
        bits = line.rstrip().split()
        assert len(bits) == 3
        process_pack(*bits)

if __name__ == "__main__":
    main()

Re: ChangeLogs in commit messages

[Andreas Arnez]

On Fri, Aug 15 2014, Gary Benson wrote:

> Joel Brobecker wrote:
>> > Does anybody have any experience writing such checks?  Or, does
>> > anybody know of any project that already uses such checks?  I can
>> > look into doing it myself (it's a pre-receive hook, right?)
>> 
>> I have loads of experience writing git hooks, but none with
>> this repository's implementation.
>> 
>> I would typically adjust the "update" hook for that, but it looks
>> like the "pre-receive" hook would also work.
>
> I've put together a quick pre-receive hook (inlined below).  Each
> received commit on the "master" branch that touches the "gdb"
> subdirectory gets its message checked.  The check itself is fairly
> cursory: it splits the message using the "YYYY-MM-DD  NAME  <EMAIL>"
> headers, checks each is preceeded by a path starting with "gdb/" and
> ending with "/", and checks each is followed by more "NAME  <EMAIL>"
> lines, blank lines, or lines starting with tab.  I don't know how
> comprehensive we want to be here as the message should already have
> been checked over by the reviewer.

Maybe (for now) we also want to check that the same ChangeLog entries
appear in the patch?

> I've never done anything server-side with git before, so there may
> well be things I'm missing here.  I was mainly experimenting to see
> how difficult this all was :)
>
> Cheers,
> Gary

Re: ChangeLogs in commit messages

[Joel Brobecker]

> I've put together a quick pre-receive hook (inlined below).  Each
> received commit on the "master" branch that touches the "gdb"
> subdirectory gets its message checked.  The check itself is fairly
> cursory: it splits the message using the "YYYY-MM-DD  NAME  <EMAIL>"
> headers, checks each is preceeded by a path starting with "gdb/" and
> ending with "/", and checks each is followed by more "NAME  <EMAIL>"
> lines, blank lines, or lines starting with tab.  I don't know how
> comprehensive we want to be here as the message should already have
> been checked over by the reviewer.
> 
> I've never done anything server-side with git before, so there may
> well be things I'm missing here.  I was mainly experimenting to see
> how difficult this all was :)

Just looking quickly at the script for the typical mistakes I made
when I first started dabbling into this kind of script:

  . The SHA1 is 000[...]0 when creating a new reference
    (Eg: creating a new branch). But that's an invalid SHA1

  . Same when deleting a reference. the "new rev" is 0.

  . I would suggest we do that on all branches except branches
    under a specific namespace (thinking "vendor" branches, TBD).

  . When creating a new branch, you really don't want to start
    checking the entire history again, so you need to find the
    branchpoint and use that as the "old rev".

I would like to push again for the format without the date and author
email but the simpler gdb/ChangeLog: instead.  The commit already
stores that information (author name and email, committer date),
whereas requiring the user to provide it in the rev log means that
the date needs to be maintained throughout the lifetime of the patch
until that patch gets pushed. Same thing when cherry-picking a patch
from master to a release branch.

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> I would like to push again for the format without the date and
> author email but the simpler gdb/ChangeLog: instead.  The commit
> already stores that information (author name and email, committer
> date), whereas requiring the user to provide it in the rev log
> means that the date needs to be maintained throughout the
> lifetime of the patch until that patch gets pushed. Same thing
> when cherry-picking a patch from master to a release branch.

I'm happy with this.  Do you want the "ChangeLog" in the path?

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Doug Evans]

On Mon, Aug 18, 2014 at 1:31 AM, Gary Benson <gbenson@redhat.com> wrote:
> Joel Brobecker wrote:
>> I would like to push again for the format without the date and
>> author email but the simpler gdb/ChangeLog: instead.  The commit
>> already stores that information (author name and email, committer
>> date), whereas requiring the user to provide it in the rev log
>> means that the date needs to be maintained throughout the
>> lifetime of the patch until that patch gets pushed. Same thing
>> when cherry-picking a patch from master to a release branch.
>
> I'm happy with this.  Do you want the "ChangeLog" in the path?

Me too.

As for ChangeLog in the path,
Do you mean prepending entries with "gdb/ChangeLog" or
"testsuite/ChangeLog" instead of just "gdb/" or "testsuite/" ?
If so, I would say skip "ChangeLog", keep just, e.g., "testsuite/".
That's the existing convention.

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > I'm happy with this.  Do you want the "ChangeLog" in the path?
> 
> Me too.
> 
> As for ChangeLog in the path,
> Do you mean prepending entries with "gdb/ChangeLog" or
> "testsuite/ChangeLog" instead of just "gdb/" or "testsuite/" ?
> If so, I would say skip "ChangeLog", keep just, e.g., "testsuite/".
> That's the existing convention.

FWIW, I think that the extra "ChangeLog" bring 2 benefits:
  - it makes it clear we're starting a ChangeLog entry:
  - it reduces the chances of a false-positive when parsing
    the revision log for one or more CL entries. I found this out
    while developing a parser that I use in the context of
    the gdb release.

Small bonus:  When you modify the root ChangeLog file, the entry
ends up looking like this...

| ChangeLog:
|
|         * file: description.

... instead of ...

| <nothing?>:
|
|         * file: description.

... or maybe ...

| /
|
|         * file: description.

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > > I'm happy with this.  Do you want the "ChangeLog" in the path?
> > 
> > Me too.
> > 
> > As for ChangeLog in the path,
> > Do you mean prepending entries with "gdb/ChangeLog" or
> > "testsuite/ChangeLog" instead of just "gdb/" or "testsuite/" ?
> > If so, I would say skip "ChangeLog", keep just, e.g., "testsuite/".
> > That's the existing convention.
> 
> FWIW, I think that the extra "ChangeLog" bring 2 benefits:
>   - it makes it clear we're starting a ChangeLog entry:
>   - it reduces the chances of a false-positive when parsing
>     the revision log for one or more CL entries. I found this out
>     while developing a parser that I use in the context of
>     the gdb release.
> 
> Small bonus:  When you modify the root ChangeLog file, the entry
> ends up looking like this...
> 
> | ChangeLog:
> |
> |         * file: description.

I'm happy with this.  I'll wait a day or so in case somebody objects,
and if not I'll update the wiki.

Thanks,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Gary Benson]

Gary Benson wrote:
> Joel Brobecker wrote:
> > > > I'm happy with this.  Do you want the "ChangeLog" in the path?
> > > 
> > > Me too.
> > > 
> > > As for ChangeLog in the path,
> > > Do you mean prepending entries with "gdb/ChangeLog" or
> > > "testsuite/ChangeLog" instead of just "gdb/" or "testsuite/" ?
> > > If so, I would say skip "ChangeLog", keep just, e.g., "testsuite/".
> > > That's the existing convention.
> > 
> > FWIW, I think that the extra "ChangeLog" bring 2 benefits:
> >   - it makes it clear we're starting a ChangeLog entry:
> >   - it reduces the chances of a false-positive when parsing
> >     the revision log for one or more CL entries. I found this out
> >     while developing a parser that I use in the context of
> >     the gdb release.
> > 
> > Small bonus:  When you modify the root ChangeLog file, the entry
> > ends up looking like this...
> > 
> > | ChangeLog:
> > |
> > |         * file: description.
> 
> I'm happy with this.  I'll wait a day or so in case somebody objects,
> and if not I'll update the wiki.

I have updated the wiki:
https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Sergio Durigan Junior]

On Thursday, August 14 2014, Joel Brobecker wrote:

>>   3. With paths but no date-and-author headers:
>> 
>>     gdb/ChangeLog:
>>       
>>             * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
>>               Add debug trace.

> #3, since date and author are often redundant with the commit's
> author. And even if not in the same, it's in the ChangeLog entry
> that should be checked in as part of the commit. Also, I feel
> like having those in the CL is an extra source of potential
> issue (eg: if forgot to update the date), and revision logs
> cannot be fixed once the commit has been pushed, whereas dates
> in ChangeLog entries can.

Sorry for reopening this discussion, but I would like to propose another
change.  I understand Joel's arguments above, and even agreed with them
when I read, but I'm not so sure anymore.

Yesterday Doug asked something interesting in the channel.  He was
trying to figure out the date when 860789c7 was pushed.  From git log:


  commit 860789c7d5d6ef2f60e757feb21845bb230582a8
  Author: Gary Benson <gbenson@redhat.com>
  Date:   Fri Aug 8 15:37:41 2014 +0100

    Use exceptions and cleanups in gdbserver
    
    This commit replaces the hacky "exception" system in gdbserver with
    the exceptions and cleanups subsystem from GDB.
    
    Only the catch/cleanup code in what was "main" has been updated to
    use the new system.  Other parts of gdbserver can now be converted
    to use TRY_CATCH and cleanups on an as-needed basis.
    
    A side-effect of this commit is that some error messages will change
    slightly, and in cases with multiple errors the error messages will
    be printed in a different order.
    
    gdb/gdbserver/ChangeLog:
    
        * server.h (setjmp.h): Do not include.
        (toplevel): Do not declare.
        (common-exceptions.h): Include.
        (cleanups.h): Likewise.
        * server.c (toplevel): Do not define.
        (exit_code): New static global.
        (detach_or_kill_for_exit_cleanup): New function.
        (main): New function.  Original main renamed to...
        (captured_main): New function.
        * utils.c (verror) [!IN_PROCESS_AGENT]: Use throw_verror.


However, this is not the push date; instead, this is the date Gary
committed the changes in his local repository.  If you go see the
ChangeLog referred in the commit message, you will see that the actual
push date was 2014-08-29.

While I understand the reasons for git to do that, I find it a bit
annoying to be "deceived" this way, and not have the push date readily
available in the commit message.  Maybe there's an easy way to recover
this information from git (though I couldn't find it), and if there is,
then disconsider my message.  However, there doesn't seem to be any
straightforward way to recover the push date.

So, what I propose is simple: let's add the date and the author back in
the ChangeLog entry that is present in the commit message.  It would
make things a bit easier, at least from my perspective.  WDYT?

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: ChangeLogs in commit messages

[Sergio Durigan Junior]

On Wednesday, September 03 2014, I wrote:

> While I understand the reasons for git to do that, I find it a bit
> annoying to be "deceived" this way, and not have the push date readily
> available in the commit message.  Maybe there's an easy way to recover
> this information from git (though I couldn't find it), and if there is,
> then disconsider my message.  However, there doesn't seem to be any
> straightforward way to recover the push date.

Siva Chandra kindly pointed to me that gitweb manages to figure out the
actual push date.  So I did a little more research on git-log's manpage,
and saw that there is a "--pretty=fuller" option which shows the push
date.

I am OK with this, so if you want, disconsider my request.  OTOH, if you
have an opinion about I proposed, I'd still would like to hear.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: ChangeLogs in commit messages

[Andreas Schwab]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Siva Chandra kindly pointed to me that gitweb manages to figure out the
> actual push date.  So I did a little more research on git-log's manpage,
> and saw that there is a "--pretty=fuller" option which shows the push
> date.

There is no such thing as a "push date".  What you see is the author
date and the committer date.  But both are set during the local commit,
and are unrelated to the point of time of pushing the commits to the
remote repository.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ChangeLogs in commit messages

[Gary Benson]

Andreas Schwab wrote:
> Sergio Durigan Junior <sergiodj@redhat.com> writes:
> > Siva Chandra kindly pointed to me that gitweb manages to figure
> > out the actual push date.  So I did a little more research on
> > git-log's manpage, and saw that there is a "--pretty=fuller"
> > option which shows the push date.
> 
> There is no such thing as a "push date".  What you see is the author
> date and the committer date.  But both are set during the local
> commit, and are unrelated to the point of time of pushing the
> commits to the remote repository.

Yeah, in git no file (object) is modified when you push or pull
commits, they're just copied from one place to another.

The committer date on the commit you mentioned is likely the time
I updated the ChangeLog prior to pushing, so that date will be the
"push date", but that's not guaranteed to be the case for all
commits.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Sergio Durigan Junior]

On Thursday, September 04 2014, Gary Benson wrote:

>> There is no such thing as a "push date".  What you see is the author
>> date and the committer date.  But both are set during the local
>> commit, and are unrelated to the point of time of pushing the
>> commits to the remote repository.
>
> Yeah, in git no file (object) is modified when you push or pull
> commits, they're just copied from one place to another.

Yeah, sorry for not being fluent in git's parlance.

> The committer date on the commit you mentioned is likely the time
> I updated the ChangeLog prior to pushing, so that date will be the
> "push date", but that's not guaranteed to be the case for all
> commits.

Right.  Anyway, I still consider it is valid to include this info
(author and date) in the commit message.

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: ChangeLogs in commit messages

[Doug Evans]

On Thu, Sep 4, 2014 at 7:22 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Thursday, September 04 2014, Gary Benson wrote:
>
>>> There is no such thing as a "push date".  What you see is the author
>>> date and the committer date.  But both are set during the local
>>> commit, and are unrelated to the point of time of pushing the
>>> commits to the remote repository.
>>
>> Yeah, in git no file (object) is modified when you push or pull
>> commits, they're just copied from one place to another.
>
> Yeah, sorry for not being fluent in git's parlance.
>
>> The committer date on the commit you mentioned is likely the time
>> I updated the ChangeLog prior to pushing, so that date will be the
>> "push date", but that's not guaranteed to be the case for all
>> commits.
>
> Right.  Anyway, I still consider it is valid to include this info
> (author and date) in the commit message.

For reference sake, what started this for me
is noticing git blame shows the author date by default.

Not picking on Gary of course, his patch is just
why I noticed this.
If I do a git blame of server.c I see patch 860789c7 with a date
of 2014-08-08.  That's three weeks before it was pushed upstream.
Bleah.  I'd really like to be able to do a git blame and have what
I see be useful, including the date.
The author date is basically useless to me.

[I realize neither author nor commit dates are the so-called push date,
but I'm guessing the commit date is generally reasonably
close to the date the patch is pushed upstream.
If not ... yikes.]

I realize any changes to what goes into the commit log
won't help solve this of course.  But that's what
started the discussion for me.

Re: ChangeLogs in commit messages

[Gary Benson]

Doug Evans wrote:
> On Thu, Sep 4, 2014 at 7:22 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
> > On Thursday, September 04 2014, Gary Benson wrote:
> > > > There is no such thing as a "push date".  What you see is the
> > > > author date and the committer date.  But both are set during
> > > > the local commit, and are unrelated to the point of time of
> > > > pushing the commits to the remote repository.
> > >
> > > Yeah, in git no file (object) is modified when you push or pull
> > > commits, they're just copied from one place to another.
> >
> > Yeah, sorry for not being fluent in git's parlance.
> >
> > > The committer date on the commit you mentioned is likely the
> > > time I updated the ChangeLog prior to pushing, so that date will
> > > be the "push date", but that's not guaranteed to be the case for
> > > all commits.
> >
> > Right.  Anyway, I still consider it is valid to include this info
> > (author and date) in the commit message.
> 
> For reference sake, what started this for me is noticing git blame
> shows the author date by default.
> 
> Not picking on Gary of course, his patch is just
> why I noticed this.
> If I do a git blame of server.c I see patch 860789c7 with a date of
> 2014-08-08.  That's three weeks before it was pushed upstream.
> Bleah.  I'd really like to be able to do a git blame and have what
> I see be useful, including the date.  The author date is basically
> useless to me.

I see some options to git-rebase, --committer-date-is-author-date and
--ignore-date.  I'll experiment with these the next time I rebase
something and see what happens.

It's a shame there's no global option you can set to make git log,
blame etc show the other date.

> I realize any changes to what goes into the commit log won't help
> solve this of course.  But that's what started the discussion for
> me.

FAOD for changelog-in-commit messages, I'm equally happy with the
format I used for 5ee44bfa (the current one described in the wiki)
or the one I used to use (eg e76df0d0).  I script all this anyway
so any format I can script is fine by me.

Thanks,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Gary Benson]

Gary Benson wrote:
> Doug Evans wrote:
> > If I do a git blame of server.c I see patch 860789c7 with a date
> > of 2014-08-08.  That's three weeks before it was pushed upstream.
> > Bleah.  I'd really like to be able to do a git blame and have what
> > I see be useful, including the date.  The author date is basically
> > useless to me.
> 
> I see some options to git-rebase, --committer-date-is-author-date
> and --ignore-date.  I'll experiment with these the next time I
> rebase something and see what happens.

Running "git rebase --ignore-date gdb/master" immediately prior to
pushing sets both author and commit dates to the present time on
all commits you are about to push.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Doug Evans]

On Fri, Sep 5, 2014 at 3:13 AM, Gary Benson <gbenson@redhat.com> wrote:
> Gary Benson wrote:
>> Doug Evans wrote:
>> > If I do a git blame of server.c I see patch 860789c7 with a date
>> > of 2014-08-08.  That's three weeks before it was pushed upstream.
>> > Bleah.  I'd really like to be able to do a git blame and have what
>> > I see be useful, including the date.  The author date is basically
>> > useless to me.
>>
>> I see some options to git-rebase, --committer-date-is-author-date
>> and --ignore-date.  I'll experiment with these the next time I
>> rebase something and see what happens.
>
> Running "git rebase --ignore-date gdb/master" immediately prior to
> pushing sets both author and commit dates to the present time on
> all commits you are about to push.

Cool.  Can you add that to the contribution checklist?

Re: ChangeLogs in commit messages

[Gary Benson]

Doug Evans wrote:
> On Fri, Sep 5, 2014 at 3:13 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Gary Benson wrote:
> > > Doug Evans wrote:
> > > > If I do a git blame of server.c I see patch 860789c7 with a
> > > > date of 2014-08-08.  That's three weeks before it was pushed
> > > > upstream.  Bleah.  I'd really like to be able to do a git
> > > > blame and have what I see be useful, including the date.  The
> > > > author date is basically useless to me.
> > >
> > > I see some options to git-rebase, --committer-date-is-author-date
> > > and --ignore-date.  I'll experiment with these the next time I
> > > rebase something and see what happens.
> >
> > Running "git rebase --ignore-date gdb/master" immediately prior to
> > pushing sets both author and commit dates to the present time on
> > all commits you are about to push.
> 
> Cool.  Can you add that to the contribution checklist?

https://sourceware.org/gdb/wiki/ContributionChecklist#Fixing_commit_dates

If we add a server-side hook to check that the ChangeLog messages in
the commit messages are correct we could also use that hook to check
that the dates in commits match the dates in the ChangeLog files.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[James Hogan]

On 08/09/14 10:50, Gary Benson wrote:
> Doug Evans wrote:
>> On Fri, Sep 5, 2014 at 3:13 AM, Gary Benson <gbenson@redhat.com> wrote:
>>> Gary Benson wrote:
>>>> Doug Evans wrote:
>>>>> If I do a git blame of server.c I see patch 860789c7 with a
>>>>> date of 2014-08-08.  That's three weeks before it was pushed
>>>>> upstream.  Bleah.  I'd really like to be able to do a git
>>>>> blame and have what I see be useful, including the date.  The
>>>>> author date is basically useless to me.
>>>>
>>>> I see some options to git-rebase, --committer-date-is-author-date
>>>> and --ignore-date.  I'll experiment with these the next time I
>>>> rebase something and see what happens.
>>>
>>> Running "git rebase --ignore-date gdb/master" immediately prior to
>>> pushing sets both author and commit dates to the present time on
>>> all commits you are about to push.
>>
>> Cool.  Can you add that to the contribution checklist?
> 
> https://sourceware.org/gdb/wiki/ContributionChecklist#Fixing_commit_dates
> 
> If we add a server-side hook to check that the ChangeLog messages in
> the commit messages are correct we could also use that hook to check
> that the dates in commits match the dates in the ChangeLog files.

IMHO this is a really bad idea.

You're suggesting discarding/falsifying genuine information (the
authorship dates) from the git history for no good reason since the
information you want to store is already there in the commit date.

1) "the commit dates are wrong". you probably mean the author dates are
unchanged, since the committer dates get reset by default whenever a
commit is recommitted (written, i.e. commit --amend, rebased, cherry
picked etc).

2) The commit date should reflect roughly what you want already. You
need to rebase if anybody else has pushed to the branch in order to keep
the history linear anyway which will reset the committer dates, which
also means they'll be in order as you'd expect.

The author date is supposed to reflect when the patch was authored, not
when it was committed/pushed (or in the case of git-send-email/git-am
it's the date it was sent to the mailing list).

FYI it allows you to:
* get information about author's habits (what hours they usually work)
and timezone, and when they were active in the project, separate from
the committer. E.g. github shows this information in it's contributions
section for each user, organised by day of the week.
* gives you a better idea of the real age of the patch.
* make it easier to distinguish which version of a patch was applied and
find it in the mailing list archives.
* allows you to see more easily how quickly patches are getting upstream

And all that regardless of whether it has been cherry picked onto a
stable branch at a later date, maybe even after the author has moved on
to other things (cherry-picking usually only changes the commit date).

Cheers
James

Re: ChangeLogs in commit messages

[James Hogan]

On 08/09/14 14:08, James Hogan wrote:
> On 08/09/14 10:50, Gary Benson wrote:
>> Doug Evans wrote:
>>> On Fri, Sep 5, 2014 at 3:13 AM, Gary Benson <gbenson@redhat.com> wrote:
>>>> Gary Benson wrote:
>>>>> Doug Evans wrote:
>>>>>> If I do a git blame of server.c I see patch 860789c7 with a
>>>>>> date of 2014-08-08.  That's three weeks before it was pushed
>>>>>> upstream.  Bleah.  I'd really like to be able to do a git
>>>>>> blame and have what I see be useful, including the date.  The
>>>>>> author date is basically useless to me.
>>>>>
>>>>> I see some options to git-rebase, --committer-date-is-author-date
>>>>> and --ignore-date.  I'll experiment with these the next time I
>>>>> rebase something and see what happens.
>>>>
>>>> Running "git rebase --ignore-date gdb/master" immediately prior to
>>>> pushing sets both author and commit dates to the present time on
>>>> all commits you are about to push.
>>>
>>> Cool.  Can you add that to the contribution checklist?
>>
>> https://sourceware.org/gdb/wiki/ContributionChecklist#Fixing_commit_dates
>>
>> If we add a server-side hook to check that the ChangeLog messages in
>> the commit messages are correct we could also use that hook to check
>> that the dates in commits match the dates in the ChangeLog files.
> 
> IMHO this is a really bad idea.

Just to clarify, I'm not referring to the adding of a hook, but the
rebase with --ignore-date. :-)

Cheers
James

Re: ChangeLogs in commit messages

[Joel Brobecker]

> IMHO this is a really bad idea.

I wouldn't say "a really bad", but I tend to agree with the reasoning.
It's a bit unnatural to me that git would chose to show the author
date by default over the commit date, but James is right that the
commit date is already there, and should largely reflect the date
when the commit was pushed. Thanks to our daily updates of the BFD
version, it's pretty much guaranteed that a contributor will always
have to rebase before being able to push.

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > IMHO this is a really bad idea.
> 
> I wouldn't say "a really bad", but I tend to agree with the
> reasoning.  It's a bit unnatural to me that git would chose to show
> the author date by default over the commit date, but James is right
> that the commit date is already there, and should largely reflect
> the date when the commit was pushed. Thanks to our daily updates of
> the BFD version, it's pretty much guaranteed that a contributor will
> always have to rebase before being able to push.

I don't really see the value of the author date in a rebase-based
workflow; by the time some series get committed I've folded tens if
not hundreds of commits into the "original" commits.  But I don't
really mind either way.  If people want people not to smush up the
dates then please remove the section I added to the wiki this morning.

Thanks,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[James Hogan]

On 08/09/14 14:24, Joel Brobecker wrote:
>> IMHO this is a really bad idea.
> 
> I wouldn't say "a really bad", but I tend to agree with the reasoning.
> It's a bit unnatural to me that git would chose to show the author
> date by default over the commit date

From the patch author point of view, cleaning up a patchset by
interactive rebasing, or rebasing to keep their branch up to date, I
think the author date probably is more natural, and in less linear
histories (look at Linux repo for example) I suspect the commit date
wouldn't be as meaningful.

FWIW though you can put:

[format]
	pretty=fuller

in your ~/.gitconfig to default to the fuller format for git-show and
git-log, which shows the committer and commit date. It'd be nice if
something similar was possible with git-blame.

You can add new pretty formats too, e.g.

[pretty]
	fullcommit = "commit %H\nAuthor:     %an <%ae>\nCommitDate: %cd\n\n%B"
[format]
	pretty=fullcommit

This shows author name, but commit date, but I didn't figure out how to
indent the body like pretty=fuller does.

Cheers
James

Re: ChangeLogs in commit messages

[Doug Evans]

On Tue, Sep 9, 2014 at 1:51 AM, James Hogan <james.hogan@imgtec.com> wrote:
> On 08/09/14 14:24, Joel Brobecker wrote:
>>> IMHO this is a really bad idea.
>>
>> I wouldn't say "a really bad", but I tend to agree with the reasoning.
>> It's a bit unnatural to me that git would chose to show the author
>> date by default over the commit date
>
> From the patch author point of view, cleaning up a patchset by
> interactive rebasing, or rebasing to keep their branch up to date, I
> think the author date probably is more natural, and in less linear
> histories (look at Linux repo for example) I suspect the commit date
> wouldn't be as meaningful.

Authors come and go, and, for me as an author, after six months I
would certainly prefer to see in the upstream tree a date as close as
possible to when the patch was pushed upstream.
[plus the needs of the many outweigh the needs of the few ... 1/2 :-)]

> [...] It'd be nice if
> something similar was possible with git-blame.

Indeed!

> You can add new pretty formats too, e.g.
>
> [pretty]
>         fullcommit = "commit %H\nAuthor:     %an <%ae>\nCommitDate: %cd\n\n%B"
> [format]
>         pretty=fullcommit
>
> This shows author name, but commit date, but I didn't figure out how to
> indent the body like pretty=fuller does.

Interesting, thanks!

Re: ChangeLogs in commit messages

[Pedro Alves]

On 08/14/2014 01:52 PM, Joel Brobecker wrote:
>>   1. With paths and with the date-and-author header:
>>
>>     gdb/
>>     2014-07-30  Gary Benson  <gbenson@redhat.com>
>>
>>             * btrace.c: Include defs.h.
>>             * common/ptid.c: Include defs.h or server.h as appropriate.
>>             * nat/mips-linux-watch.c: Likewise.
>>
>>   2. With date-and-author headers but no paths:
>>
>>     2014-08-04  Tom Tromey  <tromey@redhat.com>
>>       
>>             * gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
>>             "target_resume".
>>
>>   3. With paths but no date-and-author headers:
>>
>>     gdb/ChangeLog:
>>       
>>             * amd64-windows-tdep.c (amd64_windows_frame_decode_insns):
>>               Add debug trace.
>>
>>   4. With no preamble at all:
>>
>>             * chew.c (print_stack_level, main): Cast result of pointer
>>             difference to match format string.
>>
>> What are people's preferences here?  My preference is #1, but I could
>> live with #3.  If we come to some kind of concensus on this I'll
>> update the wiki to reflect this.
> 
> #3, since date and author are often redundant with the commit's
> author. And even if not in the same, it's in the ChangeLog entry
> that should be checked in as part of the commit. Also, I feel
> like having those in the CL is an extra source of potential
> issue (eg: if forgot to update the date), and revision logs
> cannot be fixed once the commit has been pushed, whereas dates
> in ChangeLog entries can.

Because on git, email == patch == commit, I've now noticed that we're
now actually losing author info in patch _submissions_ because of
this rule and the rule that we don't post the ChangeLog diff.

E.g., I suspect this patch:

  https://sourceware.org/ml/gdb-patches/2014-08/msg00650.html

will end up with both Gary's and Tom's name in the ChangeLog,
but that's really just a guess.

This makes it impossible for someone else to push the patch
other than the submitter, because not all the info is there.

It's a bigger issue even if someone posts a patch written or
co-written by someone that might not have copyright
assignment in place.

I think author info must be explicit in patch submissions
somehow.

Thanks,
Pedro Alves

Re: ChangeLogs in commit messages

[Doug Evans]

On Wed, Sep 10, 2014 at 3:58 AM, Pedro Alves <palves@redhat.com> wrote:
> [...]
>
> Because on git, email == patch == commit, I've now noticed that we're
> now actually losing author info in patch _submissions_ because of
> this rule and the rule that we don't post the ChangeLog diff.

Hmmm, that's a bug.

>
> E.g., I suspect this patch:
>
>   https://sourceware.org/ml/gdb-patches/2014-08/msg00650.html
>
> will end up with both Gary's and Tom's name in the ChangeLog,
> but that's really just a guess.
>
> This makes it impossible for someone else to push the patch
> other than the submitter, because not all the info is there.
>
> It's a bigger issue even if someone posts a patch written or
> co-written by someone that might not have copyright
> assignment in place.
>
> I think author info must be explicit in patch submissions
> somehow.

Agreed.

Re: ChangeLogs in commit messages

[Joel Brobecker]

> > E.g., I suspect this patch:
> >
> >   https://sourceware.org/ml/gdb-patches/2014-08/msg00650.html
> >
> > will end up with both Gary's and Tom's name in the ChangeLog,
> > but that's really just a guess.
> >
> > This makes it impossible for someone else to push the patch
> > other than the submitter, because not all the info is there.
> >
> > It's a bigger issue even if someone posts a patch written or
> > co-written by someone that might not have copyright
> > assignment in place.
> >
> > I think author info must be explicit in patch submissions
> > somehow.

That's probably the strongest argument in favor of putting
the full ChangeLog entry in the revision log. I'm just having
a hard time accepting the fact that we going to include a date
in the revision log which could be wrong because added by hand.
And once it's pushed, there is no going back, so no way to fix it.

Recognizing the fact that the majority of patches have one single
author who is also the submitter, perhaps we could use the no-date/
no-author format for those cases, and provide a way to specify
for those few times where necessary? Seems complicated, perhaps...

-- 
Joel

Re: ChangeLogs in commit messages

[Gary Benson]

Joel Brobecker wrote:
> > > E.g., I suspect this patch:
> > >
> > >   https://sourceware.org/ml/gdb-patches/2014-08/msg00650.html
> > >
> > > will end up with both Gary's and Tom's name in the ChangeLog,
> > > but that's really just a guess.
> > >
> > > This makes it impossible for someone else to push the patch
> > > other than the submitter, because not all the info is there.
> > >
> > > It's a bigger issue even if someone posts a patch written or
> > > co-written by someone that might not have copyright
> > > assignment in place.
> > >
> > > I think author info must be explicit in patch submissions
> > > somehow.
> 
> That's probably the strongest argument in favor of putting
> the full ChangeLog entry in the revision log. I'm just having
> a hard time accepting the fact that we going to include a date
> in the revision log which could be wrong because added by hand.
> And once it's pushed, there is no going back, so no way to fix it.
> 
> Recognizing the fact that the majority of patches have one single
> author who is also the submitter, perhaps we could use the no-date/
> no-author format for those cases, and provide a way to specify
> for those few times where necessary? Seems complicated, perhaps...

Optional authors lines below "path/to/ChangeLog:"?

  Introduce target/target.h

  This introduces target/target.h.  This file declares some functions
  that the shared code can use and that clients must implement.  It
  also changes some shared code to use these functions.

  gdb/ChangeLog:
    Tom Tromey <tromey@redhat.com>
    Gary Benson <gbenson@redhat.com>

          * target/target.h: New file.
          * Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
          * target.h: Include target/target.h.
          ...        
  
  gdb/gdbserver/ChangeLog:
    Tom Tromey <tromey@redhat.com>
    Gary Benson <gbenson@redhat.com>

          * target.h: Include target/target.h.
          * target.c (target_read_memory, target_read_uint32)
          ...

In terms of parsing, the "/ChangeLog:" marks the start of a ChangeLog
entry, and the blank line marks the end of any optional authors lines.
If no authors lines are present then the committer is the author.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Joel Brobecker]

> Optional authors lines below "path/to/ChangeLog:"?
> 
>   Introduce target/target.h
> 
>   This introduces target/target.h.  This file declares some functions
>   that the shared code can use and that clients must implement.  It
>   also changes some shared code to use these functions.
> 
>   gdb/ChangeLog:
>     Tom Tromey <tromey@redhat.com>
>     Gary Benson <gbenson@redhat.com>
> 
>           * target/target.h: New file.
>           * Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
>           * target.h: Include target/target.h.
>           ...        
>   
>   gdb/gdbserver/ChangeLog:
>     Tom Tromey <tromey@redhat.com>
>     Gary Benson <gbenson@redhat.com>
> 
>           * target.h: Include target/target.h.
>           * target.c (target_read_memory, target_read_uint32)
>           ...
> 
> In terms of parsing, the "/ChangeLog:" marks the start of a ChangeLog
> entry, and the blank line marks the end of any optional authors lines.
> If no authors lines are present then the committer is the author.

LGTM!

-- 
Joel

Re: ChangeLogs in commit messages

[Doug Evans]

On Mon, Sep 15, 2014 at 3:29 AM, Gary Benson <gbenson@redhat.com> wrote:
> Joel Brobecker wrote:
>> > > E.g., I suspect this patch:
>> > >
>> > >   https://sourceware.org/ml/gdb-patches/2014-08/msg00650.html
>> > >
>> > > will end up with both Gary's and Tom's name in the ChangeLog,
>> > > but that's really just a guess.
>> > >
>> > > This makes it impossible for someone else to push the patch
>> > > other than the submitter, because not all the info is there.
>> > >
>> > > It's a bigger issue even if someone posts a patch written or
>> > > co-written by someone that might not have copyright
>> > > assignment in place.
>> > >
>> > > I think author info must be explicit in patch submissions
>> > > somehow.
>>
>> That's probably the strongest argument in favor of putting
>> the full ChangeLog entry in the revision log. I'm just having
>> a hard time accepting the fact that we going to include a date
>> in the revision log which could be wrong because added by hand.
>> And once it's pushed, there is no going back, so no way to fix it.
>>
>> Recognizing the fact that the majority of patches have one single
>> author who is also the submitter, perhaps we could use the no-date/
>> no-author format for those cases, and provide a way to specify
>> for those few times where necessary? Seems complicated, perhaps...
>
> Optional authors lines below "path/to/ChangeLog:"?
>
>   Introduce target/target.h
>
>   This introduces target/target.h.  This file declares some functions
>   that the shared code can use and that clients must implement.  It
>   also changes some shared code to use these functions.
>
>   gdb/ChangeLog:
>     Tom Tromey <tromey@redhat.com>
>     Gary Benson <gbenson@redhat.com>
>
>           * target/target.h: New file.
>           * Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
>           * target.h: Include target/target.h.
>           ...
>
>   gdb/gdbserver/ChangeLog:
>     Tom Tromey <tromey@redhat.com>
>     Gary Benson <gbenson@redhat.com>
>
>           * target.h: Include target/target.h.
>           * target.c (target_read_memory, target_read_uint32)
>           ...
>
> In terms of parsing, the "/ChangeLog:" marks the start of a ChangeLog
> entry, and the blank line marks the end of any optional authors lines.
> If no authors lines are present then the committer is the author.

There's still something missing (IIUC).
One of the problems that needs to be solved is documenting the author
in the patch submission (the email that goes to the list).  The above
convention allows for a default where the absence of a name means
author == committer, but we're still not specifying an absolute
requirement that the patch author appears in the email sent to the
list.
Am I missing something?

Re: ChangeLogs in commit messages

[Joel Brobecker]

> There's still something missing (IIUC).
> One of the problems that needs to be solved is documenting the author
> in the patch submission (the email that goes to the list).  The above
> convention allows for a default where the absence of a name means
> author == committer, but we're still not specifying an absolute
> requirement that the patch author appears in the email sent to the
> list.
> Am I missing something?

I am not sure I understand what you are trying to say: You are now
establishing that there are 3 users, not 2. author, submitter, and
committer. Why do we need the submitter's name in the revision log?

-- 
Joel

Re: ChangeLogs in commit messages

[Doug Evans]

On Mon, Sep 15, 2014 at 9:05 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> There's still something missing (IIUC).
>> One of the problems that needs to be solved is documenting the author
>> in the patch submission (the email that goes to the list).  The above
>> convention allows for a default where the absence of a name means
>> author == committer, but we're still not specifying an absolute
>> requirement that the patch author appears in the email sent to the
>> list.
>> Am I missing something?
>
> I am not sure I understand what you are trying to say: You are now
> establishing that there are 3 users, not 2. author, submitter, and
> committer. Why do we need the submitter's name in the revision log?

We don't need the submitter's name in the revision log.
I was referring to the patch author appearing in the email
sent to the list.

Pedro wrote "I think author info must be explicit in patch submissions
somehow."
ref: https://sourceware.org/ml/gdb/2014-09/msg00038.html
Read the full text of 00038 for more context.

I agree.

The changes specified in
https://sourceware.org/ml/gdb/2014-09/msg00052.html
do not address this, yet in the text of that email (00052)
Pedro's comment is included (again, ref: 00052).

Therefore ISTM there is still something missing.
We still haven't solved the problem of providing a convention
where the author is explicitly specified in patch submissions.
00052 allows for a default where author == committer.
But on the patch submission side it's not clear to me
we want a default of patch author == patch submitter.

[btw, given that the "convention allows for a default where the absence
of names means author == committer" I'm ok with the proposed
change to the commit log since in general it won't mean more
manual effort.]

I'm just pointing out that, unless I'm missing something,
we still need to document a convention where patch
author is explicitly specified in patch submissions.

If I still haven't made myself clear let me know.
I'm happy to elaborate as necessary.

Re: ChangeLogs in commit messages

[Gary Benson]

Doug Evans wrote:
> On Mon, Sep 15, 2014 at 9:05 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> > > There's still something missing (IIUC).
> > > One of the problems that needs to be solved is documenting the
> > > author in the patch submission (the email that goes to the
> > > list).  The above convention allows for a default where the
> > > absence of a name means author == committer, but we're still not
> > > specifying an absolute requirement that the patch author appears
> > > in the email sent to the list.
> > > Am I missing something?
> >
> > I am not sure I understand what you are trying to say: You are now
> > establishing that there are 3 users, not 2. author, submitter, and
> > committer. Why do we need the submitter's name in the revision
> > log?
> 
> We don't need the submitter's name in the revision log.  I was
> referring to the patch author appearing in the email sent to the
> list.
> 
> Pedro wrote "I think author info must be explicit in patch submissions
> somehow."
> ref: https://sourceware.org/ml/gdb/2014-09/msg00038.html
> Read the full text of 00038 for more context.
> 
> I agree.
> 
> The changes specified in
> https://sourceware.org/ml/gdb/2014-09/msg00052.html
> do not address this, yet in the text of that email (00052)
> Pedro's comment is included (again, ref: 00052).

We inline the ChangeLog entries in the patch submission emails, eg
https://sourceware.org/ml/gdb-patches/2014-09/msg00286.html
so the optional author lines would be inlined right there.

Cheers,
Gary

-- 
http://gbenson.net/

Re: ChangeLogs in commit messages

[Doug Evans]

On Tue, Sep 16, 2014 at 2:31 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Mon, Sep 15, 2014 at 9:05 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> > > There's still something missing (IIUC).
>> > > One of the problems that needs to be solved is documenting the
>> > > author in the patch submission (the email that goes to the
>> > > list).  The above convention allows for a default where the
>> > > absence of a name means author == committer, but we're still not
>> > > specifying an absolute requirement that the patch author appears
>> > > in the email sent to the list.
>> > > Am I missing something?
>> >
>> > I am not sure I understand what you are trying to say: You are now
>> > establishing that there are 3 users, not 2. author, submitter, and
>> > committer. Why do we need the submitter's name in the revision
>> > log?
>>
>> We don't need the submitter's name in the revision log.  I was
>> referring to the patch author appearing in the email sent to the
>> list.
>>
>> Pedro wrote "I think author info must be explicit in patch submissions
>> somehow."
>> ref: https://sourceware.org/ml/gdb/2014-09/msg00038.html
>> Read the full text of 00038 for more context.
>>
>> I agree.
>>
>> The changes specified in
>> https://sourceware.org/ml/gdb/2014-09/msg00052.html
>> do not address this, yet in the text of that email (00052)
>> Pedro's comment is included (again, ref: 00052).
>
> We inline the ChangeLog entries in the patch submission emails, eg
> https://sourceware.org/ml/gdb-patches/2014-09/msg00286.html
> so the optional author lines would be inlined right there.

I don't understand.
The ChangeLog diff is not included in the "patch" in 00286 and unless
things have changed recently we still discourage that.  So for the
nonce I'm going to assume you're not talking about adding ChangeLog
diffs to patches (though that would solve the problem of making the
author explicit - I'm not suggesting this as a solution though).

I guess I still don't see how this proposed change makes author
specification *explicit* in patch submissions, unless you are also
saying that the absence of author info means the patch submitter is
the author.
This might be ok, but that's not a definition of "explicit" that I've
been employing here.
And it is a departure from how patches have been submitted in the past
where the full changelog entry is always cut-n-pasted to the top of
the patch and it includes author info.
Has your definition of the word "explicit" been different than mine?

Re: GDB bugs

[Pedro Alves]

On 09/19/2014 04:24 AM, 赂脽鹿煤脢陇 wrote:
> Hi,
> I found that, for some targets like Jlink, can not report hardware watchpoint in RSP packet(can not report "Twatch"), how GDB to support this. 

Sorry, I don't understand this question.

> 
> By the way, it looks like that GDB can not stopped by user (ctrl + C)
> When free run.
> And this is caused by the commit "329ea57934a9d4b250a0b417af1ec47bc2d0ceb6"
>  /* Controls if targets can report that they can/are async.  This is
>    just for maintainers to use when debugging gdb.  */
> int target_async_permitted = 0;//this default value must 0.

If this is on Windows, then I recall that were subsequent fixes for a bug like that.

Thanks,
Pedro Alves