[PATCH 0/2] version-gen: fixes and cleanups

[PATCH 0/2] version-gen: fixes and cleanups

[Felipe Contreras]

Felipe Contreras (2):
  version-gen: cleanup
  version-gen: avoid messing the version

 GIT-VERSION-GEN | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
1.8.4-338-gefd7fa6

[PATCH 1/2] version-gen: cleanup

[Felipe Contreras]

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
+		;;
 	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
@@ -34,9 +37,6 @@ then
 else
 	VC=unset
 fi
-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
-- 
1.8.4-338-gefd7fa6

[PATCH 2/2] version-gen: avoid messing the version

[Felipe Contreras]

If the version is 'v1.8.4-rc1' that is the version, and there's no need
to change it to anything else, like 'v1.8.4.rc1'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 GIT-VERSION-GEN | 1 -
 1 file changed, 1 deletion(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index b0db139..2b9fd2f 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -24,7 +24,6 @@ then
 		VN="$VN-dirty"
 		;;
 	esac
-	VN=$(echo "$VN" | sed -e 's/-/./g')
 else
 	VN="$DEF_VER"
 fi
-- 
1.8.4-338-gefd7fa6

Re: [PATCH 2/2] version-gen: avoid messing the version

[Junio C Hamano]

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

> If the version is 'v1.8.4-rc1' that is the version, and there's no need
> to change it to anything else, like 'v1.8.4.rc1'.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  GIT-VERSION-GEN | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index b0db139..2b9fd2f 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -24,7 +24,6 @@ then
>  		VN="$VN-dirty"
>  		;;
>  	esac
> -	VN=$(echo "$VN" | sed -e 's/-/./g')
>  else
>  	VN="$DEF_VER"
>  fi

The log message of 5c7d3c95 (Allow building of RPM from interim
snapshot., 2006-01-16) leaves a lot to be desired, but I do remember
that this was added due to user request.  Before that commit, we did
not strip dashes, and RPM packaging did not like dash in the version
name, hence folks on RH and derived distributions could not package
the software out of the box themselves.

I do not offhand know if that RPM limitation was lifted.  I do not
know if there are other packagers with a similar limitation that
have been relying on this version mangling, either.  This change may
end up being a regression for these users.

The mangling is done only for auto-generated names, so I suspect
that anybody who wanted the dash in their version names for whatever
reason has already been using the "version file" escape hatch (or
nobody had a desparate need to use dash instead of dot).

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

[Junio C Hamano]

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

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

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

Re: [PATCH 2/2] version-gen: avoid messing the version

[Felipe Contreras]

On Mon, Sep 9, 2013 at 12:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> If the version is 'v1.8.4-rc1' that is the version, and there's no need
>> to change it to anything else, like 'v1.8.4.rc1'.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  GIT-VERSION-GEN | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
>> index b0db139..2b9fd2f 100755
>> --- a/GIT-VERSION-GEN
>> +++ b/GIT-VERSION-GEN
>> @@ -24,7 +24,6 @@ then
>>               VN="$VN-dirty"
>>               ;;
>>       esac
>> -     VN=$(echo "$VN" | sed -e 's/-/./g')
>>  else
>>       VN="$DEF_VER"
>>  fi
>
> The log message of 5c7d3c95 (Allow building of RPM from interim
> snapshot., 2006-01-16) leaves a lot to be desired, but I do remember
> that this was added due to user request.  Before that commit, we did
> not strip dashes, and RPM packaging did not like dash in the version
> name, hence folks on RH and derived distributions could not package
> the software out of the box themselves.

Of course they could, by doing the replace themselves, or by simply
using a 'version' file. Didn't you just recently suggested somebody to
do exactly that?

> I do not offhand know if that RPM limitation was lifted.  I do not
> know if there are other packagers with a similar limitation that
> have been relying on this version mangling, either.  This change may
> end up being a regression for these users.

Of course not. First of all, who exactly is packaging release
candidates nowadays? And second, why they can't they use the existing
tools, like the 'version' file?

The information behind the commit 5c7d3c95 is basically non-existant,
as I cannot even find a patch for that change in the mailing list. I
guess you decided the commit was so trivial it didn't merit a patch
for review and discussion. Anyway, it happened before the 'version'
file option was present, so it's quite likely they (whatever "they"
might be) would not have any issue.

> The mangling is done only for auto-generated names, so I suspect
> that anybody who wanted the dash in their version names for whatever
> reason has already been using the "version file" escape hatch (or
> nobody had a desparate need to use dash instead of dot).

So, let me get this straight; the nominal version of Git has a dash
(v1.8.3-rc1), but the internal version of doesn't (1.8.3.rc1), there
is no real reason for that, except that you say "some people" need it
that way, but then you say the people that need it in the nominal way
should use the 'version' file. Why couldn't the people that want the
non-nominal form use the 'version' file instead?

It doesn't follow.

-- 
Felipe Contreras

Re: [PATCH 2/2] version-gen: avoid messing the version

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Mon, Sep 09, 2013 at 02:00:55AM -0500, Felipe Contreras wrote:
> Of course not. First of all, who exactly is packaging release
> candidates nowadays? And second, why they can't they use the existing
> tools, like the 'version' file?

Debian unstable, for one.  However, they don't use RPMs and the version
would be rewritten to use ~ anyway.  Fedora rawhide might.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] version-gen: avoid messing the version

[Felipe Contreras]

On Mon, Sep 9, 2013 at 6:30 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Sep 09, 2013 at 02:00:55AM -0500, Felipe Contreras wrote:
>> Of course not. First of all, who exactly is packaging release
>> candidates nowadays? And second, why they can't they use the existing
>> tools, like the 'version' file?
>
> Debian unstable, for one.  However, they don't use RPMs and the version
> would be rewritten to use ~ anyway.  Fedora rawhide might.

Debian uses their own version; 1:1.8.4~rc3-1, it doesn't use Git's
version 1.8.4.rc3, so if we change it to 1.8.4-rc3, that doesn't
affect them one bit because they are not using it anyway.

As for Fedora rawhide, they don't even use rc versions:

http://koji.fedoraproject.org/koji/packageinfo?packageID=1864

-- 
Felipe Contreras