Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move most of git out of VcpkgPaths #1381

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Thomas1664
Copy link
Contributor

No description provided.

@Thomas1664 Thomas1664 changed the title Move git out of VcpkgPaths Move most of git out of VcpkgPaths Apr 3, 2024
@Thomas1664 Thomas1664 marked this pull request as ready for review April 3, 2024 17:10
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better if the git stuff were disconnected from VcpkgPaths, but the git stuff is hard to test and risky to change, so we probably need a design description of what this kind of refactoring is going to achieve before we can seriously evaluate making this change to the product.

Refactoring changes need to be justified by:

  • the end they're trying to reach resulting in user facing product improvements, or
  • the kinds of changes they are enabling making easier (or such changes are 'obvious')

Without that they're adding risk to the product, by changing the product, for no benefit.

Notably, this change leaves the 'nexus' of getting to git in vcpkgpaths, since the entry point is still VcpkgPaths::git_cmd_builder or similar, so I'm not sure of the end it's trying to reach.

src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/base/git.cpp Outdated Show resolved Hide resolved
Comment on lines +222 to +224
auto init_registries_git_dir = git_cmd_builder(config).string_arg("init");
auto maybe_init_output = flatten(cmd_execute_and_capture_output(init_registries_git_dir), Tools::GIT);
if (!maybe_init_output)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does not appear to be well described either if it's potentially doing init.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants