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

2013-09-13 Thread Felipe Contreras 
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-09-13 Thread Junio C Hamano 
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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-09-12 Thread Felipe Contreras 
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html