Re: RFC: using AdaCore's git hooks for binutils-gdb.git ...

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Nov 28, 2014 at 5:57 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
> I would like to propose the use of AdaCore's git hooks scripts
> for handling everything that happens when updates are pushed
> to sourceware's binutils-gdb.git repository.
>
> While the scripts currently in place are serving us well, I think
> there are a number of features I'd like to have available:
>
>   - Send one email per commit, rather than one per push
>     (and have the subject of the email be the subject of
>     the commit)

I like it.

>   - Allow the use of specific policies on a per-branch basis
>     (ie - reject merges in release branches, for instance).
>
> Our scripts also correctly handle new references (new tags, new
> branches), as well as git notes. It is also Gerrit-ready,

Does it handle removing a branch

# git push origin :foo

I used to get some extra messages.

> although my understanding is that there are no plans to use it
> within either project.
>
> Of interest, in terms of features:
>
>   - Allow the use of pre-commit checking scripts that can verify
>     both files being modified, as well as the commit's revision log;
>
>   - Extensive configurability through a git-config-type file
>     which is itself under configuration management, and therefore
>     adjustable by all contributors, not just the admins with
>     login access to sourceware.
>
>     I've made a copy of our Users' Guide available at:
>     https://sourceware.org/gdb/wiki/proposed/git-hooks/UsersGuide
>
>   - A "diff" of the changes is also included in the commit-email.
>
>   - It disallows non-fast-forward changes (Eg. rebases) by default,
>     but gives the option to allow it on designated branched.
>
> The scripts are written in Python, and are pretty fast for typical
> pushes.
>
> There is a testsuite with 100% code coverage (some lines excluded,
> but those are very rare and related to sending emails, which we
> do not want to do during testing). We have been using them at
> AdaCore for a few years now, with a very low number of issues
> being reported, thanks to the fairly extensive testsuite.
>
> If people are interested in switching over to those scripts,
> there is still a bit of work for me to do:
>
>   1. Make the scripts publicly available - I will probably use
>      something like GitHub, or the open-do source forge.
>
>   2. Some adjustements will be needed in order to accomodate
>      the peculiarities of the binutils-gdb.git repo - the one
>      that comes to mind is the shared nature, with 2 mailing-lists
>      instead of one for the entire project. This should be fairly
>      easy to add by allowing the "mailinglist" configuration to
>      provide a script instead of a list of email addresses.
>
>   3. Some other adjustments to remove the use of an AdaCore Python
>      library, which is publicly available, but not necessarly
>      widely deployed.
>      https://forge.open-do.org/projects/gnatpython
>
>      The testsuite is using gnatpython as the testsuite back-bone,
>      so running it will continue requiring it.
>
>      In the same registry, sourceware's version of Python is slightly
>      older, so that may force us to make some adjustments as well.
>
> This is why I am feeling the waters before going ahead. If people
> prefer staying with the current scripts, then I won't bother!
>
> Lastly, a few example of the emails you would get:
>
> | Date: Fri, 14 Nov 2014 13:29:21 +0100 (CET)
> | From: Joel Brobecker <brobecke@adacore.com>
> | Subject: [binutils-gdb] Fix tiny GCS violatiton in
> |         ada_is_redundant_range_encoding.
> | To: [...]
> |
> | commit 246f3fa961a779b90bebd27d4c9e426f3c76c003
> | Author: Joel Brobecker <brobecker@adacore.com>
> | Date:   Fri Nov 14 16:28:26 2014 +0400
> |
> |     Fix tiny GCS violatiton in ada_is_redundant_range_encoding.
> |
> |     gdb/ChangeLog:
> |
> |             * ada-lang.c (ada_is_redundant_range_encoding): Add missing
> |             second space at end of sentence in comment.
> |
> | Diff:
> | ---
> |  gdb/ada-lang.c | 2 +-
> |  1 file changed, 1 insertion(+), 1 deletion(-)
> |
> | diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> | index ab4e0a3..e8f004e 100644
> | --- a/gdb/ada-lang.c
> | +++ b/gdb/ada-lang.c
> | @@ -8672,7 +8672,7 @@ ada_is_redundant_range_encoding (struct type *range_type,
> |    if (bounds_str == NULL)
> |      return 0;
> |
> | -  n = 8; /* Skip "___XDLU_". */
> | +  n = 8; /* Skip "___XDLU_".  */
> |    if (!ada_scan_number (bounds_str, n, &lo, &n))
> |      return 0;
> |    if (TYPE_LOW_BOUND (range_type) != lo)
>
> There isn't much to say in addition to the above, except maybe
> describe a bit the start of the email's subject. It is meant to
> indicate the name of the repository and the branch being updated.
> The hooks provide that info using the following syntax....

Do we need to show the whole diff in the commit email?  I think
a commit URL should be sufficient.  Binutils-gdb commit
can have a very large commit diff due to generated files.

>     [name-of-repo/name-of-branch]
>
> ... where "/name-of-branch" gets ommitted for branch master
> (which is just a convention). So, for instance commits to master
> would send emails where the subject's prefix would be as in the
> example above, whereas subject for commmits pushed to the
> gdb-7.8-branch, for instance, would use [binutils-gdb/gdb-7.8-branch]
> as the prefix instead.
>
> When new tags get created, an email notifying us of its creating
> will look like the following:
>
> | From: Test Suite <testsuite@example.com>
> | To: testsuite@example.com
> | Bcc: file-ci@gnat.com
> | Subject: [repo] Created tag v0.1
> | X-Act-Checkin: repo
> | X-Git-Author: Test Suite <testsuite@example.com>
> | X-Git-Refname: refs/tags/v0.1
> | X-Git-Oldrev: 0000000000000000000000000000000000000000
> | X-Git-Newrev: c4c1e91cddc3d48a2aab7d454bc6537149f37dd8
> |
> | The unsigned tag 'v0.1' was created pointing to:
> |
> |  8b9a0d6... New file: a.
> |
> | Tagger: Joel Brobecker <brobecker@adacore.com>
> | Date: Tue Jun 26 07:51:14 2012 -0700
> |
> |     This is a new tag.
>
> This is from the testsuite, and shows something interesting
> I haven't mentioned before, which is the use of private email
> headers "X-Act-Checkin:" (name of repo), as well as "X-Git-..."
> which provide some easily usable info for email processing/sorting.
>
> When creating a branch, a first email notifying us of the branch
> creation will be sent, and looks like this:
>
> | From: Test Suite <testsuite@adacore.com>
> | To: git-hooks-ci@example.com
> | Subject: [repo] Created branch 'release-0.1-branch'
> | X-Act-Checkin: repo
> | X-Git-Author: Test Suite <testsuite@adacore.com>
> | X-Git-Refname: refs/heads/release-0.1-branch
> | X-Git-Oldrev: 0000000000000000000000000000000000000000
> | X-Git-Newrev: dcc477c258baf8cf59db378fcc344dc962ad9a29
> |
> | The branch 'release-0.1-branch' was created pointing to:
> |
> |  dcc477c... New file b, add reference to it from file a.
>
> If this branch brings new commits (ie, commits that did not exist
> in any pre-existing branch), then individual emails will be sent
> for each of these new commits, just as we do when updating a branch.
>
> Merges are also handled. As always, individual commit emails
> are only sent for commits that are new in the repository.
> If the merge brings in some commits that were already part
> of another branch, a "cover email" gets sent listing all
> the commits being added, with a "(*)" telling us that no email
> will be sent for those commits. Here is an example of such
> email:
>
> | From: Test Suite <testsuite@adacore.com>
> | To: git-hooks-ci@example.com
> | Subject: [repo] (3 commits) Merge topic branch fsf-head.
> | X-Act-Checkin: repo
> | X-Git-Author: Test Suite <testsuite@adacore.com>
> | X-Git-Refname: refs/heads/master
> | X-Git-Oldrev: 33e7556e39b638aa07f769bd894e75ed1af490dc
> | X-Git-Newrev: ffb05b4a606fdb7b2919b209c725fe3b71880c00
> |
> | The branch 'master' was updated to point to:
> |
> |  ffb05b4... Merge topic branch fsf-head.
> |
> | It previously pointed to:
> |
> |  33e7556... Add new file b.
> |
> | Diff:
> |
> | Summary of changes (added commits):
> | -----------------------------------
> |
> |   ffb05b4... Merge topic branch fsf-head.
> |   b4bfa84... New file `c', update README accordingly. (*)
> |   6d62250... New file README. Update a. (*)
> |
> | (*) This commit exists in a branch whose name matches
> |     the hooks.noemail config option. No separate email
> |     sent.
> |
> | commit ffb05b4a606fdb7b2919b209c725fe3b71880c00
> | Merge: 33e7556 b4bfa84
> | Author: Joel Brobecker <brobecker@adacore.com>
> | Date:   Thu Dec 20 13:50:25 2012 +0400
> |
> |     Merge topic branch fsf-head.
> |
> |     ChangeLog:
> |
> |             * a: Add stuff.
> |             * c: New file.
> |             * README: New file.
> |
> | commit b4bfa84ef414162de60ff93005c5528f68b4c755
> | Author: Joel Brobecker <brobecker@adacore.com>
> | Date:   Thu Dec 20 13:41:24 2012 +0400
> |
> |     New file `c', update README accordingly.
> |
> |     README new refers to file `c'.
> |     ChangeLog:
> |
> |             * c: New file.
> |             * README: Add reference to new file `c'.
> |
> | commit 6d62250fdaed631cb170c0fc19c338accdba14ec
> | Author: Joel Brobecker <brobecker@adacore.com>
> | Date:   Thu Dec 20 13:40:33 2012 +0400
> |
> |     New file README. Update a.
> |
> |     Some revision history info.
>
> There are a number of other examples I could show, but I believe
> the above should illustrate the majority of our use.
>
> Before I ask what you guys think about this, I would like to quickly
> add one thing: let's not allow the discussion to drag into details.
> While I don't intend to use a "use it or leave it" attitude, I do not
> want the discussion to get bogged down because of enhancement
> suggestions and requests. If something is blocking the adoption
> of those scripts, then OK. Otherwise, please let's focus on getting
> those into production, AND THEN look at possible improvements.
> By then, access to the repo should be avaialble, and we can then
> adopt a free and open approach towards its maintenance [1].
>
> --
> Joel
>
> [1]: I only have a few days allocated each year for working on
>      those hooks.
> --
> Joel



-- 
H.J.