# [PATCH v2 1/2] version-gen: cleanup 7 messages from 2013-09-13 to 2013-09-14. Participants: Felipe Contreras, Junio C Hamano. Thread: https://gitlist.dev/t/34922 ## Felipe Contreras, 2013-09-13 01:17 Subject: [PATCH v2 0/2] version-gen: fixes and cleanups Message-ID: <1379035046-6688-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1379035046-6688-1-git-send-email-felipe.contreras%40gmail.com ``` Felipe Contreras (2): version-gen: cleanup version-gen: avoid messing the version GIT-VERSION-GEN | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) -- 1.8.4-338-gefd7fa6 ``` ## Felipe Contreras, 2013-09-13 01:17 Subject: [PATCH v2 1/2] version-gen: cleanup Message-ID: <1379035046-6688-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1379035046-6688-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1379035046-6688-1-git-send-email-felipe.contreras@gmail.com> ``` No functional changes. Signed-off-by: Felipe Contreras --- GIT-VERSION-GEN | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 06026ea..e96538d 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -6,22 +6,29 @@ DEF_VER=v1.8.4 LF=' ' -# 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 && - VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) && +describe () { + VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) || return 1 case "$VN" in - *$LF*) (exit 1) ;; + *$LF*) + return 1 + ;; v[0-9]*) git update-index -q --refresh test -z "$(git diff-index --name-only HEAD --)" || - VN="$VN-dirty" ;; + 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 - VN=$(echo "$VN" | sed -e 's/-/./g'); + VN=$(echo "$VN" | sed -e 's/-/./g') else VN="$DEF_VER" fi @@ -34,9 +41,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 ``` ## Felipe Contreras, 2013-09-13 01:17 Subject: [PATCH v2 2/2] version-gen: avoid messing the version Message-ID: <1379035046-6688-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1379035046-6688-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1379035046-6688-1-git-send-email-felipe.contreras@gmail.com> ``` 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'. If RedHat, or somebody else, needs a specific version, they can use the 'version' file, like everybody else. Signed-off-by: Felipe Contreras --- GIT-VERSION-GEN | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index e96538d..b3de2db 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -26,10 +26,8 @@ describe () { if test -f version then VN=$(cat version) || VN="$DEF_VER" -elif test -d ${GIT_DIR:-.git} -o -f .git && describe +elif test ! -d ${GIT_DIR:-.git} -o ! -f .git || ! describe then - VN=$(echo "$VN" | sed -e 's/-/./g') -else VN="$DEF_VER" fi -- 1.8.4-338-gefd7fa6 ``` ## Junio C Hamano, 2013-09-13 06:10 Subject: Re: [PATCH v2 1/2] version-gen: cleanup Message-ID: URL: https://gitlist.dev/e/xmqqy571j7t6.fsf%40gitster.dls.corp.google.com In-Reply-To: <1379035046-6688-2-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras 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. ``` ## Felipe Contreras, 2013-09-13 12:16 Subject: Re: [PATCH v2 1/2] version-gen: cleanup Message-ID: URL: https://gitlist.dev/e/CAMP44s23dDGK3ymMNR6u7BNR3uD5E_aE14att5AyBeSv-UKpqg%40mail.gmail.com In-Reply-To: ``` On Fri, Sep 13, 2013 at 1:10 AM, Junio C Hamano wrote: > Felipe Contreras 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. No, it's much easier to follow, the code clearly says "if the version is up to date, there's nothing else to do". -- Felipe Contreras ``` ## Junio C Hamano, 2013-09-13 18:16 Subject: Re: [PATCH v2 2/2] version-gen: avoid messing the version Message-ID: URL: https://gitlist.dev/e/xmqqbo3wk2ok.fsf%40gitster.dls.corp.google.com In-Reply-To: <1379035046-6688-3-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras 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'. > > If RedHat, or somebody else, needs a specific version, they can use the > 'version' file, like everybody else. > > Signed-off-by: Felipe Contreras > --- I already explained to you why this is a bad change. When we say "we try to avoid regressions", we really mean it. Before coming up with a change to pay Paul by robbing Peter, we must make an honest effort to see if there is a way to pay Paul without robbing anybody. This change forces existing users who depend on how dashes are mangled into dots to change their tooling. We do occasionally make deliberate regressions that force existing users to change the way they work, but only when there is no other way, and when the benefit of the change far outweighs the cost of such an adjustment, and with careful planning to ease the pain of transition. The updates to "git add" and "git push" planned for 2.0 fall into that category. There has to be a benefit that far outweighs the inconvenience this patch imposes on existing users, but I do not see there is any. "If somebody needs a specific version, they can use the 'version' file" does not justify it at all; it equally applies to those who want to use a version name with a dash. Besides, the patch does not even do what it claims to do; if the version is "v1.8.4-rc1", what you get out of the updated code is "1.8.4-rc1", still losing the leading "v". I'd be more receptive if the patch were like this instead, though. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index b444c18..c6d78ec 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -3,12 +3,16 @@ GVF=GIT-VERSION-FILE DEF_VER=v1.8.4.GIT +READ_RAW_VERSION= LF=' ' # First see if there is a version file (included in release tarballs), # then try git-describe, then default. -if test -f version +if test -f raw-version && VN=$(cat raw-version) +then + READ_RAW_VERSION=yes +elif test -f version then VN=$(cat version) || VN="$DEF_VER" elif test -d ${GIT_DIR:-.git} -o -f .git && @@ -26,7 +30,10 @@ else VN="$DEF_VER" fi -VN=$(expr "$VN" : v*'\(.*\)') +if test -z "$READ_RAW_VERSION" +then + VN=$(expr "$VN" : v*'\(.*\)') +fi if test -r $GVF then ``` ## Felipe Contreras, 2013-09-14 05:59 Subject: Re: [PATCH v2 2/2] version-gen: avoid messing the version Message-ID: URL: https://gitlist.dev/e/CAMP44s3o%3DDCu6X-%2B36B07gPbSZv-soYja8GjdCZkRAaK0o9UPw%40mail.gmail.com In-Reply-To: ``` On Fri, Sep 13, 2013 at 1:16 PM, Junio C Hamano wrote: > Felipe Contreras 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'. >> >> If RedHat, or somebody else, needs a specific version, they can use the >> 'version' file, like everybody else. >> >> Signed-off-by: Felipe Contreras >> --- > > I already explained to you why this is a bad change. No, you did not. All you did is throw a non sequitur argument which I already exposed as such. > When we say "we try to avoid regressions", we really mean it. Maybe by "regressions" you mean progress. > Before coming up with a change to pay Paul by robbing Peter, we must > make an honest effort to see if there is a way to pay Paul without > robbing anybody. There is, because Peter, the RedHat maintainer, can do exactly the same that Alice does; use the 'version' file. In fact, Peter, cannot possibly use Git's version because of this: % rpmdev-vercmp git-1.8.4 git-1.8.4.rc1 git-1.8.4 < git-1.8.4.rc1 % rpmdev-vercmp git-1.8.4 git-1.8.4-rc1 git-1.8.4 < git-1.8.4-rc1 % rpmdev-vercmp git-1.8.4 git-1.8.4~rc1 git-1.8.4 > git-1.8.4~rc1 Fedora's guideline[1] is to use a format like git-1.8.4-1.rc1, then the final release would be git-1.8.4-2, in other words, the '.rc1' is mostly informative, but that has nothing to do with Git's internal version (git-1.8.4.rc1). openSUSE's guideline[1] prefers to use git-1.8.4~rc1. Either way, none of them could possibly use git-1.8.4.rc1, because that's greater than git-1.8.4. > This change forces existing users who depend on how dashes are > mangled into dots to change their tooling. No, it does not. You just say that, and you assume because you said it, it must be true; it's not. First of all, nor RedHat, nor any other RPM-based distribution needs this any more, because as a simple search demonstrates, they don't use release candidates any more. http://www.rpmfind.net/linux/rpm2html/search.php?query=git&submit=Search+... Essentially, Peter is a figment of your imagination. The closest thing to Peter 1) doesn't use release candidates, and 2) if he did, he wouldn't use a version like git-1.8.4.rc1. > We do occasionally make deliberate regressions that force existing > users to change the way they work, but only when there is no other > way, and when the benefit of the change far outweighs the cost of > such an adjustment, and with careful planning to ease the pain of > transition. The updates to "git add" and "git push" planned for 2.0 > fall into that category. > > There has to be a benefit that far outweighs the inconvenience this > patch imposes on existing users, but I do not see there is any. "If > somebody needs a specific version, they can use the 'version' file" > does not justify it at all; it equally applies to those who want to > use a version name with a dash. > > Besides, the patch does not even do what it claims to do; if the > version is "v1.8.4-rc1", what you get out of the updated code is > "1.8.4-rc1", still losing the leading "v". Wrong. % git tag -m '' v1.8.5-rc1 % ./GIT-VERSION-GEN GIT_VERSION = 1.8.5-rc1-dirty [1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease [2] http://en.opensuse.org/openSUSE:Package_naming_guidelines -- Felipe Contreras ```