Fix incomplete-record-selection warning in cabal-install

[sgillespie]

Fixes #11173

I couldn't seem to find a reliable way to test this, either with the integration tests or checking the build plan. Feel free to let me know if there's a better way.

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

[ulysses4ever]

I had a more principled way to fix it in mind initially leveraging the fact that repoTarballPkgsWithMetadata only contains secure, so, named, repositories by construction. But I didn't find time to implement it, so maybe we should just go ahead with this patch.

A little help for future generations may be to leave a comment that all repos here are secure ones and will have a name so we're sorting Justs, so that should not have any surprises.

[sgillespie]

I had a more principled way to fix it in mind initially leveraging the fact that repoTarballPkgsWithMetadata only contains secure, so, named, repositories by construction. But I didn't find time to implement it, so maybe we should just go ahead with this patch.

A little help for future generations may be to leave a comment that all repos here are secure ones and will have a name so we're sorting Justs, so that should not have any surprises.

I suspected this might be the case, but I was trying to keep it consistent. I'm happy to iterate on this a bit more.

[ulysses4ever]

thanks @sgillespie for adding the comment! You're now welcome to add a merge label so that the bot handles it onwards. You probably want the "squash+merge" label since the second commit is logically attached to the first.

[sgillespie]

I attempted the RepoSecure by construction method by creating a new data type SecureRepo to carry around (to avoid going back and forth between Justs). However, I'm still learning the codebase and would appreciate feedback on whether this is consistent with the project conventions, or if there's a better way to approach it. Thanks for reviewing my PR!

[ulysses4ever]

hey @sgillespie! Sorry for radio silence here.

I was looking at your latest patch. While sensible on its own, it adds a bit of redundancy in the Repo space. E.g., ideally, you'd expect Repo to use your new SecureRepo type. But that would be a hefty refactoring that would touch many places. I'm not eager to go there...

When reading your patch, I realized that there's already a repoName function in place that works for any Repo, so maybe it should suffice? https://github.com/sgillespie/cabal/blob/2b0c150d2246f5e8d2362aa71a912616a82556a9/cabal-install/src/Distribution/Client/Types/Repo.hs#L224

[sgillespie]

When reading your patch, I realized that there's already a repoName function in place that works for any Repo, so maybe it should suffice? https://github.com/sgillespie/cabal/blob/2b0c150d2246f5e8d2362aa71a912616a82556a9/cabal-install/src/Distribution/Client/Types/Repo.hs#L224

Thanks for the suggestion! As far as I can tell, that works for me here. I've applied that and rebased away all of my other attempts.

[ulysses4ever]

Terrific, thanks! The merge bot will take care of it by the end of the week.