{"thread":{"id":"34922","subject":"[PATCH v2 1/2] version-gen: cleanup","startedAt":"2013-09-13T01:17:24Z","lastAt":"2013-09-14T05:59:31Z","messageCount":7,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"227600","messageId":"1379035046-6688-1-git-send-email-felipe.contreras@gmail.com","threadId":"34922","inReplyTo":null,"subject":"[PATCH v2 0/2] version-gen: fixes and cleanups","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-13T01:17:24Z","receivedAt":"2013-09-13T01:17:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras (2):\n  version-gen: cleanup\n  version-gen: avoid messing the version\n\n GIT-VERSION-GEN | 36 +++++++++++++++++++-----------------\n 1 file changed, 19 insertions(+), 17 deletions(-)\n\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"227599","messageId":"1379035046-6688-2-git-send-email-felipe.contreras@gmail.com","threadId":"34922","inReplyTo":"1379035046-6688-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/2] version-gen: cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-13T01:17:25Z","receivedAt":"2013-09-13T01:17:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"No functional changes.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n GIT-VERSION-GEN | 36 ++++++++++++++++++++----------------\n 1 file changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex 06026ea..e96538d 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -6,22 +6,29 @@ DEF_VER=v1.8.4\n LF='\n '\n \n-# First see if there is a version file (included in release tarballs),\n-# then try git-describe, then default.\n-if test -f version\n-then\n-\tVN=$(cat version) || VN=\"$DEF_VER\"\n-elif test -d ${GIT_DIR:-.git} -o -f .git &&\n-\tVN=$(git describe --match \"v[0-9]*\" --abbrev=7 HEAD 2>/dev/null) &&\n+describe () {\n+\tVN=$(git describe --match \"v[0-9]*\" --abbrev=7 HEAD 2>/dev/null) || return 1\n \tcase \"$VN\" in\n-\t*$LF*) (exit 1) ;;\n+\t*$LF*)\n+\t\treturn 1\n+\t\t;;\n \tv[0-9]*)\n \t\tgit update-index -q --refresh\n \t\ttest -z \"$(git diff-index --name-only HEAD --)\" ||\n-\t\tVN=\"$VN-dirty\" ;;\n+\t\tVN=\"$VN-dirty\"\n+\t\treturn 0\n+\t\t;;\n \tesac\n+}\n+\n+# First see if there is a version file (included in release tarballs),\n+# then try 'git describe', then default.\n+if test -f version\n+then\n+\tVN=$(cat version) || VN=\"$DEF_VER\"\n+elif test -d ${GIT_DIR:-.git} -o -f .git && describe\n then\n-\tVN=$(echo \"$VN\" | sed -e 's/-/./g');\n+\tVN=$(echo \"$VN\" | sed -e 's/-/./g')\n else\n \tVN=\"$DEF_VER\"\n fi\n@@ -34,9 +41,6 @@ then\n else\n \tVC=unset\n fi\n-test \"$VN\" = \"$VC\" || {\n-\techo >&2 \"GIT_VERSION = $VN\"\n-\techo \"GIT_VERSION = $VN\" >$GVF\n-}\n-\n-\n+test \"$VN\" = \"$VC\" && exit\n+echo >&2 \"GIT_VERSION = $VN\"\n+echo \"GIT_VERSION = $VN\" >$GVF\n-- \n1.8.4-338-gefd7fa6\n"},{"id":"227601","messageId":"1379035046-6688-3-git-send-email-felipe.contreras@gmail.com","threadId":"34922","inReplyTo":"1379035046-6688-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/2] version-gen: avoid messing the version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-13T01:17:26Z","receivedAt":"2013-09-13T01:17:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"If the version is 'v1.8.4-rc1' that is the version, and there's no need\nto change it to anything else, like 'v1.8.4.rc1'.\n\nIf RedHat, or somebody else, needs a specific version, they can use the\n'version' file, like everybody else.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n GIT-VERSION-GEN | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex e96538d..b3de2db 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -26,10 +26,8 @@ describe () {\n if test -f version\n then\n \tVN=$(cat version) || VN=\"$DEF_VER\"\n-elif test -d ${GIT_DIR:-.git} -o -f .git && describe\n+elif test ! -d ${GIT_DIR:-.git} -o ! -f .git || ! describe\n then\n-\tVN=$(echo \"$VN\" | sed -e 's/-/./g')\n-else\n \tVN=\"$DEF_VER\"\n fi\n \n-- \n1.8.4-338-gefd7fa6\n"},{"id":"227622","messageId":"xmqqy571j7t6.fsf@gitster.dls.corp.google.com","threadId":"34922","inReplyTo":"1379035046-6688-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/2] version-gen: cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T06:10:32Z","receivedAt":"2013-09-13T06:10:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> +describe () {\n> +\tVN=$(git describe --match \"v[0-9]*\" --abbrev=7 HEAD 2>/dev/null) || return 1\n>  \tcase \"$VN\" in\n> +\t*$LF*)\n> +\t\treturn 1\n> +\t\t;;\n>  \tv[0-9]*)\n>  \t\tgit update-index -q --refresh\n>  \t\ttest -z \"$(git diff-index --name-only HEAD --)\" ||\n> +\t\tVN=\"$VN-dirty\"\n> +\t\treturn 0\n> +\t\t;;\n>  \tesac\n> +}\n> +\n> +# First see if there is a version file (included in release tarballs),\n> +# then try 'git describe', then default.\n> +if test -f version\n> +then\n> +\tVN=$(cat version) || VN=\"$DEF_VER\"\n> +elif test -d ${GIT_DIR:-.git} -o -f .git && describe\n>  then\n\nMakes sense; using a helper function makes the primary logic easier\nto read.\n\n> -test \"$VN\" = \"$VC\" || {\n> -\techo >&2 \"GIT_VERSION = $VN\"\n> -\techo \"GIT_VERSION = $VN\" >$GVF\n> -}\n> -\n> -\n> +test \"$VN\" = \"$VC\" && exit\n> +echo >&2 \"GIT_VERSION = $VN\"\n> +echo \"GIT_VERSION = $VN\" >$GVF\n\nThe point of this part is \"if the version file is already up to\ndate, do not rewrite it only to smudge the mtime to confuse make\",\nso it would be much easier to read to write it like so:\n\n\tif test \"$VN\" != \"$VC\"\n        then\n\t\t... two echoes ...\n\tfi\n\ncompared to \"exiting in the middle\" which is harder to follow,\nespecially if we consider that we may have to grow the remaining two\nlines in the future.\n"},{"id":"227629","messageId":"CAMP44s23dDGK3ymMNR6u7BNR3uD5E_aE14att5AyBeSv-UKpqg@mail.gmail.com","threadId":"34922","inReplyTo":"xmqqy571j7t6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] version-gen: cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-13T12:16:56Z","receivedAt":"2013-09-13T12:16:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Sep 13, 2013 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> +describe () {\n>> +     VN=$(git describe --match \"v[0-9]*\" --abbrev=7 HEAD 2>/dev/null) || return 1\n>>       case \"$VN\" in\n>> +     *$LF*)\n>> +             return 1\n>> +             ;;\n>>       v[0-9]*)\n>>               git update-index -q --refresh\n>>               test -z \"$(git diff-index --name-only HEAD --)\" ||\n>> +             VN=\"$VN-dirty\"\n>> +             return 0\n>> +             ;;\n>>       esac\n>> +}\n>> +\n>> +# First see if there is a version file (included in release tarballs),\n>> +# then try 'git describe', then default.\n>> +if test -f version\n>> +then\n>> +     VN=$(cat version) || VN=\"$DEF_VER\"\n>> +elif test -d ${GIT_DIR:-.git} -o -f .git && describe\n>>  then\n>\n> Makes sense; using a helper function makes the primary logic easier\n> to read.\n>\n>> -test \"$VN\" = \"$VC\" || {\n>> -     echo >&2 \"GIT_VERSION = $VN\"\n>> -     echo \"GIT_VERSION = $VN\" >$GVF\n>> -}\n>> -\n>> -\n>> +test \"$VN\" = \"$VC\" && exit\n>> +echo >&2 \"GIT_VERSION = $VN\"\n>> +echo \"GIT_VERSION = $VN\" >$GVF\n>\n> The point of this part is \"if the version file is already up to\n> date, do not rewrite it only to smudge the mtime to confuse make\",\n> so it would be much easier to read to write it like so:\n>\n>         if test \"$VN\" != \"$VC\"\n>         then\n>                 ... two echoes ...\n>         fi\n>\n> compared to \"exiting in the middle\" which is harder to follow,\n> especially if we consider that we may have to grow the remaining two\n> lines in the future.\n\nNo, it's much easier to follow, the code clearly says \"if the version\nis up to date, there's nothing else to do\".\n\n-- \nFelipe Contreras\n"},{"id":"227639","messageId":"xmqqbo3wk2ok.fsf@gitster.dls.corp.google.com","threadId":"34922","inReplyTo":"1379035046-6688-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/2] version-gen: avoid messing the version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T18:16:11Z","receivedAt":"2013-09-13T18:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> If the version is 'v1.8.4-rc1' that is the version, and there's no need\n> to change it to anything else, like 'v1.8.4.rc1'.\n>\n> If RedHat, or somebody else, needs a specific version, they can use the\n> 'version' file, like everybody else.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n\nI already explained to you why this is a bad change.\n\nWhen we say \"we try to avoid regressions\", we really mean it.\nBefore coming up with a change to pay Paul by robbing Peter, we must\nmake an honest effort to see if there is a way to pay Paul without\nrobbing anybody.\n\nThis change forces existing users who depend on how dashes are\nmangled into dots to change their tooling.  \n\nWe do occasionally make deliberate regressions that force existing\nusers to change the way they work, but only when there is no other\nway, and when the benefit of the change far outweighs the cost of\nsuch an adjustment, and with careful planning to ease the pain of\ntransition.  The updates to \"git add\" and \"git push\" planned for 2.0\nfall into that category.\n\nThere has to be a benefit that far outweighs the inconvenience this\npatch imposes on existing users, but I do not see there is any.  \"If\nsomebody needs a specific version, they can use the 'version' file\"\ndoes not justify it at all; it equally applies to those who want to\nuse a version name with a dash.\n\nBesides, the patch does not even do what it claims to do; if the\nversion is \"v1.8.4-rc1\", what you get out of the updated code is\n\"1.8.4-rc1\", still losing the leading \"v\".\n\nI'd be more receptive if the patch were like this instead, though.\n\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex b444c18..c6d78ec 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -3,12 +3,16 @@\n GVF=GIT-VERSION-FILE\n DEF_VER=v1.8.4.GIT\n \n+READ_RAW_VERSION=\n LF='\n '\n \n # First see if there is a version file (included in release tarballs),\n # then try git-describe, then default.\n-if test -f version\n+if test -f raw-version && VN=$(cat raw-version)\n+then\n+\tREAD_RAW_VERSION=yes\n+elif test -f version\n then\n \tVN=$(cat version) || VN=\"$DEF_VER\"\n elif test -d ${GIT_DIR:-.git} -o -f .git &&\n@@ -26,7 +30,10 @@ else\n \tVN=\"$DEF_VER\"\n fi\n \n-VN=$(expr \"$VN\" : v*'\\(.*\\)')\n+if test -z \"$READ_RAW_VERSION\"\n+then\n+\tVN=$(expr \"$VN\" : v*'\\(.*\\)')\n+fi\n \n if test -r $GVF\n then\n"},{"id":"227670","messageId":"CAMP44s3o=DCu6X-+36B07gPbSZv-soYja8GjdCZkRAaK0o9UPw@mail.gmail.com","threadId":"34922","inReplyTo":"xmqqbo3wk2ok.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] version-gen: avoid messing the version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-14T05:59:31Z","receivedAt":"2013-09-14T05:59:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Sep 13, 2013 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> If the version is 'v1.8.4-rc1' that is the version, and there's no need\n>> to change it to anything else, like 'v1.8.4.rc1'.\n>>\n>> If RedHat, or somebody else, needs a specific version, they can use the\n>> 'version' file, like everybody else.\n>>\n>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> ---\n>\n> I already explained to you why this is a bad change.\n\nNo, you did not.\n\nAll you did is throw a non sequitur argument which I already exposed as such.\n\n> When we say \"we try to avoid regressions\", we really mean it.\n\nMaybe by \"regressions\" you mean progress.\n\n> Before coming up with a change to pay Paul by robbing Peter, we must\n> make an honest effort to see if there is a way to pay Paul without\n> robbing anybody.\n\nThere is, because Peter, the RedHat maintainer, can do exactly the\nsame that Alice does; use the 'version' file.\n\nIn fact, Peter, cannot possibly use Git's version because of this:\n\n% rpmdev-vercmp git-1.8.4 git-1.8.4.rc1\ngit-1.8.4 < git-1.8.4.rc1\n% rpmdev-vercmp git-1.8.4 git-1.8.4-rc1\ngit-1.8.4 < git-1.8.4-rc1\n% rpmdev-vercmp git-1.8.4 git-1.8.4~rc1\ngit-1.8.4 > git-1.8.4~rc1\n\nFedora's guideline[1] is to use a format like git-1.8.4-1.rc1, then\nthe final release would be git-1.8.4-2, in other words, the '.rc1' is\nmostly informative, but that has nothing to do with Git's internal\nversion (git-1.8.4.rc1).\n\nopenSUSE's guideline[1] prefers to use git-1.8.4~rc1.\n\nEither way, none of them could possibly use git-1.8.4.rc1, because\nthat's greater than git-1.8.4.\n\n> This change forces existing users who depend on how dashes are\n> mangled into dots to change their tooling.\n\nNo, it does not.\n\nYou just say that, and you assume because you said it, it must be\ntrue; it's not.\n\nFirst of all, nor RedHat, nor any other RPM-based distribution needs\nthis any more, because as a simple search demonstrates, they don't use\nrelease candidates any more.\n\nhttp://www.rpmfind.net/linux/rpm2html/search.php?query=git&submit=Search+...\n\nEssentially, Peter is a figment of your imagination.\n\nThe closest thing to Peter 1) doesn't use release candidates, and 2)\nif he did, he wouldn't use a version like git-1.8.4.rc1.\n\n> We do occasionally make deliberate regressions that force existing\n> users to change the way they work, but only when there is no other\n> way, and when the benefit of the change far outweighs the cost of\n> such an adjustment, and with careful planning to ease the pain of\n> transition.  The updates to \"git add\" and \"git push\" planned for 2.0\n> fall into that category.\n>\n> There has to be a benefit that far outweighs the inconvenience this\n> patch imposes on existing users, but I do not see there is any.  \"If\n> somebody needs a specific version, they can use the 'version' file\"\n> does not justify it at all; it equally applies to those who want to\n> use a version name with a dash.\n>\n> Besides, the patch does not even do what it claims to do; if the\n> version is \"v1.8.4-rc1\", what you get out of the updated code is\n> \"1.8.4-rc1\", still losing the leading \"v\".\n\nWrong.\n\n% git tag -m '' v1.8.5-rc1\n% ./GIT-VERSION-GEN\nGIT_VERSION = 1.8.5-rc1-dirty\n\n[1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease\n[2] http://en.opensuse.org/openSUSE:Package_naming_guidelines\n\n-- \nFelipe Contreras\n"}]}