Fast-track of 'git config list --type' correctness#863
Open
derrickstolee wants to merge 13 commits intomicrosoft:vfs-2.53.0from
Open
Fast-track of 'git config list --type' correctness#863derrickstolee wants to merge 13 commits intomicrosoft:vfs-2.53.0from
derrickstolee wants to merge 13 commits intomicrosoft:vfs-2.53.0from
Conversation
In anticipation of using format_config() in this method, move show_all_config() lower in the file without changes. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This parameter is set to 0 for all current callers and is UNUSED. However, we will start using this option in future changes and in a critical change that requires gentle parsing (not using die()) to try parsing all values in a list. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previously, the --type=<X> argument to 'git config list' was ignored and did nothing. Now, we add the use of format_config() to the show_all_config() function so each key-value pair is attempted to be parsed. This is our first use of the 'gently' parameter with a nonzero value. When listing multiple values, our initial settings for the output format is different. Add a new init helper to specify the fact that keys should be shown and also add the default delimiters as they were unset in some cases. Our intention is that if there is an error in parsing, then the row is not output. This is necessary to avoid the caller needing to build their own validator to understand the difference between valid, canonicalized types and other raw string values. The raw values will always be available to the user if they do not specify the --type=<X> option. The current behavior is more complicated, including error messages on bad parsing or potentially complete failure of the command. We add tests at this point that demonstrate the current behavior so we can witness the fix in future changes that parse these values quietly and gently. This is a change in behavior! We are starting to respect an option that was previously ignored, leading to potential user confusion. This is probably still a good option, since the --type argument did not change behavior at all previously, so users can get the behavior they expect by removing the --type argument or adding the --no-type argument. t1300-config.sh is updated with the current behavior of this formatting logic to justify the upcoming refactoring of format_config() that will incrementally fix some of these cases to be more user-friendly. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting int64 config values into a helper method and use gentle parsing when needed. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting bool config values into a helper method and use gentle parsing when needed. This makes 'git config list --type=bool' not fail when coming across a non-boolean value. Such unparseable values are filtered out quietly. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting bool-or-int config values into a helper method and use gentle parsing when needed. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting bool-or-string config values into a helper. This parsing has always been gentle, so this is not unlocking new behavior. This extraction is only to match the formatting of the other cases that do need a behavior change. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting path config values into a helper method and use gentle parsing when needed. We need to be careful about how to handle the ':(optional)' macro, which as tested in t1311-config-optional.sh must allow for ignoring a missing path when other multiple values exist, but cause 'git config get' to fail if it is the only possible value and thus no result is output. In the case of our list, we need to omit those values silently. This necessitates the use of the 'gently' parameter here. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting expiry date config values into a helper method and use quiet parsing when needed. Note that git_config_expiry_date() will show an error on a bad parse and not die() like most other git_config...() parsers. Thus, we use 'quietly' here instead of 'gently'. There is an unfortunate asymmetry in these two parsing methods, but we need to treat a positive response from parse_expiry_date() as an error or we will get incorrect values. This updates the behavior of 'git config list --type=expiry-date' to be quiet when attempting parsing on non-date values. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When parsing colors, a failed parse leads to an error message due to the result returning error(). To allow for quiet parsing, create color_parse_quietly(). This is in contrast to an ..._gently() version because the original does not die(), so both options are technically 'gentle'. To accomplish this, convert the implementation of color_parse_mem() into a static color_parse_mem_1() helper that adds a 'quiet' parameter. The color_parse_quietly() method can then use this. Since it is a near equivalent to color_parse(), move that method down in the file so they can be nearby while also appearing after color_parse_mem_1(). Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move the logic for formatting color config value into a helper method and use quiet parsing when needed. This removes error messages when parsing a list of config values that do not match color formats. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The recent changes have replaced the bodies of most if/else-if cases with simple helper method calls. This makes it easy to adapt the structure into a clearer switch statement, leaving a simple if/else in the default case. Make things a little simpler to read by reducing the nesting depth via a new goto statement when we want to skip values. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The --type=<X> option for 'git config' has previously been defined using macros, but using a typed enum is better for tracking the possible values. Move the definition up to make sure it is defined before a macro uses some of its terms. Update the initializer for config_display_options to explicitly set 'type' to TYPE_NONE even though this is implied by a zero value. This assists in knowing that the switch statement added in the previous change has a complete set of cases for a properly-valued enum. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho
approved these changes
Mar 2, 2026
| * 0 on success, 1 on a missing optional value (i.e., telling the | ||
| * caller to pretend that <key_,value_> did not exist). | ||
| * | ||
| * Note: 'gently' is currently ignored, but will be implemented in |
Member
Author
There was a problem hiding this comment.
You're right. It should have. Something to clean up later upstream.
Member
|
Don't worry about the CodeQL run; It already failed on the |
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
2 participants
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.
See gitgitgadget#2044 for the original review of this change.
The main feature here in Git is that
git config list --type=<X>works correctly.But the real benefit is that we can update GCM to use
git config list --type=<X>to get the full list of config and store those values in the cache without dozens ofgit config --getcalls.See git-ecosystem/git-credential-manager#2268 for the change that adds this cache with a version check for Git 2.54.0. If we additionally add derrickstolee/Git-Credential-Manager-Core#1, then a v2.53.0.vfs.0.1 release would also allow this behavior change.
I tested all of these features together against my copy of the Office monorepo running a basic
git fetch. Note that these runs are not actually downloading much data, if any.During this test, I used a single GCM version with these changes and modified the environment variables to change the Git version executed by GCM.
next.)