config: read both home and xdg files for --global
#1938
Conversation
Welcome to GitGitGadgetHi @delilahw, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit 3a615ee: |
delilahw
force-pushed
the
lilah/fix-config-list-global-home-and-xdg/patchset
branch
4 times, most recently
from
3f97f81 to
a44ef7c
Compare
|
/allow |
|
User delilahw is now allowed to use GitGitGadget. |
delilahw
force-pushed
the
lilah/fix-config-list-global-home-and-xdg/patchset
branch
3 times, most recently
from
aed07f9 to
5b253d6
Compare
|
naauurrr the |
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 `cleanup_path()` by modifying it to use `convert_slashes()` on Windows. Since `convert_slashes()` modifies the path in-place, change the argument and return type of `cleanup_path()` from `const char *` to `char *`. All existing callers of `cleanup_path()` pass `char *` anyways, so this change is compatible. The next patch, config: test home and xdg files in `list --global`, will assert that the XDG config path uses forward slashes. Suggested-by: Johannes Schindelin <[email protected]> Signed-off-by: Delilah Ashley Wu <[email protected]>
delilahw
force-pushed
the
lilah/fix-config-list-global-home-and-xdg/patchset
branch
from
5b253d6 to
6d205b9
Compare
The `git config list --global` output includes `$HOME/.gitconfig` (home
config), but ignores `$XDG_CONFIG_HOME/git/config` (XDG config). It
should include both files.
Modify tests to check the following and expect a failure:
- `git config list --global` should include contents from both the
home and XDG config locations (assuming they are readable), not
just the former.
- `--show-origin` should print correct paths to both config files,
assuming they exist.
The next patch will implement a fix to include both config files when
`--global` is specified.
Also, add tests to ensure subsequent patches 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.
Reported-by: Jade Lovelace <[email protected]>
Helped-by: Derrick Stolee <[email protected]>
Signed-off-by: Delilah Ashley Wu <[email protected]>
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. We assumed each config 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. Because the global scope includes two files, we should read the configs via another method. The output of `git config list --show-scope --show-origin` (without `--global`) correctly includes 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/[email protected]. Then, modify the tests to check that `git config list --global` includes both home and XDG configs. This change introduces a regression: if both global config files are unreadable, then `git config list --global` should exit non-zero. This is no longer the case, so mark the corresponding test as a "TODO known breakage" and address the issue in the next patch. 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 confile 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 <[email protected]> Helped-by: Derrick Stolee <[email protected]> Suggested-by: Glen Choo <[email protected]> Signed-off-by: Delilah Ashley Wu <[email protected]>
The expected behaviour for `git config list` is:
A. Without `--global`, it should not bail on unreadable/non-existent
global config files.
B. With `--global`, it should bail when both `$HOME/.gitconfig` and
`$XDG_CONFIG_HOME/git/config` are unreadable. It should not bail
when one or more of them is readable.
The previous patch introduced a regression in scenario B. Running
`git config list --global` would not fail when both global config files
are unreadable. For example,
`GIT_CONFIG_GLOBAL=does-not-exist git config list --global` exits with
status code 0.
Assuming that `config_source->scope == CONFIG_SCOPE_GLOBAL` iff the
`--global` argument is specified, use this to determine whether to bail.
When reading only the global scope and both config files are unreadable,
then adjust the return code to be non-zero.
Note: 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.
Lastly, modify the tests to remove the known breakage/regression. The
tests for scenario B will now pass.
Helped-by: Derrick Stolee <[email protected]>
Signed-off-by: Delilah Ashley Wu <[email protected]>
delilahw
force-pushed
the
lilah/fix-config-list-global-home-and-xdg/patchset
branch
from
6d205b9 to
8fd6c76
Compare
Hi!
As reported in [1]: both
$HOME/.gitconfigand$XDG_CONFIG_HOME/git/configare valid config locations for the global scope, but
git config list --globalonly 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
--globalonly shows the home config:In reality, Git correctly applies the XDG config in its effective
configuration, but it just doesn't show up when
--globalis specified.We can confirm this by checking the output without the
--globalflag:The expected behavior is that both configs should be shown when
--globalis specified, so we'd expect its output to look the same as above. This
was confirmed in [2] and also in the
git configdocumentation:> OPTIONS
> --global::
> For writing options: write to global
~/.gitconfigfile> rather than the repository
.git/config, write to>
$XDG_CONFIG_HOME/git/configfile if this file exists and the>
~/.gitconfigfile doesn't.>
> For reading options: read only from global
~/.gitconfigand from>
$XDG_CONFIG_HOME/git/configrather than from all available files.The first patch introduces tests and regression checks. The second and
third patches implement the fix to include both config files when
--globalis specified.Johannes has kindly pre-reviewed this patch series on GitHub
(GitGitGadget #1938).
[1]: https://lore.kernel.org/git/CAFA9we-QLQRzJdGMMCPatmfrk1oHeiUu9msMRXXk1MLE5HRxBQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/[email protected]/
cc: Delilah Ashley Wu [email protected], Delilah Ashley Wu [email protected]
cc: Derrick Stolee [email protected]
cc: Johannes Schindelin [email protected]
cc: Junio C Hamano [email protected]
cc: Patrick Steinhardt [email protected]