Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

Jeff King [off-list ref] writes:

quoted

@@ -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"?

I think that is sensible.  I think s/from_buf/from_mem/ may also be
sensible (it would match the naming used in the index_{fd,mem,...}
functions in he hashing code).

quoted

 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.

Makes sense.

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.

This too.

quoted

+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

On 13 Feb 2016, at 22:04, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:

quoted

quoted

@@ -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"?

I think that is sensible.  I think s/from_buf/from_mem/ may also be
sensible (it would match the naming used in the index_{fd,mem,...}
functions in he hashing code).

OK, I will change that, too!

quoted

quoted

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.

Makes sense.

quoted

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.

This too.

quoted

quoted

+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

Thanks,
Lars