Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type
- From
Jeff King <peff@peff.net>
- Date
- Feb 13, 2016, 17:24 UTC
- Message-ID
- <20160213172435.GG30144@sigill.intra.peff.net>
- In-Reply-To
- <1455373456-64691-3-git-send-email-larsxschneider@gmail.com>
On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com> > > Use the config type to print more detailed error messages that inform > the user about the origin of a config error (file, stdin, blob).
This looks OK overall. A few minor nits...
Show 8 quoted lines
> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf, > top.u.buf.buf = buf; > top.u.buf.len = len; > top.u.buf.pos = 0; > + top.type = "blob"; > top.name = name; > top.path = NULL; > top.die_on_error = 0;
This function is about reading config from a memory buffer. The only reason we do so _now_ is when reading from a blob, but I think it is laying a trap for somebody who wants to reuse the function later.
Should git_config_from_buf() take a "type" parameter, and git_config_from_blob_sha1() pass in "blob"?
Show 17 quoted lines
> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
> struct config_source top;
>
> top.u.file = f;
> - top.name = name;
> + top.type = path ? "file" : "stdin";
> + top.name = name ? name : "";
> top.path = path;
> top.die_on_error = 1;
> top.do_fgetc = config_file_fgetc;
> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,
>
> static int git_config_from_stdin(config_fn_t fn, void *data)
> {
> - return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
> + return do_config_from_file(fn, NULL, NULL, stdin, data);
> }Likewise here, we make assumptions in do_config_from_file() about what the NULL path means. I think this is less likely to be a problem than the other case, but it seems like it would be cleaner for "file" or "stdin" to come from the caller, which knows for sure what we are doing.
Similarly, I think git_config_from_stdin() can simply pass in an empty name rather than NULL to avoid do_config_from_file() having to fix it up.
Show 5 quoted lines
> +test_expect_success 'invalid stdin config' ' > + echo "fatal: bad config line 1 in stdin " >expect && > + echo "[broken" | test_must_fail git config --list --file - >output 2>&1 && > + test_cmp expect output > +'
The original would have been "bad config file line 1 in <stdin>"; I think this is an improvement to drop the "file" here.
-Peff