Re: [PATCH 4/4] git-ack: record an ack

From: Fabian Ruch <bafain@gmail.com>
Date: Jun 3, 2014, 23:53 UTC
Message-ID: <538E5FF2.2080506@gmail.com>
In-Reply-To: <1400447743-18513-5-git-send-email-mst@redhat.com>

Hi Michael,

I have some inline comments below. Also, some parts of the patch do not adhere to the style rules

 - tabs for indentation
 - $(...) for command substitution
 - no space after redirection operators
 - double-quotes around redirection targets

for shell scripts (from the file `Documentation/CodingGuidelines`).

On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote:

Show 19 quoted lines

> diff --git a/contrib/git-ack b/contrib/git-ack
> new file mode 100755
> index 0000000..4aeb16a
> --- /dev/null
> +++ b/contrib/git-ack
> @@ -0,0 +1,84 @@
> +msg=`mktemp`
> +patch=`mktemp`
> +info=`git mailinfo $msg $patch`
> +subj=`echo "$info"|sed -n 's/^Subject: //p'`
> +author=`echo "$info"|sed -n 's/^Author: //p'`
> +email=`echo "$info"|sed -n 's/^Email: //p'`
> +auth="$author <$email>"
> +date=`echo "$info"|sed -n 's/^Date: //p'`
> +sign=`mktemp`
> +echo "ack! $subj" > $sign
> +echo "" >> $sign
> +if
> +    git diff --cached HEAD

If I am not mistaken, the exit code of `git-diff(1)` doesn't change according to whether there are differences or not, unless the option `--exit-code` is given.

> +then
> +    nop < /dev/null

Is it correct that this is a do-nothing operation? Is that a common idiom? I found the null command (`:`, colon) to be used in many places instead.

Show 5 quoted lines

> +else
> +    echo "DIFF in cache. Not acked, reset or commit!"
> +    exit 1
> +fi
> +GIT_DIR=`pwd`/${GIT_DIR}

This seems incorrect to me. If `GIT_DIR` is already set, it might point to an absolute path and not `.git`. If the environment variable is not set, the state file `ACKS` ends up in the working directory.

Maybe `git-sh-setup(1)` can be of help. It uses

    git rev-parse --git-dir

to probe the path to the .git directory.

Show 10 quoted lines

> +
> +usage () {
> +	echo "Usage: git ack " \
> +            "[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
> +        exit 1;
> +}
> +
> +append=
> +save=
> +clear=

The restore flag seems to be missing from this list of declarations.

Show 54 quoted lines

> +
> +while test $# != 0
> +do
> +	case "$1" in
> +	-a|--append)
> +		append="y"
> +		;;
> +	-s|--s)
> +		save="y"
> +		;;
> +	-r|--restore)
> +		restore="y"
> +		;;
> +	-c|--clear)
> +		clear="y"
> +                ;;
> +	*)
> +		usage ;;
> +	esac
> +	shift
> +done
> +
> +if
> +    test "$clear"
> +then
> +    rm -f "${GIT_DIR}/ACKS"
> +fi
> +
> +if
> +    test "$save"
> +then
> +    if
> +        test "$append"
> +    then
> +        cat $msg >> "${GIT_DIR}/ACKS"
> +    else
> +        cat $msg > "${GIT_DIR}/ACKS"
> +    fi
> +fi
> +
> +if
> +    test "$restore"
> +then
> +    msg = ${GIT_DIR}/ACKS
> +fi
> +
> +if
> +    grep '^[A-Z][A-Za-z-]*-by:' $msg >> $sign
> +then
> +    git commit --allow-empty -F $sign --author="$auth" --date="$date"
> +else
> +    echo "No signature found!"
> +    exit 2
> +fi

   Fabian