Hi Martin, On Sun, 29 Dec 2013 22:48:01 +0100, Martin Quinson wrote: > I took all of your suggestions and added a small test case. It does > not test what happens with binary diffs because I could not think of > how to generate a binary file in a portable manner, but uses chmod -r. > > The new patch is inline here while the old one is below, quoted in the > mail for reference.
Looks overall good, although I have a few more comments, see below. > Bye, Mt. > > Description: Exit with an error when diff's retcode=2 (error) on patch I am just realizing that the description seems to be truncated. I think you mean "on patch refresh"? > This is trigered e.g. when you try to add a binary file to a patch. > This is actually creepy to think that we were not checking the > retcode of diff :) > Forwarded: 2013-12-29 > Bug-Debian: http://bugs.debian.org/638313 > Author: Martin Quinson > > --- > quilt/refresh.in | 2 +- > quilt/scripts/patchfns.in | 6 ++++++ > test/faildiff.test | 18 ++++++++++++++++++ > 3 files changed, 25 insertions(+), 1 deletion(-) > > pipestatus: 1 > Index: b/quilt/scripts/patchfns.in > =================================================================== > --- a/quilt/scripts/patchfns.in > +++ b/quilt/scripts/patchfns.in > @@ -766,6 +766,12 @@ Can you please generate your patches with diff option -p in the future? I makes reviewing easier. > 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 > die 1 > fi > > Index: b/test/faildiff.test > =================================================================== > --- /dev/null > +++ b/test/faildiff.test > @@ -0,0 +1,18 @@ > + $ 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 > + > + $ chmod -r test.txt > + > + $ quilt refresh > + > diff: test.txt: Permission denied > + > Diff failed on file 'test.txt', aborting. Please check the return value here: $ echo %{?} > 1 BTW, the return value of "quilt diff" in case a binary file is included is still 0. I think it needs to be fixed as well, > + > + $ chmod +r test.txt -- Jean Delvare Suse L3 Support _______________________________________________ Quilt-dev mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/quilt-dev
