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:
> 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.
> +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.
> +
> +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.
> +
> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html