Re: [PATCH 1/2] hooks: replace irrelevant hook sample

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2017, 18:27 UTC
Message-ID: <xmqqinj4ayqb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170707160740.9748-1-kaarticsivaraam91196@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 14 quoted lines

> The pre-commit-msg hook sample has an example that comments
> the "Conflicts:" part of the merge commmit. It isn't relevant
> anymore as it's done by default since 261f315b ("merge & sequencer:
> turn "Conflicts:" hint into a comment", 2014-08-28).
>
> Add an alternative example that replaces it. This ensures there's
> at the least a simple example that illustrates what could be done
> using the hook just by enabling it.
>
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  I have made the second patch depend on the first one to avoid
>  conflicts that may occur. Further, it was meaningful to join
>  the two patches as they would go together (or) not at all.

Whether the two samples these patches implement are useful ones or not, the early part of 2/2 that starts using named variables instead of positional ones is an improvement. It makes it easier to see what is going on.

Patch 2/2 as posted forgets to update some references to $1, $2 and $3, both in the actual code and also commented out part, e.g.

    sed -e ... "$COMMIT_MSG_FILE" >"$SED_OUTPUT_TEMP" && mv "$SED_OUTPUT_TEMP" "$1"
    # case "$2,$3" in

In the first one, $COMMIT_MSG_FILE and $1 refer to the same thing; they should be consistent.

If these updates were to be done as multiple patches, the change to use named not positional variables, without changing anything else, should come second, I think, after the one that simply removes the now-useless "Conflicts:" sample from the script, as a preparatory clean-up step. You can build other things like hints-removal and sign-off with interpret-trailers on top of these two steps.

Have you considered using "@PERL_PATH@ -i" instead of "sed" in this step, by the way? That would allow you not to worry about the temporary file left behind (e.g. what happens when somebody else runs this in a shared repository setting, creating and leaving the temporary file that you may not be able to write into later because her umask is tighter, and then you try to make a commit).

Thanks.

Show 42 quoted lines

>  templates/hooks--prepare-commit-msg.sample | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
> index 86b8f227e..5a638ebda 100755
> --- a/templates/hooks--prepare-commit-msg.sample
> +++ b/templates/hooks--prepare-commit-msg.sample
> @@ -9,8 +9,9 @@
>  #
>  # To enable this hook, rename this file to "prepare-commit-msg".
>  
> -# This hook includes three examples.  The first comments out the
> -# "Conflicts:" part of a merge commit.
> +# This hook includes three examples.  The first one removes three
> +# comment lines starting from the line that has the words
> +# "# Please enter the" in it's beginning.
>  #
>  # The second includes the output of "git diff --name-status -r"
>  # into the message, just before the "git status" output.  It is
> @@ -20,17 +21,16 @@
>  # 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
> -  merge,)
> -    @PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' "$1" ;;
> +sed -e '/^. Please enter the commit message /,/^#$/d' "$1" >'.sed-output.temp' && mv '.sed-output.temp' "$1"
>  
> +# case "$2,$3" in
>  # ,|template,)
>  #   @PERL_PATH@ -i.bak -pe '
>  #      print "\n" . `git diff --cached --name-status -r`
>  #	 if /^#/ && $first++ == 0' "$1" ;;
> -
> -  *) ;;
> -esac
> +#
> +#  *) ;;
> +# esac
>  
>  # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
>  # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"