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

2013-09-13 Thread Junio C Hamano 
Felipe Contreras felipe.contre...@gmail.com 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

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 gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com 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