Re: [Quilt-dev] [patch 2/5] Return 2 (instead of 1) when there is nothing to do

[Jean Delvare]

Hi Martin,

Sorry for the delay, I have finally made up my mind, cleaned up the
patches, and applied them all.

Here's the current status:

Le Friday 18 January 2013 à 15:37 +0100, Martin Quinson a écrit :
> On Fri, Jan 18, 2013 at 09:47:09AM +0100, Jean Delvare wrote:
> > Le mercredi 19 décembre 2012 à 15:35 +0100, address@hidden a
> > >  [ -z "$opt_verbose" ] && silent_unless_verbose=-s
> > >  [ -n "$opt_force" ] && opt_leave_rejects=1
> > > --- a/quilt/scripts/patchfns.in
> > > +++ b/quilt/scripts/patchfns.in
> > > @@ -477,7 +477,7 @@ find_last_patch()
> > >           else
> > >                   printf $"No series file found\n" >&2
> > >           fi
> > > -         return 1
> > > +         return 2
> > >   fi
> > >  
> > >   echo "$patch"
> > 
> > This change doesn't have any effect in practice. The only caller of
> > find_last_patch() is "quilt mail" and it overwrites the returned value.
> 
> This is currently the only caller of find_last_patch. I personally
> find dangerous to leave personnal mines in the source code just
> because nobody currently runs on it. 

I dropped this. "Personal mine" seems to be a rather strong term for a
return status. I'd rather have quilt return "2" only on clearly
identified situations, otherwise it will become inconsistent and lose
its value. So any change to return "2" somewhere should come with a
matching status test check in at least one test case.

At this point I only know of two commands which can return a status of
2: quilt push at the top of the stack and quilt pop at the bottom of the
stack. Both are checked in test/three.test. If there are others, please
let me know and we'll document them.

> > Plus this would make find_first_patch and find_last_patch diverge for no
> > good reason.
> 
> Should we possibly fix find_first_patch, then?

Up to you. I want things to be consistent and documented. If you send
any patch fulfilling these conditions, I'll be happy to review and test
it. But at this point I see no urge to change find_last_patch or
find_first_patch to be honest.

> > I would love if this patch would also add test cases for all conditions
> > which are supposed to return 2. This would not only guarantee that such
> > a regression isn't reintroduced later, but this would also ensure that
> > the fix itself is complete. Unfortunately the testing script doesn't
> > support for this yet, but I'm working on it.
> 
> Do you mind integrating this even before the test code is working?

I have implemented the checking side of things meanwhile, so I committed
it with your patches.

> > It would also be great to update the manual page to document these
> > cases, as it currently lacks an EXIT STATUS section.
> 
> Actually, was part of another patch in Debian, as we have a proper
> EXIT STATUS section. I just merged both patches, and the result is in
> attachment. I know that the patch to the manpage contains an unrelated
> change, but this is only a cosmetic improvement so I don't feel like
> providing a proper patch. If it really hurts your feeling, I'll commit
> this unrelated change myself once you pushed your changes to the tree.

I've split the unrelated change to a separate commit.

Thanks for the EXIT STATUS section in the man page. I have extended it a
bit to make things even clearer, hope you like it.

With this, you should be able to drop one or two patches from Debian's
quilt package, finally :)

Please let me know if I missed anything or got anything wrong.

-- 
Jean Delvare
Suse L3 Support