Opened 8 years ago

Closed 8 years ago

#5656 closed Bug (fixed)

Problems renaming files

Reported by: rb07 Owned by: jordan
Priority: Normal Milestone: 2.90
Component: libtransmission Version: 2.82
Severity: Normal Keywords: backport-candidate-2.8x
Cc:

Description

When the contents of a torrent have a common prefix, libtransmission assumes its the top directory and tries to rename every file with that prefix.

Example .torrent attached, the file structure is:

$ transmission-show TestA.torrent
Name: TestA
File: TestA.torrent

GENERAL

  Name: TestA
  Hash: b73b245c17f1b2105baa037e8b6c67cdea9c6cc3
  Created by: Transmission/2.82.1 (14216)
  Created on: Sun Mar 30 16:09:54 2014
  Piece Count: 1
  Piece Size: 32.00 KiB
  Total Size: 1.78 kB
  Privacy: Public torrent

TRACKERS

FILES

  TestA/TestA.a (0.27 kB)
  TestA/TestA.a_1.b (0.82 kB)
  TestA/TestB.a_2.b (0.69 kB)

Use case is rename first file, result is that second file is also renamed in the metadata and that causes a program failure (on Transmission-Qt an assertion is raised).

Proposed fix also attached. Tested with Transmission-Qt, the assertion didn't fire, nor the renaming of the second file.

Attachments (6)

000-torrent.c.diff (720 bytes) - added by rb07 8 years ago.
Fix.
TestA.torrent (291 bytes) - added by rb07 8 years ago.
Test case
001-torrent.c.diff (786 bytes) - added by rb07 8 years ago.
Corrected fix.
TestA.2.torrent (339 bytes) - added by rb07 8 years ago.
A test with sub-directory.
001-rename.patch (636 bytes) - added by rb07 8 years ago.
Improved patch.
002-rename.patch (3.1 KB) - added by rb07 8 years ago.
Improved (again), and test added.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by rb07

Fix.

Changed 8 years ago by rb07

Test case

comment:1 Changed 8 years ago by rb07

My fix is wrong, it doesn't work for renaming inner directories (it prevents that operation).

Renaming files has only 3 cases:

  1. Rename one file;
  2. Rename all files (a.k.a. rename top directory);
  3. Rename a subset, renaming an inner directory.

The first version of the fix only contemplates the first 2 cases.

Changed 8 years ago by rb07

Corrected fix.

Changed 8 years ago by rb07

A test with sub-directory.

comment:2 Changed 8 years ago by rb07

Fixed the problem described in comment:1.

But... testing this I also see there is need for preventing operation that are not allowed, for instance renaming a file or directory to the same name of another file or directory (at the same level), and in Windows there's the problem with case insensitivity. And what about an user which tries to "delete" a file by erasing the complete name.

Last edited 8 years ago by rb07 (previous) (diff)

comment:3 Changed 8 years ago by rb07

  • Summary changed from Renaming a file with common prefix to others causes failure to Problems renaming files

I ran into another 2 problems:

  1. Renaming with any rpc using tool (I'm testing with a modified transmission-remote) does not propagate the change to monitoring clients, Transmission-Qt in my test (testing now with the Web client... nothing, unlike when you delete a torrent which is shown immediately, renaming requires to trigger a reload).
  1. The test for a top directory rename is not robust enough. Any prefix of the top dir causes a rename of all files. I'm testing a change, will post the new patch later.

comment:4 Changed 8 years ago by cfpp2p

I don't know if the below external reports could be related or even helpful but quickly I see related to rename. So poorly reported maybe just junk to waste anyone's time. :-(

http://code.google.com/p/transmisson-remote-gui/issues/detail?id=801 http://code.google.com/p/transmisson-remote-gui/issues/detail?id=803#c1

Changed 8 years ago by rb07

Improved patch.

comment:5 Changed 8 years ago by jordan

rb07's 001-rename.patch looks sane and the unit tests pass, so I'm happy enough to take it as-is.

rb07, as a "stretch goal" I'd really like to see a new test added to rename-test.c that demonstrates the bug -- i.e., the test fails without your patch but passes with it. Could you update your diff to add such a test?

Last edited 8 years ago by jordan (previous) (diff)

Changed 8 years ago by rb07

Improved (again), and test added.

comment:6 Changed 8 years ago by rb07

While writing the test I found another error in the old code, and another in the test code, pretty obvious so I just fixed them.

The new rename-test.c fails with current 2.84, passes with patched 2.84.

comment:7 Changed 8 years ago by jordan

\o/

comment:8 Changed 8 years ago by jordan

  • Keywords backport-candidate-2.8x added
  • Milestone changed from None Set to 2.90
  • Status changed from new to assigned

comment:9 Changed 8 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

Added to trunk in r14317.

Note: See TracTickets for help on using tickets.