r123/repo syntax in commit messages

[shesek]

There's a small issue when using the r123/repo syntax (#9570) in commit log messagess.

It seems like r123 is automatically parsed to r123/<repository commit was made to>, so using the r123/repo syntax causes it to think its r123/<repository commit was made to>/<'repo' as the path>.

This is especially problematic as my client (Eclipse) allows me to commit to multiple repositories together (which is actually a separate commit to each repository with the same commit message), so I have no way to make it link to the correct revision.

Also, I'm used to the r123/repo syntax and prefer to keep it unified across wiki/tickets/commits.

On top of that, it seems like its currently not possible to link to revisions on repositories other than the one the commit was made to (from the commit message).

Any chance it could check if a revision is already mentioned after r123, so when it does it wouldn't assume its the repository the commit was made to?

[shesek]

Maybe I should've added an example - r393/kohana is parsed as a link to /changeset/393/kohana/kohana when written in the commit message, which returns No node kohana at revision 393. It does work when I remove the extra kohana from the link.

It should be noted that, oddly, the title attribute of the link is correct, so it seems like it can find the commit I'm referring to, but it doesn't link to it.

[Remy Blank]

Replying to shesek:

It seems like r123 is automatically parsed to r123/<repository commit was made to>, so using the r123/repo syntax causes it to think its r123/<repository commit was made to>/<'repo' as the path>.

Does the [123/repo] syntax have the same issue? I would guess so.

On top of that, it seems like its currently not possible to link to revisions on repositories other than the one the commit was made to (from the commit message).

I can see why this would be the case, the rendering context is used to find the repository. Fixing this would probably require a small addition in syntax, something like [123//repo] or r123//repo to mean "don't use the context to find the repository".

Any chance it could check if a revision is already mentioned after r123, so when it does it wouldn't assume its the repository the commit was made to?

I don't understand that part. Did you mean "if a repository is already mentioned"?

[shesek]

Replying to rblank:

Replying to shesek:

It seems like r123 is automatically parsed to r123/<repository commit was made to>, so using the r123/repo syntax causes it to think its r123/<repository commit was made to>/<'repo' as the path>.

Does the [123/repo] syntax have the same issue? I would guess so.

I haven't tested it as I don't use that syntax, but yes, I think it does too.

On top of that, it seems like its currently not possible to link to revisions on repositories other than the one the commit was made to (from the commit message).

I can see why this would be the case, the rendering context is used to find the repository. Fixing this would probably require a small addition in syntax, something like [123//repo] or r123//repo to mean "don't use the context to find the repository".

Any chance it could check if a revision is already mentioned after r123, so when it does it wouldn't assume its the repository the commit was made to?

I don't understand that part. Did you mean "if a repository is already mentioned"?

Yes, that's what I meant. Sorry about the typo, can you update the ticket to fix that?

The // solution would be great too, but checking if a repository name is already mentioned (as I tried to say in the ticket but typod) would make it more transparent. If no repository name is mentioned, it'll default to the one the commit was made to. If one is mentioned, it'll ignore the context and use that instead.

[Remy Blank]

Replying to shesek:

The // solution would be great too, but checking if a repository name is already mentioned (as I tried to say in the ticket but typod) would make it more transparent. If no repository name is mentioned, it'll default to the one the commit was made to. If one is mentioned, it'll ignore the context and use that instead.

The reason I suggest an additional syntax is that we use the same syntax to specify a repository for a changeset, and to restrict a changeset to a sub-path. So for example, if changeset 123 in your repository modifies files in dir1 and dir2, and you would like to create a link to only the changes in dir2, you would use [123/dir2] in the changeset message. The context provides the repository name, and the /dir2 restricts the view to files below dir2.

Of course, outside of a changeset message, the context cannot be used to infer the repository, and in that case [123/repo] and [123//repo] would be equivalent.

Another solution would be to use the /repo suffix to find the repository first, and only if it cannot be found, use the context information. It would be ambiguous in rare cases, but is probably good enough.

Thoughts?

[shesek]

The last solution you wrote is exactly what I meant - only if a repository name is mentioned, one that exists in Trac, it'll be considered the repository name, otherwise it'll be considered as the path.

And yes, it can cause some ambiguosity when a repository contains a directory with the same name as the repository name - but I also think its good enough, as it should be quite rare.

Thanks!

[Christian Boos]

We have a similar situation for differentiating between a repository root and a path within the default repository (e.g. is /browser/xxx pointing to repository xxx or path xxx in the default repository?). The approach taken there is similar, first look for a repository named xxx, and if not found, target the path xxx in the default repository.

In the present situation, replace "default repository" by the repository given by the context.

[shesek]

If that's the solution being used for other places (tickets/wiki/comments), it seems completely reasonable to use that for commit messages too, even if it can be ambiguous in some cases.

[Christian Boos]

Well, this it's still not completely clear cut - one might think that when you're in the context of a repository, you would like to refer first to the local path, only to another repository if there's no such path. We just need to pick the "least surprising" behavior…

[shesek]

Well, its hard for me to tell. Since multiple-repositories support was added, I just got used to the fact that I have to tell it which repository I'm talking about when referring to a changeset.

For me, a contextual/trac-wide default repository could be completely dropped. I would prefer to always refer to a changeset with the repository it belongs to, even in commits, just for the sake of consistency. Its also much more readable that way, as a human would find 'parsing' those links as ambiguous as Trac does.

But other than that… I think checking for a path first is a better approach, as the chances of a collision happening are lower (usually there are less top-level directories ['trunk', 'tags' and 'branches'] than repository. a collision would only happen if the repository name is one of those. On the other hand, when trunk/tags/branches isn't used, there'll likely be more top-level directories than repositories)

[Remy Blank]

Reverse the priority of context and path to determine repository.

[Remy Blank]

9636-context-priority-r10116.patch​ reverses the priorities, and uses the path first to determine the repository, and only if a repository isn't found, uses the context. I find this to be more intuitive as well, so I vote for this as "least surprising".

[Christian Boos]

The only concern I have is that if you first start with a single repository and write e.g. r123/lib to target a path in this repository, then this will stop working if later on you add a new repository named lib. But you'd then get what you deserved for introducing the ambiguity in the first place, I suppose ;-)

[Remy Blank]

I take this as an "ok for me", then :)

[Remy Blank]

Patch applied in [10124].