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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2020, 20:43 UTC
Message-ID: <xmqqo8l3dxu7.fsf@gitster.c.googlers.com>
In-Reply-To: <20201015184703.5015-1-git@adamspiers.org>

Adam Spiers <git@adamspiers.org> writes:

Show 21 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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