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
From: Jean Delvare <[email protected]>
Subject: series: Complain if no series file is found
If no series file is found, let "quilt series" complain about it
instead of returning silently. This aligns the behavior of this
command on what almost all other commands already do.
Suggested by Martin Quinson. This fixes Debian bug #369908:
https://bugs.debian.org/369908
Signed-off-by: Jean Delvare <[email protected]>
---
quilt/scripts/patchfns.in | 12 +++++++++---
quilt/series.in | 4 ++--
test/no-series.test | 2 ++
3 files changed, 13 insertions(+), 5 deletions(-)
--- a/quilt/scripts/patchfns.in
+++ b/quilt/scripts/patchfns.in
@@ -353,11 +353,17 @@ backup_file_name()
cat_series()
{
+ local complain=$1
+
if [ -e "$SERIES" ]
then
sed -e '/^#/d' -e 's/^[ '$'\t'']*//' \
-e 's/[ '$'\t''].*//' -e '/^$/d' "$SERIES"
else
+ if [ -n "$complain" ]
+ then
+ printf $"No series file found\n" >&2
+ fi
return 1
fi
}
@@ -415,16 +421,16 @@ patches_before()
patches_after()
{
- local patch=$1
+ local patch=$1 complain=$2
if [ -n "$patch" ]
then
- cat_series \
+ cat_series $complain \
| awk '
seen == 1 { print }
$0 == "'"$patch"'" { seen=1 }
'
else
- cat_series
+ cat_series $complain
fi
}
--- a/quilt/series.in
+++ b/quilt/series.in
@@ -103,9 +103,9 @@ then
"${opt_verbose:+= }" $top
fi
cat_patches "$color_series_una" \
- "${opt_verbose:+ }" $(patches_after $top)
+ "${opt_verbose:+ }" $(patches_after "$top" 1)
else
- cat_patches "" "" $(cat_series)
+ cat_patches "" "" $(cat_series 1)
fi
### Local Variables:
### mode: shell-script
--- a/test/no-series.test
+++ b/test/no-series.test
@@ -42,7 +42,9 @@ $ quilt refresh
> No series file found
$ quilt series
+> No series file found
$ quilt series -v
+> No series file found
$ quilt snapshot
From: Jean Delvare <[email protected]>
Subject: Consistently complain early if no series file is found
If no series file is found, let all quilt commands which need it
complain about it immediately. This aligns the behavior of the "pop",
"series" and "snapshot" command with how all other commands behave.
The "import", "new", "setup" and "upgrade" commands are not affected
by this change, as they are legitimately called without a series
file.
A side effect of this change is that the "pop", "push", "top" and
"next" commands will now return with error code 1 instead of 2 when
called outside of a quilt-managed tree.
Inspired by Martin Quinson. This fixes Debian bug #369908:
https://bugs.debian.org/369908
Signed-off-by: Jean Delvare <[email protected]>
---
quilt/import.in | 3 +++
quilt/new.in | 3 +++
quilt/scripts/patchfns.in | 27 +++++++++++++++------------
quilt/setup.in | 3 ++-
quilt/upgrade.in | 3 ++-
test/no-series.test | 7 +++++--
test/three.test | 4 ++--
7 files changed, 32 insertions(+), 18 deletions(-)
--- a/quilt/import.in
+++ b/quilt/import.in
@@ -6,6 +6,9 @@
#
# See the COPYING and AUTHORS files for more details.
+# One of the few commands which does not need a series file
+skip_series_check=1
+
# Read in library functions
if [ "$(type -t patch_file_name)" != function ]
then
--- a/quilt/new.in
+++ b/quilt/new.in
@@ -6,6 +6,9 @@
#
# See the COPYING and AUTHORS files for more details.
+# One of the few commands which does not need a series file
+skip_series_check=1
+
# Read in library functions
if [ "$(type -t patch_file_name)" != function ]
then
--- a/quilt/scripts/patchfns.in
+++ b/quilt/scripts/patchfns.in
@@ -489,12 +489,7 @@ find_first_patch()
local patch=$(cat_series | head -n 1)
if [ -z "$patch" ]
then
- if [ -e "$SERIES" ]
- then
- printf $"No patches in series\n" >&2
- else
- printf $"No series file found\n" >&2
- fi
+ printf $"No patches in series\n" >&2
return 1
fi
@@ -506,12 +501,7 @@ find_last_patch()
local patch=$(cat_series | tail -n 1)
if [ -z "$patch" ]
then
- if [ -e "$SERIES" ]
- then
- printf $"No patches in series\n" >&2
- else
- printf $"No series file found\n" >&2
- fi
+ printf $"No patches in series\n" >&2
return 1
fi
@@ -1168,9 +1158,13 @@ then
elif [ -f "$QUILT_PC/$QUILT_SERIES" ]
then
SERIES=$QUILT_PC/$QUILT_SERIES
+ # We know the file exists, no need to re-check later
+ skip_series_check=1
elif [ -f "$QUILT_SERIES" ]
then
SERIES=$QUILT_SERIES
+ # We know the file exists, no need to re-check later
+ skip_series_check=1
else
SERIES=$QUILT_PATCHES/$QUILT_SERIES
fi
@@ -1193,6 +1187,15 @@ then
exit 1
fi
fi
+
+if [ -z "$skip_series_check" ]
+then
+ if [ ! -f "$SERIES" ]
+ then
+ printf $"No series file found\n" >&2
+ exit 1
+ fi
+fi
### Local Variables:
### mode: shell-script
### End:
--- a/quilt/setup.in
+++ b/quilt/setup.in
@@ -6,8 +6,9 @@
#
# See the COPYING and AUTHORS files for more details.
-# Version check is irrelevant to this command.
+# Version and series checks are irrelevant to this command.
skip_version_check=1
+skip_series_check=1
# Read in library functions
if [ "$(type -t patch_file_name)" != function ]
--- a/quilt/upgrade.in
+++ b/quilt/upgrade.in
@@ -6,8 +6,9 @@
#
# See the COPYING and AUTHORS files for more details.
-# Don't abort in version check.
+# Don't abort in version or series check.
skip_version_check=1
+skip_series_check=1
# Read in library functions
if [ "$(type -t patch_file_name)" != function ]
--- a/test/no-series.test
+++ b/test/no-series.test
@@ -26,9 +26,9 @@ $ quilt next
> No series file found
$ quilt pop
-> No patch removed
+> No series file found
$ quilt pop -v
-> No patch removed
+> No series file found
$ quilt previous
> No series file found
@@ -42,9 +42,12 @@ $ quilt refresh
> No series file found
$ quilt series
+> No series file found
$ quilt series -v
+> No series file found
$ quilt snapshot
+> No series file found
$ quilt top
> No series file found
--- a/test/three.test
+++ b/test/three.test
@@ -29,9 +29,9 @@
> No series file found
$ quilt pop
- > No patch removed
+ > No series file found
$ echo %{?}
- > 2
+ > 1
$ quilt new patch1.diff
> Patch %{P}patch1.diff is now on top
_______________________________________________
Quilt-dev mailing list
[email protected]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
