Re: Re: [PATCH v2 4/4] teach config parsing to read from strbuf
Hi Peff, On Thu, Mar 14, 2013 at 03:39:14AM -0400, Jeff King wrote: On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote: > I looked into this a little. The first sticking point is that > git_config_with_options needs to support the alternate source. Here's a > sketch of what I think the solution should look like, on top of your > patches. Here it is again, with two changes: 1. Rather than handling the blob lookup inline in git_config_with_options, it adds direct functions for reading config from blob sha1s and blob references. I think this should make it easier to reuse when you are trying to read .gitmodules from C code. 2. It adds some basic tests. I'll leave it here for tonight. The next step would be to rebase it on your modified series (in particular, I think git_config_from_strbuf should become git_config_from_buf, which will impact this). Feel free to use, pick apart, rewrite, or discard as you see fit for your series. Sorry for the late reply, I was not online during the last days. Thanks a lot for this. I will use it in the next iteration of the series. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:
> I looked into this a little. The first sticking point is that
> git_config_with_options needs to support the alternate source. Here's a
> sketch of what I think the solution should look like, on top of your
> patches.
Here it is again, with two changes:
1. Rather than handling the blob lookup inline in
git_config_with_options, it adds direct functions for reading
config from blob sha1s and blob references. I think this should
make it easier to reuse when you are trying to read .gitmodules
from C code.
2. It adds some basic tests.
I'll leave it here for tonight. The next step would be to rebase it on
your modified series (in particular, I think git_config_from_strbuf
should become git_config_from_buf, which will impact this).
Feel free to use, pick apart, rewrite, or discard as you see fit for
your series.
diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
static int use_global_config, use_system_config, use_local_config;
static const char *given_config_file;
+static const char *given_config_blob;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config
file")),
OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config
file")),
OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given
config file")),
+ OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read
config from given blob object")),
OPT_GROUP(N_("Action")),
OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"),
ACTION_GET),
OPT_BIT(0, "get-all", &actions, N_("get all values: key
[value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
}
git_config_with_options(collect_config, &values,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
ret = !values.nr;
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
+static void check_blob_write(void)
+{
+ if (given_config_blob)
+ die("writing config blobs is not supported");
+}
+
int cmd_config(int argc, const char **argv, const char *prefix)
{
int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
builtin_config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
- if (use_global_config + use_system_config + use_local_config +
!!given_config_file > 1) {
+ if (use_global_config + use_system_config + use_local_config +
+ !!given_config_file + !!given_config_blob > 1) {
error("only one config file at a time.");
usage_with_options(builtin_config_usage,
builtin_config_options);
}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
given_config_file,
+ given_config_blob,
respect_includes) < 0) {
if (given_config_file)
die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (!given_config_file && nongit)
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Thu, Mar 14, 2013 at 07:39:33AM +0100, Heiko Voigt wrote:
> > I am on the fence. I do not want to create more work for you, but I do
> > think it may come in handy, if only for doing submodule things from
> > shell. And it is hopefully not a very large patch. I'd say try it, and
> > if starts looking like it will be very ugly, the right thing may be to
> > leave it until somebody really wants it.
>
> Thats what I will do: Try it and see where I get.
I looked into this a little. The first sticking point is that
git_config_with_options needs to support the alternate source. Here's a
sketch of what I think the solution should look like, on top of your
patches.
diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
static int use_global_config, use_system_config, use_local_config;
static const char *given_config_file;
+static const char *given_config_blob;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config
file")),
OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config
file")),
OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given
config file")),
+ OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read
config from given blob object")),
OPT_GROUP(N_("Action")),
OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"),
ACTION_GET),
OPT_BIT(0, "get-all", &actions, N_("get all values: key
[value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
}
git_config_with_options(collect_config, &values,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
ret = !values.nr;
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
+static void check_blob_write(void)
+{
+ if (given_config_blob)
+ die("writing config blobs is not supported");
+}
+
int cmd_config(int argc, const char **argv, const char *prefix)
{
int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
builtin_config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
- if (use_global_config + use_system_config + use_local_config +
!!given_config_file > 1) {
+ if (use_global_config + use_system_config + use_local_config +
+ !!given_config_file + !!given_config_blob > 1) {
error("only one config file at a time.");
usage_with_options(builtin_config_usage,
builtin_config_options);
}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
given_config_file,
+ given_config_blob,
respect_includes) < 0) {
if (given_config_file)
die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (!given_config_file && nongit)
die("not in a git directory");
+ if (given_config_blob)
+ die("editing blobs is not supported");
git_config(git_default_config, NULL);
launch_editor(giv
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Tue, Mar 12, 2013 at 03:29:59PM -0400, Jeff King wrote: > On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote: > > > > Your series does not actually add any callers of the new function. The > > > obvious "patch 5/4" would be to plumb it into "git config --blob", and > > > then we can just directly test it there (there could be other callers > > > besides reading from a blob, of course, but I think the point of the > > > series is to head in that direction). > > > > Since this is a split of the series mentioned above there are no real > > callers yet. The main reason for the split was that I wanted to reduce > > the review burden of one big series into multiple reviews of smaller > > chunks. If you think it is useful to add the --blob option I can also > > test from there. It could actually be useful to look at certain > > .gitmodules options from the submodule script. > > I am on the fence. I do not want to create more work for you, but I do > think it may come in handy, if only for doing submodule things from > shell. And it is hopefully not a very large patch. I'd say try it, and > if starts looking like it will be very ugly, the right thing may be to > leave it until somebody really wants it. Thats what I will do: Try it and see where I get. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote: > > Your series does not actually add any callers of the new function. The > > obvious "patch 5/4" would be to plumb it into "git config --blob", and > > then we can just directly test it there (there could be other callers > > besides reading from a blob, of course, but I think the point of the > > series is to head in that direction). > > Since this is a split of the series mentioned above there are no real > callers yet. The main reason for the split was that I wanted to reduce > the review burden of one big series into multiple reviews of smaller > chunks. If you think it is useful to add the --blob option I can also > test from there. It could actually be useful to look at certain > .gitmodules options from the submodule script. I am on the fence. I do not want to create more work for you, but I do think it may come in handy, if only for doing submodule things from shell. And it is hopefully not a very large patch. I'd say try it, and if starts looking like it will be very ugly, the right thing may be to leave it until somebody really wants it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Tue, Mar 12, 2013 at 07:18:06AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:
>
> > This can be used to read configuration values directly from gits
> > database.
> >
> > Signed-off-by: Heiko Voigt
>
> This is lacking motivation. IIRC, the rest of the story is something
> like "...so we can read .gitmodules directly from the repo" or something
> like that?
Will add some more here.
> > +struct config_strbuf {
> > + struct strbuf *strbuf;
> > + int pos;
> > +};
> >
> > +static int config_strbuf_fgetc(struct config_source *conf)
> > +{
> > + struct config_strbuf *str = conf->data;
>
> Yuck. If you used a union in the previous patch, then this could just go
> inline into the "struct config_source".
>
> > +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf
> > *strbuf, void *data)
>
> Should this be a "const struct strbuf *strbuf"? For that matter, is
> there any reason not to take a bare pointer/len combination? It seems
> likely that callers would get the data from read_sha1_file, which means
> they have to stuff it into a strbuf for no good reason.
pointer/len should be fine too. I just used strbuf since when you find
out later that you need to modify the string its easier to handle. A
config parser should not need to do that so I will change that.
> > diff --git a/test-config.c b/test-config.c
> > new file mode 100644
> > index 000..c650837
> > --- /dev/null
> > +++ b/test-config.c
> > @@ -0,0 +1,40 @@
>
> I'm slightly "meh" on this test-config program. Having to add a C test
> harness like this is a good indication that we are short-changing users
> of the shell API in favor of builtin C code.
I mainly did this because I needed some test for the config part while
developing the "fetch renamed submodules" series.
> Your series does not actually add any callers of the new function. The
> obvious "patch 5/4" would be to plumb it into "git config --blob", and
> then we can just directly test it there (there could be other callers
> besides reading from a blob, of course, but I think the point of the
> series is to head in that direction).
Since this is a split of the series mentioned above there are no real
callers yet. The main reason for the split was that I wanted to reduce
the review burden of one big series into multiple reviews of smaller
chunks. If you think it is useful to add the --blob option I can also
test from there. It could actually be useful to look at certain
.gitmodules options from the submodule script.
Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] teach config parsing to read from strbuf
On Sun, Mar 10, 2013 at 06:00:52PM +0100, Heiko Voigt wrote:
> This can be used to read configuration values directly from gits
> database.
>
> Signed-off-by: Heiko Voigt
This is lacking motivation. IIRC, the rest of the story is something
like "...so we can read .gitmodules directly from the repo" or something
like that?
> +struct config_strbuf {
> + struct strbuf *strbuf;
> + int pos;
> +};
>
> +static int config_strbuf_fgetc(struct config_source *conf)
> +{
> + struct config_strbuf *str = conf->data;
Yuck. If you used a union in the previous patch, then this could just go
inline into the "struct config_source".
> +int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf
> *strbuf, void *data)
Should this be a "const struct strbuf *strbuf"? For that matter, is
there any reason not to take a bare pointer/len combination? It seems
likely that callers would get the data from read_sha1_file, which means
they have to stuff it into a strbuf for no good reason.
> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 000..c650837
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,40 @@
I'm slightly "meh" on this test-config program. Having to add a C test
harness like this is a good indication that we are short-changing users
of the shell API in favor of builtin C code.
Your series does not actually add any callers of the new function. The
obvious "patch 5/4" would be to plumb it into "git config --blob", and
then we can just directly test it there (there could be other callers
besides reading from a blob, of course, but I think the point of the
series is to head in that direction).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] teach config parsing to read from strbuf
This can be used to read configuration values directly from gits
database.
Signed-off-by: Heiko Voigt
---
.gitignore | 1 +
Makefile | 1 +
cache.h| 2 ++
config.c | 48
t/t1300-repo-config.sh | 24
test-config.c | 40
6 files changed, 116 insertions(+)
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 6669bf0..386b7f2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -178,6 +178,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 26d3332..1a9ea10 100644
--- a/Makefile
+++ b/Makefile
@@ -541,6 +541,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/cache.h b/cache.h
index e493563..a2621fa 100644
--- a/cache.h
+++ b/cache.h
@@ -1128,6 +1128,8 @@ extern int update_server_info(int);
typedef int (*config_fn_t)(const char *, const char *, void *);
extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern int git_config_from_strbuf(config_fn_t fn, const char *name,
+ struct strbuf *strbuf, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
diff --git a/config.c b/config.c
index fe1c0e5..b8c8640 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,37 @@ static long config_file_ftell(struct config_source *conf)
return ftell(f);
}
+struct config_strbuf {
+ struct strbuf *strbuf;
+ int pos;
+};
+
+static int config_strbuf_fgetc(struct config_source *conf)
+{
+ struct config_strbuf *str = conf->data;
+
+ if (str->pos < str->strbuf->len)
+ return str->strbuf->buf[str->pos++];
+
+ return EOF;
+}
+
+static int config_strbuf_ungetc(int c, struct config_source *conf)
+{
+ struct config_strbuf *str = conf->data;
+
+ if (str->pos > 0)
+ return str->strbuf->buf[--str->pos];
+
+ return EOF;
+}
+
+static long config_strbuf_ftell(struct config_source *conf)
+{
+ struct config_strbuf *str = conf->data;
+ return str->pos;
+}
+
#define MAX_INCLUDE_DEPTH 10
static const char include_depth_advice[] =
"exceeded maximum include depth (%d) while including\n"
@@ -961,6 +992,23 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
return ret;
}
+int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf
*strbuf, void *data)
+{
+ struct config_source top;
+ struct config_strbuf str;
+
+ str.strbuf = strbuf;
+ str.pos = 0;
+
+ top.data = &str;
+ top.name = name;
+ top.fgetc = config_strbuf_fgetc;
+ top.ungetc = config_strbuf_ungetc;
+ top.ftell = config_strbuf_ftell;
+
+ return do_config_from_source(&top, fn, data);
+}
+
const char *git_etc_gitconfig(void)
{
static const char *system_wide;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c96fda..5103f66 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1087,4 +1087,28 @@ test_expect_success 'barf on incomplete string' '
grep " line 3 " error
'
+test_expect_success 'reading config from strbuf' '
+ cat >expect <<-\EOF &&
+ var: some.value, value: content
+ EOF
+ test-config strbuf 12345:.gitmodules >actual <<-\EOF &&
+ [some]
+ value = content
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'reading config from strbuf with error' '
+ touch expect.out &&
+ cat >expect.err <<-\EOF &&
+ fatal: bad config file line 2 in 12345:.gitmodules
+ EOF
+ test_must_fail test-config strbuf 12345:.gitmodules >actual.out
2>actual.err <<-\EOF &&
+ [some]
+ value = "
+ EOF
+ test_cmp expect.out actual.out &&
+ test_cmp expect.err actual.err
+'
+
test_done
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 000..c650837
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,40 @@
+#include "cache.h"
+
+static int config_strbuf(const char *var, const char *value, void *data)
+{
+ printf("var: %s, value: %s\n", var, value);
+
+ return 1;
+}
+
+static void die_usage(int argc, char **argv)
+{
+ fprintf(stderr, "Usage: %s strbuf \n", argv[0]);
+ exit(1);
+}
+
+int main(int argc, char **argv)
+{
+ if (argc < 3)
+ die_usage(argc, argv);
+
+ if (!st
