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

[Junio C Hamano <gitster@pobox.com>]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> 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.

>  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"