Do not use shallow clones for short revisions

[alt-romes]

    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:

• [x] Patches conform to the coding conventions.
• [x] 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.
• [x] The documentation has been updated, if necessary.
• [x] Manual QA notes have been included.
• [x] 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]

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

[alt-romes]

Fixes #10605

[ulysses4ever]

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

[alt-romes]

@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]

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]

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]

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

[alt-romes]

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

[geekosaur]

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]

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]

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]

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

[alt-romes]

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]

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.

I've fixed this now in a new commit.

@9999years What do you envision we could test additionally?

[alt-romes]

The current property testing infrastructure for VCS uses local clones. This is problematic with shallow clones. In particular, the change that makes fetching a tag shallowly breaks the testsuite because the reference repository is shallow

fatal: reference repository '/private/var/folders/tv/35hlch6s3y15hfvndc71l6d40000gn/T/vcstest-57113/dest1' is shallow

It looks to me like the best path forward is to make local clones never shallow, which will make the property tests for VCS essentially incapable of testing shallow clones (here @9999years suggestion becomes much more relevant)

[9999years]

@alt-romes I'm not super familiar with the VCS testing infrastructure, but would it be possible to make the reference repository not shallow...?

[alt-romes]

@alt-romes I'm not super familiar with the VCS testing infrastructure, but would it be possible to make the reference repository not shallow...?

I don't think so. The VCS property tests use local clones, and local clones don't work shallowly (git limitation).

I think we should:

• Use full clones for local clones
• And make sure there are tests which do use the shallow clones (some tests that uses a remote repository, with and without tag)

Does that sound good?

[9999years]

So it says fatal: reference repository '/private/var/...' is shallow -- I'm wondering if that's a file path (/private/var/...) and not a file URL (file:///private/var/...), which I think Git treats differently...?

But otherwise, yeah that seems fine.

[Mikolaj]

A cheerful ping! How is this work going? Can I help?

[Mikolaj]

Actually, the plan of action laid out in https://github.com/haskell/cabal/pull/10639#issuecomment-2556709669 makes sense to me. @9999years: does it work for you, too? If so, could you review?

Anybody else, any objections, comments, reviews?

[Mikolaj]

@9999years: a humble ping?

[9999years]

@Mikolaj That plan seems fine. I do think it's worth investigating the file:// URLs, but if that's not possible (and please document it if not...) then I'm happy to just have a few manual tests for shallow clones.

[Mikolaj]

Let me just remind all the wonderful people involved in this PR that we are very much eager to merge it, because it's very much needed. Is there a way to help move it forward?

[ffaf1]

Ping @alt-romes If this needs help, let us know!

[ulysses4ever]

It looks like this PR needs more work. Should we remove the needs-review label?