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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 9, 2013, 06:03 UTC
Message-ID: <7vppsiila6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1378702889-21638-2-git-send-email-felipe.contreras@gmail.com>

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

Show 28 quoted lines

> No functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  GIT-VERSION-GEN | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 06026ea..b0db139 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -7,21 +7,24 @@ LF='
>  '
>  
>  # First see if there is a version file (included in release tarballs),
> -# then try git-describe, then default.
> +# 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 &&
> -	VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
> +	VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null)
> +then
>  	case "$VN" in
> -	*$LF*) (exit 1) ;;
> +	*$LF*)
> +		exit 1

The funnily written "false" is misleading and a clean-up may be a good idea to turn it to a straight "false".

I however think this actually changes the behaviour.

In any case, if you want to keep this step a "no functional change" rewrite, this section has to be part of the condition of this "elif" (because it logically is). If describe couldn't describe HEAD, or even if it could, if its output was multi-line for any reason, we wanted to punt and let the DEF_VER in the last "else" clause kick in.

After the update, describe output that is not a single-liner will fail the entire script, instead of falling back to DEF_VER. I actually think it is a good change to fail it (even though it is unlikely that the "describe" command give above would give more than one line).

Show 14 quoted lines

> +		;;
>  	v[0-9]*)
>  		git update-index -q --refresh
>  		test -z "$(git diff-index --name-only HEAD --)" ||
> -		VN="$VN-dirty" ;;
> +		VN="$VN-dirty"
> +		;;
>  	esac
> -then
> -	VN=$(echo "$VN" | sed -e 's/-/./g');
> +	VN=$(echo "$VN" | sed -e 's/-/./g')
>  else
>  	VN="$DEF_VER"
>  fi