8000 [halted] Support Git dependencies by lynzrand · Pull Request #107 · moonbitlang/moon · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[halted] Support Git dependencies #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[halted] Support Git dependencies #107

wants to merge 12 commits into from

Conversation

lynzrand
Copy link
Collaborator
@lynzrand lynzrand commented Aug 2, 2024 •
edited
Loading

Development on this branch has halted. Meanwhile, you can use local dependencies + git submodules to achieve a similar effect.

Problems

  • Libgit2 is HUGE
  • More tests needed

@lynzrand lynzrand force-pushed the rynco/git-package branch 3 times, most recently from a9c1776 to cc60c8d Compare August 2, 2024 10:29
@Young-Flash Young-Flash mentioned this pull request Dec 17, 2024
Open
1 task
@lynzrand lynzrand marked this pull request as ready for review February 25, 2025 07:11
@lynzrand lynzrand force-pushed the rynco/git-package branch from 55f663f to b6b9ed0 Compare April 22, 2025 02:38
@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented Apr 22, 2025 •
edited
Loading
The GitOps trait implementation has duplicated error handling logic in command-based and libgit2-based implementations

Category
Maintainability
Code Snippet
fn run_stdout(command: &mut Command) -> Result<String, std::io::Error> {
// Error handling duplicated between command_impl.rs and libgit2_impl.rs
}
Recommendation
Create a common error handling module and share error mapping logic between both implementations. For example:

mod errors {
  fn map_git_error(err: &str) -> std::io::Error { ... }
}

Reasoning
Having duplicate error handling makes the code harder to maintain and more prone to inconsistencies. A shared error module would ensure consistent error handling and messages across both implementations.

Git operations are not cached or memoized between runs

Category
Performance
Code Snippet
fn resolve<G: GitOps>(source: &GitSource, moon_home: &Path) -> anyhow::Result {
// Git operations performed fresh each time
}
Recommendation
Add a cache to store previously resolved Git references and their checkout locations. Cache invalidation can be based on timestamp or explicit version updates.
Reasoning
Git operations are expensive involving network I/O. Caching resolved references would improve performance for repeated dependencies.

Git error handling needs more context for user troubleshooting

Category
Correctness
Code Snippet
return Err(std::io::Error::new(
std::io::ErrorKind::Other,
format!( "Command {:?} failed...",
command, output.status)
));
Recommendation
Enhance error messages with more context and troubleshooting hints:

Err(format!("Git command failed: {} (exit code {}). 
Please check your Git installation and repository permissions.", 
cmd, status))

Reasoning
Current error messages lack sufficient context to help users understand and fix Git-related issues. More detailed errors would improve the debugging experience.

@lynzrand lynzrand force-pushed the rynco/git-package branch 4 times, most recently from e1e2cfd to 83c4e88 Compare April 22, 2025 09:21
@lynzrand lynzrand force-pushed the rynco/git-package branch from 83c4e88 to 00865e8 Compare April 22, 2025 09:50
@lynzrand lynzrand changed the title Support Git dependencies [halted] Support Git dependencies Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@lynzrand
0