Re: [PATCH v1] hook: add sample template for push-to-checkout

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

Adam Spiers <git@adamspiers.org> writes:

> The template is a more-or-less exact translation to shell of the C
> code for the default behaviour for git's push-to-checkout hook defined
> in the push_to_deploy() function in builtin/receive-pack.c, to serve
> as a convenient starting point for modification.
>
> It also contains relevant text extracted from the git-config(1) and
> githooks(5) man pages.
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  templates/hooks--push-to-checkout.sample | 74 ++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100755 templates/hooks--push-to-checkout.sample
>
> diff --git a/templates/hooks--push-to-checkout.sample b/templates/hooks--push-to-checkout.sample
> new file mode 100755
> index 0000000000..2c6e06f8f1
> --- /dev/null
> +++ b/templates/hooks--push-to-checkout.sample
> @@ -0,0 +1,74 @@
> +#!/bin/bash

If we want to make this part of the sample hooks shown to everybody,
we should stick to /bin/sh if we could.  Do you have to rely on any
bash-ism that are not found in other shells to write this script, or
is this just shows your inertia that you always work with bash?

> +# The hook receives the commit with which the tip of the current
> +# branch is going to be updated:
> +commit="$1"

Strictly speaking, a parameter on the right hand side of an
assignment does not have to get dquoted to protect it from getting
munged at $IFS, so this can be

	commit=$1

but it gives us an important clue.  If $1 is worth protecting from
getting munged at $IFS, then we should assume later use of $commit
must be careful the same way.

> +#echo "push-to-checkout $commit"

Leftover debugging statement to be removed (instead of commented
out).

> +# It can exit with a non-zero status to refuse the push (when it does
> +# so, it must not modify the index or the working tree).
> +die () {
> +    echo >&2 "$*"
> +    exit 1
> +}

Style (Documentation/CodingGudielines).  One level of indent is a
single tab byte in our codebase.


> +if ! git update-index -q --ignore-submodules --refresh; then

Style (Documentation/CodingGudielines).

> +    die "Up-to-date check failed"

This is "failed to refresh" (in other words, not being up-to-date is
OK and that is not what we are checking), but the patch is meant as
a faithful translation of the original, so it is OK here.

> +fi
> +
> +if ! git diff-files --quiet --ignore-submodules --; then
> +    die "Working directory has unstaged changes"
> +fi
> +
> +# This is a rough translation of:
> +#
> +#    head_has_history() ? "HEAD" : EMPTY_TREE_SHA1_HEX
> +if git cat-file -t HEAD >&/dev/null; then

Is ">&/dev/null" portable?  I'd rather see it written like

	git cat-file -t HEAD >/dev/null 2>&1

to be portable.

cf. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07 

But there is a better way to spell it (see below).

> +    head=HEAD
> +else
> +    head=4b825dc642cb6eb9a060e54bf8d69288fbee4904

This hardcodes the object name from SHA-1 world, and will break
after the user migrates to SHA-256.

> +fi

Here is probably a better version.

	if git cat-file -e HEAD
	then
		head=HEAD
	else
		head=$(git hash-object -t tree --stdin </dev/null)
	fi




> +
> +if ! git diff-index --quiet --cached --ignore-submodules $head --; then
> +    die "Working directory has staged changes"
> +fi
> +
> +if ! git read-tree -u -m $commit; then

You forgot "" around $commit here.  Here, an $IFS byte in $commit will
get the command line munged.  I.e.

	if ! git read-tree -u -m "$commit"
	then
		...

> +    die "Could not update working tree to new HEAD"
> +fi

Thanks.