Re: [PATCH v3] mklog: handle Signed-off-by, minor cleanup

[Richard Sandiford <richard.sandiford@arm.com>]

Marc Poulhiès via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> +# this regex matches the first line of the "end" in the initial commit message
>>> +FIRST_LINE_OF_END_RE = re.compile('(?i)^(signed-off-by|co-authored-by|#): ')
>>
>> The current code only requires an initial "#", rather than an initial "#: ".
>> Is that a deliberate change?
>>
>> The patch LGTM apart from that.
>
> Hello Richard,
>
> Thanks for the review and sorry for the delayed answer as I was away the
> past weeks. This issue was catched early this month
> (https://github.com/Rust-GCC/gccrs/pull/2504), but I didn't want to send
> something here before leaving. Here's a fixed patched.
>
> Ok for master?
>
> Thanks,
> Marc
>
> ---
>  contrib/mklog.py           | 34 +++++++++++++++++++++++++++++-----
>  contrib/prepare-commit-msg | 20 ++++++++++----------
>  2 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/contrib/mklog.py b/contrib/mklog.py
> index 26230b9b4f2..496780883fb 100755
> --- a/contrib/mklog.py
> +++ b/contrib/mklog.py
> @@ -41,7 +41,34 @@ from unidiff import PatchSet
>  
>  LINE_LIMIT = 100
>  TAB_WIDTH = 8
> -CO_AUTHORED_BY_PREFIX = 'co-authored-by: '
> +
> +# Initial commit:
> +#   +--------------------------------------------------+
> +#   | gccrs: Some title                                |
> +#   |                                                  | This is the "start"
> +#   | This is some text explaining the commit.         |
> +#   | There can be several lines.                      |
> +#   |                                                  |<------------------->
> +#   | Signed-off-by: My Name <my@mail.com>             | This is the "end"
> +#   +--------------------------------------------------+
> +#
> +# Results in:
> +#   +--------------------------------------------------+
> +#   | gccrs: Some title                                |
> +#   |                                                  |
> +#   | This is some text explaining the commit.         | This is the "start"
> +#   | There can be several lines.                      |
> +#   |                                                  |<------------------->
> +#   | gcc/rust/ChangeLog:                              |
> +#   |                                                  | This is the generated
> +#   |         * some_file (bla):                       | ChangeLog part
> +#   |         (foo):                                   |
> +#   |                                                  |<------------------->
> +#   | Signed-off-by: My Name <my@mail.com>             | This is the "end"
> +#   +--------------------------------------------------+
> +
> +# this regex matches the first line of the "end" in the initial commit message
> +FIRST_LINE_OF_END_RE = re.compile('(?i)^(signed-off-by:|co-authored-by:|#) ')

Personally I think it would be safer to drop the final space in the regexp.

OK with that change if you agree.

Thanks,
Richard

>  
>  pr_regex = re.compile(r'(\/(\/|\*)|[Cc*!])\s+(?P<pr>PR [a-z+-]+\/[0-9]+)')
>  prnum_regex = re.compile(r'PR (?P<comp>[a-z+-]+)/(?P<num>[0-9]+)')
> @@ -330,10 +357,7 @@ def update_copyright(data):
>  
>  
>  def skip_line_in_changelog(line):
> -    if line.lower().startswith(CO_AUTHORED_BY_PREFIX) or line.startswith('#'):
> -        return False
> -    return True
> -
> +    return FIRST_LINE_OF_END_RE.match(line) == None
>  
>  if __name__ == '__main__':
>      extra_args = os.getenv('GCC_MKLOG_ARGS')
> diff --git a/contrib/prepare-commit-msg b/contrib/prepare-commit-msg
> index 48c9dad3c6f..1e94706ba40 100755
> --- a/contrib/prepare-commit-msg
> +++ b/contrib/prepare-commit-msg
> @@ -32,11 +32,11 @@ if ! [ -f "$COMMIT_MSG_FILE" ]; then exit 0; fi
>  # Don't do anything unless requested to.
>  if [ -z "$GCC_FORCE_MKLOG" ]; then exit 0; fi
>  
> -if [ -z "$COMMIT_SOURCE" ] || [ $COMMIT_SOURCE = template ]; then
> +if [ -z "$COMMIT_SOURCE" ] || [ "$COMMIT_SOURCE" = template ]; then
>      # No source or "template" means new commit.
>      cmd="diff --cached"
>  
> -elif [ $COMMIT_SOURCE = message ]; then
> +elif [ "$COMMIT_SOURCE" = message ]; then
>      # "message" means -m; assume a new commit if there are any changes staged.
>      if ! git diff --cached --quiet; then
>  	cmd="diff --cached"
> @@ -44,23 +44,23 @@ elif [ $COMMIT_SOURCE = message ]; then
>  	cmd="diff --cached HEAD^"
>      fi
>  
> -elif [ $COMMIT_SOURCE = commit ]; then
> +elif [ "$COMMIT_SOURCE" = commit ]; then
>      # The message of an existing commit.  If it's HEAD, assume --amend;
>      # otherwise, assume a new commit with -C.
> -    if [ $SHA1 = HEAD ]; then
> +    if [ "$SHA1" = HEAD ]; then
>  	cmd="diff --cached HEAD^"
>  	if [ "$(git config gcc-config.mklog-hook-type)" = "smart-amend" ]; then
>  	    # Check if the existing message still describes the staged changes.
>  	    f=$(mktemp /tmp/git-commit.XXXXXX) || exit 1
> -	    git log -1 --pretty=email HEAD > $f
> -	    printf '\n---\n\n' >> $f
> -	    git $cmd >> $f
> +	    git log -1 --pretty=email HEAD > "$f"
> +	    printf '\n---\n\n' >> "$f"
> +	    git $cmd >> "$f"
>  	    if contrib/gcc-changelog/git_email.py "$f" >/dev/null 2>&1; then
>  		# Existing commit message is still OK for amended commit.
> -		rm $f
> +		rm "$f"
>  		exit 0
>  	    fi
> -	    rm $f
> +	    rm "$f"
>  	fi
>      else
>  	cmd="diff --cached"
> @@ -72,7 +72,7 @@ fi
>  
>  # Save diff to a file if requested.
>  DIFF_FILE=$(git config gcc-config.diff-file)
> -if ! [ -z "$DIFF_FILE" ]; then
> +if [ -n "$DIFF_FILE" ]; then
>      tee="tee $DIFF_FILE"
>  else
>      tee="cat"