Edgewall Software
Modify

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#8919 closed defect (fixed)

[PATCH] Condition for zip download of dir

Reported by: Martin.vGagern@… Owned by: Martin.vGagern@…
Priority: high Milestone: 1.0.2
Component: version control/changeset view Version: 0.12dev
Severity: normal Keywords: review zip
Cc: Thijs Triemstra Branch:
Release Notes:

Downloading directories from the TracBrowser works more reliably, especially for version control systems other than Subversion which had issues with that feature.

API Changes:
Internal Changes:

Description

source:trunk/trac/versioncontrol/web_ui/changeset.py@8899#L311 activates special behaviour for old_path == '/'. Other tests in that same method treat '/' and '' as equivalent, e.g. source:trunk/trac/versioncontrol/web_ui/changeset.py@8899#L249 has new_path not in ('', '/').

I think that it would be consistent to include the case of old_path == '' in that special case for the zip file name as well. It would help in addressing one aspect of an issue with the VersioningSystemBackend trac-bzr which has been reported in LP #394204.

Attachments (2)

ZipDownload.patch (6.8 KB ) - added by Martin.vGagern@… 11 years ago.
Add zip download functionality to browser
ChangesetEmptyPath.diff (1.4 KB ) - added by Martin.vGagern@… 11 years ago.
Unify treatment of '' and '/' in ChangesetModule

Download all attachments as: .zip

Change History (24)

comment:1 by Martin.vGagern@…, 11 years ago

There is another piece of code which treats '/' and '' differently: source:trunk/trac/versioncontrol/web_ui/changeset.py@8899#L278 checks if not old_path which evaluates to True as an empty string is treated as boolean False. Therefore old_path will be set to new_path, resulting in an equal diff between identical versions. Therefore the result will likely be an empty ZIP file.

On the whole I feel that this approach to dir downloads as changesets is a bit of a hack, and therefore likely to cause trouble with backends other than svn. This is true in particular because the dirs to be compared are not related by a rename or similar, so calculating their diferences might be expensive or completely unsupported. In my opinion it would be cleaner to walk repository nodes in order to generate the list of files present in a given revision. Any chance you'll implement this?

in reply to:  1 ; comment:2 by Christian Boos, 11 years ago

Milestone: 2.0

Replying to Martin.vGagern@…:

… On the whole I feel that this approach to dir downloads as changesets is a bit of a hack,

True.

In my opinion it would be cleaner to walk repository nodes in order to generate the list of files present in a given revision. Any chance you'll implement this?

Maybe… The odds change dramatically depending on whether you provide a patch or not ;-)

by Martin.vGagern@…, 11 years ago

Attachment: ZipDownload.patch added

Add zip download functionality to browser

in reply to:  2 comment:3 by Martin.vGagern@…, 11 years ago

Replying to cboos:

The odds change dramatically depending on whether you provide a patch or not ;-)

So let's see them change. How about trac-0.13? Or maybe even earlier?

You might wish to move all this stuff for turning an iterable of nodes into a zip into some method to be used by both browser and changeset. I'll leave a proper location for it to you. I suggest you base it on my method, which supports empty directories, symlinks, executable files as well as standards-compliant flagging of non-ascii file names. You could add things like a customizable root_name (i.e. top level path inside the ZIP) and similar.

comment:4 by Christian Boos, 11 years ago

That's a very good start indeed :-)

I'll need some time to digest the code and refactor it along the lines you suggested, but 0.12 sounds plausible now ;-)

by Martin.vGagern@…, 11 years ago

Attachment: ChangesetEmptyPath.diff added

Unify treatment of '' and '/' in ChangesetModule

comment:5 by Martin.vGagern@…, 11 years ago

And this patch should address the issues that I actually considered broken in changeset.py. Although it's now far less likely that one encounters one of these problems, they should be fixed nonetheless. I also got rid of the download special case, as I assume you'll apply my zip download patch in one form or another.

How about changing the milestone to 0.12 so you won't forget this ticket?

comment:6 by Remy Blank, 11 years ago

Milestone: 2.00.12
Owner: set to Christian Boos

I can't resist to so much enthusiasm :-)

Assigning to cboos, as he's much more familiar with that part of the code than I am.

comment:7 by Christian Boos, 11 years ago

Milestone: 0.12next-minor-0.12.x
Priority: normalhigh
Severity: trivialnormal

comment:8 by Thijs Triemstra <lists@…>, 10 years ago

Cc: lists@… added
Keywords: review added
Summary: Condition for zip download of dir[PATCH] Condition for zip download of dir

This is a patch that needs a review.

comment:9 by Christian Boos, 10 years ago

Milestone: next-minor-0.12.x0.13

That's for trunk.

comment:10 by Martin.vGagern@…, 10 years ago

If you're aiming for 0.13 with this, you might consider making it less dependent on SVN. Currently the patch attached here relies on properties like svn:special and svn:executable. This isn't very portable for other repository backends. I would suggest adding interface methods for is_executable and is_symlink, so that backends can deal with this in their own way.

comment:11 by Thijs Triemstra, 10 years ago

Cc: Thijs Triemstra added; lists@… removed

comment:12 by Thijs Triemstra, 10 years ago

Keywords: zip added

comment:13 by Martin.vGagern@…, 9 years ago

Ping? You've had the patches for 17 months now. Care to review them, and either commit them or ask for some specific improvement?

comment:14 by Christian Boos, 9 years ago

#10332 was closed in favor of this one. I believe this approach will also help for fixing #8336 (though I haven't verified yet).

comment:15 by Remy Blank, 8 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:16 by Christian Boos, 8 years ago

See also #10960 for a glitch we have with the current method.

comment:17 by Martin.vGagern@…, 8 years ago

Three years have passed since I first posted my patch. There were at least three other tickets which could probably be resolved along with this one here. Any chances of eventually including this into the codebase? Do you need detailed explanations for any part of my patch?

comment:18 by Christian Boos, 8 years ago

Milestone: next-stable-1.0.x1.0.2
Status: newassigned

Nobody got around addressing the concerns in comment:10 … but now that I look again at the patch, that doesn't seem to be a really blocking factor.

comment:19 by Martin.vGagern@…, 8 years ago

It might be easier to address those issues from comment:10 in another patch, once this one here has been merged. If you want me to, I can file a separate ticket for that, so you won't loose track of that room for improvement. The only reason why I added this comment here was that I thought a major release might be a good opportunity to introduce this kind of change.

comment:20 by Christian Boos, 8 years ago

Resolution: fixed
Status: assignedclosed

A case study in slacking, this one ;-)

There are a few more follow-ups expected, as the download a zip in the changeset module doesn't work as expected for git and hg. Nothing new, though, and not related to these changes.

comment:21 by Christian Boos, 8 years ago

Owner: changed from Christian Boos to Martin.vGagern@…

comment:22 by Christian Boos, 8 years ago

Release Notes: modified (diff)

Modify Ticket

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