From a060cccc0a51ee7eff92b674039bc7fc91dc5d46 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Thu, 16 Apr 2015 10:53:22 -0400 Subject: [PATCH 1/3] Unittest to validate config entry deletion bug. Add a unittest to validate bug #3043, where a duplicate empty config header could cause deletion of a config entry to fail silently. The bug is currently unresolved and this test will fail. --- tests/config/write.c | 31 +++++++++++++++++++++++++++++++ tests/resources/config/config21 | Bin 0 -> 166 bytes 2 files changed, 31 insertions(+) create mode 100644 tests/resources/config/config21 diff --git a/tests/config/write.c b/tests/config/write.c index 32e6f27b4..6318c49fe 100644 --- a/tests/config/write.c +++ b/tests/config/write.c @@ -6,6 +6,7 @@ void test_config_write__initialize(void) cl_fixture_sandbox("config/config9"); cl_fixture_sandbox("config/config15"); cl_fixture_sandbox("config/config17"); + cl_fixture_sandbox("config/config21"); } void test_config_write__cleanup(void) @@ -13,6 +14,7 @@ void test_config_write__cleanup(void) cl_fixture_cleanup("config9"); cl_fixture_cleanup("config15"); cl_fixture_cleanup("config17"); + cl_fixture_cleanup("config21"); } void test_config_write__replace_value(void) @@ -106,6 +108,35 @@ void test_config_write__delete_value_at_specific_level(void) git_config_free(cfg); } +/* + * This test exposes a bug where duplicate empty section headers could prevent + * deletion of config entries. + */ +void test_config_write__delete_value_with_duplicate_header(void) +{ + const char *file_name = "config21"; + const char *entry_name = "remote.origin.url"; + git_config *cfg; + git_config_entry *entry; + + /* Make sure the expected entry exists */ + cl_git_pass(git_config_open_ondisk(&cfg, file_name)); + cl_git_pass(git_config_get_entry(&entry, cfg, entry_name)); + + /* Delete that entry */ + cl_git_pass(git_config_delete_entry(cfg, entry_name)); + + /* Reopen the file and make sure the entry no longer exists */ + git_config_entry_free(entry); + git_config_free(cfg); + cl_git_pass(git_config_open_ondisk(&cfg, file_name)); + cl_git_fail(git_config_get_entry(&entry, cfg, entry_name)); + + /* Cleanup */ + git_config_entry_free(entry); + git_config_free(cfg); +} + void test_config_write__write_subsection(void) { git_config *cfg; diff --git a/tests/resources/config/config21 b/tests/resources/config/config21 new file mode 100644 index 0000000000000000000000000000000000000000..a63b52ff5fdac6f1572e1c729388e329a6d2171b GIT binary patch literal 166 zcmZw9I}QRd3pdy`9M;s+=h{}3K9u4 zrD34z#b9OECrblKp5aV6ujIWzjT_5bcnRB*UJyTZyoK# Date: Thu, 16 Apr 2015 15:20:33 -0400 Subject: [PATCH 2/3] Specify mock config file content in test. Instead of using a config file in resources, include the config file content to be tested directly in the test. --- tests/config/write.c | 15 +++++++++++---- tests/resources/config/config21 | Bin 166 -> 0 bytes 2 files changed, 11 insertions(+), 4 deletions(-) delete mode 100644 tests/resources/config/config21 diff --git a/tests/config/write.c b/tests/config/write.c index 6318c49fe..bcc87571c 100644 --- a/tests/config/write.c +++ b/tests/config/write.c @@ -6,7 +6,6 @@ void test_config_write__initialize(void) cl_fixture_sandbox("config/config9"); cl_fixture_sandbox("config/config15"); cl_fixture_sandbox("config/config17"); - cl_fixture_sandbox("config/config21"); } void test_config_write__cleanup(void) @@ -14,7 +13,6 @@ void test_config_write__cleanup(void) cl_fixture_cleanup("config9"); cl_fixture_cleanup("config15"); cl_fixture_cleanup("config17"); - cl_fixture_cleanup("config21"); } void test_config_write__replace_value(void) @@ -114,12 +112,21 @@ void test_config_write__delete_value_at_specific_level(void) */ void test_config_write__delete_value_with_duplicate_header(void) { - const char *file_name = "config21"; + const char *file_name = "config-duplicate-header"; const char *entry_name = "remote.origin.url"; git_config *cfg; git_config_entry *entry; - /* Make sure the expected entry exists */ + /* This config can occur after removing and re-adding the origin remote */ + const char *file_content = + "[remote \"origin\"]\n" \ + "[branch \"master\"]\n" \ + " remote = \"origin\"\n" \ + "[remote \"origin\"]\n" \ + " url = \"foo\"\n"; + + /* Write the test config and make sure the expected entry exists */ + cl_git_mkfile(file_name, file_content); cl_git_pass(git_config_open_ondisk(&cfg, file_name)); cl_git_pass(git_config_get_entry(&entry, cfg, entry_name)); diff --git a/tests/resources/config/config21 b/tests/resources/config/config21 deleted file mode 100644 index a63b52ff5fdac6f1572e1c729388e329a6d2171b..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 166 zcmZw9I}QRd3pdy`9M;s+=h{}3K9u4 zrD34z#b9OECrblKp5aV6ujIWzjT_5bcnRB*UJyTZyoK# Date: Thu, 16 Apr 2015 15:32:16 -0400 Subject: [PATCH 3/3] git_config_delete: search until last section. If git_config_delete is to work properly in the presence of duplicate section headers, it cannot stop searching at the end of the first matching section, as there may be another matching section later. When config_write is used for deletion (value = NULL), it may only terminate when the desired key is found or there are no sections left to parse. --- src/config_file.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/config_file.c b/src/config_file.c index 350473434..7c6cb81fe 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1460,9 +1460,12 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p * don't loose that information, but we only need to * update post_start if we're going to use it in this * iteration. + * If the section doesn't match and we are trying to delete an entry + * (value == NULL), we must continue searching; there may be another + * matching section later. */ if (!section_matches) { - if (!last_section_matched) { + if (!last_section_matched || value == NULL) { reader_consume_line(reader); continue; }