Sanitize git branch names

[gpetiot]

Fix #216

[NathanReb]

I don't think this is the right fix, according to the issue, the tag wasn't what caused troubles but the branch created on the local opam-repository repo was. It's indeed likely that the tag needs to be sanitized as well but this change has other implications.

Changing the tag will change the version number we use accross the whole tool and that's not something we want, there are places where we want the actual version provided by the user (in the CHANGELOG) I mean.

I think we need to distinguish those first and have a clear policy, where we use the provided version number as much as possible and only sanitize when strictly necessary! We could even introduce a type for sanitized names to prevent further issues with this!

[NathanReb]

It looks like we use the tag in most places instead of the actual version and that's what I'd like to avoid, i.e. only use a sanitized string where it is strictly necessary.

How about the following:

• we create a Git.branch_or_tag_name type and a string -> branch_or_tag_name function. Internally it should probably just be a string.
• we make it so that the functions that do the tagging or branch creation take that type as an input

That way we have an abtract type that can't be wrongly used elsewhere. What do you think?

[gpetiot]

I rebased the branch and made substitutions more explicit (for example ~ is now replaced with _TILDE_ instead of _) to avoid name collisions.

[NathanReb]

I just gave this a spin and it seems it replaces the characters pretty much everywhere:

• The release name: https://github.com/NathanReb/dune-release-testing/releases/tag/v0.3.0_TILDE_beta1 (also I'm not sure why it comes with : Distribution ... instead of it being just the name, it might be unrelated to this PR but we need to investigate!)
• The tarball: Wrote archive _build/dummy-v0.3.0_TILDE_beta1.tbz
• Most importantly, it releases the wrong version to opam: [new release] dummy (0.3.0_TILDE_beta1) NathanReb/opam-repository#16 (you can see it wrote the opam file in the wrong folder)

Before this the tag would be equal to the version in the changelog and therefor the version in the changelog would be used everywhere. I think that should remain true and we should only rewrite it for git branches and tags.

I know we use the tag in the code to get the version so I suggest we do one of two things:

1. either we change that to use the version in the changelog (or provided on the CLI if that's something we support) and use that as a reference. This make break some usecases for users that weren't running dune-release tag and therefore didn't depend on the changelog so strongly, we'd need to look into this.
2. we still use the tag as a reference but apply the substition the other way around when doing so to get the actual version, that seems relatively safe but might in some very rare cases substitute something it wasn't supposed to.

What do you think?

[gpetiot]

I addressed your last review, now the substitutions should only be made for the tags/branches, and we don't need to reverse it, on the other hand we rely on the tag from the changelog to create the name of the archive and so on (with a fallback to the git tag name if no changelog is present).

I've kept the changes in the mli files (some types changing from string to Vcs.commit_ish as I find it clearer), but we can revert that if that's too much noise.

The "Distribution ..." name for tags is set by dune-release tag but that's not related to this PR!

[NathanReb]

I was not talking about the tag message but about the release name, see the following picture:

I'll test again on master to see if it is indeed unrelated to this PR, in which case we really need to fix it.

[NathanReb]

Master is indeed broken

[gpetiot]

I've rebased this PR, and made sure we only sanitize the name and not the tag_name of a release.

[NathanReb]

I tested it to make sure it all works and there are a few things to note:

1. It creates the release with the right name (thanks for fixing this!) but the log line says:

[?] Create release v0.3.0_TILDE_beta1 on https://github.com/NathanReb/dune-release-testing.git? [Y/n]

This is purely a user experience thing but I wonder if it shouldn't use the name for the release here, i.e.:

[?] Create release v0.3.0~beta1 on https://github.com/NathanReb/dune-release-testing.git? [Y/n]

That's more of an open question, I feel like it should use the future release name here in the logs. What do you think?

2. This new version doesn't sanitize the name of the branch on opam-repository and therefore when running dune-release opam submit I get the following errror:

$ dune-release opam submit --opam-repo NathanReb/opam-repository
[-] Submitting 
[-] Preparing pull request to NathanReb/opam-repository
[-] Fetching https://github.com/NathanReb/opam-repository.git#master
[-] Checking out a local release-dummy-0.3.0~beta1 branch
dune-release: [ERROR] Exit code 128 from command 
    `git --git-dir .git --work-tree ./ checkout --quiet -b
       release-dummy-0.3.0~beta1 b2dab0c51409d50bb6fc208451ceb64d1af76bda`:
  fatal: 'release-dummy-0.3.0~beta1' is not a valid branch name.

[gpetiot]

I fixed this, and also the message associated to the tag set by dune-release tag that would be "Distribution (sanitized tag)" instead of "Distribution (version)". I've run the full dune-release set of commands and it looks good.

[NathanReb]

It looks good! Thanks, I think this is a great change.

I have added a few minor remarks. One thing I also want to mention is that the cram test is a bit verbose, I'm not sure we need all of the logs. Do you think we could eventually grep for the parts we care about?
Otherwise there's going to be diffs very often there, we'll get use to it and simply always promote until one day we actually miss a meaningful change in the output.

Another thing is that it might be useful to make commit_ish abstract and provide pretty printers and/or a to_string function but we don't have to do this straight away, the change is already valuable as it is.

[gpetiot]

I have added a few minor remarks. One thing I also want to mention is that the cram test is a bit verbose, I'm not sure we need all of the logs. Do you think we could eventually grep for the parts we care about?

I've filtered the output of the cram test with (part of) the tag name so we only get the unsanitized version and the sanitized tag in the output.

Another thing is that it might be useful to make commit_ish abstract and provide pretty printers and/or a to_string function but we don't have to do this straight away, the change is already valuable as it is.

It sounds good to me, but it can be done in another PR indeed!

[NathanReb]

Awesome, thanks!

That's a great feature and I think people are really going to enjoy being able to release beta versions with dune-release!