Re: [Quilt-dev] [PATCH] quilt refresh: Add ability to not break symlinks

[Jean Delvare]

Hi Jason,

Le Monday 12 January 2015 à 10:39 -0600, Jason Wessel a écrit :
> On 01/12/2015 06:37 AM, Jean Delvare wrote:
> > On Wed, 7 Jan 2015 11:39:12 -0600, Jason Wessel wrote:
> >> This is an 6 year old patch that is still actively used:
> >>
> >> http://lists.nongnu.org/archive/html/quilt-dev/2008-01/msg00004.html
> >>
> >> Revised again here:
> >>
> >> http://comments.gmane.org/gmane.comp.handhelds.openembedded/34224
> >>
> >> This patch was then updated to properly work with or without the
> >> --backup option to the quilt refresh by using cat_to_new_file option
> >> instead of mv for refresh. A reference to the QUILT_NO_RM_SYMLINKS was
> >> also added to the usage information for quilt refresh.
> > 
> > Is there a legitimate use case for not setting QUILT_NO_RM_SYMLINKS?
> 
> Probably the most obvious is when what the link points to is not
> writable.  For the Yocto Project or WR Linux where this patch to quilt
> is employed that is certainly not relevant.

I suppose this is a purely theoretical scenario and you aren't aware of
anyone actually doing that? I prefer to focus on things people actually
do with quilt and keep the code as simple as possible. As you noticed,
interaction between different options can already be quite tricky to
figure out.

For example I am not going to add support for patch files being in fact
named pipes or sockets. Certainly this could be done but without a use
case this would just add gratuitous complexity.

> As you pointed out in your response, it is more about backward
> compatibility.  I imagine some folks might actually be surprised if
> their symlinks suddenly do not get replaced by a file because that is
> how quilt has always worked.  Others would of course be happy.

> > (...)
> > I doubt that symbolic links were considered at that point (and this is
> > why you have the problem you have.) So maybe it would be the right time
> > to set a sane default behavior for them (and add option
> > QUILT_RM_SYMLINKS later if anyone ever asks for it - but I suspect
> > this won't happen.)
> > 
> > What do you think? Opinion anyone?
> 
> 
> 
> I am fine with changing the default, I have run with quilt set to
> never remove my symlinks for many years at this point, and prior to
> that had a script that restored the links which dates back to 2005.  I
> have used quilt for many, many projects in conjunction with cvs, svn,
> and git always using the symlinks.  I just thought it might be time to
> actually try to get the logic integrated once and for all.  :-)

And quite rightly so, thanks for contributing!

> >> (...)
> >> @@ -324,7 +328,7 @@ 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~" ) && \
> >> +       cat_to_new_file "$patch_file~" < "$patch_file" ) && \
> > 
> > This comes with a small performance penalty in the general case. You
> > probably won't notice it for a single patch, but people using the
> > --refresh option of push or pop on large patch sets are likely to
> > notice it. I wonder if this could be avoided, maybe by letting
> > cat_to_new_file handle the backup by itself.
>
> Or simply not use cat_to_new_file in the case where there is no link.

Yes, that would work too, at the cost of testing for the same condition
twice.

> Initially, I opted for the most simple fix here, and I would be happy
> to rework the patch to optimize the behavior. First we should decide
> on an answer for the default behavior with symlinks.

I agree that optimizations can be sorted out later.

Well I have expressed my opinion already, we can wait for others to
speak but to be honest I don't expect a flood of comments on this, as
this is a specific use case most users simply don't care about.

So I would go for the most simple fix for now and wait for people to
report if something breaks. Also note that, if we really want to support
an alternative behavior when refreshing patches behind symlinks, I'd
rather make it a command line option than an environment variable. That
way the user can select the behavior on a case-by-case basis, rather
than globally.

Thanks,
-- 
Jean Delvare
SUSE L3 Support