[PATCH] Condition for zip download of dir

[Martin.vGagern@…]

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.

[Martin.vGagern@…]

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?

[Christian Boos]

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 ;-)

[Martin.vGagern@…]

Add zip download functionality to browser

[Martin.vGagern@…]

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.

[Christian Boos]

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 ;-)

[Martin.vGagern@…]

Unify treatment of '' and '/' in ChangesetModule

[Martin.vGagern@…]

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?

[Remy Blank]

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.

[Thijs Triemstra <lists@…>]

This is a patch that needs a review.

[Christian Boos]

That's for trunk.

[Martin.vGagern@…]

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.

[Martin.vGagern@…]

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

[Christian Boos]

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

[Remy Blank]

Preparing for 1.0.

[Christian Boos]

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

[Martin.vGagern@…]

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?

[Christian Boos]

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.

[Martin.vGagern@…]

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.

[Christian Boos]

A case study in slacking, this one ;-)

• [11743] committed part of ChangesetEmptyPath.diff​ (fumbled on the ticket ref in this one)
• [11744] committed a reworked version of ZipDownload.patch​
• [11745] implemented suggestion from comment:3

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.