Re: [Quilt-dev] RFC: quilt pop and read-only files

[Jean Delvare]

Hi Benjamin,

Sorry for the late reply, I wanted to investigate this case completely
before replying and it took much more time than I originally expected.

Le mardi 27 novembre 2012 à 09:30 -0500, Benjamin Poirier a écrit :
> Hello,
> 
> I believe I've run into some issues with quilt pop and read-only files:
>       $ mkdir /tmp/quilt-test && cd /tmp/quilt-test
>       $ echo "step 1" > file
>       $ chmod a-w file
>       $ quilt new demo
>       Patch patches/demo is now on top
>       $ quilt add file
>       File file added to patch patches/demo
>       $ echo "step 2" > file
>       $ quilt refresh
>       Refreshed patch patches/demo
>       $ echo "blabla" | quilt header -r
>       Replaced header of patch patches/demo
>       $ quilt pop
>       Patch patches/demo does not remove cleanly (refresh it or enforce with 
> -f)

This behavior depends on the version of patch. GNU patch up to version
2.6.1 would accept patching read-only files. This changed with alpha
version 2.6.1.116 or so, up to and including 2.6.1.164. I noticed the
change and knew it would cause trouble to quilt, so I reported it
upstream:
http://lists.gnu.org/archive/html/bug-patch/2012-04/msg00020.html

The exact problem I reported back then was with "quilt push -f" but your
"quilt pop" problem has the same root cause.

While my proposal (accepting to patch read-only files as long as
--backup is used) was not accepted, a workaround was implemented in the
form of the --read-only parameter:

  --read-only=BEHAVIOR  How to handle read-only input files: 'ignore' that they
                        are read-only, 'warn' (default), or 'fail'.

'ignore' was the default up to version 2.6.1, 'fail' was the default in
versions 2.6.1.116 to 2.6.1.164, and the option was added in version
2.6.1.169 with 'warn' being the default.

I am not a big fan of this parameter, because older versions did not
have it so quilt can't use it unless we make patch version >= 2.7
mandatory (which I don't want to do, it's way too recent.) But at least
the default behavior since version 2.6.1.169 is somewhat compatible with
the previous stable versions of GNU patch (although I am still concerned
about the warnings.)

> The first problem is here. This message is misleading, in fact we are in the
> case "Failed to patch temporary files", the output from patch was:
> "File file is read-only; refusing to patch
> 1 out of 1 hunk ignored -- saving rejects to file file.rej"

I agree.

> Nevertheless, we can proceed with the suggestions:
>       $ quilt refresh
>       Patch patches/demo is unchanged
>       $ quilt pop
>       Patch patches/demo does not remove cleanly (refresh it or enforce with 
> -f)
> 
> One way to back out of this situation is to force the pop:
>       $ quilt pop -f
>       Removing patch patches/demo
>       Restoring file
> 
>       No patches applied

Yes, that works indeed.

> That's the second problem, I find that solution somewhat inelegant and
> counter-intuitive. Usually, "-f" leaves me with the impression that
> changes will be discarded.

Normally -f means "I don't care about what I'll lose" but in this
particular case, where quilt doesn't properly understand the situation,
it means "I know I'm not going to lose anything." That's not
fundamentally different. That being said, I agree the current situation
is not good and should be improved. But the real problem isn't with the
-f semantics but the fact that you shouldn't have to use it in the first
place.

> In the example above "file" is edited manually but the issue is the same
> when working with quilt import/push. The handling of the --force option
> to `patch` is reversed between push and pop. By default, `quilt push`
> leads to using `patch -f` which can work on read only files whereas
> `quilt pop` doesn't pass "-f" to patch in "check_for_pending_changes".

Correct.

> Two conditions are required for the problem to manifest itself:
> 1) the .timestamp for a patch becomes outdated but the patch body is unchanged
> (because of quilt header above).
> 2) the patch touches a file which was originally read-only.

Still correct.

> What does the quilt developer community think about this issue? Is "pop
> -f" warranted and accepted or should pop's behavior be altered?

The behavior should be altered. That being said, patch versions
2.6.1.116 and 2.6.1.136, which are in openSUSE 12.1 and 12.2
respectively (which I suppose you're running), were alpha versions. They
are no longer available for download. It was openSUSE's (questionable)
decision to package these alpha versions, and quilt doesn't have to be
modified to work with broken alpha versions. If needed, quilt should be
modified to work with GNU patch versions 2.7 and 2.7.1, as these are
official releases.

But it turns out that your problematic test case above no longer fails
with these versions.

So step 1 as far as I am concerned is upgrading patch to version 2.7.1
in openSUSE Factory, and then probably backport upstream patch commit
9a26fde226660a628d58ed7d987a71b9a4123244:

Author: Andreas Gruenbacher <address@hidden>
Date:   Tue Apr 17 16:13:08 2012 +0200

    Only warn when trying to modify read-only files

to openSUSE 12.1 and 12.2.

Then my own problematic case (quilt push -f) needs to be reevaluated.

> These problems can be cured by the two small patches that follow. I'll
> be happy to submit them properly if that proves to be desired.

I'll review and comment on them in their own thread.

-- 
Jean Delvare
Suse L3