Re: [Quilt-dev] [PATCH 1/2] pop: Correctly report pop failure cause.

[Jean Delvare]

Hi Benjamin,

Le vendredi 30 novembre 2012 à 16:43 -0500, Benjamin Poirier a écrit :
> Because of the extra condition that was present, in the case where patching
> the original file(s) fails, the situation was incorrectly reported as "Patch
> [...] does not remove cleanly" whereas it should be "Failed to patch temporary
> files".
> ---
>  quilt/pop.in |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/quilt/pop.in b/quilt/pop.in
> index efacf09..8b69f64 100644
> --- a/quilt/pop.in
> +++ b/quilt/pop.in
> @@ -111,12 +111,9 @@ check_for_pending_changes()
>                            --no-backup-if-mismatch -E \
>                            >/dev/null 2>/dev/null
>               then
> -                     if ! [ -e $QUILT_PC/$patch ]
> -                     then
> -                             printf $"Failed to patch temporary files\n" >&2
> -                             rm -rf $workdir
> -                             return 1
> -                     fi
> +                     printf $"Failed to patch temporary files\n" >&2
> +                     rm -rf $workdir
> +                     return 1
>               fi
>       fi
>  

This change breaks the test suite:

[failpop.test]
[1] $ mkdir patches -- ok
[3] $ cat > test.txt -- ok
[6] $ quilt new test.diff -- ok
[9] $ quilt add test.txt -- ok
[12] $ cat >> test.txt -- ok
[15] $ quilt refresh -- ok
[18] $ sleep 2 -- ok
[19] $ cat patches/test.diff > /tmp/test.diff -- ok
[21] $ sed -e "s/ /_/g" patches/test.diff > patches/test.new -- ok
[22] $ cat patches/test.new > /tmp/test.new -- ok
[23] $ mv patches/test.new patches/test.diff -- ok
[24] $ quilt pop -- failed
Failed to patch temporary files       != Patch patches/test.diff does not 
remove cleanly (refresh it or enforce with -f)
12 commands (11 passed, 1 failed)

I am not claiming that this test makes a lot of sense... Command [21]
trashes the patch file completely, leaving patch only garbage to
process. I am not sure what was the intent, as this test is not very
representative of real-world situations.

I am also skeptical about the tests on "$QUILT_PC/$patch" in function
check_for_pending_changes(). I just can't see how "$QUILT_PC/$patch"
could not exist. It is created unconditionally by "quilt new" and "quilt
push". Except by sabotaging "$QUILT_PC" manually, I don't think the
error message "Failed to patch temporary files" can ever be printed. If
someone can think of a scenario leading to this message being printed,
please let me know.

So I am fine dropping the test ! [ -e "$QUILT_PC/$patch" ] as you
suggested. But I'm now sure what to do when patching fails. The
unexpected failure due to the files being read-only is not the only case
that would trigger the error. For example, a patch with just a header
and no contents would do as well. Unfortunately "patch" returns with
status 2 in both cases, so there's no easy way to differentiate.

While the read-only error is something that should get fixed and thus no
longer happen, patches with just headers is something we want to keep
supporting, at least with push -f and pop -f. So I won't take your patch
as is, it needs more work.

-- 
Jean Delvare
Suse L3