Do not use shallow clones for short revisions #10639

alt-romes · 2024-12-13T18:10:01Z

    Do not use shallow clones for short revisions

    When the `tag` of a `source-repository-stanza` is not a full hash,
    we cannot do a shallow clone and fetch it separately.

    Therefore, in those cases, we fall back to doing a full clone.

    Fixes #10605

    Includes a regression test by @9999years.

    Co-authored-by: Rebecca Turner <[email protected]>

Additionally

Fix verbosity of git

    The lack of parenthesis in this line meant that we'd only see the
    verbose output from git when we had a `peerLocalDir`.

    This should fix verbosity of git

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
- Is the change significant? If so, remember to add significance: significant in the changelog file.
The documentation has been updated, if necessary.
Manual QA notes have been included.
Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

alt-romes · 2024-12-13T18:18:11Z

(Just noting that it's quite unfortunate how the versions of the formatter are out of sync between my computer and CI)

alt-romes · 2024-12-13T18:25:26Z

Fixes #10605

9999years · 2024-12-13T18:56:46Z

cabal-install/src/Distribution/Client/VCS.hs

+          -- full hashes are 40 characters
+          | Just tg <- srpTag
+          , length tg >= 40 =


It should be exactly 40 characters and all hexadecimal characters; e.g. 48c42600594a381af18afb24d8eb857396876f0ff is not valid and 48c42600594a38Iaf18afb24d8eb857396876f0f is not valid.

9999years · 2024-12-13T18:58:22Z

cabal-install/src/Distribution/Client/VCS.hs

+        -- Then, reset to the appropriate ref.
+        git localDir $
+          "reset"
+            : verboseArg
+            ++ [ "--hard"
+               , ref
+               , "--"
+               ]


I think that git reset --hard HEAD is needed to check out the sources when git clone --no-checkout is used, i.e. even if the clone is not shallow.

EDIT: I suspect this is why the tests are failing.

Maybe you can do something like Just ref | doShallow -> ... to guard that case and then put the Nothing -> branch at the bottom. (That would also avoid reindenting the whole block, not that it matters very much.)

You're right! Indeed, we must still reset when the clone uses --no-checkout. I've fixed this now. Thanks

ulysses4ever · 2024-12-13T19:07:33Z

I wonder if we should issue a notice or an info when we can’t do a shallow clone.

9999years

Currently, the test suite is not able to test shallow clones at all -- let's make sure we get those tested before we merge this.

If you run the tests right now, you'll see that the --depth flag is ignored:

$ ./validate.sh -s build -s cli-tests -p VCS
$ /Users/wiggles/cabal/master/dist-newstyle-validate-ghc-9.8.2/build/aarch64-osx/ghc-9.8.2/cabal-install-3.15.0.0/t/long-tests/build/long-tests/long-tests --num-threads 20 --pattern VCS
File modification time resolution calibration completed, maximum delay
observed: 1.778 ms. Will be using delay of 20.0 for test runs.
Long-running tests
  UnitTests.Distribution.Client.VCS
    git
      check VCS test framework: warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
      check VCS test framework: 20% warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
      check VCS test framework: 30% warning: --depth is ignored in local clones; use file:// instead.
      check VCS test framework: OK (33.93s)
        +++ OK, passed 10 tests.
      cloneSourceRepo:          60% warning: --depth is ignored in local clones; use file:// instead.
      cloneSourceRepo:          80% warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
warning: --depth is ignored in local clones; use file:// instead.
      cloneSourceRepo:          OK (54.19s)
        +++ OK, passed 10 tests.
      syncSourceRepos:          OK (57.73s)
        +++ OK, passed 10 tests.

Let's make sure to add a couple test cases for shallow and non-shallow clones as well.

The --depth is ignored warning can be seen in #10587 as well.

alt-romes · 2024-12-16T10:34:32Z

@9999years what kind of property do you think we should test additionally?

Since this PR, we test that short hashes work (which means doing a full clone).
The VCS quickcheck testsuite tests the remaining "normal" situations using full hashes, which do shallow clones

alt-romes · 2024-12-16T10:36:47Z

I don't understand why the whitespace test is failing. Perhaps @ulysses4ever or @geekosaur knows?

This file is not in my tree?

[ Checked ] cabal-testsuite/PackageTests/NewBuild/CustomSetup/LocalPackageWithCustomSetup/pkg.cabal
[ Violation detected ] changelog.d/pr-10546:
changelog.d/pr-10546:41: ····$·cat·z-empty.config·

make: *** [Makefile:46: whitespace] Error 1

geekosaur · 2024-12-16T10:40:55Z

It's in mine?

(Careful: the failing file is a changelog.d entry, which were only just added to the whitespace check a day or so ago, which is why nobody saw this before.)

alt-romes · 2024-12-16T10:44:03Z

Interestingly I managed to get the file only after rebasing. It was introduced in 33b19e4.
It's still failing for some reason

alt-romes · 2024-12-16T10:45:04Z

I've patched it now, but note that the whitespace violation wasn't introduced in this PR.

geekosaur · 2024-12-16T10:58:29Z

I have a sneaking suspicion that the whitespace job isn't listed as required under branch protection. Mergify doesn't care about CI passing, it cares only about the branch protection rules being satisfied. (And merges as soon as they pass even if CI is still running; I still haven't heard back from them about that.)

ETA: confirmed, whitespace job isn't in branch protection rules.

geekosaur · 2024-12-16T11:02:05Z

I'd suggest you remove the patches from this PR and make a separate one fixing the whitespace breakages, which we can push in at high priority if necessary since master is in some sense broken. And consider adding the whitespace job to branch protection for master and 3.*.

alt-romes · 2024-12-16T11:10:48Z

I'd suggest you remove the patches from this PR and make a separate one fixing the whitespace breakages, which we can push in at high priority if necessary since master is in some sense broken. And consider adding the whitespace job to branch protection for master and 3.*.

Sure. In #10642

geekosaur · 2024-12-16T11:14:38Z

I just noticed you're missing the PR templates both here and in the new PR.

@9999years

When the `tag` of a `source-repository-stanza` is not a full hash, we cannot do a shallow clone and fetch it separately. Therefore, in those cases, we fall back to doing a full clone. Fixes haskell#10605 Includes a regression test by @9999years. Co-authored-by: Rebecca Turner <[email protected]>

The lack of parenthesis in this line meant that we'd only see the verbose output from git when we had a `peerLocalDir`. This should fix verbosity of git

alt-romes · 2024-12-17T23:46:33Z

Oh no! I've just noticed that git fetch can also take --depth 1, and, without it, the git fetch to a different revision will still trigger a full clone. I'll do that in a separate commit tomorrow.

alt-romes force-pushed the wip/romes/10619 branch from 8ee89f0 to 9669788 Compare December 13, 2024 18:14

alt-romes mentioned this pull request Dec 13, 2024

Do not use shallow clones for source-repository-packages #10619

Closed

6 tasks

alt-romes force-pushed the wip/romes/10619 branch from 9669788 to 3e0a447 Compare December 13, 2024 18:17

geekosaur approved these changes Dec 13, 2024

View reviewed changes

9999years reviewed Dec 13, 2024

View reviewed changes

9999years requested changes Dec 13, 2024

View reviewed changes

alt-romes force-pushed the wip/romes/10619 branch from e93d885 to c9388d9 Compare December 16, 2024 10:33

alt-romes force-pushed the wip/romes/10619 branch from c9388d9 to 702e39e Compare December 16, 2024 10:41

geekosaur mentioned this pull request Dec 16, 2024

Fix whitespace issue in changelog #10642

Merged

2 tasks

9999years and others added 3 commits December 16, 2024 11:25

Fix verbosity of git

9ea6d4e

The lack of parenthesis in this line meant that we'd only see the verbose output from git when we had a `peerLocalDir`. This should fix verbosity of git

Fix whitespace issue in changelog

9406ecf

alt-romes force-pushed the wip/romes/10619 branch from 2f62765 to 9406ecf Compare December 16, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use shallow clones for short revisions #10639

Do not use shallow clones for short revisions #10639

alt-romes commented Dec 13, 2024 •

edited

Loading

alt-romes commented Dec 13, 2024

alt-romes commented Dec 13, 2024

9999years Dec 13, 2024

9999years Dec 13, 2024 •

edited

Loading

9999years Dec 13, 2024

alt-romes Dec 16, 2024

ulysses4ever commented Dec 13, 2024

9999years left a comment •

edited

Loading

alt-romes commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024

alt-romes commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024 •

edited

Loading

geekosaur commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024

alt-romes commented Dec 17, 2024

Do not use shallow clones for short revisions #10639

Are you sure you want to change the base?

Do not use shallow clones for short revisions #10639

Conversation

alt-romes commented Dec 13, 2024 • edited Loading

alt-romes commented Dec 13, 2024

alt-romes commented Dec 13, 2024

9999years Dec 13, 2024

Choose a reason for hiding this comment

9999years Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

9999years Dec 13, 2024

Choose a reason for hiding this comment

alt-romes Dec 16, 2024

Choose a reason for hiding this comment

ulysses4ever commented Dec 13, 2024

9999years left a comment • edited Loading

Choose a reason for hiding this comment

alt-romes commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024

alt-romes commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024 • edited Loading

geekosaur commented Dec 16, 2024

alt-romes commented Dec 16, 2024

geekosaur commented Dec 16, 2024

alt-romes commented Dec 17, 2024

alt-romes commented Dec 13, 2024 •

edited

Loading

9999years Dec 13, 2024 •

edited

Loading

9999years left a comment •

edited

Loading

geekosaur commented Dec 16, 2024 •

edited

Loading