From d8193743e0883e7331c102a4d04cee5324eb3b5f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 May 2017 23:28:50 -0400 Subject: [PATCH 1/4] usage.c: add BUG() function There's a convention in Git's code base to write assertions as: if (...some_bad_thing...) die("BUG: the terrible thing happened"); with the idea that users should never see a "BUG:" message (but if they, it at least gives a clue what happened). We use die() here because it's convenient, but there are a few draw-backs: 1. Without parsing the messages, it's hard for callers to distinguish BUG assertions from regular errors. For instance, it would be nice if the test suite could check that we don't hit any assertions, but test_must_fail will pass BUG deaths as OK. 2. It would be useful to add more debugging features to BUG assertions, like file/line numbers or dumping core. 3. The die() handler can be replaced, and might not actually exit the whole program (e.g., it may just pthread_exit()). This is convenient for normal errors, but for an assertion failure (which is supposed to never happen), we're probably better off taking down the whole process as quickly and cleanly as possible. We could address these by checking in die() whether the error message starts with "BUG", and behaving appropriately. But there's little advantage at that point to sharing the die() code, and only downsides (e.g., we can't change the BUG() interface independently). Moreover, converting all of the existing BUG calls reveals that the test suite does indeed trigger a few of them. Instead, this patch introduces a new BUG() function, which prints an error before dying via SIGABRT. This gives us test suite checking and core dumps. The function is actually a macro (when supported) so that we can show the file/line number. We can convert die("BUG") invocations to BUG() in further patches, dealing with any test fallouts individually. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 9 +++++++++ usage.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index bd04564a69..4575b3890b 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1064,6 +1064,15 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size, #define HAVE_VARIADIC_MACROS 1 #endif +#ifdef HAVE_VARIADIC_MACROS +__attribute__((format (printf, 3, 4))) NORETURN +void BUG_fl(const char *file, int line, const char *fmt, ...); +#define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__) +#else +__attribute__((format (printf, 1, 2))) NORETURN +void BUG(const char *fmt, ...); +#endif + /* * Preserves errno, prints a message, but gives no warning for ENOENT. * Returns 0 on success, which includes trying to unlink an object that does diff --git a/usage.c b/usage.c index ad6d2910fb..7e6cb20280 100644 --- a/usage.c +++ b/usage.c @@ -201,3 +201,35 @@ void warning(const char *warn, ...) warn_routine(warn, params); va_end(params); } + +static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params) +{ + char prefix[256]; + + /* truncation via snprintf is OK here */ + if (file) + snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line); + else + snprintf(prefix, sizeof(prefix), "BUG: "); + + vreportf(prefix, fmt, params); + abort(); +} + +#ifdef HAVE_VARIADIC_MACROS +void BUG_fl(const char *file, int line, const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + BUG_vfl(file, line, fmt, ap); + va_end(ap); +} +#else +void BUG(const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + BUG_vfl(NULL, 0, fmt, ap); + va_end(ap); +} +#endif From 588a538ae554f61a37d43c972da75d0f7c3ed484 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 May 2017 23:29:18 -0400 Subject: [PATCH 2/4] setup_git_env: convert die("BUG") to BUG() Converting to BUG() makes it easier to detect and debug cases where we hit this assertion. Coupled with a new test in t1300, this shows that the test suite can detect such corner cases. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- environment.c | 2 +- t/t1300-repo-config.sh | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/environment.c b/environment.c index ff6e4f06e9..1f0bda5afa 100644 --- a/environment.c +++ b/environment.c @@ -169,7 +169,7 @@ static void setup_git_env(void) git_dir = getenv(GIT_DIR_ENVIRONMENT); if (!git_dir) { if (!startup_info->have_repository) - die("BUG: setup_git_env called without repository"); + BUG("setup_git_env called without repository"); git_dir = DEFAULT_GIT_DIR_ENVIRONMENT; } gitfile = read_gitfile(git_dir); diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index afcca0d52c..867704a642 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1539,4 +1539,10 @@ test_expect_success !MINGW '--show-origin blob ref' ' test_cmp expect output ' +test_expect_failure '--local requires a repo' ' + # we expect 128 to ensure that we do not simply + # fail to find anything and return code "1" + test_expect_code 128 nongit git config --local foo.bar +' + test_done From 25cd291963e4b0fae0eabe7fe02be693702d79bb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 May 2017 23:29:31 -0400 Subject: [PATCH 3/4] config: complain about --local outside of a git repo The "--local" option instructs git-config to read or modify the repository-level config. This doesn't make any sense if you're not actually in a repository. Older versions of Git would blindly try to read or write ".git/config". For reading, this would result in a quiet failure, since there was no config to read (and thus no matching config value). Writing would generally fail noisily, since ".git" was unlikely to exist. But since b1ef400ee (setup_git_env: avoid blind fall-back to ".git", 2016-10-20), we catch this in the call to git_pathdup() and die with an assertion. Dying is the right thing to do, but we should catch the problem early and give a more human-friendly error message. Note that even without --local, git-config will sometimes default to using local repository config (e.g., when writing). These cases are already protected by similar checks, and covered by a test in t1308. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 3 +++ t/t1300-repo-config.sh | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/config.c b/builtin/config.c index 3a554ad50c..7f6c25d4d9 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_with_options(builtin_config_usage, builtin_config_options); } + if (use_local_config && nongit) + die(_("--local can only be used inside a git repository")); + if (given_config_source.file && !strcmp(given_config_source.file, "-")) { given_config_source.file = NULL; diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 867704a642..13b7851f7c 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1539,7 +1539,7 @@ test_expect_success !MINGW '--show-origin blob ref' ' test_cmp expect output ' -test_expect_failure '--local requires a repo' ' +test_expect_success '--local requires a repo' ' # we expect 128 to ensure that we do not simply # fail to find anything and return code "1" test_expect_code 128 nongit git config --local foo.bar From 3d7dd2d3b6ccc8903a37cffe3a2f39cf1be21c86 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Sun, 21 May 2017 23:25:39 +0100 Subject: [PATCH 4/4] usage: add NORETURN to BUG() function definitions Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the BUG() functions and macros as a replacement for calls to die("BUG: .."). The use of NORETURN on the declarations (in git-compat-util.h) and the lack of NORETURN on the function definitions, however, leads sparse to complain thus: SP usage.c usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers In order to suppress the sparse error, add the NORETURN to the function definitions. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- usage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/usage.c b/usage.c index 7e6cb20280..1f63e033e9 100644 --- a/usage.c +++ b/usage.c @@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis } #ifdef HAVE_VARIADIC_MACROS -void BUG_fl(const char *file, int line, const char *fmt, ...) +NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) { va_list ap; va_start(ap, fmt); @@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...) va_end(ap); } #else -void BUG(const char *fmt, ...) +NORETURN void BUG(const char *fmt, ...) { va_list ap; va_start(ap, fmt);