Edgewall Software
Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11741 closed defect (fixed)

Renaming a wiki page with attachments does not delete the old name attachment folder

Reported by: Francesca Mazzoni <francesca.mazzoni@…> Owned by: Jun Omae
Priority: normal Milestone: 0.12.7
Component: attachment Version: 0.12.5
Severity: minor Keywords:
Cc:
Release Notes:

Fix not deleting the old attachment directory when wiki page is renamed.

API Changes:

Description (last modified by Jun Omae)

I'm on Debian stable with Python 2.7.3 and installed from Debian package.

When I rename a Wiki page that contains attachments, the (empty) folder in which the attachments were stored before the page renaiming is still in the file system.

Steps to reproduce:

1) Create a new wiki page (say MyOldPage), add an attachment (say MyAttach.tgz). The attached file is stored in <trac-env>/attachments/wiki/MyOldPage/MyAttach.tgz.

2) Rename MyOldPage to MyNewPage.

The attachments are now (correctly) stored in <trac-env>/attachments/wiki/MyNewPage/.

I still have the <trac-env>/attachments/wiki/MyOldPage folder.

I think that the folder should be removed, since it is empty.

Attachments (0)

Change History (10)

comment:1 Changed 4 years ago by Jun Omae

Component: wiki systemattachment
Description: modified (diff)
Keywords: attachment removed

comment:2 Changed 4 years ago by Jun Omae

Milestone: 0.12.7
Owner: set to Jun Omae
Status: newassigned

Thanks for the report. Reproduced on 0.12-stable. The following patch would fix it.

  • trac/attachment.py

    diff --git a/trac/attachment.py b/trac/attachment.py
    index af1a71d..040214e 100644
    a b class Attachment(object):  
    348348        def do_reparent(db):
    349349            for attachment in list(cls.select(env, parent_realm, parent_id,
    350350                                              db)):
    351                 attachment_dir = os.path.dirname(attachment.path)
     351                attachment_dir[0] = os.path.dirname(attachment.path)
    352352                attachment.reparent(new_realm, new_id)
    353353        if attachment_dir[0]:
    354354            try:

I'll push the patch with unit test on 0.12.7.

comment:3 Changed 4 years ago by Jun Omae

This issue occurs on only 0.12.x. Milestone 0.12.7 for only security fixes. But this issue is not a security issue. Should we fix it? or leave it?

comment:4 Changed 4 years ago by Ryan J Ollos

My feeling is that it is really up to you, particularly since we've committed to at least one more release from the 0.12-stable branch. I'd probably go ahead and push the fix though if I had any use for 0.12.x.

After we do the 0.12.7 release though, if we are treating it as a security-fix only branch then future releases will only be as needed.

comment:5 Changed 4 years ago by Jun Omae

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for the comments. Fixed in [13610] and recorded only in [13611-13612].

comment:6 Changed 4 years ago by Ryan J Ollos

Strangely, trunk still shows [13610] from the 0.12-stable branch as eligible for merge. The record-only merges look correct. I guess that must be a bug in Trac?

comment:7 Changed 4 years ago by Jun Omae

Subversion client shows the same. I don't think that is a bug.

$ svn mergeinfo --show-revs eligible ^/branches/0.12-stable ^/trunk
r13610

Seems the revision should be recorded from 0.12-stable.

$ svn merge -c13610 --record-only ^/branches/0.12-stable

comment:8 Changed 4 years ago by Ryan J Ollos

Maybe it is an issue with Subversion client then, or the way in which the merge was done. Based on previous cases I've seen, it seems like merging [13611] would "carry-over" the record-only of [13610]. Here are some examples: [13155], [13127], [13121], [13091] (more).

I'm not sure what is different in this case, compared to [13090]/[13091]:

$ svn diff -c 13090 https://svn.edgewall.org/repos/trac
Index: branches/1.0-stable
===================================================================
--- branches/1.0-stable (revision 13089)
+++ branches/1.0-stable (revision 13090)

Property changes on: branches/1.0-stable
___________________________________________________________________
Modified: svn:mergeinfo
   Merged /branches/0.12-stable:r13089
$ svn diff -c 13091 https://svn.edgewall.org/repos/trac
Index: trunk
===================================================================
--- trunk       (revision 13090)
+++ trunk       (revision 13091)

Property changes on: trunk
___________________________________________________________________
Modified: svn:mergeinfo
   Merged /branches/1.0-stable:r13090
   Merged /branches/0.12-stable:r13089

My usual process is to run this command:

$ svn merge -c 13090 --record-only ^/branches/1.0-stable trac-trunk
$ svn --version
svn, version 1.8.8 (r1568071)
   compiled Aug 13 2014, 17:12:39 on x86_64-pc-linux-gnu

Copyright (C) 2013 The Apache Software Foundation.
This software consists of contributions made by many people;
see the NOTICE file for more information.
Subversion is open source software, see http://subversion.apache.org/

The following repository access (RA) modules are available:

* ra_svn : Module for accessing a repository using the svn network protocol.
  - with Cyrus SASL authentication
  - handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
  - handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol using serf.
  - using serf 1.3.3
  - handles 'http' scheme
  - handles 'https' scheme
Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:9 in reply to:  8 ; Changed 4 years ago by Jun Omae

My usual process is to run this command:

$ svn merge -c 13090 --record-only ^/branches/1.0-stable trac-trunk

Yes. After I committed [13609], I've merged [13609] to 1.0-stable and [13610] to trunk as record-only like this.

$ svn merge -c13609 --record-only ^/branches/0.12-stable branches/1.0-stable
$ svn ci
....
Committed revision 13610.
$ svn merge -c13610 --record-only ^/branches/1.0-stable trunk
$ svn ci
....
Committed revision 13611.

I investigated that.

$ svn --version | head -1
svn, version 1.6.17 (r1128011)
$ svnadmin create svntest
$ svn co file://$PWD/svntest svntest.wc
Checked out revision 0.
$ cd svntest.wc
$ mkdir trunk branches branches/{0.12,1.0}-stable
$ svn add trunk branches
A         trunk
A         branches
A         branches/1.0-stable
A         branches/0.12-stable
$ svn ci -q -m 'testing'
$ touch branches/0.12-stable/test.txt
$ svn add branches/0.12-stable/test.txt
A         branches/0.12-stable/test.txt
$ svn ci -q -m testing
$ svn up
At revision 2.
$ svn merge -c2 --record-only branches/0.12-stable branches/1.0-stable
$ svn ci -q -m merging
$ svn up
At revision 3.

Revision 3 is record-only merge.

Merging this revision as record-only, as the result, we get only changes on 1.0-stable in svn:mergeinfo. That's unexpected result.

$ svn merge -c3 --record-only ^/branches/1.0-stable trunk
$ svn diff

Property changes on: trunk
___________________________________________________________________
Added: svn:mergeinfo
   Merged /branches/1.0-stable:r3

Instead, merging without record-only, we would get changes on 0.12-stable and 1.0-stable in svn:mergeinfo. That's expected result.

$ svn revert -R .
Reverted 'trunk'
$ svn merge -c3 ^/branches/1.0-stable trunk
--- Merging r3 into 'trunk':
 U   trunk
$ svn diff

Property changes on: trunk
___________________________________________________________________
Added: svn:mergeinfo
   Merged /branches/1.0-stable:r3
   Merged /branches/0.12-stable:r2

Therefore, we should merge without record-only for merging changes of record-only merge.

comment:10 in reply to:  9 Changed 4 years ago by Christian Boos

Replying to jomae:

Merging this revision as record-only, as the result, we get only changes on 1.0-stable in svn:mergeinfo. That's unexpected result.

Well, unintuitive perhaps, but that's a perfectly reasonable behavior: with --record-only you only manipulate the target's svn:mergeinfo related to the specified source path and you tell it to not apply any other changes (changes to files, properties, including changes to other paths in the svn:mergeinfo property itself).

Therefore, we should merge without record-only for merging changes of record-only merge.

Yep, that's the way. It's a common gotcha here at work and the source of endless fun ;-)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.