Re: [PATCH] hooks: add signature using "interpret-trailers"

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2017, 19:37 UTC
Message-ID: <xmqqvan6hdzh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170705173508.28711-1-kaarticsivaraam91196@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 8 quoted lines

> The sample hook to prepare the commit message before
> a commit allows users to opt-in to add the signature
> to the commit message. The signature is added at a place
> that isn't consistent with the "-s" option of "git commit".
> Further, it could go out of view in certain cases.
>
> Add the signature in a way similar to "-s" option of
> "git commit" using git's interpret-trailers command.

OK.

> It works well in all cases except when the user invokes
> "git commit" without any arguments. In that case manually
> add a new line after the first line to ensure it's consistent
> with the output of "-s" option.

OK (but its execution can be simplified).

> While at it, name the input parameters to improve readability
> of script.

Good.

Show 23 quoted lines

>
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  Added a close quote that got missed in the previous patch.
>
>  templates/hooks--prepare-commit-msg.sample | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
> index 86b8f227e..4ddab7896 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -20,17 +20,28 @@
>  # The third example adds a Signed-off-by line to the message, that can
>  # still be edited.  This is rarely a good idea.
>  
> -case "$2,$3" in
> +COMMIT_MSG_FILE=$1
> +COMMIT_SOURCE=$2
> +SHA1=$3
> +NEW_LINE='\
> +'
> +SED_TEMP_FILE='.sed-output.temp'

Move the latter two variable definitions to where they matter, i.e. the commented-out part that computes and adds $SOB.

Show 19 quoted lines

> +case "$COMMIT_SOURCE,$SHA1" in
>    merge,)
> -    @PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' "$1" ;;
> +    @PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' "$COMMIT_MSG_FILE" ;;
>  
>  # ,|template,)
>  #   @PERL_PATH@ -i.bak -pe '
>  #      print "\n" . `git diff --cached --name-status -r`
> -#	 if /^#/ && $first++ == 0' "$1" ;;
> +#	 if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
>  
>    *) ;;
>  esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
> -# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
> +# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
> +# if [ -z "$COMMIT_SOURCE" ]
> +# then

In our codebase (see Documentation/CodingGuidelines) we tend to prefer "test" over "[". I.e.

	if test -z "$COMMIT_SOURCE"
	then

> +#  sed -e "1i$NEW_LINE" "$COMMIT_MSG_FILE" >"$SED_TEMP_FILE" && mv "$SED_TEMP_FILE" "$COMMIT_MSG_FILE"

This looks like a more complex way to say

	{
		echo 
		cat "$COMMIT_MSG_FILE"
	} >"$SED_TEMP_FILE" &&
	mv "$SED_TEMP_FILE" "$COMMIT_MSG_FILE"

I wondered if it is safe to use a single hardcoded "$SED_TEMP_FILE", but I think it is OK.

> +# fi

Thanks.