Re: [PATCH v2 1/2] version-gen: cleanup

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2013, 06:10 UTC
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:

Show 22 quoted lines

> +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.

Show 9 quoted lines

> -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.