Re: FYI: do-release-commit-and-tag: fix typo

[Jim Meyering]

Akim Demaille wrote:
> Le 5 juil. 2012 à 15:24, Akim Demaille a écrit :
>
>> Hi friends,
>>
>> I just installed this.
>>
>> # simple check: no question marks on line 3 of NEWS
>> -test "$(sed -n 3p NEWS)" = "$noteworthy_stub" \
>> +test "$(sed -n 3p NEWS)" != "$noteworthy_stub" \
>>   || die 'line 3 of NEWS looks fishy!'
>
> This was wrong, the = was right, it is the comments that
> are misleading.  I'm trying to make a beta of Bison, and
> because an initial run changed the NEWS file (which I did
> not notice) the NEWS line was changed, and this check
> was triggered.  I thought that this check was ensuring
> that the template was properly filled, but actually,
> it's exactly the opposite: this is run _before_.
>
> I'm sorry for noise.  I propose the following patch
> which is first performing the checks, to avoid useless
> changes of NEWS, and comment/message fixes.
>
> Also, meanwhile Jim answered:
>
>>> # simple check: no question marks on line 3 of NEWS
>>> -test "$(sed -n 3p NEWS)" = "$noteworthy_stub" \
>>> +test "$(sed -n 3p NEWS)" != "$noteworthy_stub" \
>>>   || die 'line 3 of NEWS looks fishy!'
>>
>> First, that's sort of like a double negative.
>> This is a more readable equivalent of what you've committed:
>>
>>    test "$(sed -n 3p NEWS)" = "$noteworthy_stub" \
>>      && die 'line 3 of NEWS looks fishy!'
>
> Well, while I agree in general, I don't have the same
> feeling in this case, for two reasons.  The first one
> being that under set -e, && propagate the failure,
> and the script will die.  || is safer than && when

This script doesn't use set -e.
I'd say that having to limit such use of "&&" is a good reason
not to use "set -e".

> not in a condition (inside an if for instance).  And second,
> this pattern (check || die) implements assert is a quite
> visual way.  Using &&, on the contrary IMHO, would make
> things harder to read (because not consistent).
>
> Anyway, this was wrong in this place.
>
> Here is my proposal:
>
> From 6df4446131184e583ebc2c472a626fed07aab012 Mon Sep 17 00:00:00 2001
> From: Akim Demaille <address@hidden>
> Date: Thu, 5 Jul 2012 15:41:20 +0200
> Subject: [PATCH] do-release-commit-and-tag: fix the previous commit
>
> * build-aux/do-release-commit-and-tag: Actually the test was right,
> but the error message was wrong.

As far as I can tell, the message is not wrong.

> Fix comment, and improve error message.

This is accurate.

> Perform check first, so that NEWS is not modified uselessly.
> ---
>  ChangeLog                           |  6 ++++++
>  build-aux/do-release-commit-and-tag | 41 
> ++++++++++++++++++++++++-------------
>  2 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index c257cb2..57478ed 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,11 @@
>  2012-07-05  Akim Demaille  <address@hidden>
>
> +     do-release-commit-and-tag: fix the previous commit
> +     * build-aux/do-release-commit-and-tag: Actually the test was right,
> +     but the error message was wrong.
> +     Fix comment, and improve error message.
> +     Perform check first, so that NEWS is not modified uselessly.
> +
>       do-release-commit-and-tag: fix typo
>       * build-aux/do-release-commit-and-tag: Be sure that NEWS does
>       _not_ start with a stub.
> diff --git a/build-aux/do-release-commit-and-tag 
> b/build-aux/do-release-commit-and-tag
> index 329d0eb..78223be 100755
> --- a/build-aux/do-release-commit-and-tag
> +++ b/build-aux/do-release-commit-and-tag
> @@ -3,7 +3,7 @@
>  # controlled .prev-version file, automate the procedure by which we record
>  # the date, release-type and version string in the NEWS file.  That commit
>  # will serve to identify the release, so apply a signed tag to it as well.
> -VERSION=2012-07-05.13 # UTC
> +VERSION=2012-07-05.14 # UTC
>
>  # Note: this is a bash script (could be zsh or dash)
>
> @@ -77,6 +77,10 @@ EOF
>    exit
>  }
>
> +## ------ ##
> +## Main.  ##
> +## ------ ##
> +
>  branch=master
>  builddir=.
>
> @@ -107,6 +111,11 @@ test $# = 2 \
>  ver=$1
>  type=$2
>
> +
> +## ---------------------- ##
> +## First, sanity checks.  ##
> +## ---------------------- ##
> +
>  # Verify that $ver looks like a version number, and...
>  echo "$ver"|grep -E '^[0-9][0-9.]*[0-9]$' > /dev/null \
>    || die "invalid version: $ver"
> @@ -124,32 +133,36 @@ case $type in
>    *) die "invalid release type: $type";;
>  esac
>
> +# No dirt allowed.

s/dirt/local modifications/

> +case $(git diff-index --name-only HEAD) in
> +  '') ;;
> +  *) die 'this tree is dirty; commit your changes first';;
> +esac
> +
> +# Ensure the current branch name is correct:
> +curr_br=$(git rev-parse --symbolic-full-name HEAD)
> +test "$curr_br" = refs/heads/$branch || die not on branch $branch
> +
>  # Extract package name from Makefile.
>  Makefile=$builddir/Makefile
>  pkg=$(sed -n 's/^PACKAGE = \(.*\)/\1/p' "$Makefile") \
>    || die "failed to determine package name from $Makefile"
>
> -# simple check: no question marks on line 3 of NEWS
> -test "$(sed -n 3p NEWS)" != "$noteworthy_stub" \
> -  || die 'line 3 of NEWS looks fishy!'
> +# Check that line 3 of NEWS is the stub line about to be filled.

s/filled/replaced/ (or "with blanks about to be filled in")

ACK with those changes.