On Tue, Jul 15, 2025 at 03:00:56PM +0100, Phillip Wood wrote:
> We could fix this by interning the "path" field as we do
> for the "filename" field but the "path" field is not actually
> needed. It is populated with a copy of the "path" field from "struct
> config_source". That field was added in d14d42440d8 (config: disallow
> relative include paths from blobs, 2014-02-19) to distinguish between
> relative include directives in files and those in blobs. However,
> since 1b8132d99d8 (i18n: config: unfold error messages marked for
> translation, 2016-07-28) we can differentiate these by looking at the
> "origin_type" field in "struct key_value_info". So let's remove the
> "path" members from "struct config_source" and "struct key_value_info"
> and instead use a combination of the "filename" and "origin_type"
> fields to determine the absolute path of relative includes.
Nicely explained. The interesting bit of the patch is here:
> @@ -173,14 +172,14 @@ static int handle_path_include(const struct key_value_info *kvi,
> if (!is_absolute_path(path)) {
> char *slash;
>
> - if (!kvi || !kvi->path) {
> + if (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE) {
> ret = error(_("relative config includes must come from files"));
> goto cleanup;
> }
...which is where we no longer need the separate variable to make the
path/filename distinction.
Just playing devil's advocate, my big questions would be:
- can filename ever be unexpectedly NULL, even if type is
CONFIG_ORIGIN_FILE?
- do we always set filename anywhere we would have set path?
It looks like it was usually set here:
> @@ -1036,7 +1034,6 @@ static void kvi_from_source(struct config_source *cs,
> out->origin_type = cs->origin_type;
> out->linenr = cs->linenr;
> out->scope = scope;
> - out->path = cs->path;
> }
and there we'd always set out->filename from cs->name (just outside of
the context shown here). And we do so with strintern() which would
segfault if cs->name were NULL, so I think we can always depend on it. ;)
> static int do_config_from_file(config_fn_t fn,
> const enum config_origin_type origin_type,
> - const char *name, const char *path, FILE *f,
> - void *data, enum config_scope scope,
> + const char *name, FILE *f, void *data,
> + enum config_scope scope,
> const struct config_options *opts)
> {
> struct config_source top = CONFIG_SOURCE_INIT;
> int ret;
>
> + if (origin_type == CONFIG_ORIGIN_FILE && (!name || !*name))
> + BUG("missing filename for CONFIG_ORIGIN_FILE");
> +
OK and here we have a sanity check that our callers will always feed an
actual name. The main caller being...
> @@ -1891,7 +1890,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
> f = fopen_or_warn(filename, "r");
> if (f) {
> ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
> - filename, f, data, scope, opts);
> + f, data, scope, opts);
> fclose(f);
> }
> return ret;
...this one. Which previously passed "filename" as both the "name" and
"path" fields, so they were always identical.
Makes sense, and the patch looks good to me. Thanks for finding it!
-Peff
