Don't use --detach when cloning, fixes #6888

[elldritch]

---

Please 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 (add file to changelog.d directory).
  • Help needed: Should add a changelog file for this fix?
• The documentation has been updated, if necessary.
  • I have not made any documentation updates. This PR does not change intended behaviour - it merely fixes a bug in an existing implementation.

Please also shortly describe how you tested your change. Bonus points for added tests!

---

I think this fixes #6888 but I'm not sure how to write an automated test for this.

#6888 happens because we use --detach even when we are checking out branch names. This causes git to fail with the error message fatal: '--detach' cannot be used with '-b/-B/--orphan'. To the best of my knowledge, there's no reason to ever prefer --detached over regular tag names, so I've removed that flag.

I tested this fix by using the binary built from this commit to successfully build and run my project that depends on a non-master branch of a Git repository for an external package. Here is the exact commit that does not compile on with cabal-install 3.2.0.0, but does compile with this patch.

[elldritch]

This is now ready for review.

[phadej]

I'd expect to see some additions to https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests/Distribution/Client/VCS.hs testing VCS support.

To run tests:

# use cabal-install.cabal with tests
make cabal-install-dev

# build and run test-suite
cabal run cabal-install:unit-tests

[elldritch]

I'd love to help, but I'm not really sure where to start on adding tests.

The errors that this PR fix occur when you run git checkout --detach B on a newly cloned repository without first running git branch B (i.e. when B does not yet exist as branch).

When you first git clone a remote repository, none of the remote branches have their equivalent local branches created yet. The fully expanded way to check out a remote branch B is:

# Clone the repository.
git clone repo

# Construct a new local branch named "B" that tracks the branch named "B" from the remote "origin".
git branch B --track origin/B

# Switch to the local branch named "B".
git checkout B

Running git checkout B when there exists a branch B on the remote origin is actually shorthand for git checkout -b B --track origin/B, and git checkout -b B --track origin/B is actually shorthand for git branch B --track origin/B && git checkout B. See the documentation on shorthand for remotes and shorthand for -b.

This is why an error occurs when passing --detach to git checkout when checking out a remote branch from a freshly cloned repository: the shorthand git checkout --detach B (which is what Cabal currently runs) expands to an illegal command.

I'm not sure how to construct this scenario in the tests. I think the right answer might be to modify vcsTestDriverGit to initialize a freshly cloned repository, or to construct a new set of git-specific property tests. I'd appreciate any advice here.

[avieth]

Would it suffice to add a test which verifies that checking out a branch name (rather than a commit hash) works as expected? Seems to me we can be very confident that this patch won't break anything... cabal won't ever make any commits to the repositories it clones, so it doesn't matter whether they are in detached HEAD state, right?

[liskin]

While this does make the error go away, it's not the correct fix I am afraid, as it never updates the branch from the remote. The git fetch does fetch remote updates, but the git checkout branchname uses the same old local branch if it already exists.

A better fix is to keep the --detach but prepend origin/ to the branch name.

[jneira]

@liftM hi, thanks for the pr, are you planning to continue it? not sure if you readed @liskin suggestion above to correct the fix and the @avieth one on how to add a test (which would be definitely needed)

[jneira]

I was revising conflicts and git checkout has been replaced by git reset and it does not have --detach anymore. But it continues failing with branches other than master.
So i think it is better to open a new pr following @liskin suggestion. Thanks for the pr anyways!