Re: [Quilt-dev] [patch] verbose error message when the serie file does not exist

[Jean Delvare]

Hi Martin,

On Tue, 2020-03-24 at 18:44 +0100, Martin Quinson wrote:
> On Tue, Mar 24, 2020 at 12:40:36PM +0100, Jean Delvare wrote:
> > By the way, this test case revealed a few inconsistencies. While most
> > commands properly report "No series file found" if you are not in a
> > quilt-managed tree, 3 commands diverge from this behavior:
> > 
> > * "quilt pop" reports "No patch removed" instead, which while not
> > incorrect, is not consistent.
> > 
> > * "quilt series" report nothing. That's what Martin's patch aimed at
> > fixing. I wonder it should report "No series file found" even when not
> > in verbose mode? That would seem more consistent.
> > 
> > * "quilt snapshot" creates a .pc/ subdirectory and an empty .snap/
> > directory there. Again while not fundamentally wrong, it doesn't seem
> > particularly useful in the absence of a series file.
> > 
> > I'll look into fixing "pop" and "snapshot" to behave the same as all
> > other commands.
> 
> Thanks for your investigation, Jean. quilt is lucky to have you: you
> are doing a great job here. And I manage to mess even simple patches :)
> 
> IMHO, the only thing that is important to fix the former Debian bug is
> that something is reported when using 'quilt series' out of a
> quilt-managed tree. Reporting "No series file found" even when not in
> verbose mode would be really perfect. I guess that whoever did that
> patch back in 2014 wanted to reduce the divergence with upstream code,
> thus reducing the visibility of the fix to verbose situations.

OK, I worked on this and came up with two options how this can be fixed.

Option #1: Fix the "series" command so that it reports "No series file
found" if not in a quilt-managed tree. This is essentially your patch
fixed up to not introduce regressions and extended to the non-verbose
case.

Pros: No side effects.
Cons: The code is kind of quirky, and it duplicates the error message.
That can be addressed subsequently (I have code for it already), but
that makes the code even more quirky. Also, similar changes would be
needed for the "pop" and "snapshot" commands (I don't have code for
that yet).

Option #2: Check for the presence of a series file early for all quilt
commands (except a handful) before doing anything else.

Pros: This guarantees consistency across all commands by design, and
fixes the "pop" and "snapshot" commands. The code is neat, one error
message is de-duplicated. Functions find_first_patch and
find_last_patch are simplified, which should compensate for the time
spent in the initial presence check.
Cons: This approach has a side effect on the value returned by the
"pop", "push", "top" and "next" commands when run outside of a quilt-
managed tree. So far, they would return 2 in that case (same as when
trying to pop when you have no patch applied, or trying to push when
all patches are already applied). After this change, they would return
1 instead (plain error).

I would like to hear opinions about which way we should go. Personally
I have a clear preference for option #2, as I believe the new returned
values are more consistent (push/pop and previous/next were not
symmetric). However changing return values shouldn't be done lightly as
people may be relying on them.

Martin, Ondřej, will anything break on your side if quilt no longer
exits with code 2 when no series file exists?

I'm attaching patches for both options so you can review and test them.

-- 
Jean Delvare
SUSE L3 Support

option-1-series-check-series-exists.patch
Description: Text Data

option-2-check-series-exists.patch
Description: Text Data