Re: [Quilt-dev] [PATCH v2] quilt refresh: Do not remove symlinks

[Jean Delvare]

Hi Jason,

Sorry for the late review.

Le Monday 12 January 2015 à 12:56 -0600, Jason Wessel a écrit :
> Change the core behavior of quilt to by default not remove symlinks to
> the patch files.  A backward compatibility option of
> QUILT_RM_SYMLINKS=yes was introduced for the case where a developer
> desires the symlinks are erased and replaced by files.

As mentioned before, I think this is overkill, and I'd rather not
implement this alternative behavior until someone asks for it. But I
understand you may not want to take responsibility for this. No problem,
I can add a note in the commit message that angry users should complain
to me, not you ;-) Or I can split the change in two parts if you prefer.

I would, however, check whether the link target is writable. If it is
not, then breaking the link (and letting the user know about it) is the
only thing we can do (as you mentioned earlier.) This will transparently
restore compatibility if users were using quilt that way.

> Historical notes:
> 
>     This is an 6 year old patch that is still actively used for the
>     Yocto Project and others.
> 
>     http://lists.nongnu.org/archive/html/quilt-dev/2008-01/msg00004.html
> 
>     Revised again here:
> 
>     http://comments.gmane.org/gmane.comp.handhelds.openembedded/34224
> 
> Signed-off-by: Jason Wessel <address@hidden>
> ---
>  quilt.quiltrc             |    4 +++
>  quilt/refresh.in          |    7 +++-
>  quilt/scripts/patchfns.in |    8 ++++-
>  test/symlink.test         |   81 
> +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 98 insertions(+), 2 deletions(-)
>  create mode 100644 test/symlink.test
> 
> diff --git a/quilt.quiltrc b/quilt.quiltrc
> index 58f665e..316452a 100644
> --- a/quilt.quiltrc
> +++ b/quilt.quiltrc
> @@ -35,3 +35,7 @@ QUILT_PATCHES_PREFIX=yes
>  # Use a specific editor for quilt (defaults to the value of $EDITOR before
>  # sourcing this configuration file, or vi if $EDITOR wasn't set).
>  #EDITOR=nedit
> +
> +#If your patch files are actually symlinks, this option set to yes
> +#will break the symlinks should quilt update one of the files.
> +#QUILT_RM_SYMLINKS=yes
> diff --git a/quilt/refresh.in b/quilt/refresh.in
> index 41d43a7..f39a877 100644
> --- a/quilt/refresh.in
> +++ b/quilt/refresh.in
> @@ -324,7 +324,12 @@ if [ -e "$patch_file" ] && \
>  then
>       printf $"Patch %s is unchanged\n" "$(print_patch "$patch")"
>  elif ( [ -z "$QUILT_BACKUP" -o ! -e "$patch_file" ] || \
> -       mv "$patch_file" "$patch_file~" ) && \
> +       if [ -L "$patch_file" -a "$QUILT_RM_SYMLINKS" != "yes" ] ; \
> +       then \
> +          cp "$patch_file" "$patch_file~"; \
> +       else \
> +          mv "$patch_file" "$patch_file~"; \
> +       fi ) && \
>       cat_to_new_file "$patch_file" < $tmp_result
>  then
>       if [ -n "$opt_fork" ]
> diff --git a/quilt/scripts/patchfns.in b/quilt/scripts/patchfns.in
> index 3fa26bb..7c7a67f 100644
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -785,7 +785,13 @@ cat_to_new_file()
>  {
>       local filename="$1"
>  
> -     [ -e "$filename" ] && rm -f "$filename"
> +     if [ -L "$filename" ]
> +     then
> +         [ "$QUILT_RM_SYMLINKS" = "yes" ] && rm -f "$filename"
> +     elif [ -e "$filename" ]
> +     then
> +         rm -f "$filename"
> +     fi
>
>       case "$filename" in
>       *.gz)
> diff --git a/test/symlink.test b/test/symlink.test
> new file mode 100644
> index 0000000..dc83858
> --- /dev/null
> +++ b/test/symlink.test
> @@ -0,0 +1,81 @@
> +     $ mkdir patches
> +
> +# quilt should not remove symlinks to patch files
> +     $ echo foo > foo
> +     $ quilt new test.diff
> +     > Patch patches/test.diff is now on top
> +
> +     $ quilt add foo
> +     > File foo added to patch patches/test.diff
> +
> +     $ echo "foo changed" > foo
> +     $ quilt refresh
> +     > Refreshed patch patches/test.diff
> +
> +# Setup a symlink
> +     $ mv patches/test.diff .
> +     $ ln -s ../test.diff patches/test.diff
> +     $ readlink patches/test.diff
> +     > ../test.diff
> +
> +     $ echo "foo changed 2" > foo
> +     $ quilt refresh
> +     > Refreshed patch patches/test.diff
> +
> +     $ test -L patches/test.diff && echo FOUND_LINK
> +     > FOUND_LINK
> +
> +     $ readlink patches/test.diff
> +     > ../test.diff

Isn't the call to readlink sufficient? If patches/test.diff isn't a link
it won't return anything.

> +
> +# Test the refresh --backup without a link
> +     $ echo "foo changed 3" > foo
> +     $ quilt refresh --backup
> +     > Refreshed patch patches/test.diff
> +
> +     $ test -L patches/test.diff && echo FOUND_LINK
> +     > FOUND_LINK
> +
> +     $ readlink patches/test.diff
> +     > ../test.diff

At this point I think we also want to check that patches/test.diff~ was
properly created, is a regular file, and contains the previous version
of the patch.

> +
> +# Test the refresh --backup with a link
> +        $ cp test.diff test.diff~
> +        $ rm -f patches/test.diff~
> +        $ ln -s ../test.diff~ patches/test.diff~

Leading spaces need to be replaced by tabs in the three lines above.
Also I'd rather do:

        $ mv patches/test.diff~ test.diff~
        $ ln -s ../test.diff~ patches/test.diff~

which is more efficient and avoids that test.diff and test.diff~ hold
the same contents (which is a problem because then you don't know for
sure if the backup worked.)

> +     $ echo "foo changed 4" > foo
> +     $ quilt refresh --backup
> +     > Refreshed patch patches/test.diff
> +
> +     $ test -L patches/test.diff && echo FOUND_LINK
> +     > FOUND_LINK
> +
> +     $ readlink patches/test.diff
> +     > ../test.diff
> +
> +     $ readlink patches/test.diff~
> +     > ../test.diff~

Again you should check the contents here, otherwise you have no
guarantee that the backup worked.

That being said I'm not entirely sure if we want to test this behavior,
because I'm not sure we want to officially support that use case. It
seems highly impractical as the user would have to create the symbolic
link before calling "quilt refresh --backup" the first time for every
patch. You don't actually need this feature for yourself, do you?

BTW, if you (or anyone else) really want all patches to link outside the
patches/ directory then I am curious why you don't just have patches/
itself be a link to the directory where all the patches are actually
stored. That's what we do for the SUSE kernel. It's also what "quilt
setup" does and it works just fine. And it seems a lot easier to setup
than per-patch links.

> +
> +# Now test that the symlinks are broken on a refresh with
> +# QUILT_RM_SYMLINKS=yes
> +
> +     $ echo "foo changed 5" > foo
> +     $ export QUILT_RM_SYMLINKS=yes
> +     $ quilt refresh --backup
> +     > Refreshed patch patches/test.diff
> +
> +     $ test -L patches/test.diff || echo NO_OLD_LINKS_HERE
> +     > NO_OLD_LINKS_HERE
> +
> +     $ readlink patches/test.diff~
> +     > ../test.diff
> +
> +     $ echo "foo changed 6" > foo
> +     $ quilt refresh --backup
> +     > Refreshed patch patches/test.diff
> +
> +     $ test -L patches/test.diff || echo NO_OLD_LINKS_HERE
> +     > NO_OLD_LINKS_HERE
> +
> +     $ test -L patches/test.diff~ || echo NO_OLD_LINKS_HERE
> +     > NO_OLD_LINKS_HERE

As mentioned before, I'd get rid of this feature altogether for the time
being.

-- 
Jean Delvare
SUSE L3 Support