Allow git tags with / in the name and properly slugify

[humitos]

Closes #1989

[stsewd]

Related #2563 (comment)

[stsewd]

My above comment is about an issue with branches, no tags. So, can we allow / on branches names/slugs too?

[humitos]

@stsewd I think what you mentioned is probably another issue.

I mean, there two things here:

• tag/branch name that is returned by the parse_tags/branches method
• verbose_name that is saved in the database

It seems that some time ago, we weren't using VersionSlugFied so the slug was created in a hacky way all around the code.

Anyway, thanks to your comment I found that the parse_branches was doing what the VersionSlugField should be doing but in a hacky way. So, I updated that code to return the proper name of the branch instead of a manipulated one.

[humitos]

I suppose that my last commit should fix the comment that you linked because the slug is used to build the URL of the documentation but the verbose_name is used to create the link to Edit on Github. @stsewd can you confirm this?

[stsewd]

@humitos yes, this solves the problem.

[ericholscher]

Will this cause existing versions to be deleted and re-created? What happens with existing active versions that would be "re-tagged"? This should likely include a data migration, as with all changes to our slugging, but that will also breaking existing URL's to active versions, so we'll likely need redirects too.

[humitos]

Will this cause existing versions to be deleted and re-created?

I don't think so. We are using identifier to decide if the tag/branch comming from git is the same or not and this shouldn't touch the identifier.

https://github.com/rtfd/readthedocs.org/blob/1af444173481df9a7b16e015fc6b12abe8155e7e/readthedocs/restapi/utils.py#L27-L30

What happens with existing active versions that would be "re-tagged"?

I suppose that this won't happen.

This should likely include a data migration, as with all changes to our slugging, but that will also breaking existing URL's to active versions, so we'll likely need redirects too.

A couple of things here:

• we can't make a data migration because we don't have the full tag name since we were dropping it. I mean release/foobar was converted to foobar when parsing the git output, so we don't have a way to re-construct the missing part
• existing version slugs won't be updated because of the identifier mentioned above; verbose_name won't be updated and slug either
• Broken "Edit on GitHub" links will still be broken
• new versions will have the new verbose_name, which will create a better slug and the "Edit on Github" link will work

So, I'd say that it's safe to merge this PR but I would like to know if my thoughts are right and write a couple of tests with the scenario described in the bullets. What do you think? Do you see any other case that I'm not considering?

[ericholscher]

Yea. I see how this is a better approach. I still worry it will cause active versions that came from the pre-existing broken names to not be updated properly.

Was there a bug where we weren't properly updating these versions because the post-commit logic was parsing them properly, but the VCS code wasn't?

[ericholscher]

Also, Versions should really have slugs, heh.

[humitos]

I'm not 100% sure to understand your comments, but

I still worry it will cause active versions that came from the pre-existing broken names to not be updated properly.

Mmm... Yes. This won't be updated automatically. The sync versions is not too aggressive and do nothing if the identifier doesn't change but, what we can do here? I'm not sure how to fix already broken names.

Also, Versions should really have slugs, heh.

😕, what do you mean here? Versions have slugs, it's populated from verbose_name though.