Repository loading: update load from diff mechanism - #6871
Conversation
40a76f6 to
bbdea9b
Compare
does this mean the test results are from 2.4.1? |
|
at commit f370351, just before incremental loading merge |
kit-ty-kate
left a comment
kit-ty-kate
left a comment
There was a problem hiding this comment.
partial review (i'll look at the more complicated load_opams_from_diff later)
| Filename.concat (Dir.to_string t.dirname) (Base.to_string t.basename) | ||
|
|
||
| let to_list t = | ||
| (String.split_on_char Filename.dir_sep.[0] t.dirname) |
There was a problem hiding this comment.
i'm not a huge fan of this pattern but if we have to use it i think we should put it in a separate internal function so that we can at least use it for OpamFilename.link. I would rather use the implementation from link instead of this one in case we ever get problems with multiple directory separators (e.g. a//b == a/b)
There was a problem hiding this comment.
In the end, we only need to split on "packages" and "files" (nv can be retrieved with dirname), so maybe we don't need this function in the end
| let install_nv_dir filename = | ||
| let rec find_files prefix_files tail = | ||
| match tail with | ||
| | "files"::_ -> Some prefix_files |
There was a problem hiding this comment.
Ideally we'd also check that files is a directory and that there exist a valid opam file at the same level named opam
There was a problem hiding this comment.
agree, but i don't know if we should do it here or in a wrapping function. For plain repo, it is simple to check, but for tarred repo, we'll need to take all the other information (files, contents) to be able to have if. Maybe a check function ?
| | Some pkg -> | ||
| let dir = | ||
| OpamFilename.Dir.of_list | ||
| (List.rev (nv :: prefix_files @ ("packages"::pre))) |
There was a problem hiding this comment.
i'm unsure what pre is here. If it is because filename can be an absolute filename i think we should avoid that and always start at the repository root directory. Otherwise it's a recipe for disaster (e.g. users are allowed to put their opam root in a directory called packages)
There was a problem hiding this comment.
ftm it is used directly with paths from diff file, so from repo root directly. But we can restrict it yes
i think it would be great to have that information in the commit message |
|
general notes:
|
| Filename.concat (Dir.to_string t.dirname) (Base.to_string t.basename) | ||
|
|
||
| let to_list t = | ||
| (String.split_on_char Filename.dir_sep.[0] t.dirname) |
There was a problem hiding this comment.
In the end, we only need to split on "packages" and "files" (nv can be retrieved with dirname), so maybe we don't need this function in the end
| let install_nv_dir filename = | ||
| let rec find_files prefix_files tail = | ||
| match tail with | ||
| | "files"::_ -> Some prefix_files |
There was a problem hiding this comment.
agree, but i don't know if we should do it here or in a wrapping function. For plain repo, it is simple to check, but for tarred repo, we'll need to take all the other information (files, contents) to be able to have if. Maybe a check function ?
| | Some pkg -> | ||
| let dir = | ||
| OpamFilename.Dir.of_list | ||
| (List.rev (nv :: prefix_files @ ("packages"::pre))) |
There was a problem hiding this comment.
ftm it is used directly with paths from diff file, so from repo root directly. But we can restrict it yes
…nd constructor (OpamFilename.Dir.of_list) to and from string list
…tion (package, main directory) of a filename if it is an extra file
…nd constructor (OpamFilename.Dir.of_list) to and from string list
…tion (package, main directory) of a filename if it is an extra file
17de064 to
aa59255
Compare
|
Updated |
aa59255 to
d666138
Compare
…tructor (OpamFilename.Dir.of_list) to and from string list
…nd constructor (OpamFilename.Dir.of_list) to and from string list
…age, main directory) of a filename if it is an extra file
…n (package, main directory) of a filename if it is an extra file
Simplify code readability and no longer apply updates from unrelated files
d666138 to
f6356b2
Compare
2 participants
It simplifies the readability of the code and no longer loads unrelated paths.
TODO:
In the story of #6625