Edgewall Software
Modify

Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#295 closed enhancement (fixed)

Branches, tags and renames should be indicated by the changeset view

Reported by: Jonas Borgström Owned by: Christopher Lenz
Priority: high Milestone: 0.9
Component: version control/changeset view Version: 0.6.1
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description (last modified by daniel)

The changeset view should indicate what kind of operations were performed in the revision.

Attachments (4)

svn_cp_mv_support.diff (6.1 KB ) - added by cboos@… 15 years ago.
Fix for #295, as a diff against [1090]
svn_cp_mv_support.1092.diff (7.7 KB ) - added by cboos@… 15 years ago.
The previous patch was buggy, but this one works and is well tested against [1092]
patch_for_295-1107.diff (35.3 KB ) - added by cboos@… 15 years ago.
Copy and Rename support patch, against [1107]
pop_to_del_python2_2.patch (1.1 KB ) - added by jmanning@… 15 years ago.
Revert pop to del to support python 2.2

Download all attachments as: .zip

Change History (23)

comment:1 by daniel, 15 years ago

Milestone: 0.7
Priority: normalhigh

comment:2 by daniel, 15 years ago

Milestone: 0.70.8

comment:3 by daniel, 15 years ago

Description: modified (diff)
Milestone: 0.80.9
Summary: Branches, tags and renames should be indicated by the changset viewBranches, tags and renames should be indicated by the changeset view

Related to #531.

by cboos@…, 15 years ago

Attachment: svn_cp_mv_support.diff added

Fix for #295, as a diff against [1090]

comment:4 by cboos@…, 15 years ago

The previous attachment is a patch that fixes this issue.

After applying it, simply resync the database, and that's it.

No more hundreds of entries for each tag or branch creation…

comment:5 by Christopher Lenz, 15 years ago

Your patch changes changeset.cs to include a diff_macros.cs file, which itself is not in the patch.

comment:6 by cboos@…, 15 years ago

Oops, sorry, I thought I checked for that. That line is not needed. I'll submit a new patch shortly.

by cboos@…, 15 years ago

Attachment: svn_cp_mv_support.1092.diff added

The previous patch was buggy, but this one works and is well tested against [1092]

comment:7 by cboos@…, 15 years ago

The patch svn_cp_mv_support.1092.diff is now correct except for the line

<?cs include "diff_macros.cs"?>

which should be removed.

Apparently that file was found in another trac installation, because my test server didn't complain.

It's certainly too late for me because I should have checked better…

comment:8 by cboos@…, 15 years ago

There's more work to do for this issue:

  • The links from the modified files to the diffs are mixed up, because the order of the node changes in the database doesn't match anymore the order given by the diff editor (this is fixed)
  • Renamings with modifications (as obtained after a svn mv --force a b are not displayed properly (to be done)

Once this is completed, I'll submit a new patch.

comment:9 by cboos@…, 15 years ago

Ok, I'm nearly there. It's working great, I have the information about rename and copy operations even under the funniest cases, but I'll test more heavily before submitting the patch.

I'm just finishing mirroring the little Subversion repository over there at http://svn.collab.net/repos/svn

I should be able to upload the patch tomorrow if no serious problem occurs.

comment:10 by cboos@…, 15 years ago

I've just finished to successfully test the first 11407 revs from a mirror of Subversion's svn repos: thanks wget''

… so I believe that I have a good fix for #295 now.

The proposed patch (attachment:patch_for_295-1107.diff) is up-to-date with the latest [1107] (i.e. the Zip support and the property changes support is also there).

New major features

  • Detection of rename and copy operations, for files and folders. The old path is shown in the summary inside (copied from …) or (renamed from a/b/c) group, where a/b/c links to the original. This is especially useful when looking at changesets which are semantically tags or branches: they usually involve a copy of a big tree, if not of the entire /trunk. Without this patch, you would get one entry for each file in the copied tree, with this patch there's just 1 copy entry to look at!
  • Support of copy and rename with changes operations (i.e. svn cp/mv --force operations). In addition to the link to the original, there's also a link to the diffs.
  • … in general, the patch seems pretty robust (see Testing below).

Here's a summary of all the possible changes that can happen in a Subversion Repository which can be now reported:

Action File Folder
Addition [p] [p]
Deletion
Rename [p] [d] [p]
Copy [p] [d] [p]
Modification [p] [d] [p]

[p] means : + eventually property changes [d] means : + eventually diffs

Note: a Modification doesn't necessarily imply there are diffs to display. This is what happens for binary files. In this case, I added a link to the (previous) version, to make the comparison easier.

There are two new colors: blue for Added, grey for Renamed. In case there are changes in addition to the base operation ([p] or [d]), the right half of the cell will be orange (the color of Modified).

New minor features and fixes

  • The count of diffs and prop changes are shown in the summary, e.g. instead of the (diff) link, one can see now something like (3 diffs, 1 prop)
  • Deleted entries are linked to the removed path (so one can actually see what was removed)
  • The content of properties is HTML-escaped
  • Links to modified files without diffs (i.e. binary files) are working again. In addition, an explicit link to the previous version is shown.
  • The diff options panel is also shown when no diffs are present but some ignore options have been set, so that they can be deselected.
  • The diff format ouput now also contains deleted/added files

Testing

  • Browsing 2100+ changesets on our Subversion repository
  • Browsing 1200+ changesets on a SVK repository (containing, among other things, a mirror of Trac)
  • Browsing 10000+ changesets on a mirror of the Subversion Subversion repository

It seems to work for me :)

Oh, I nearly forgot to mention:

in order to test this patch, a resync operation is needed'''

Implementation details

  • As shown in my early patches, the key for tracking rename and copy operations was to use repos.svn_repos_replay as the delta.Editor driver, during the 'resync' process, instead of the repos.svn_repos_dir_delta: the latter doesn't provide the copyfrom_path/copyfrom_rev information.
  • However repos.svn_repos_replay doesn't support reporting property changes, that's why repos.svn_repos_dir_delta must still be used while generating the diffs.
  • I kept the same node_change table in the database, and I trivially encode the (copyfrom_rev, copyfrom_path, path information for the new R (rename) and C (copy) actions.
  • Recording of the copyfrom_rev information is needed because one can't rely on svn.fs.node_created_rev in case part of a path has been renamed (see this thread, which seems to be still valid as of 1.1.1).

TODOs

  • Minor buglets:
    • Files modified below a renamed directory are wrongly shown as C instead of R. The rest (the diffs and links) is correct, though, so I think it's not a blocker (note to myself: check [B61])
    • A directory property change which actually is nothing more than a property copy might nevertheless show the entry as modified (note to myself: check [S340])
    • The display order of entries could be improved upon the lexicographic order. This would require knowing the type of nodes (file/dir).
    • There might be Python2.1 compatibility issues. I did my best to avoid them, but I couldn't check.
  • Future enhancements:
    • Record and display the type of nodes (file/dir)
    • Work on SVK support, by doing a special treatment of the svm:* and svk:* properties.

by cboos@…, 15 years ago

Attachment: patch_for_295-1107.diff added

Copy and Rename support patch, against [1107]

comment:11 by Christopher Lenz, 15 years ago

Owner: changed from Jonas Borgström to Christopher Lenz
Status: newassigned

Looks great, I'll try applying your patch.

comment:12 by Christopher Lenz, 15 years ago

Resolution: fixed
Status: assignedclosed

Patch applied in [1109]. Resync required for testing.

Please report problems with this change as new tickets.

comment:13 by jmanning@…, 15 years ago

I just discovered that this patch breaks python 2.2. The pop method is not available to a dictionary in 2.2. Maybe this is intentional, maybe not, but a patch to fix this regression will be attached shortly. (providing that my understanding is correct - del in 2.2 is equivalent to pop in 2.3 if the return value is not needed…). If you don't mind dropping 2.2 support, then feel free to ignore this patch.

by jmanning@…, 15 years ago

Attachment: pop_to_del_python2_2.patch added

Revert pop to del to support python 2.2

comment:14 by cboos@…, 15 years ago

As I said in the patch comment above, I didn't test against Python 2.1, simply tried to avoid incompatibilities. So of course your patch is welcome and I'm glad there's so few incompatibilities!

As of dropping 2.1 support completely in favor of Python 2.3, my understanding is that there seem to be many users/contributors supporting the idea, but the original Trac developers insist on compatibility with Debian stable.

comment:15 by Christopher Lenz, 15 years ago

pop_to_del_python2_2.patch applied in [1211]. Thanks!

comment:16 by cboos@…, 15 years ago

I noticed one situation in which a rename operation is not detected by Trac: it's when the two related changes (the delete 'D' and the add with history 'A+') are split into two distinct commits.

e.g. this situation:

$ svn mv projectA/src/file projectB/src/file
D  projectA/src/file
A  projectB/src/file 

$ svn status projectA/src/file projectB/src/file
D        projectA/src/file 
A   +    projectB/src/file 

$ svn ci -m "I moved that file to the project B..." projectB

$ svn ci -m "Oops, forgot to commit projectA..." projectA

I don't really know how to handle this. If anyone has an idea…

comment:17 by kkkkoaa01102006aaaa, 13 years ago

Keep a good job up!

comment:18 by Emmanuel Blot, 13 years ago

(do not remove the latest spam comment, it demonstrates a small bug in the th:wiki:TicketDeletePlugin)

comment:19 by Emmanuel Blot, 13 years ago

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christopher Lenz.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christopher Lenz 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.