On Fri, 30 Jan 2015, Lubomir I. Ivanov wrote:
I'll just nitpick on this one:
> diff --git a/scripts/write-version b/scripts/write-version
> new file mode 100644
> index 0000000..42505ae
> --- /dev/null
> +++ b/scripts/write-version
> @@ -0,0 +1,61 @@
> +#!/bin/sh
Try this same thing with adding:
set -eu
#set -x #xtrace
> +#
> +# arguments:
> +# $1 - target H file
> +# $2 - fallback version string
> +# $3 - os name {linux|darwin|win}
> +#
> +# !!! has no error checking ATM !!!
> +# should be started from where .git is!
> +#
> +
> +# set OS and TARGET
Any of these may fail in absence of
[ $# -eq 3 ] || {
echo "ERROR: missing argument(s)" # alternative, usage message
exit 1
}
> +TARGET=$1
> +TARGET_TEMP=$TARGET.tmp
> +VERSION=$2
> +OS=$3
> +
> +# check if git is installed
> +GIT_ERROR=1
This is, AFAIK, not portable:
> +HAS_GIT=`git --version 2> NUL`
^^^
I'd write this:
> +if [ "$HAS_GIT" ]; then
as:
if HAS_GIT=$(git --version 2>/dev/null) && [ "$HAS_GIT" ]; then
Backticks are considered deprecated.
> + # check if .git exists
> + if [ -d ".git" ]; then
^ ^
Quotes are useless in that context.
> + FULL_VER=`sh ./scripts/get-version linux`
> + GIT_ERROR=0
> + else
> + echo "WARNING: not a git repository"
> + fi
> +else
> + echo "WARNING: git command not found"
> +fi
> +
> +#if there was an error, fallback to .gitversion or use the dummy version
> +if [ "$GIT_ERROR" == "1" ]; then
^^ ^ ^
'==' is a bashism and quotes are useless.
> + if [ -f .gitversion ]; then
^^
Existence of that regular file and readabylity of the same is not
realy the same thing.
> + FULL_VER=`cat .gitversion`
read FULL_VER <.gitversion
is more efficient.
> + else
> + FULL_VER=$VERSION
> + fi
> +fi
> +
> +# grab some strings from get-version
Both these:
> +CANONICAL_VER=`sh ./scripts/get-version full $FULL_VER`
> +OS_USABLE_VER=`sh ./scripts/get-version $OS $FULL_VER`
lack error handling, and deprecated backticks apply.
> +
> +# write to a temp file
Error handling (I know, this is really paranoic, but):
> +echo "#define VERSION_STRING \"$OS_USABLE_VER\"" > $TARGET_TEMP
> +echo "#define GIT_VERSION_STRING \"$FULL_VER\"" >> $TARGET_TEMP
> +echo "#define CANONICAL_VERSION_STRING \"$CANONICAL_VER\"" >> $TARGET_TEMP
that won't work if you happen to not have write permissions.
> +
> +# if the target file is missing create it
> +if [ ! -f $TARGET ]; then
or [ ! -w $TARGET ] ?
and more backticks:
> + CMD=`touch $TARGET`
> +fi
> +
> +# if the temp file and the target file differ, replace the target file with
> the temp file
> +CMD=`diff -q $TARGET $TARGET_TEMP || cp $TARGET_TEMP $TARGET`
> +
> +# remove the temp file
> +CMD=`rm -f $TARGET_TEMP`
Cheers,
--
Cristian
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
