gosrc: Parsing file field of go-source meta tag does not implement spec fully.

[dmitshur]

The documentation of go-source meta tag, https://github.com/golang/gddo/wiki/Source-Code-Links, states:

The file field is a URL template for a link to a line in a source file. The following substitutions are made in the file template:

{file} - The name of the file
{line} - The decimal line number.

However, the actual implementation does not implement that behavior fully and correctly.

Instead, it makes a number of assumptions about what the file field URL template will look like, and fails to do what's expected given the spec above if any of the assumptions are not met.

Put simply, the file field URL template is expected to have {file} in the first part, followed by a # character, followed by optional single {line} in the second part.

I've tried to document the exact consequences of the current implementation here:

https://github.com/golang/gddo/wiki/Source-Code-Links/_compare/c3ec2f0e81d8689d50447495bf06f0c813cd766f...f5b0915990618ae8a8cda98fa47f184a5f3e1c6a

It's not pretty, and it's not simple. 😧

When I tried to have a file field URL template that had both {file} and {line} come after the first #, the file field was effectively ignored. /cc @slimsag

The file field URL template should be implemented as described in the spec and then the limitations section should be removed from the wiki.

[dmitshur]

@adg, do you have a preference on how to resolve this?

I have a working MVP fix that basically adds an "else if" clause to the current matching code and handles the case that I want to support (having both {file} and {line} after the #, which the current special-cased code doesn't match), it looks something like this:

commit b9f313cf725b09b92d9068494dea846d56b17623
Author: Dmitri Shuralyov <[email protected]>
Date:   Tue Mar 8 22:44:39 2016 -0800

    WIP.

diff --git a/gosrc/gosrc.go b/gosrc/gosrc.go
index 41b41b3..f658dbe 100644
--- a/gosrc/gosrc.go
+++ b/gosrc/gosrc.go
@@ -387,6 +387,15 @@ func getDynamic(client *http.Client, importPath, etag string) (*Directory, error

    if isHTTPURL(sm.fileTemplate) {
        fileTemplate := replaceDir(sm.fileTemplate, dirName)
        parts := strings.SplitN(fileTemplate, "#", 2)
        if strings.Contains(parts[0], "{file}") {
            for _, f := range dir.Files {
                f.BrowseURL = strings.Replace(parts[0], "{file}", f.Name, -1)
            }
            if len(parts) == 2 && strings.Count(parts[1], "{line}") == 1 {
                s := strings.Replace(parts[1], "%", "%%", -1)
                s = strings.Replace(s, "{line}", "%d", 1)
                dir.LineFmt = "%s#" + s
            }
+       } else if strings.Contains(fileTemplate, "{file}") && strings.Contains(fileTemplate, "{line}") {
+           parts := strings.SplitN(fileTemplate, "{line}", 2)
+           for _, f := range dir.Files {
+               s := parts[0]
+               s = strings.Replace(s, "{file}", f.Name, 1)
+               f.BrowseURL = s
+           }
+           dir.LineFmt = "%s%d" + parts[1]
        }
    }

It's short and works and supports additional valid formats for the file field, but it only makes the code even more complex.

A good eventual solution, IMO, is to completely refactor the entire thing. It's very hard to work with and it's much more complicated than it needs to be as a result, because it was done by adding onto existing logic from much earlier times, and made many assumptions about what links to support.

However, going down that path, it's going to have to touch a lot of code. The way LineFmt is used now needs to get replaced.

Should I submit a CL that does the simple, additive fix first (it'll resolve the issue for me for now)? Or try to work on a large refactor that simplifies this functionality significantly and completely resolves the case (making the implementation work exactly as specified in the spec)?

[adg]

Thanks for the detailed analysis.

Should I submit a CL that does the simple, additive fix first (it'll resolve the issue for me for now)?

Do this, then add tests, then this (if you have time/energy):

Or try to work on a large refactor that simplifies this functionality significantly and completely resolves the case (making the implementation work exactly as specified in the spec)?

Sound good?

[dmitshur]

Ok, after adding tests, I was able to gain a better understanding of how the current code works, and was able to refactor the existing loop into a single case that handles both all previous behavior, and the added case I needed (when a {file} section is after the #).

It's slightly simpler and more general, but still doesn't implement the full spec. But I think this is the best that's possible before refactoring the entire LineFmt thing, which is extremely constraining. But refactoring LineFmt may not be easy, even lintapp depends on it.

[dmitshur]

CL submitted at https://go-review.googlesource.com/20749.