Whitespace at the start of first line of commit

Whitespace at the start of first line of commit

[Jakub Jelinek]

Hi!

I've noticed that a couple of Jason's commits show up in gcc-cvs
in mutt as:
[gcc r10-5937] ?PR c++/92582 - ICE with member template as requirement.
The ? in there comes from a tab character, the full subject is like
Subject: =?utf-8?q?=5Bgcc_r10-5937=5D_=09PR_c++/92582_-_ICE_with_member_template_a?=
 =?utf-8?q?s_requirement=2E?=

One possibility to deal with this is:
--- hooks/updates/__init__.py	2020-01-12 22:30:37.143193572 +0100
+++ hooks/updates/__init__.py	2020-01-14 11:20:05.746749843 +0100
@@ -315,7 +315,7 @@ class AbstractUpdate(object):
         subject = '[%(repo)s%(branch)s] %(subject)s' % {
             'repo': self.email_info.project_name,
             'branch': branch,
-            'subject': commit.subject[:SUBJECT_MAX_SUBJECT_CHARS],
+            'subject': commit.subject[:SUBJECT_MAX_SUBJECT_CHARS].strip (),
             }
 
         # Generate the body of the email in two pieces:
(untested), another, suggested by Richard on IRC, would be to reject
commits where the first line starts with whitespace.

So, what do we want to do here?

	Jakub

Re: Whitespace at the start of first line of commit

[Joseph Myers]

On Tue, 14 Jan 2020, Jakub Jelinek wrote:

> (untested), another, suggested by Richard on IRC, would be to reject
> commits where the first line starts with whitespace.

I'd suggest making the hooks reject whitespace at the start of the first 
line of the commit message.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Whitespace at the start of first line of commit

[Joseph Myers]

On Tue, 14 Jan 2020, Joseph Myers wrote:

> On Tue, 14 Jan 2020, Jakub Jelinek wrote:
> 
> > (untested), another, suggested by Richard on IRC, would be to reject
> > commits where the first line starts with whitespace.
> 
> I'd suggest making the hooks reject whitespace at the start of the first 
> line of the commit message.

Unfortunately, that's not as simple to implement as I'd hoped, because 
git.py:git_run removes all leading and trailing whitespace from the output 
of the git command it runs, so the code checking commit messages can't 
tell whether there was whitespace at the start of the first line (or the 
end of the last line, I suppose) at all.  I don't know what git commands 
the hooks are using that might depend on removing leading whitespace (no 
doubt the removal of trailing whitespace is needed in various places to 
remove a final newline output by commands giving single-line output).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Whitespace at the start of first line of commit

[Joel Brobecker]

> Unfortunately, that's not as simple to implement as I'd hoped, because 
> git.py:git_run removes all leading and trailing whitespace from the output 
> of the git command it runs, so the code checking commit messages can't 
> tell whether there was whitespace at the start of the first line (or the 
> end of the last line, I suppose) at all.  I don't know what git commands 
> the hooks are using that might depend on removing leading whitespace (no 
> doubt the removal of trailing whitespace is needed in various places to 
> remove a final newline output by commands giving single-line output).

A quick run of the testsuite reveals that this assumption is made
all over. I am opposed to having this feature be a standard feature
of the git-hooks, so you wouldn't have to add an ad hoc check.
The way I would do it is by enhancing the git_run function to check
for a parameter named "_no_strip" and block the strip() operation
when passed as True. You can then implement your check using the usual
git interface, with the extra "_no_strip=True" parameter.

Do open a GitHub issue if you'd like me to add this check to
the git-hooks. I will likely give it less priority because
you'll have a way to implement it on your on, but I will get to it
eventually.

-- 
Joel

Re: Whitespace at the start of first line of commit

[Joel Brobecker]

> A quick run of the testsuite reveals that this assumption is made
> all over. I am opposed to having this feature be a standard feature
> of the git-hooks, so you wouldn't have to add an ad hoc check.
> The way I would do it is by enhancing the git_run function to check
> for a parameter named "_no_strip" and block the strip() operation
> when passed as True. You can then implement your check using the usual
> git interface, with the extra "_no_strip=True" parameter.
> 
> Do open a GitHub issue if you'd like me to add this check to
> the git-hooks. I will likely give it less priority because
> you'll have a way to implement it on your on, but I will get to it
> eventually.

I apologize.  Re-reading myself, I'm mixing various issues and not being
clear as a result, let me rephrase:

   For your immediate need, I am proposing a way to adapt the git_run
   function so you can use it to implement your check. Another option
   is to use Python's subprocess module directly, but I think this will
   be more work than what I suggest.

   For the longer term, you'll have access to a hook that you can
   use to call a script to validate an update. In that hook, you'll
   have total implementation freedom, so you can implement that check
   there, and not have to worry about that implementation detail in
   the git-hooks.

   Also longer term, I can add that check directly in the git-hooks
   as well. I don't remember anyone ever really making this kind
   of mistake, but it doesn't mean that it's unlikely either, nor
   that it would be specific to the GCC community. Hence the merit
   of having it in git-hooks directly.

-- 
Joel

Re: Whitespace at the start of first line of commit

[Joseph Myers]

On Fri, 17 Jan 2020, Joel Brobecker wrote:

> Do open a GitHub issue if you'd like me to add this check to
> the git-hooks. I will likely give it less priority because
> you'll have a way to implement it on your on, but I will get to it
> eventually.

I think https://github.com/AdaCore/git-hooks/issues/7 generally covers all 
the things we've come up with regarding custom checks *on commit 
messages*, both those that are clearly GCC-specific (From-SVN:) and those 
that are more generic for projects where people formerly used ChangeLog 
content as their commit messages (rejecting a first line looking like a 
ChangeLog header, rejecting a first line starting with whitespace, 
rejecting a single-word first line).

-- 
Joseph S. Myers
joseph@codesourcery.com