Versioncontrol resource links

[osimons]

In versioncontrol resource support, the links generated by the resources are wrong for use by others. The 'source' realm should really link to browser and not source. Either that, or versioncontrol web handler should know how to respond to the /source links it generates. Either strategy is fine by me.

trac/versioncontrol/api.py

@@ -384 +384 @@
         if resource.realm == 'changeset':
             return href.changeset(resource.id, resource.parent.id or None)
         elif resource.realm == 'source':
-            return href.source(resource.parent.id or None, resource.id)
+            return href.browser(resource.parent.id or None, resource.id)
         elif resource.realm == 'repository':
-            return href.source(resource.id or None)
+            return href.browser(resource.id or None)
 
     def resource_exists(self, resource):
         if resource.realm == 'repository':

[Steffen Hoffmann]

Minor nit-pick, but it applies to current trunk too, where I found it by testing.

[Stefan]

Version should always point to the version where an issue/bug got introduced, so that it is correctly listed when querying for known/fixed bugs/issues for a given version.

[Steffen Hoffmann]

And your implicit assertion, that this is broken since 0.12dev, is due to first appearance of the get_resource_url method in trac.versioncontrol.api in r9125, right? Reasonable, I see. Learned one more thing today, thanks.

[Remy Blank]

The patch looks good to me. Please apply!

[osimons]

Committed in [10875], and also including support for rev that is quite essential for source URLs.

[Christian Boos]

Well, sorry for the late participation, but I'd have preferred the other option, be consistent between the request handler and the realm (maybe we can still do that on trunk?).

[osimons]

Well, the consistency is largely an illusion even though the basic default modules like Wiki and Ticket work that way. Other resource providers outside Trac use similar constructs, and having a prominent inconsistency in Trac allows us and others not to take a 1:1 mapping for granted…

So having both 'repository' and 'source' link to /browser makes sense to me. Or were you proposing that we should also handle /repository as well as /source URLs?

[Christian Boos]

It's a gratuitous inconsistency, and had we been stricter about this from the start, the plugins would have followed… A 1:1 mapping is just an intuitive to have, I can't see a downside. We should indeed handle /repos as well. Not that I'm not proposing to enforce anything or to assume this equivalence in the code, just to be consistent for the sake of helping people typing either TracLinks or direct URLs.

[osimons]

Just to add to this mix - this in from jomae as we had a quick interchange about merge backlog and a related issue turned up: "source as realm in a report does not work because SOURCE_VIEW permission which such a realm requires does not exist."

So actually, it is the expectation of 1:1:1 relationship between realm, URL and permission pattern. Which will never work unless we enforce it through API change in some future version - like GenericTrac perhaps…? ;-)

Make it match if we can, but if not then we should be fine with that too. As you say cboos, it is not like this inconsistency is a new thing.

[Steffen Hoffmann]

I'd suggest to add full support, including versions/revisions as osimons did for the 'browser'/'source' realm, to get_resource_url for tickets and wiki too, even if it is less common for tickets (/ticket/1?version=3) but works after all. For wiki pages it's essential, like for resources inside repositories. Will provide a patch, if you agree and osimons is not faster again.

[Christian Boos]

Replying to osimons:

Just to add to this mix - this in from jomae as we had a quick interchange about merge backlog and a related issue turned up: "source as realm in a report does not work because SOURCE_VIEW permission which such a realm requires does not exist."

As you well know, the <XYZ>_VIEW permissions are a legacy from the pre-fine grained permissions days, and I always wanted to have simpler permissions actions (verbs like 'view') to be asked on resources. Much simpler, not redundant, so more robust. IIRC that was even in the code (at this very place in the report module) before I was "asked" to remove it…

So actually, it is the expectation of 1:1:1 relationship between realm, URL and permission pattern. Which will never work unless we enforce it through API change in some future version - like GenericTrac perhaps…? ;-)

Like many things … yes. I still hope (dream?) that at some point I'll be able to resume serious work on Trac before it gets really too late ;-)

Make it match if we can, but if not then we should be fine with that too.

Exactly.

[osimons]

Replying to cboos:

Make it match if we can, but if not then we should be fine with that too.

Exactly.

So no cause for further changes then? I'll merge it as soon as the merge backlog is cleared.

BTW, I see the report realm/permission inconsistency dates all the way back to [6245]… I suppose that change would not have been made if trac.versioncontrol had supported resources at the time. At that time we even planned to deprecate the report module, so who knows ;-)

[osimons]

Replying to osimons:

I'll merge it as soon as the merge backlog is cleared.

FYI: It was merged at some point and is now part of trunk (according to svn merge tracking).