Move when default revision should be supplied

[SeanTAllen]

By default, when working with a git or hg repo, corral defaults not to putting a revision in the corral.json when the dependency is added instead it tries at runtime to pick a revision (current released version uses master, next release will use main).

This is problematic.

We should move to requiring a revision for git and hg sources at the time it's added OR we should provide a default (main) at that time.

Trying to read a corral.json that has no revision for git or hg should fail.

This is a breaking change.

[jasoncarr0]

Would you think we should have the VCS pick the default name at runtime (asynchronously)? Since it can be either main or master for git a there's a git incantation to get the default branch (that requires accessing the remote)
Seems this would also be a fix for #172 . I was looking into that and my first thought had been treating "" or another distinguished value as getting the default branch from VCS at fetch-time (or update-time, but I would hope it would be resolved by then)

Late-resolving the main-branch has the benefit that it could handle a repository which switches from master to main (or anything else)

[SeanTAllen]

I'm not a fan of spooky action at a distance. There is no "default" that everyone uses. I think it is best to not have a default.

Hopefully this will become less important once I introduce the packaging and release features so that we stop using git repos for everything and have actual downloadable packages.

[jemc]

I would agree with requiring the dependency entry to specify either a branch name, commit hash, or similar, with no implicit default.

[redvers]

Explicit > Implicit

[jasoncarr0]

Is anyone opposed to putting the revision into the locator, instead of having it be a separate field?
Otherwise it turns out to be a bit awkward when we want things like local files (as are in some of the tests). While these don't need a VCS, they certainly shouldn't have a required revision as it doesn't make sense.

Unfortunately in general this seems to create some redundancy for VCS sources? For a package repository, it should be possible to get the URL from the version. And that guessing seems to be what's often happening for VCS versions that it fetches.

For what it's worth:

In the case of other package management systems, including it seems common.
For npm, it's git+https://github.com/foo/bar.git#tag (and one can also do git+ssh)
Maybe there ought to be a more thought-out format change we should do here (and a well-chosen next step here that doesn't create too much weirdness).

[SeanTAllen]

Yes, I'm opposed to putting the revision into the locator.

So, for something like "local files", I think that the version should be ignored if there is one. It shouldn't be a valid part of the format. When there is a backend for "local file", you create that via corral and "version" isn't an option. It's whatever is there at that path.

For a package repository, it should be possible to get the URL from the version. And that guessing seems to be what's often happening for VCS versions that it fetches.

I'm not sure what "that guessing" means. For VCS's with this issue when implemented will require a "version" as I think it should. The only other reasonable option to me is "whatever you happen to get". The idea that it tried to check out "master" and now "main" if you didn't give anything is a bad idea in my mind. It's making way to many assumptions. Either require the version or don't.

I think it is reasonable to state that there are no defaults for the basics.

• For VCS the "basics" are a location and version.
• For local file the "basics" are a location in the local file system.
• For packages (which I will be adding support for in the not-do-distant future), it will be a location where the package and be retrieved and the version.
  Eventually there's additional things I will be adding to packages around supply chain verification but that's a project unto itself.

Eventually hopefully almost everything will be done using packages, with VCS as a fallback for code that isn't participating in the package system for whatever reason and local file for development workflows that cross bundles and also monorepo scenarios.

n.b. my usage of "package" here is a little confusing. because, corral handles bundles and bundles contain one or more pony packages.

I mean package in the sense of "a think I can download from a place like cloudsmith or github packages that is a release artifact". I think in the future I will probably start calling them "release artifacts" to disambiguate from the pony meaning of "package".

[jasoncarr0]

Sorry, the "guessing" I'm referring to is where the Constraints.best_revision function attempts to use the version as a revision, if the version happens to look like a simple version. This would be a similar situation with a package repository, where the final URL may well depend on the version similarly (although it can be smarter for constraints).

Agreed about the assumptions. The ickiest thing to me here is that the constraints code is hard-coding a default based on Github (not even Git: this is a Github specific!) and trying that

[SeanTAllen]

Agreed. I want to remove the guessing. It's bad.

[jasoncarr0]

Just to clarify above, for VCS you mean the "basics" would be a location and a revision? Not a version? Or else I'm not sure I understand

[SeanTAllen]

@jasoncarr0 are you using revision/version to distinguish different things? I'm not, I suspect you are.

[jasoncarr0]

@SeanTAllen Yes, I would think they have to be different. It doesn't make much sense to me to have a package with version "master". Right now each dependency in a bundle has two separate fields: revision and version. When fetching, revision is used first and then version if the version is simple.

[SeanTAllen]

@jasoncarr0 ah right so, I see what you are thinking.

I'm using it to mean the same thing. Via corral you set a locator and version for release artifact or VCS, you have to set both.

If a version/revision doesn't exist at the location, you error out.

You can put anything for revision/version but it might be invalid. So master probably makes no sense for a release artifact but 20210301 would of you are getting a nightly artifact.

[SeanTAllen]

This appears to break proper version resolution right now such that only versions that are branch name, tags, or commit shas actually work.

For example, I did this as a test:

    {
      "locator": "github.com/ponylang/ponycheck.git",
      "version": ">=0.6.0 <1.0.0"
    },

and it ended up setting to main which is very not correct.

However the constraint is being solved correctly and a revision is being calculated as 0.6.0