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

[Ondřej Lysoněk]

Hi Jean,

Jean Delvare <address@hidden> writes:

> 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.

I'm afraid I'm unable to provide any feedback at this point. I've only
started looking into Quilt yesterday (hence the patches - I'm fixing
stuff as I go :)) and I'm not really using it for anything (yet). I'll
try to come back to this in the future, if time allows.

Thanks for including me.

Ondra