`opam source --dev` fetching git repositories without `--depth 1` #5888

moyodiallo · 2024-03-19T15:38:21Z

This PR is about to fix #5061. It turns out we doesn't need a flag for fixing the issue, It is agreed during the opam-dev meeting to avoid fetching with --depth 1 for opam source because on the client side it could be confusing, an idea from @kit-ty-kate.

Add an option in the backend
~~Add an flag for opam source client~~
opam source is going to be --deph 1 free when fetching source from dev_repo.
Add test for opam source command
Add test for opam repository command, to be sure the behavior remains unchanged.

kit-ty-kate

LGTM for the code, apart from the few cosmetic changes pointed below.

However I feel like this type of change (especially when dealing with optional arguments) warrants some addition to the testsuite, so that regressions do not happen in the future. Would it be possible to add one? If so DM me and i can help you navigate in the testsuite.

src/repository/opamGit.ml

kit-ty-kate · 2024-03-25T16:19:06Z

@moyodiallo you seem to have lost all of your changes except the first commit in the rebase somehow. You can get them back from 2469168

moyodiallo · 2024-03-25T16:30:17Z

@moyodiallo you seem to have lost all of your changes except the first commit in the rebase somehow. You can get them back from 2469168

Done, I was reorganizing it a bit. It's done and ready for an ultimate review.

kit-ty-kate · 2024-03-27T11:45:51Z

* [x]  Add test for `opam repository` command, to be sure the behavior remains unchanged.

~~This todo from your own PR description doesn't seem to be in the final PR. Is there a missing commit lost somewhere or you chose not to do this in the end?~~

EDIT: Nevermind I'm seeing it now. Sorry for the noise

rjbou

Thanks for this first PR!
On the idea, and general implementation, it is good.
I've left some comments, most of them concern the "coding style" of this repository.

master_changes.md

src/repository/opamRepositoryBackend.mli

tests/reftests/source.test

kit-ty-kate · 2024-04-02T13:29:22Z

src/repository/opamVCS.ml

    if VCS.exists repo_root then
      OpamProcess.Job.catch (fun e -> Done (OpamRepositoryBackend.Update_err e))
      @@ fun () ->
      OpamRepositoryBackend.job_text repo_name "sync"
-        (VCS.fetch ?cache_dir repo_root repo_url)
+        (VCS.fetch ?full_fetch ?cache_dir repo_root repo_url)


@rjbou like this?

Suggested change

(VCS.fetch ?full_fetch ?cache_dir repo_root repo_url)

(VCS.fetch ~full_fetch:false ?cache_dir repo_root repo_url)

Linked to this #5888 (comment).

not really. My comment was linked to a discussion during the dev meeting where we wondered if ?full_fetch should be given explicitly or if it should be omitted when applied

@kit-ty-kate did you want to keep full_fetch:false for fetch_repo_update that is used opam opam repo updates ?

I would prefer personally but as you wish

Good for me in that case :) It is a good safeguard regarding current opam repository.

I'll add it

src/repository/opamVCS.mli

src/repository/opamRepositoryBackend.mli

rjbou · 2024-04-02T17:57:16Z

tests/reftests/repository.test

@@ -316,8 +316,13 @@ first  --
 ### git -C ./OPER add repo
 ### git -C ./OPER add packages/first
 ### git -C ./OPER commit -qm "third"
+### git -C ./OPER commit --allow-empty -m "second empty commit" --quiet


this is no more needed too, as everything is seen in source test

~~This was commented by @kit-ty-kate. It's about avoiding regression in the future. Maybe we could refactor it like source.~~

You're right.

Done in 29bc4a5.

why is it not needed? I'm not seeing that test in source.test (which also isn't really the place to test for this if it was there)

You're right, I've just realized that I'm mixing everything.

@kit-ty-kate what is the purpose of this test ? I'm guessing that it is to be sure that opam repo download is kept with --depth 1, is it? Its code path remain untouched, but indeed it calls VCS.fetch.

yeah exactly. So that if in the future the behaviour changes (by mistake or on purpose) we already have a test for it

Ok! It needs then a comment to explain why it is here. I'll add it.

… when retrieved with `opam source --dev`, and no change for opam repository checkout Co-authored-by: Raja Boujbel <raja.boujbel@ocamlpro.com> Co-authored-by: Kate <kit-ty-kate@outlook.com>

rjbou

Thanks!

…h 1` - Fixes ocaml#5061

kit-ty-kate · 2024-04-05T10:07:47Z

Thanks a lot!

moyodiallo force-pushed the full-fetch branch 2 times, most recently from bcef852 to deca4d4 Compare March 20, 2024 10:44

moyodiallo marked this pull request as ready for review March 20, 2024 11:22

moyodiallo requested review from rjbou and kit-ty-kate March 20, 2024 11:23

kit-ty-kate reviewed Mar 20, 2024

View reviewed changes

src/repository/opamGit.ml Outdated Show resolved Hide resolved

src/repository/opamGit.ml Outdated Show resolved Hide resolved

src/repository/opamGit.ml Outdated Show resolved Hide resolved

kit-ty-kate added this to the 2.2.0~beta2 milestone Mar 25, 2024

moyodiallo force-pushed the full-fetch branch from 2469168 to a761e06 Compare March 25, 2024 16:11

moyodiallo force-pushed the full-fetch branch from a761e06 to 14a457b Compare March 25, 2024 16:21

kit-ty-kate approved these changes Mar 27, 2024

View reviewed changes

kit-ty-kate force-pushed the full-fetch branch from cd4c8ba to f880f1d Compare March 27, 2024 11:52

kit-ty-kate added STATE: READY TO MERGE PR: WAITING FOR REVIEW and removed STATE: READY TO MERGE labels Mar 27, 2024

rjbou requested changes Apr 2, 2024

View reviewed changes

master_changes.md Outdated Show resolved Hide resolved

src/repository/opamRepositoryBackend.mli Outdated Show resolved Hide resolved

tests/reftests/source.test Outdated Show resolved Hide resolved

rjbou changed the title ~~opam source, fetched with deph 1 could be avoided~~ opam source, fetched with --depth 1 could be avoided Apr 2, 2024

kit-ty-kate reviewed Apr 2, 2024

View reviewed changes

rjbou reviewed Apr 2, 2024

View reviewed changes

src/repository/opamVCS.mli Outdated Show resolved Hide resolved

rjbou reviewed Apr 2, 2024

View reviewed changes

src/repository/opamRepositoryBackend.mli Show resolved Hide resolved

moyodiallo force-pushed the full-fetch branch from f880f1d to ee893f1 Compare April 2, 2024 16:45

rjbou reviewed Apr 2, 2024

View reviewed changes

moyodiallo force-pushed the full-fetch branch from 29bc4a5 to 54b827d Compare April 3, 2024 12:19

kit-ty-kate requested a review from rjbou April 3, 2024 14:40

rjbou changed the title ~~opam source, fetched with --depth 1 could be avoided~~ opam source --dev fetching git repositories without --depth 1 Apr 4, 2024

Add test in source to show retrieval of full git repository history…

33c3729

… when retrieved with `opam source --dev`, and no change for opam repository checkout Co-authored-by: Raja Boujbel <raja.boujbel@ocamlpro.com> Co-authored-by: Kate <kit-ty-kate@outlook.com>

rjbou force-pushed the full-fetch branch from 54b827d to 4d94eb5 Compare April 4, 2024 17:12

rjbou approved these changes Apr 4, 2024

View reviewed changes

rjbou added STATE: READY TO MERGE and removed PR: WAITING FOR REVIEW labels Apr 4, 2024

moyodiallo and others added 2 commits April 4, 2024 20:53

Avoid opam source --dev command fetches git repository with `--dept…

4307e5e

…h 1` - Fixes ocaml#5061

repository: add some documentation

10882b2

rjbou force-pushed the full-fetch branch from 4d94eb5 to 10882b2 Compare April 4, 2024 18:54

kit-ty-kate merged commit f7def5c into ocaml:master Apr 5, 2024
29 checks passed

rjbou removed the STATE: READY TO MERGE label Apr 10, 2024

moyodiallo deleted the full-fetch branch May 24, 2024 08:26

hannesm mentioned this pull request Aug 8, 2024

Git --depth 1 breaks orb/dune #6145

Closed

rjbou mentioned this pull request Aug 9, 2024

Disable shallow clone by default except for opam repositories #6146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`opam source --dev` fetching git repositories without `--depth 1` #5888

`opam source --dev` fetching git repositories without `--depth 1` #5888

moyodiallo commented Mar 19, 2024 •

edited

Loading

kit-ty-kate left a comment

kit-ty-kate commented Mar 25, 2024

moyodiallo commented Mar 25, 2024 •

edited

Loading

kit-ty-kate commented Mar 27, 2024 •

edited

Loading

rjbou left a comment

kit-ty-kate Apr 2, 2024

moyodiallo Apr 2, 2024

kit-ty-kate Apr 3, 2024

rjbou Apr 4, 2024

kit-ty-kate Apr 4, 2024

rjbou Apr 4, 2024

rjbou Apr 4, 2024

rjbou Apr 2, 2024

moyodiallo Apr 3, 2024 •

edited

Loading

moyodiallo Apr 3, 2024

moyodiallo Apr 3, 2024

kit-ty-kate Apr 3, 2024

moyodiallo Apr 3, 2024

moyodiallo Apr 3, 2024

rjbou Apr 4, 2024 •

edited

Loading

kit-ty-kate Apr 4, 2024

rjbou Apr 4, 2024

rjbou left a comment

kit-ty-kate commented Apr 5, 2024

	(VCS.fetch ?full_fetch ?cache_dir repo_root repo_url)
	(VCS.fetch ~full_fetch:false ?cache_dir repo_root repo_url)

opam source --dev fetching git repositories without --depth 1 #5888

opam source --dev fetching git repositories without --depth 1 #5888

Conversation

moyodiallo commented Mar 19, 2024 • edited Loading

kit-ty-kate left a comment

Choose a reason for hiding this comment

kit-ty-kate commented Mar 25, 2024

moyodiallo commented Mar 25, 2024 • edited Loading

kit-ty-kate commented Mar 27, 2024 • edited Loading

rjbou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moyodiallo Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbou Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbou left a comment

Choose a reason for hiding this comment

kit-ty-kate commented Apr 5, 2024

`opam source --dev` fetching git repositories without `--depth 1` #5888

`opam source --dev` fetching git repositories without `--depth 1` #5888

moyodiallo commented Mar 19, 2024 •

edited

Loading

moyodiallo commented Mar 25, 2024 •

edited

Loading

kit-ty-kate commented Mar 27, 2024 •

edited

Loading

moyodiallo Apr 3, 2024 •

edited

Loading

rjbou Apr 4, 2024 •

edited

Loading