Bug#691389: [checkbashisms] multiple bug fixes and new checks

[Benjamin Drung]

Am Donnerstag, den 25.10.2012, 17:33 -0500 schrieb Raphael Geissert:
> Hi,
> 
> On Thursday 25 October 2012 16:44:03 Benjamin Drung wrote:
> > Am Mittwoch, den 24.10.2012, 21:15 -0500 schrieb Raphael Geissert:
> > > Attached is an mbox with a bunch of bug fixes and new checks, #687450
> > > included.
> > 
> > Thanks. I have applied your patches 1 up to 14. I grabbed the working
> > test cases from your git repository and added them to devscripts. After
> > applying a patch, I added the newly passing test cases. Feel free to add
> > more test cases to devscripts or to improve/simplify the shunit2 tests.
> 
> Thanks.
> 
> For now I'm going to continue using my test suite, it: has more test cases, 
> allows TODO items, and allows two versions of checkbashisms to be compared.
> I like the output-comparison approach of the shunit-based suite, but it is 
> annoying to use while developing.

I would like to see a movement of the passed test cases from your test
suite to the shunit2 test suite. The current situation is that 38 files
are moved to shunit2 and 25 files remain in your test suite.

The output comparison in shunit2 could be improved. It could print a
diff -u output instead of just printing the expected and the actual
output.

> > Patch 15 refers to commit 5dc48224, but the commit hashes do not survive
> > git format-patch & git am. To which commit do you refer?
> 
> Patch 15 refers to patch 14.

Thanks. I have applied the remaining patches.

> (the commit hash _is_ included in the git format-patch output.)

Evolution stripped the first From line from the patches when I saved
them. The commit hash is included in the mbox.

> > Please do not sign off your own patches. Your patches will be signed off
> > by the person that applies your patches.
> 
> Perhaps we differ on what the sign off means, and that shouldn't cause any 
> issue for another person to sign off them as well.

I found two meaning of signing off patches:

1) The signer certifies that he/she have created the patch in question
and take responsibility for the copyright status of the code in
question.

2) The signer reviewed the patch and has done some sort of QA.

We use the second meaning.

> > You can send your patches directly to the mailing list if have more than
> > one instead of opening a new bug report.
> > 
> > Can you add a commit with your changelog entries?
> 
> Since doing that oftentimes leads to merge conflicts it is easier if the 
> committer adds it after merging, and on whatever branch those changes are 
> merged.

Since you provided a big bunch of patches, I ask for one patch that adds
all the changelog entries.

> > > On IRC I mentioned a "regression" when checking autoconf, but the issue
> > > is now visible thanks to some bug fixes. In one configure script the
> > > bug fix actually revealed a bashism.
> > 
> > Have you created a test case for this regression?
> 
> Not per se, I've only copied autoconf(1) to my test suite. 
> 
> It all comes down to fixing the parsing of things like
> printf '#!/bin/sh\nbar="moo"; foo="foo \"bar\" ... \nmoo"\n'
> 
> And this (which is what actually triggers the 'error' in the autoconf case):
> checkbashisms <<'EOF'
> case foo in
> *\'*) arg=`$as_echo "$1" | sed "s/'/'\\\\\\\\''/g"` ;; #'
> esac
> EOF
> 
> (this actually made me notice that the error messages aren't displaying 
> '(stdin)', so attache patch fixes that.)

I applied that patch too. Attached the stripped down test case for the
regression (should pass without output).

-- 
Benjamin Drung
Debian & Ubuntu Developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: commented-quote.sh
Type: application/x-shellscript
Size: 87 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/devscripts-devel/attachments/20121026/a4818725/attachment.bin>