Edgewall Software

Ticket #295 (closed enhancement: fixed)

Opened 5 years ago

Last modified 2 years ago

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

Reported by: jonas Owned by: cmlenz
Priority: high Milestone: 0.9
Component: version control/changeset view Version: 0.6.1
Severity: normal Keywords:
Cc:

Description (last modified by daniel) (diff)

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

Attachments

svn_cp_mv_support.diff (6.1 KB) - added by cboos@… 4 years ago.
Fix for #295, as a diff against [1090]
svn_cp_mv_support.1092.diff (7.7 KB) - added by cboos@… 4 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@… 4 years ago.
Copy and Rename support patch, against [1107]
pop_to_del_python2_2.patch (1.1 KB) - added by jmanning@… 4 years ago.
Revert pop to del to support python 2.2

Change History

Changed 5 years ago by daniel

  • priority changed from normal to high
  • milestone set to 0.7

Changed 5 years ago by daniel

  • milestone changed from 0.7 to 0.8

Changed 4 years ago by daniel

  • summary changed from Branches, tags and renames should be indicated by the changset view to Branches, tags and renames should be indicated by the changeset view
  • description modified (diff)
  • milestone changed from 0.8 to 0.9

Related to #531.

Changed 4 years ago by cboos@…

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

Changed 4 years ago by cboos@…

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...

Changed 4 years ago by cmlenz

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

Changed 4 years ago by cboos@…

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

Changed 4 years ago by cboos@…

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

Changed 4 years ago by cboos@…

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...

Changed 4 years ago by cboos@…

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.

Changed 4 years ago by cboos@…

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.

Changed 4 years ago by cboos@…

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.

Changed 4 years ago by cboos@…

Copy and Rename support patch, against [1107]

Changed 4 years ago by cmlenz

  • owner changed from jonas to cmlenz
  • status changed from new to assigned

Looks great, I'll try applying your patch.

Changed 4 years ago by cmlenz

  • status changed from assigned to closed
  • resolution set to fixed

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

Please report problems with this change as new tickets.

Changed 4 years ago by jmanning@…

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.

Changed 4 years ago by jmanning@…

Revert pop to del to support python 2.2

Changed 4 years ago by cboos@…

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.

Changed 4 years ago by cmlenz

pop_to_del_python2_2.patch applied in [1211]. Thanks!

Changed 4 years ago by cboos@…

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...

Changed 2 years ago by kkkkoaa01102006aaaa

Keep a good job up!

Changed 2 years ago by eblot

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

Changed 2 years ago by eblot

Add/Change #295 (Branches, tags and renames should be indicated by the changeset view)

Author



Change Properties
<Author field>
Action
as closed
Next status will be 'reopened'
to The owner will change from cmlenz. Next status will be 'closed'
 
Note: See TracTickets for help on using tickets.