bug#10686: mv: moving hardlink of a softlink to the softlink does nothing

[Jim Meyering]

Bernhard Voelker wrote:

> On 02/01/2012 04:45 PM, Jim Meyering wrote:
>> Thanks for the clarification and quotes, Eric.
>>
>> Bernhard, here's a better patch.
>> With it, mv simply unlinks the "s" in your example:
>>
>> diff --git a/src/copy.c b/src/copy.c
>> index 4810de8..73f5cc5 100644
>> --- a/src/copy.c
>> +++ b/src/copy.c
>> @@ -1229,7 +1229,17 @@ same_file_ok (char const *src_name, struct stat const 
>> *src_sb,
>>           know this here IFF preserving symlinks), then it's ok -- as long
>>           as they are distinct.  */
>>        if (S_ISLNK (src_sb->st_mode) && S_ISLNK (dst_sb->st_mode))
>> -        return ! same_name (src_name, dst_name);
>> +        {
>> +          bool sn = same_name (src_name, dst_name);
>> +          if ( ! sn && same_link)
>> +            {
>> +              *unlink_src = true;
>> +              *return_now = true;
>> +              return true;
>> +            }
>> +
>> +          return ! sn;
>> +        }
>>
>>        src_sb_link = src_sb;
>>        dst_sb_link = dst_sb;
>
> Thank you both.
> The patch works.
>
> I wonder if just removing the x->hard_link constraint at the beginning
> of same_file_ok() would alsoo do (although the comment sounds like this
> is no good idea):
>
> @@ -1213,11 +1213,11 @@ same_file_ok (char const *src_name, struct stat const 
> *src_sb,
>    /* FIXME: this should (at the very least) be moved into the following
>       if-block.  More likely, it should be removed, because it inhibits
>       making backups.  But removing it will result in a change in behavior
>       that will probably have to be documented -- and tests will have to
>       be updated.  */
> -  if (same && x->hard_link)
> +  if (same)

I doubt that would work (it doesn't set *unlink_src) -- though you're
welcome to try it.  Even if it did, I'd prefer not to put a change like
this in code that might be considered for removal.

>        *return_now = true;
>        return true;
>      }
>
> Please don't think I just want to give you more work,
> but I think this deserves a new test, doesn't it?

Of course ;-)  That was not a complete patch by a long shot.
It didn't even have a commit log.  You obviously know by now that
any change like this absolutely requires an addition to the tests suite.
And since (thanks to Eric's clarification) it does officially count
as a bug fix, I'll write a NEWS entry, too.

Thanks again for spotting the problem.