lint: add errors about duplicated extra-files and checksums & when extra-file paths contains '..' #5561
Conversation
rjbou
added
the
PR: QUEUED
rjbou
removed
the
PR: QUEUED
src/core/opamFilename.ml
Outdated
| @@ -9,6 +9,10 @@ | |||
| (* *) | |||
| (**************************************************************************) | |||
|
|
|||
| let might_escape path = | |||
| List.exists (String.equal Filename.parent_dir_name) | |||
| Re.(split (compile (str Filename.dir_sep)) path) | |||
There was a problem hiding this comment.
I always struggle to understand Re (and regular expressions in general) -- I understand that Filename.dir_sep is returning a string, and String.split_on_char expects a character to split on.
But since all known values of Filename.dir_sep are a single-character string, I'd go down that path.
(of course, this is just a minor thing, and please ignore if you're happy with regular expressions).
|
to me this looks great, thanks for your work on it; and your patience. |
src/core/opamFilename.ml
Outdated
| let sep = | ||
| match sep with | ||
| | `Unix -> "/" | ||
| | `Windows -> "\\" |
There was a problem hiding this comment.
are we sure all the / in the given path have been replaced by \ at this point? If both are present then we should check for both on Windows given they are both valid path separators
There was a problem hiding this comment.
Updates windows with `/|' search
|
Should I split the PR ? it is quite big and there is several purposes (fix, new lint, update) |
There was a problem hiding this comment.
Should I split the PR ? it is quite big and there is several purposes (fix, new lint, update)
No i think it's fine. Although the PR title is not very descriptive and is a bit confusing.
What about:
lint: add errors about duplicated extra-files and checksums & when extra-file paths might point to a parent directory
To good PR titles would be: |
Adding test cases is a normal part of every PR so i don't think it should be part of the title. My contraction of checksums is in my opinion a compact way of talking about url and extra-source without mentioning them, and the update to W59 is pretty minor and i don't think it's worth mentioning in the title. |
rjbou
force-pushed
the
lint-security
branch
3 times, most recently
from
e48ae8a
to
c5cca86
Compare
…out url section It highlights that when no url is present, W37 is not triggered
…eral times for a given url in url section
…eral times for a given url in extra-sources section
* Shows the difference of behaviour between 'opam lint' and 'opam lint --check-upstream' * Add test cases for special cases: conf flag, url containing git url
… more tied to --check-upstream option
rjbou
changed the title
New lints:
Update:
More testcases:
/cc @hannesm @reynir