This patch series originally only tried to help fixing that annoying bug that has been reported several times over the years, where `git config --unset` would leave empty sections behind, and `git config --add` would not reuse them. The first patch is somewhat of a "while at it" bug fix that I first thought would be a lot more critical than it actually is: It really only affects config files that start with a section followed immediately (i.e. without a newline) by a one-letter boolean setting (i.e. without a `= <value>` part). So while it is a real bug fix, I doubt anybody ever got bitten by it. The next swath of patches add and fix some tests, while also fixing the bug where --replace-all would sometimes insert extra line breaks. Then, I introduce a couple of building blocks: a "config parser event stream", i.e. an optional callback that can be used to report events such as "comment", "white-space", etc together with the corresponding extents in the config file. Finally, the interesting part, where I do two things, essentially (with preparatory steps for each thing): 1. I add the ability for `git config --unset/--unset-all` to detect that it can remove a section that has just become empty (see below for some more discussion of what I consider "become empty"), and 2. I add the ability for `git config [--add] key value` to re-use empty sections. To reiterate why does this patch series not conflict with my very early statements that we cannot simply remove empty sections because we may end up with stale comments? Well, the patch in question takes pains to determine *iff* there are any comments surrounding, or included in, the section. If any are found: previous behavior. Under the assumption that the user edited the file, we keep it as intact as possible (see below for some argument against this). If no comments are found, and let's face it, this is probably *the* common case, as few people edit their config files by hand these days (neither should they because it is too easy to end up with an unparseable one), the now-empty section *is* removed. So what is the argument against this extra care to detect comments? Well, if you have something like this: [section] ; Here we comment about the variable called snarf snarf = froop and we run `git config --unset section.snarf`, we end up with this config: [section] ; Here we comment about the variable called snarf which obviously does not make sense. However, that is already established behavior for quite a few years, and I do not even try to think of a way how this could be solved. Changes since v2: - removed the `inline` attribute from the `do_event()` function. - renamed `struct config_set_store` to `struct config_store_data`, to make its roled more obvious. - a whole slew of concocted test cases were added to the test to verify that a section that becomes empty is removed, based on Peff's analysis at https://public-inbox.org/git/20180329213229.GG2939@xxxxxxxxxxxxxxxxxxxxx/ Johannes Schindelin (15): git_config_set: fix off-by-two t1300: rename it to reflect that `repo-config` was deprecated t1300: demonstrate that --replace-all can "invent" newlines config --replace-all: avoid extra line breaks t1300: avoid relying on a bug t1300: remove unreasonable expectation from TODO t1300: add a few more hairy examples of sections becoming empty t1300: `--unset-all` can leave an empty section behind (bug) config: introduce an optional event stream while parsing config: avoid using the global variable `store` config_set_store: rename some fields for consistency git_config_set: do not use a state machine git_config_set: make use of the config parser's event stream git config --unset: remove empty sections (in the common case) git_config_set: reuse empty sections config.c | 448 ++++++++++++++------ config.h | 25 ++ t/{t1300-repo-config.sh => t1300-config.sh} | 102 ++++- 3 files changed, 439 insertions(+), 136 deletions(-) rename t/{t1300-repo-config.sh => t1300-config.sh} (95%) base-commit: 468165c1d8a442994a825f3684528361727cd8c0 Published-As: https://github.com/dscho/git/releases/tag/empty-config-section-v3 Fetch-It-Via: git fetch https://github.com/dscho/git empty-config-section-v3 Interdiff vs v2: diff --git a/config.c b/config.c index ee7ea24123d..6155d0651bd 100644 --- a/config.c +++ b/config.c @@ -659,8 +659,7 @@ struct parse_event_data { const struct config_options *opts; }; -static inline int do_event(enum config_event_t type, - struct parse_event_data *data) +static int do_event(enum config_event_t type, struct parse_event_data *data) { size_t offset; @@ -2297,7 +2296,7 @@ void git_die_config(const char *key, const char *err, ...) * Find all the stuff for git_config_set() below. */ -struct config_set_store { +struct config_store_data { int baselen; char *key; int do_not_match; @@ -2313,7 +2312,7 @@ struct config_set_store { }; static int matches(const char *key, const char *value, - const struct config_set_store *store) + const struct config_store_data *store) { if (strcmp(key, store->key)) return 0; /* not ours */ @@ -2329,7 +2328,7 @@ static int matches(const char *key, const char *value, static int store_aux_event(enum config_event_t type, size_t begin, size_t end, void *data) { - struct config_set_store *store = data; + struct config_store_data *store = data; ALLOC_GROW(store->parsed, store->parsed_nr + 1, store->parsed_alloc); store->parsed[store->parsed_nr].begin = begin; @@ -2360,7 +2359,7 @@ static int store_aux_event(enum config_event_t type, static int store_aux(const char *key, const char *value, void *cb) { - struct config_set_store *store = cb; + struct config_store_data *store = cb; if (store->key_seen) { if (matches(key, value, store)) { @@ -2401,7 +2400,7 @@ static int write_error(const char *filename) } static struct strbuf store_create_section(const char *key, - const struct config_set_store *store) + const struct config_store_data *store) { const char *dot; int i; @@ -2424,7 +2423,7 @@ static struct strbuf store_create_section(const char *key, } static ssize_t write_section(int fd, const char *key, - const struct config_set_store *store) + const struct config_store_data *store) { struct strbuf sb = store_create_section(key, store); ssize_t ret; @@ -2436,7 +2435,7 @@ static ssize_t write_section(int fd, const char *key, } static ssize_t write_pair(int fd, const char *key, const char *value, - const struct config_set_store *store) + const struct config_store_data *store) { int i; ssize_t ret; @@ -2495,7 +2494,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value, * array. * This index may be incremented if a section has more than one * entry (which all are to be removed). */ -static void maybe_remove_section(struct config_set_store *store, +static void maybe_remove_section(struct config_store_data *store, const char *contents, size_t *begin_offset, size_t *end_offset, int *seen_ptr) @@ -2625,7 +2624,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, char *filename_buf = NULL; char *contents = NULL; size_t contents_sz; - struct config_set_store store; + struct config_store_data store; memset(&store, 0, sizeof(store)); @@ -2969,7 +2968,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename FILE *config_file = NULL; struct stat st; struct strbuf copystr = STRBUF_INIT; - struct config_set_store store; + struct config_store_data store; memset(&store, 0, sizeof(store)); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 6d0e13020d1..eef0bbe4f9f 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1449,7 +1449,50 @@ test_expect_success '--unset last key removes section (except if commented)' ' EOF git config --unset section.key && - test_cmp expect .git/config + test_cmp expect .git/config && + + q_to_tab >.git/config <<-\EOF && + [one] + Qkey = "multiline \ + QQ# with comment" + [two] + key = true + EOF + git config --unset two.key && + ! grep two .git/config && + + q_to_tab >.git/config <<-\EOF && + [one] + Qkey = "multiline \ + QQ# with comment" + [one] + key = true + EOF + git config --unset-all one.key && + test_line_count = 0 .git/config && + + q_to_tab >.git/config <<-\EOF && + [one] + Qkey = true + Q# a comment not at the start + [two] + Qkey = true + EOF + git config --unset two.key && + grep two .git/config && + + q_to_tab >.git/config <<-\EOF && + [one] + Qkey = not [two "subsection"] + [two "subsection"] + [two "subsection"] + Qkey = true + [TWO "subsection"] + [one] + EOF + git config --unset two.subsection.key && + test "not [two subsection]" = "$(git config one.key)" && + test_line_count = 3 .git/config ' test_expect_success '--unset-all removes section if empty & uncommented' ' -- 2.17.0.windows.1.4.g7e4058d72e3
