Skip to content

Commit

Permalink
usage API: add "core.usageAddSource" config to add <file>:<line>
Browse files Browse the repository at this point in the history
Optionally extend the support that BUG() has had for emitting line
numbers to the {usage,fatal,error,warning}{,_errno}() functions.

Before we'd unconditionally get error messages like:

    $ git -c core.x=y config --get --bool core.x
    fatal: bad boolean config value 'y' for 'core.x'

Which can be changed with core.usageAddSource=true to include the file
and line numbers:

    $ git -c core.usageAddSource=true -c core.x=y config --get --bool core.x
    fatal: config.c:1241: bad boolean config value 'y' for 'core.x'

As the added documentation notes this is primarily intended to be
useful to developers of git itself, but is being exposed as a user
setting to e.g. help file better bug reports.

This also adds a "GIT_TEST_USAGE_ADD_SOURCE" setting intended to run
the test suite in this mode.

Currently it has a lot of failures. Most of those are rather trivial,
and can be "fixed" by pointing GIT_TEST_CMP to a "diff -u" that does a
s/^(usage|fatal|error|warning): [^:]+:[0-9]+/$1/g on its input files,
and likewise for a "grep" wrapper that does the same.

Even if we can't run the tests in this mode yet I'd like to have this
for ad-hoc debugging, and to make it easier to work towards running
the tests in this mode. If we can turn this on permanently it'll be
much easier to read test output, as we won't need to worry about the
indirection of looking up where an error might have been emitted,
which can be especially painful when the message being emitted isn't
unique within git.git.

This new code needs to be guarded by the "dying" variable for the
reasons explained in 2d3c02f (die(): stop hiding errors due to
overzealous recursion guard, 2017-06-21), and for those same reasons
it's racy under multi-threading.

Here the worst case is that incrementing the variable will run away
from us, and we won't get our desired <file>:<line> number. That's OK
in this case, those cases should be isolated to the config code (if we
can't parse the config), memory allocation etc, but we'll get it right
in the common cases.

Using GIT_TEST_USAGE_ADD_SOURCE should be immune from any racyness, as
it only needs a getenv() and git_parse_maybe_bool(), which won't die.

Add a repo_cfg_bool_env() wrapper to repo-settings.c for
GIT_TEST_USAGE_ADD_SOURCE, in 3050b6d (repo-settings.c: simplify
the setup, 2021-09-21) I indicated that the GIT_TEST_MULTI_PACK_INDEX
env variable/config pair in that file has odd semantics, but users of
repo_cfg_bool_env() have straightforward and expected semantics. If
the environment variable is set (true or false) we'll use it,
otherwise we'll use the config, and finally fall back on the
default (of "false", in this case).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
  • Loading branch information
avar committed Nov 19, 2021
1 parent 69426dd commit ce0c266
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 2 deletions.
7 changes: 7 additions & 0 deletions Documentation/config/core.txt
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ core.untrackedCache::
`feature.manyFiles` is enabled which sets this setting to
`true` by default.

core.usageAddSource::
Adds the source "<file>:<line>" numbers to usage messages
("usage:", "fatal:", "error:", "warning:"). This setting is
primarily intended for those debugging or developing git
itself, or to compose a bug report against git itself (see
linkgit:git-bugreport[1]).

core.checkStat::
When missing or is set to `default`, many fields in the stat
structure are checked to detect if a file has been modified
Expand Down
11 changes: 11 additions & 0 deletions repo-settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
*dest = def;
}

static void repo_cfg_bool_env(struct repository *r, const char *env,
const char *key, int *dest, int def)
{
*dest = git_env_bool(env, -1);
if (*dest != -1)
return;
repo_cfg_bool(r, key, dest, def);
}

void prepare_repo_settings(struct repository *r)
{
int experimental;
Expand Down Expand Up @@ -45,6 +54,8 @@ void prepare_repo_settings(struct repository *r)
repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
repo_cfg_bool_env(r, "GIT_TEST_USAGE_ADD_SOURCE", "core.usageaddsource",
&r->settings.usage_add_source, 0);
repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);

/*
Expand Down
2 changes: 2 additions & 0 deletions repository.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ struct repo_settings {
enum fetch_negotiation_setting fetch_negotiation_algorithm;

int core_multi_pack_index;

int usage_add_source;
};

struct repository {
Expand Down
24 changes: 22 additions & 2 deletions usage.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
#include "git-compat-util.h"
#include "cache.h"
#include "config.h"

enum usage_kind {
USAGE_USAGE,
Expand All @@ -14,6 +15,8 @@ enum usage_kind {
USAGE_BUG,
};

static int dying;

static void vreportf(enum usage_kind kind,
const char *file, int line,
const char *err, va_list params)
Expand All @@ -23,6 +26,24 @@ static void vreportf(enum usage_kind kind,
char msg[4096];
char *p, *pend = msg + sizeof(msg);
size_t prefix_len;
static int addln = -1;

if (addln < 0 && dying <= 1) {
struct repository *r = the_repository;
prepare_repo_settings(r);
addln = r->settings.usage_add_source;
} else if (addln < 0 && dying) {
/*
* We're already dying when trying to read our config,
* presumably via prepare_repo_settings(). Do a
* last-ditch effort of trying to get it from the
* environment.
*/
const char *v = getenv("GIT_TEST_USAGE_ADD_SOURCE");
addln = v ? git_parse_maybe_bool(v) : 0;
if (addln < 1)
addln = 0;
}

switch (kind) {
case USAGE_USAGE:
Expand All @@ -43,7 +64,7 @@ static void vreportf(enum usage_kind kind,
}

/* truncation via snprintf is OK here */
if (kind == USAGE_BUG)
if (kind == USAGE_BUG || addln)
snprintf(prefix, sizeof(prefix), "%s%s:%d: ", prefix_i18n, file, line);
else
snprintf(prefix, sizeof(prefix), "%s", prefix_i18n);
Expand Down Expand Up @@ -123,7 +144,6 @@ static void warning_builtin(const char *file, int line, const char *warn, va_lis

static int die_is_recursing_builtin(void)
{
static int dying;
/*
* Just an arbitrary number X where "a < x < b" where "a" is
* "maximum number of pthreads we'll ever plausibly spawn" and
Expand Down

0 comments on commit ce0c266

Please sign in to comment.