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

Slightly adjust internal gix_path::env::git-related organization #1568

Merged
merged 6 commits into from
Aug 31, 2024

Conversation

EliahKagan
Copy link
Member

This is a small follow-up to #1567. As suggested in #1567 (comment), it moves the first_file_from_config_with_origin test case into the nested exe_info module, and it changes NULL_DEVICE from static to const.

This also changes EXE_NAME, which provides git or git.exe and never holds a longer path even when one is ultimately used, from static to const. While EXE_NAME is non-private, it is crate-internal via pub(super), so this change is still non-breaking and non-user-facing.

In contrast, the static items ALTERNATIVE_LOCATIONS and EXE_INFO cannot be made const. No change is made to them. It may make sense in the future for EXE_INFO to hold an Option<PathBuf> rather than an Option<BString>; depending in part on the future direction of changes related to #1523 and #1567 or to the question of which scopes are eligible to inform GitInstallation, that may be required in the future. But nothing toward that is included here.

This moves the `first_file_from_config_with_origin` test into the
nested `exe_info` module, because like the other functions tested
there, `first_file_from_config_with_origin` is a helper function
for `EXE_INFO` that is separate largely to facilitate testing.
The `gix_path::env::git::EXE_NAME` static item, while more visible
than `NULL_DEVICE`, was crate-internal via `pub(super)`. So making
it a `const` instead, as done here, is not a breaking nor otherwise
user-facing change.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your help with this, and for catching that indentation issue. Cargo-fmt doesn't deal as well with macros as one would hope.

@Byron Byron enabled auto-merge August 31, 2024 11:20
@Byron Byron merged commit adbaa2a into GitoxideLabs:main Aug 31, 2024
15 checks passed
@EliahKagan
Copy link
Member Author

I agree with renaming EXE_INFO, since its name was not very descriptive, and since it holds information from but not actually about the executable, so that name was arguably even inaccurate. Furthermore, there is no compatibility reason to keep the name EXE_INFO, since it is pub(super).

However, I think this name runs the risk making it seem like the opposite of what it really is:

https://github.com/Byron/gitoxide/blob/adbaa2ab6840fd247c14701c39dcda3d280d86c0/gix-path/src/env/git/mod.rs#L80-L83

Although the meaning may vary subtly across contexts, in my mind this is the configuration file whose configuration has the lowest priority, because the variables in it have the highest scope; setting those variables in any other scope takes precedence.

I am not sure what the best name is, but maybe a better name could be GIT_HIGHEST_SCOPE_CONFIG_PATH?

@EliahKagan EliahKagan deleted the config-origin-next branch August 31, 2024 11:41
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Aug 31, 2024
This renames the crate-internal static item with the cached path
of the highest scope nonempty config, treated as associated with
the `git` installation, from `GIT_HIGHEST_PRIORITY_CONFIG_PATH` to
`GIT_HIGHEST_SCOPE_CONFIG_PATH`.

This reflects the propensity of "priority" to have the opposite of
the intended meaning here, since configuration in this file stands
to be superseded by configuration in any lower (i.e. any other)
scope. See GitoxideLabs#1568 for context.

I think this change from "PRIORITY" to "SCOPE" in the name
preserves the benefits of the change from `EXE_INFO` to
`GIT_HIGHEST_PRIORITY_CONFIG_PATH` (dd2d666), while making clearer
what is going on and avoiding misinterpretations.
@EliahKagan
Copy link
Member Author

I have opened #1569 to propose the PRIORITY -> SCOPE naming adjustment I suggested above in #1568 (comment).

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