config: read both home and xdg files for --global - #2196
Open
delilahw wants to merge 3 commits into
Open
Conversation
Git prefers forward slashes as directory separators across all platforms. On Windows, the backslash is the native directory separator, but all Windows versions supported by Git also accept the forward slash in all but rare circumstances. Our tests expect forward slashes. Git generates relative paths with forward slashes. Forward slashes are more convenient to use in shell scripts. For these reasons, we enforced forward slashes in `interpolate_path()` in 5ca6b7b (config --show-origin: report paths with forward slashes, 2016-03-23). However, other code paths may generate paths containing backslashes. For example, `config --show-origin` prints the XDG config path with mixed slashes on Windows: $ git config --list --show-origin file:C:/Program Files/Git/etc/gitconfig system.foo=bar file:"C:\\Users\\delilah/.config/git/config" xdg.foo=bar file:C:/Users/delilah/.gitconfig home.foo=bar file:.git/config local.foo=bar Let's enforce forward slashes in all code paths that directly or indirectly call `xdg_config_home_for()` by modifying it to use `convert_slashes()` on Windows. Convert slashes only in the required path segments. Calling `xdg_config_home_for(subdir, filename)` will interpolate one of two templates: either `$XDG_CONFIG_HOME/<subdir>/<filename>` or `$HOME/.config/<subdir>/<filename>`. In all call paths, the `subdir` and `filename` arguments are hardcoded to values without backslashes. This leaves `$XDG_CONFIG_HOME` and `$HOME` as the only segments that could contain a backslash, so we can restrict the `convert_slashes()` operation to those two path segments. Lastly, it's safe to perform this slash conversion because callers of `xdg_config_home_for()` handle mixed slashes correctly. If we ensure all slashes are forward slashes, it's reasonable to assume that the callers would still be able to handle it. todo(delilahwu): remove trace2 tracing.
Add an argument for `do_git_config_sequence()` to return nonzero when all files in the sequence are nonexistent or cannot be read for some other reason. When bailing, the exit code is not determined by sum of the return codes of the underlying operations. Instead, the exit code is modified via a single decrement. If this is undesirable, we can change it to sum the return codes of the underlying operations instead. The next patch changes how `git config list --global` reads the global configuration. It uses this new flag to ensure the command continues to bail (as expected) when both global config files are nonexistent. Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
The output of `git config list --global` should include both the home (`$HOME/.gitconfig`) and XDG (`$XDG_CONFIG_HOME/git/config`) configs, but it only reads from the former. It should include both files, to be consistent with `git config list` (not limited to `--global`), which includes entries from both files (in addition to system-wide and repository-specific entries, of course). Running `git config list --global` would only read from the home config because we assume each scope corresponds to a single config file. Under this assumption, `git config list --global` reads the global config by calling `git_config_from_file_with_options(...,"~/.gitconfig", ...)`. This function usage restricts us to a single config file. Since the global scope includes two files, we should read the configs using another method. Running `git config list --show-scope --show-origin` (without `--global`) correctly reads both the home and XDG config files. So there's existing code that respects both locations, namely the `do_git_config_sequence()` function which reads from all scopes. Introduce flags to make it possible to ignore all but the global scope (i.e. ignore system, local, worktree, and cmdline). Then, reuse the function to read only the global scope when `--global` is specified. This was the suggested solution in the bug report: https://lore.kernel.org/git/kl6ly1oze7wb.fsf@chooglen-macbookpro.roam.corp.google.com. Modify the tests to check that `git config list --global` includes both home and XDG configs. Also, add tests to ensure we do not introduce regressions to `git config list`. Specifically, check that: - The home config should take precedence over the XDG config. - Without `--global`, it should not bail on unreadable/non-existent global config files. - With `--global`, it should bail when both `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail if at least one of them is readable. Implementation notes: 1. The `ignore_global` flag is not set anywhere, so the `if (!opts->ignore_global)` condition is always met. We can remove this flag if desired. 2. I've assumed that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff `--global` is specified. This comparison determines whether to call `do_git_config_sequence()` for the global scope, or to keep calling `git_config_from_file_with_options()` for other scopes. 3. Keep populating `opts->source.file` in `builtin/config.c` because it is used as the destination config file for write operations. The proposed changes could convolute the code because there is no single source of truth for the config file locations in the global scope. Add a comment to help clarify this. Please let me know if it's unclear. Reported-by: Jade Lovelace <lists@jade.fyi> Suggested-by: Glen Choo <glencbz@gmail.com> Helped-by: Derrick Stolee <stolee@gmail.com> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
|
There is an issue in commit 4c0da9e:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
Here is my reroll.
As reported in [1]: `$HOME/.gitconfig` and `$XDG_CONFIG_HOME/git/config` are both valid global config locations, but `git config list --global` only includes the former in its output.
Suppose we have this config in `$HOME/.gitconfig`:
And this config in `$XDG_CONFIG_HOME/git/config`:
Then, to reproduce the issue that `--global` only shows the home config:
Git correctly applies the XDG config in its effective configuration, but it doesn't show up when `--global` is specified. We can confirm this by checking the output without the `--global` flag:
The expected behaviour is both configs should be shown when `--global` is specified, so we'd expect its output to look the same as above. This was confirmed in [2], which quoted the `git config` documentation:
The first patch fixes forward slash normalisation on Windows paths. The second patch adds a flag for error handling when reading configuration files. The third patch implements the fix to include both config files when `--global` is specified.
Changes in v2:
[1]: https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/xmqqmt5lezi3.fsf@gitster.g/
[3]: #1938
Thank you all for your time!
Delilah
cc: Delilah Ashley Wu delilahwu@microsoft.com
cc: Derrick Stolee stolee@gmail.com
cc: Johannes Schindelin johannes.schindelin@gmx.de
cc: Junio C Hamano gitster@pobox.com
cc: Patrick Steinhardt ps@pks.im
cc: Kristoffer Haugsbakk kristofferhaugsbakk@fastmail.com