On Fri, Sep 13, 2013 at 1:10 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html