Skip to content
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.

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

Avoid clobbering existing repositories & README changes #78

Merged
merged 5 commits into from
Jan 27, 2024

Conversation

GHTaarn
Copy link
Contributor

@GHTaarn GHTaarn commented Jan 14, 2024

When creating a new registry in a repository that already contains a registry, the existing registry is (partially) overwritten. When this happens, I would expect that it often is a mistake; This proposed change in src/LocalRepository.jl prevents this.

There are also some proposed changes to the README.md to make it easier for new users to understand how to create a repository.

I would like to hear your opinion on these proposed changes. I am of course willing to fix the TODO if you feel that this proposal is a step in the right direction.

@GunnarFarneback
Copy link
Owner

It's a good idea to refuse to overwrite anything when creating the registry, but I would just error in that case, and it needs a test.

For the README I'd rather not extend the description; it would just push everything else further down. Also it's not correct that you must have an existing repository, even though it's often a good idea. More explanations would be better in a tutorial document and I guess I should revive the tutorial branch.

@GHTaarn
Copy link
Contributor Author

GHTaarn commented Jan 18, 2024

Okay, I have implemented your comments about throwing an ErrorException and making some tests. Regarding handling of branches, I have assumed that create_registry will only accept branch names that do not exist in the repo and clarified this in the docstring in one of my commits. The error message when branches exist is sometimes difficult to understand:

julia> create_registry("./Registry3", "../bare/r3"; push=true, branch="b1")
Switched to a new branch 'b1'
To /home/msj/Program/mine_other/registryplay/xx/Registry3/../bare/r3
 ! [rejected]        HEAD -> b1 (non-fast-forward)
error: failed to push some refs to '/home/msj/Program/mine_other/registryplay/xx/Registry3/../bare/r3'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
ERROR: failed process: Process(`git -C /home/msj/Program/mine_other/registryplay/xx/Registry3 push -u origin HEAD`, ProcessExited(1)) [1]

Stacktrace:
 [1] pipeline_error
   @ ./process.jl:565 [inlined]
 [2] run(::Cmd; wait::Bool)
   @ Base ./process.jl:480
 [3] run
   @ ./process.jl:477 [inlined]
 [4] create_registry(name_or_path::String, repo::String; description::Nothing, gitconfig::Dict{Any, Any}, uuid::Nothing, push::Bool, branch::String)
   @ LocalRegistry ~/.julia/packages/LocalRegistry/Nshlw/src/LocalRegistry.jl:108
 [5] top-level scope
   @ REPL[7]:1

julia> 

but this is also the case on the current master branch of LocalRegistry.jl. With my proposed changes to the docstring, hopefully it will be easier to find the cause of the problem in this case.

A tutorial would be very helpful to new users. The current Julia documentation only covers client side usage of registries and does not cover how to create them and manage their contents, but actually links to this project as one of the ways to do this.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb8ba40) 96.93% compared to head (2b7a60e) 96.59%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   96.93%   96.59%   -0.35%     
==========================================
  Files           1        1              
  Lines         261      264       +3     
==========================================
+ Hits          253      255       +2     
- Misses          8        9       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GHTaarn
Copy link
Contributor Author

GHTaarn commented Jan 24, 2024

@GunnarFarneback The master branch of LocalRegistry.jl has failing tests on the nightly build of Julia and I believe that this is the reason that my proposal fails as well. Regarding the codecov failure, I also believe that I have done what I can, I added two real lines of code to src/LocalRegistry.jl and both are tested. So, for now I will await a response from you.

@GunnarFarneback
Copy link
Owner

Yeah, the CI is fine, don't worry about that. I just haven't had time to finish this up.

@GunnarFarneback
Copy link
Owner

I'll merge this as is. I'm not completely happy with the README changes but the page is in need of some bigger revisions, so I'll get back to that later.

Thanks for the contribution.

@GunnarFarneback GunnarFarneback merged commit d0f008a into GunnarFarneback:master Jan 27, 2024
7 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants