From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 13 Sep 2013 06:10:32 GMT
Subject: Re: [PATCH v2 1/2] version-gen: cleanup
Message-ID: <xmqqy571j7t6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1379035046-6688-2-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +describe () {
> +	VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) || return 1
>  	case "$VN" in
> +	*$LF*)
> +		return 1
> +		;;
>  	v[0-9]*)
>  		git update-index -q --refresh
>  		test -z "$(git diff-index --name-only HEAD --)" ||
> +		VN="$VN-dirty"
> +		return 0
> +		;;
>  	esac
> +}
> +
> +# First see if there is a version file (included in release tarballs),
> +# then try 'git describe', then default.
> +if test -f version
> +then
> +	VN=$(cat version) || VN="$DEF_VER"
> +elif test -d ${GIT_DIR:-.git} -o -f .git && describe
>  then

Makes sense; using a helper function makes the primary logic easier
to read.

> -test "$VN" = "$VC" || {
> -	echo >&2 "GIT_VERSION = $VN"
> -	echo "GIT_VERSION = $VN" >$GVF
> -}
> -
> -
> +test "$VN" = "$VC" && exit
> +echo >&2 "GIT_VERSION = $VN"
> +echo "GIT_VERSION = $VN" >$GVF

The point of this part is "if the version file is already up to
date, do not rewrite it only to smudge the mtime to confuse make",
so it would be much easier to read to write it like so:

	if test "$VN" != "$VC"
        then
		... two echoes ...
	fi

compared to "exiting in the middle" which is harder to follow,
especially if we consider that we may have to grow the remaining two
lines in the future.