Re: Re: [PATCH v2 4/4] teach config parsing to read from strbuf

2013-03-18 Thread Heiko Voigt 

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

2013-03-14 Thread Heiko Voigt 
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

2013-03-14 Thread Jeff King 
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(given_config_file ?
  given_config_file :

Re: [PATCH v2 4/4] teach config parsing to read from strbuf

2013-03-14 Thread Jeff King 
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)
die(not in a git directory);
+

Re: [PATCH v2 4/4] teach config parsing to read from strbuf

2013-03-12 Thread Heiko Voigt 
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 hvo...@hvoigt.net
 
 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

2013-03-12 Thread Jeff King 
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

[PATCH v2 4/4] teach config parsing to read from strbuf

2013-03-10 Thread Heiko Voigt 
This can be used to read configuration values directly from gits
database.

Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
 .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 
2actual.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 name\n, argv[0]);
+   exit(1);
+}
+
+int main(int argc, char **argv)
+{
+   if (argc  3)
+   die_usage(argc, argv);
+
+   if (!strcmp(argv[1], strbuf)) {
+