Re: [PATCH] Add a sample hook which saves push certs as notes

From: Todd Zullinger <tmz@pobox.com>
Date: Dec 3, 2017, 00:45 UTC
Message-ID: <20171203004543.GO3693@zaya.teonanacatl.net>
In-Reply-To: <20171202091248.6037-1-root@shikherverma.com>

Hi Shikher,

I'm not familiar with push certs, but I did notice some general issues in the sample hook. I hope they're helpful.

Shikher Verma wrote:

> index 000000000..b4366e43f
> --- /dev/null
> +++ b/templates/hooks--post-receive.sample
> +#!/bin/sh

...

> +if test -z GIT_PUSH_CERT ; then
> +    exit 0
> +fi

The $ is missing from GIT_PUSH_CERT. test -z GIT_PUSH_CERT will always be false. :)

The variable should also be quoted. Not all sh implementations accept a missing argument to test -z, as bash does.

More minor, Documentation/CodingGuidelines suggests placing 'then' on a new line:

    if test -z "$GIT_PUSH_CERT"
    then
        exit 0
    fi

(There is plenty of code that doesn't follow that, so I don't know how strong that preference is.)

This could also be written as:

    test -z "$GIT_PUSH_CERT" && exit 0

I don't know if there's any general preference to shorten it in git's code or not.

> +push_cert=$(git cat-file -p  $GIT_PUSH_CERT)

Very minor: there's an extra space before the variable here.

(I also noticed the tests which use $GIT_PUSH_CERT, like t5534, use 'cat-file blob ...' rather than 'cat-file -p ...'. I don't know if that's much safer/better than letting cat-file guess the object type in the hook. I have no idea if there's a chance that "$GIT_PUSH_CERT" has some unexpected, non-blob object type.)

> +while read oval nval ref
> +do
> +	# Verify that the ref update matches that in push certificate.
> +	if [[ $push_cert == *$oval" "$nval" "$ref* ]]; then

[[ isn't portable across all the sh implementations git strives to support, as far as I know.

The minor point about 'then' on new line is applicable here too. It would also better match the outer 'while' loop.

> +		# add the push cert as note (namespaced pushcerts) to nval.
> +		git notes --ref=pushcerts add -m "$push_cert" $nval -f
> +	fi
> +done

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Learn from the mistakes of others--you can never live long enough to
make them all yourself.
    -- John Luther