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 <[email protected]> > 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 <[email protected]> > > + 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.
