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

Add --keep-git-dir flag to the pull command. #160

Merged
merged 7 commits into from
Oct 19, 2021

Conversation

rizo
Copy link
Contributor

@rizo rizo commented Feb 4, 2021

Allows to keep the pulled sources as git repositories. This in combination with having duniverse in .gitignore results in a nice workflow for contributing changes to vendored packages.

I named the option --keep-clone instead of --trim-clone=false to keep it as a flag. I can can the name if it's not very clear.

@rizo
Copy link
Contributor Author

rizo commented Oct 18, 2021

Rebased onto main. Is there any interest in merging this?

I have been using this for a while to make changes to pinned dependencies as part of duniverse.

@NathanReb
Copy link
Contributor

It doesn't make as much sense as it used to since we default to pulling tarballs instead of always relying on git but if you have a use for this I guess it's a good enough reason!

It would be nice to have a way to propagate the local changes back into the lockfile but I guess we still need to see how people will use this option first!

Sorry for the delay, I'll look into it.

Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all looks good to me!

The clone wording, both in the CLI flag and the code trouble me a little bit here. This wording was present before your PR, probably even introduced it myself, but I wonder if it's not the right time and place to replaceclone with git_dir here to be perfectly clear what this is whole about.

The documentation is clear enough so it's not that much of a problem but I think we should at least rename the CLI flag because it's user visible, I'll take care of the renaming in the code.

Does that sound good to you? Can you rename the flag and then we can merge and cut a release!

@rizo
Copy link
Contributor Author

rizo commented Oct 19, 2021

Thanks for looking into this, @NathanReb!

It doesn't make as much sense as it used to since we default to pulling tarballs instead of always relying on git (...)

I was also slightly concerned about this at first, but we are using git for pins which is arguably when having this flag is most needed. I would even say that keeping the repo for non-pinned deps is not a good idea because that workflow makes it harder to get reproducibility for the changes.

It would be nice to have a way to propagate the local changes back into the lockfile but I guess we still need to see how people will use this option first!

It would be pretty amazing. Happy to do some work on this if you have any concrete suggestions. Right now, updating the pin entry in the opam file works well enough I think.

Updated the flag name as per your suggestion.

@rizo rizo changed the title Add --keep-clone flag to the pull command. Add --keep-git-dir flag to the pull command. Oct 19, 2021
Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the changelog entry to reflect the latest update and added the PR and author reference!

Thanks for working on this!

@NathanReb NathanReb merged commit b9ba6c1 into tarides:main Oct 19, 2021
@rizo rizo deleted the add-keep-clone-flag branch October 19, 2021 12:55
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Oct 19, 2021
CHANGES:

### Added

- Add a depext subcommand to install the external system dependencies listed
  in lock file (tarides/opam-monorepo#207, @samoht)
- Add the `--keep-git-dir` flag to the `pull` command that can be used to keep
  the [.git] directory after pulling the vendored sources. (tarides/opam-monorepo#160, @rizo)

### Fixed

- Fix tool name in generated dune file so that it does not refer to the tool as
  `duniverse` (tarides/opam-monorepo#206, @emillon)
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.

2 participants