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

[Akim Demaille]

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
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.
Fix comment, and improve error message.
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.
+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.
+test "$(sed -n 3p NEWS)" = "$noteworthy_stub" \
+  || die "line 3 of NEWS must be exactly '$noteworthy_stub'"
 
-# No dirt allowed.
-case $(git diff-index --name-only HEAD) in
-  '') ;;
-  *) die 'this tree is dirty; commit your changes first';;
-esac
+## --------------- ##
+## Then, changes.  ##
+## --------------- ##
 
-# update NEWS to have today's date, plus desired version number and $type
+# Update NEWS to have today's date, plus desired version number and $type.
 perl -MPOSIX -ni -e 'my $today = strftime "%F", localtime time;' \
  -e 'my ($type, $ver) = qw('"$type $ver"');' \
  -e 'my $pfx = "'"$noteworthy"'";' \
  -e 'print $.==3 ? "$pfx $ver ($today) [$type]\n" : $_' \
      NEWS || die 'failed to update NEWS'
 
-# 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
-
 printf "version $ver\n\n* NEWS: Record release date.\n" \
     | git commit -F -  -a || die 'git commit failed'
 git tag -s -m "$pkg $ver" v$ver HEAD || die 'git tag failed'
-- 
1.7.11.1