[WIP] Add source.revision #623

Alloyed · 2016-10-06T05:35:47Z

Resolves #569.

In hg, this is implemented by passing revision into the clone command (hg clone myrepo --rev myrev).

In git, this is implemented by performing a full clone (git clone myrepo) and then doing git checkout myrev.

Originally I planned to have a special case that compares source.tag and source.revision when both are defined. This would double check the tag's commit-id, but from a security perspective that just moves the trust from the repository host to the rockspec host. Git already supports tag/commit signing with GPG, which are better ways to verify trust.

todo:

input validation (revisions should only be full commit/changeset IDs)
git optimizations

tests:

error on revision in rockspec < 3.0
error on revision in unsupported URL type (other schemes don't do this it looks like)
simple revision in git
simple revision in hg
revision + tag
revision + branch (with --single-branch this can be special-cased to avoid full clone)
revision + tag + branch

hishamhm

(This is my first time using the new code-review feature in Github, hope I'm doing this right!)

hishamhm · 2016-10-06T14:57:03Z

src/luarocks/type_check.lua

@@ -58,6 +58,7 @@ local rockspec_types = {
      dir = string_1,
      tag = string_1,
      branch = string_1,
+      revision = string_1,


You should use revision = { _type = "string", _version = "3.0" } here (or you can even create a string_3 variable)

I can do this, but I'm wondering where I should be putting my more complex validation. I need to make sure

the URL specifies either a git or hg repo when revision is specified

that revision is a 40-character hex string

I spotted _pattern, which led me to try _pattern = string.rep("%x", 40), and that works, but it has a very silly error message:

Error: Error loading rockspec: /home/kyle/src/luarocks/test/testfiles/invalid_revision-1.0-1.rockspec: Type mismatch on field source.revision: invalid value HEAD does not match '%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x%x'

hishamhm · 2016-10-06T14:57:47Z

src/luarocks/fetch.lua

@@ -235,6 +235,10 @@ function fetch.load_local_rockspec(filename, quick)
   if rockspec.source.cvs_module then rockspec.source.module = rockspec.source.cvs_module end
   if rockspec.source.cvs_tag then rockspec.source.tag = rockspec.source.cvs_tag end

+   if rockspec.source.revision and not deps.format_is_at_least("3.0") then


With the version checking for the revision field in the type checker, you don't need to version-check again here.

hishamhm · 2016-10-06T14:59:10Z

Yay, thanks for taking on this feature! I added some comments.

Alloyed · 2016-10-08T21:26:57Z

Thanks for the review, I'm working on a commit that resolves it and adds a few of the simpler tests right now.

Something else: I noticed that luarocks already calls the "-1" part of a rockspec version a "rockspec revision", so I'm worried about the two getting confused for each other. Wikipedia suggests changeset as a possible alternate name I'd be okay with.

hishamhm · 2016-10-11T03:55:47Z

I think source.revision won't be ambiguous (that's what the table namespacing is for, after all) and is more obvious than "changeset" — every time I saw people asking for this feature they said "I'd like my rockspec to point to a particular revision", not "changeset". (I'm trying to avoid here a situation where we pick a technically correct name that ends up being more cumbersome in regular use, as it happened when we picked "scm" to refer to in-development rocks).
Do we need to force all 40 characters in the revision field? A lot more often I see people using much shorter substrings of revisions and that works fine in practice. Again, I'm thinking about convenience here. Those who want the safety of the full hash can still have it. But it's customary for Git-based tools to accept substrings of the hash.
As for the error message, how about adding an _expecting field to be used for composing the error message? As in _expecting = "a hexadecimal revision hash"

Alloyed · 2016-10-11T19:28:40Z

Understood on the name/typechecking
The CLI tools will allow up to the shortest unique substring, but the issue is that this leads to a potential time-bomb: what if another commit made after the fact shares that substring? If the original commit is still available a rockspec that used to work will break, or if (say, a bad actor) came along and replaced the original git history, you could get a completely different commit.
If a rockspec is temporary this isn't a big deal, so maybe this is an argument to be more lenient here, and more strict in luarocks.org.

hishamhm · 2016-10-12T01:04:00Z

If a rockspec is temporary this isn't a big deal, so maybe this is an argument to be more lenient here, and more strict in luarocks.org.

This is not a big practical problem at luarocks.org because luarocks upload also stores a .src.rock file which contains a copy of the sources. luarocks build uses the .src.rock by default instead of fetching from git.

mpeterv · 2016-10-12T09:07:15Z

luarocks build uses the .src.rock by default instead of fetching from git.

So does luarocks install

codecov-io · 2016-12-05T11:24:14Z

Current coverage is 87.93% (diff: 100%)

Merging #623 into luarocks-3 will increase coverage by 0.23%

@@           luarocks-3       #623   diff @@
============================================
  Files              66         69     +3   
  Lines            6936       6998    +62   
  Methods             0          0          
  Messages            0          0          
  Branches            0          0          
============================================
+ Hits             6083       6154    +71   
+ Misses            853        844     -9   
  Partials            0          0

Powered by Codecov. Last update 3df4bd5...b923f22

When a branch is specified, only that branch will be cloned, even when revision is specified.

Source: https://github.com/git/git/blob/master/Documentation/RelNotes/1.7.10.txt#L80

Alloyed · 2017-01-03T19:41:43Z

Something I noticed: right now, if you use luarocks with an http/https/ssh url, luarocks is forced to do a full clone. Do you remember the reasoning behind it? At least on my machine doing a shallow clone works fine for all three types.

Anyways, an in-words description of how cloning works in this PR:

If you don't need a specific revision, you get a shallow clone (git clone --depth=1). this includes normal branches and tags.
If you do need a specific revision, and you specify a branch, you get only the commits on that branch (git clone --single-branch)
If you specify a revision by itself you get the entire repo. (git clone --)

daurnimator · 2017-01-04T02:52:28Z

src/luarocks/fetch/git.lua

+      else
+         -- otherwise, we need a full clone to make sure we fetch all available
+         -- revisions
+         depth = "--"


This seems prone to later issues: -- signifies "things after this point are arguments not options".

daurnimator · 2017-01-04T02:53:01Z

src/luarocks/fetch/git.lua

+      end
+   end
+
+   local command = {fs.Q(git_cmd), "clone", depth, rockspec.source.url, module}


Do url and module need to be quoted?

daurnimator · 2017-01-04T02:53:25Z

src/luarocks/fetch/git.lua

+
+   local committish = rockspec.source.revision or tag_or_branch
+   if committish then
+      if not fs.execute(fs.Q(git_cmd), "checkout", committish) then


Quote commitish (and/or use -- before it)

daurnimator · 2017-01-04T02:54:20Z

src/luarocks/fetch/hg.lua

+   -- TODO: if revision is specified and so are tag/branch, then validate to
+   -- make sure tag/branch matches revision
+   local rev = rockspec.source.revision
+   rev = rev or rockspec.source.tag


No need to keep assigning, just start the line from or

daurnimator · 2017-01-04T02:55:02Z

src/luarocks/fetch/hg.lua

+   rev = rev or rockspec.source.branch
+
+   if rev then
+      command = {hg_cmd, "clone", "--rev", rev, url, module}


Why did git_cmd need fs.Q but hg_cmd doesn't?

hishamhm · 2018-04-24T20:14:10Z

This was auto-closed by accident when the (now merged) luarocks-3 branch was deleted. Sorry!

hishamhm requested changes Oct 6, 2016

View reviewed changes

Alloyed force-pushed the scm-revisions branch from b385a44 to 9137141 Compare October 18, 2016 21:35

Alloyed added 7 commits January 2, 2017 13:36

Add rockspec.source.revision

49d9892

Validate source.revision in type checker

b4d56d6

Add tests

81e9827

Add tests for successful checkouts

a273384

Allow short hashes

ac5eba9

Implement _expecting in type checker

5168888

Special case rockspecs with branch and revision

6029e29

When a branch is specified, only that branch will be cloned, even when revision is specified.

Alloyed force-pushed the scm-revisions branch from 9137141 to 6029e29 Compare January 2, 2017 20:46

Don't clone single branches if git is too old

b923f22

Source: https://github.com/git/git/blob/master/Documentation/RelNotes/1.7.10.txt#L80

daurnimator requested changes Jan 4, 2017

View reviewed changes

hishamhm closed this Mar 12, 2018

hishamhm changed the base branch from luarocks-3 to master April 24, 2018 20:13

hishamhm reopened this Apr 24, 2018

hishamhm force-pushed the master branch from ba7e692 to f893acf Compare August 11, 2018 01:56

hishamhm force-pushed the master branch from 97b100d to c451dd0 Compare April 26, 2019 22:02

hishamhm force-pushed the master branch from e14fc10 to 1411cd7 Compare April 16, 2022 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add source.revision #623

[WIP] Add source.revision #623

Alloyed commented Oct 6, 2016 •

edited

hishamhm left a comment

hishamhm Oct 6, 2016

Alloyed Oct 8, 2016

hishamhm Oct 6, 2016

hishamhm commented Oct 6, 2016

Alloyed commented Oct 8, 2016

hishamhm commented Oct 11, 2016 •

edited

Alloyed commented Oct 11, 2016

hishamhm commented Oct 12, 2016

mpeterv commented Oct 12, 2016

codecov-io commented Dec 5, 2016 •

edited

Alloyed commented Jan 3, 2017

daurnimator Jan 4, 2017

daurnimator Jan 4, 2017

daurnimator Jan 4, 2017

daurnimator Jan 4, 2017

daurnimator Jan 4, 2017

hishamhm commented Apr 24, 2018

[WIP] Add source.revision #623

Are you sure you want to change the base?

[WIP] Add source.revision #623

Conversation

Alloyed commented Oct 6, 2016 • edited

hishamhm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm commented Oct 6, 2016

Alloyed commented Oct 8, 2016

hishamhm commented Oct 11, 2016 • edited

Alloyed commented Oct 11, 2016

hishamhm commented Oct 12, 2016

mpeterv commented Oct 12, 2016

codecov-io commented Dec 5, 2016 • edited

Current coverage is 87.93% (diff: 100%)

Alloyed commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm commented Apr 24, 2018

Alloyed commented Oct 6, 2016 •

edited

hishamhm commented Oct 11, 2016 •

edited

codecov-io commented Dec 5, 2016 •

edited