On Monday 18 February 2008 04:38:25 am Christian Boos wrote: > Hi Eli, > > Eli wrote: > > All, > > > > I wanted to draw attention to http://trac.edgewall.org/changeset/6513 > > just to make sure that the change is correct. (The concensus on IRC was > > to make this change.) > > Well, that change certainly looks correct, but it's not. I admit that > due to the various refactorings (and the nice way Subversion has to > throw away all the history that happened on the branches), it was not > easy to get back to the original changeset, see below. > > > This will fix a couple of places where we had a trailing '/' in the URL > > that did not appear to belong. In particular, the url while attaching a > > file to a ticket or wiki page had something/?arg=... instead of > > something?arg=... > > That trailing '/' has its use. This was introduced quite some time after > we introduced the attachment list page, in order to fix > http://trac.edgewall.org/ticket/4812 and be able to differentiate > between links to attachments and links to /lists/ of attachments. > One alternative solution would have been to URL escape the id part, but > I think this would lead to even more complexity and is contrary to what > we do for other URLs showing "hierarchical" resource names (wiki > subpages, milestones containing '/' in their name). > > > The original change that introduced that trailing '/' was > > http://trac.edgewall.org/changeset/6139 , the merging of > > context-refactoring to trunk. > > No, it was actually http://trac.edgewall.org/changeset/4813.
In that case, the trailing '/' change was reverted somewhere between r4813 and r6139 because r6139 changed the trailing '/' behavior on trunk. > So I > reverted your change, leaving in place some additional comments > explaining the need for the trailing "/" (see > http://trac.edgewall.org/changeset/6535). Thanks Christian. I added a testcase for #4812 to the testing branch, and updated the other testcases to expect the trailing '/'. http://trac.edgewall.org/log/sandbox/testing?rev=6540&stop_rev=6539 > Oh, btw, I'm curious about the reason why you keep r6120 and r6122 out > of the testing branch? Because r6120 and r6122 cause a regression: Test for regression of http://trac.edgewall.org/ticket/5022 ... FAIL According to that ticket, we should see a trac error, but we aren't anymore. I hadn't brought it up because sandbox/testing is still 3 months behind trunk, and it may have been fixed in trunk already. Eli ------------------. "If it ain't broke now, Eli Carter \ it will be soon." -- crypto-gram [EMAIL PROTECTED] `------------------------------------------------- --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to trac-dev@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en -~----------~----~----~----~------~----~------~--~---
