Hi Martin, Le Saturday 18 January 2014 à 01:54 +0100, [email protected] a écrit : > Description: Exit with an error when diff's retcode=2 (error) on patch refresh > This is trigered e.g. when you try to add a binary file to a patch.
Spelling: triggered > This is actually creepy to think that we were not checking the > retcode of diff :) > Forwarded: 2014-01-18 > Bug-Debian: http://bugs.debian.org/638313 > Author: Martin Quinson > > --- > quilt/diff.in | 6 ++++++ > quilt/refresh.in | 2 +- > quilt/scripts/patchfns.in | 6 ++++++ > test/faildiff.test | 43 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 56 insertions(+), 1 deletion(-) You didn't test this new version of the patch, did you? It doesn't pass the test suite... > Index: b/quilt/diff.in > =================================================================== > --- a/quilt/diff.in > +++ b/quilt/diff.in > @@ -119,6 +119,12 @@ > fi > else > diff_file "$file" "$old_file" "$new_file" | colorize > + > + # Test the return value of diff, and propagate the error if any > + if [ ${PIPESTATUS[0]} != 0 ] > + then > + die ${PIPESTATUS[0]} > + fi Just like $?, $PIPESTATUS gets a new value with every command bash executes. So if you need to use the value more than once, you must save it first and then only use the copy: # Test the return value of diff, and propagate the error if any status=${PIPESTATUS[0]} if [ $status != 0 ] then die $status fi > fi > } > > Index: b/quilt/scripts/patchfns.in > =================================================================== > --- a/quilt/scripts/patchfns.in > +++ b/quilt/scripts/patchfns.in > @@ -763,6 +763,12 @@ > echo "$line" > cat > fi > + > + # Test the return value of diff, and propagate the error retcode if any > + if [ ${PIPESTATUS[0]} == 2 ] > + then > + return 1 > + fi > } > > cat_file() > Index: b/quilt/refresh.in > =================================================================== > --- a/quilt/refresh.in > +++ b/quilt/refresh.in > @@ -231,7 +231,7 @@ > fi > if ! diff_file "$file" "$old_file" "$new_file" > then > - printf $"Diff failed, aborting\n" >&2 > + printf $"Diff failed on file '%s', aborting.\n" "$new_file" >&2 We normally don't have dots at the end of error messages. > die 1 > fi > > Index: b/test/faildiff.test > =================================================================== > --- /dev/null > +++ b/test/faildiff.test > @@ -0,0 +1,43 @@ > + $ mkdir patches > + > + $ cat > test.txt > + < This is test.txt. > + > + $ quilt new test.diff > + > Patch %{P}test.diff is now on top > + > + $ quilt add test.txt > + > File test.txt added to patch %{P}test.diff > + > +What happens when diff fails because of a permission error? > + > + $ chmod -r test.txt > + > + $ quilt refresh > + > diff: test.txt: Permission denied > + > Diff failed on file 'test.txt', aborting. > + > + $ echo %{?} > + > 1 > + > + $ chmod +r test.txt > + > +What happens on binary files? > + > + $ bash -c "printf '\\x02\\x00\\x01'" > test.bin Indented with spaces instead of a tab. > + > + $ quilt add test.bin > + > File test.bin added to patch %{P}test.diff > + > + $ printf "\\x03\\x00\\x01" > test.bin Indented with spaces instead of a tab, plus it is inconsistent with how you generated test.bin in the first place. We should use the same syntax everywhere to limit the portability issues. As explained in a previous mail, I now believe that: $ printf "\\003\\000\\001" > test.bin is the best approach as it is both portable and fast. > + $ quilt diff -pab --no-index > + >~ (Files|Binary files) a/test\.bin and b/test\.bin differ > + > + $ echo %{?} > + > 1 > + > + $ quilt refresh > + > Diff failed on file 'test.bin', aborting. > + > + $ echo %{?} > + > 1 I've made all the suggested fixes and changes myself and committed the patch under your name. -- Jean Delvare Suse L3 Support _______________________________________________ Quilt-dev mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/quilt-dev
