Git-Mediawiki : cloning a set of pages

Git-Mediawiki : cloning a set of pages

[Claire Fousse]

Hi,

I work on the Git-Mediawiki project.
(http://lists-archives.org/git/746959-gate-between-git-and-mediawiki-remote-helpers.html).

I'm trying to establish a feature to clone and work on some pages of a
mediawiki instead of all of them.

The problem is not the feature in itself but the way you call it.
Just so you remember, here is the command  to clone the mediawiki :
git clone mediawiki::http://yourwiki.com

As it is now, git clone does not implement a way to define a set of pages.
The 2 solutions we think of are :
	* git clone mediawiki::http://yourwiki.com$$page1$$page2 ...
	Where $$ is a separator still to be determined. It should not be
something which could appear in the title of a page.
	It is a simple way to proceed but it becomes horrible when you want
to clone many pages.

	* write a git-mw-clone script which asks the user to enter a set of
pages  and may store this set of
	titles in the git config. This script should then call git-clone
which will call the remote-mediawiki functions.
	git-mw-clone would clone the entire wiki and git-mw-clone --pages
would ask the user to enter their set.
	The problem here is that a not git-like command is required.

What do you think about those solutions? Do you think of a better one?

Thank you very much.

--
Claire Fousse
Grenoble INP - Ensimag
claire.fousse@ensimag.imag.fr

Re: Git-Mediawiki : cloning a set of pages

[Jeff King]

On Wed, Jun 08, 2011 at 01:19:38PM +0200, Claire Fousse wrote:

> The problem is not the feature in itself but the way you call it.
> Just so you remember, here is the command  to clone the mediawiki :
> git clone mediawiki::http://yourwiki.com
> 
> As it is now, git clone does not implement a way to define a set of pages.
> The 2 solutions we think of are :
> 	* git clone mediawiki::http://yourwiki.com$$page1$$page2 ...
> 	Where $$ is a separator still to be determined. It should not be
> something which could appear in the title of a page.
> 	It is a simple way to proceed but it becomes horrible when you want
> to clone many pages.

Ick, yeah, that is kind of ugly. I think this is a general problem with
the clone and remote helper interface that we are going to need to
solve. It seems like clone should allow transport-specific options to
pass through the command line and make it to the transport.

Something like:

  git clone -c option=value mediawiki::http://...

where the "option" is up to the transport to interpret.  It could be
implemented as a set of in-core options that get passed to the remote
helper over the pipe. But that leaves the helper with probably having to
store the options for future runs.

Maybe it would be even simpler and more flexible to give clone a "-c"
flag that writes specific config variables in the newly-created
repository. Like:

  git clone -c mediawiki.page=page1 \
            -c mediawiki.page=page2 \
            http://...

Then the remote helper can just consult the git config. As a bonus, it
also lets you do things like:

  git clone -c core.ignorecase=true git://...

which is currently awkward (you either have to have set such variable in
your ~/.gitconfig, or you must use init+config+fetch to do a clone
manually.

Getting back to mediawiki, that gives us a slightly nicer syntax, but
we're still specifying each page on the command line (and now it's even
more verbose!). I would think two things could help:

  1. Some kind of globbing, like mediawiki.page="foo_*". The usefulness
     of this will depend on how well pages in the wiki are named,
     though.

  2. Have a config option to point to a file containing page entries,
     one per line.

> 	* write a git-mw-clone script which asks the user to enter a set
> 	of pages  and may store this set of titles in the git config.
> 	This script should then call git-clone which will call the
> 	remote-mediawiki functions.  git-mw-clone would clone the entire
> 	wiki and git-mw-clone --pages would ask the user to enter their
> 	set.  The problem here is that a not git-like command is
> 	required.

Yeah, I like this less because you lose a lot of the seamlessness of the
remote helper solution.

-Peff

Re: Git-Mediawiki : cloning a set of pages

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 17:19, Jeff King <peff@peff.net> wrote:
> Maybe it would be even simpler and more flexible to give clone a "-c"
> flag that writes specific config variables in the newly-created
> repository.

This makes a lot of sense. What about a use case like git-svn supports
currently, where you want to indicate "use the default
trunk/branches/tags setup"? Yes, you could do `git clone -c defaults
svn://example.com` and then git-remote-svn can set the relevant config
options itself. The only downside is that git-remote-svn then needs to
unset 'defaults' and set the appropriate values itself (to avoid
cluttering the config file). Thoughts?

-- 
Cheers,

Sverre Rabbelier

Re: Git-Mediawiki : cloning a set of pages

[Jeff King]

On Wed, Jun 08, 2011 at 07:04:27PM +0200, Sverre Rabbelier wrote:

> On Wed, Jun 8, 2011 at 17:19, Jeff King <peff@peff.net> wrote:
> > Maybe it would be even simpler and more flexible to give clone a "-c"
> > flag that writes specific config variables in the newly-created
> > repository.
> 
> This makes a lot of sense. What about a use case like git-svn supports
> currently, where you want to indicate "use the default
> trunk/branches/tags setup"? Yes, you could do `git clone -c defaults
> svn://example.com` and then git-remote-svn can set the relevant config
> options itself. The only downside is that git-remote-svn then needs to
> unset 'defaults' and set the appropriate values itself (to avoid
> cluttering the config file). Thoughts?

Yeah, I'm not sure. There is a similar case in the mediawiki helper
itself, too. If I say:

  git clone -c mediawiki.pagesfiles /path/to/pages mediawiki::...

Should that add config that looks at /path/to/pages every time I fetch?
Should it parse the pages file and load it into the config file as a set
of config options? Should it copy the pages file into the .git directory
and rewrite the config option to point to the local version?

So I think that while setting config options may be useful for some
things, the right model for other things is more like a command-line
option. Maybe we need to offer both forms.

-Peff

Re: Git-Mediawiki : cloning a set of pages

[Jakub Narebski]

Claire Fousse <claire.fousse@ensimag.imag.fr> writes:

> The problem is not the feature in itself but the way you call it.
> Just so you remember, here is the command  to clone the mediawiki :
> git clone mediawiki::http://yourwiki.com
> 
> As it is now, git clone does not implement a way to define a set of pages.
[...]

Well, what you need to do is to implement API for partial _clone_ (we
have some SPI for partial checkout, but that is slightly different
beast).

Currently we have --depth=<n> to limit depth of history when cloning,
and "git remote add -t <branch>" (repeated if necessary) to consider
only a subset of branches, though unfortunately not in "git clone"
yet.

Not what you wanted to hear, I guess... :-(
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: Git-Mediawiki : cloning a set of pages

[Claire Fousse]

>> The problem is not the feature in itself but the way you call it.
>> Just so you remember, here is the command  to clone the mediawiki :
>> git clone mediawiki::http://yourwiki.com
>>
>> As it is now, git clone does not implement a way to define a set of pages.
> [...]
>
> Well, what you need to do is to implement API for partial _clone_ (we
> have some SPI for partial checkout, but that is slightly different
> beast).
>
> Currently we have --depth=<n> to limit depth of history when cloning,
> and "git remote add -t <branch>" (repeated if necessary) to consider
> only a subset of branches, though unfortunately not in "git clone"
> yet.
>
> Not what you wanted to hear, I guess... :-(


Yes, not really what I wanted to hear, but I had the feeling it would
go this way =).

Our school project ends tomorrow, so starting tomorrow we will have less time
to work on this.

We chose to implement the first solution with ##:
git clone mediawiki::http://yourwiki.com##page1##page2..
This way that feature can be used for now, and It will be really
simple to change the
command later.

-- 
Claire Fousse
Grenoble INP - Ensimag
2A Télécommunication
claire.fousse@ensimag.imag.fr

Re: Git-Mediawiki : cloning a set of pages

[Jeff King]

On Wed, Jun 08, 2011 at 11:19:40AM -0400, Jeff King wrote:

> Maybe it would be even simpler and more flexible to give clone a "-c"
> flag that writes specific config variables in the newly-created
> repository. Like:
> 
>   git clone -c mediawiki.page=page1 \
>             -c mediawiki.page=page2 \
>             http://...
> 
> Then the remote helper can just consult the git config. As a bonus, it
> also lets you do things like:
> 
>   git clone -c core.ignorecase=true git://...
> 
> which is currently awkward (you either have to have set such variable in
> your ~/.gitconfig, or you must use init+config+fetch to do a clone
> manually.

This turned out to be a very tiny amount of code, but I found a ton
of other bugs while working on it. :)

So the patch series is long, but the important bits are at the end. I
factored the code from the existing "git -c", so most of the bugfixes
are there.

  [01/10]: strbuf_split: add a max parameter
  [02/10]: fix "git -c" parsing of values with equals signs

These two are the first bugfix.

  [03/10]: config: die on error in command-line config

Another bugfix.

  [04/10]: config: avoid segfault when parsing command-line config

Another bugfix.

  [05/10]: strbuf: allow strbuf_split to work on non-strbufs
  [06/10]: config: use strbuf_split_str instead of a temporary strbuf

Plugging a memory leak.

  [07/10]: parse-options: add OPT_STRING_LIST helper
  [08/10]: remote: use new OPT_STRING_LIST
  [09/10]: config: make git_config_parse_parameter a public function

These are refactoring for 10/10.

  [10/10]: clone: accept config options on the command line

And this is the actual patch.

-Peff

[PATCH 01/10] strbuf_split: add a max parameter

[Jeff King]

Sometimes when splitting, you only want a limited number of
fields, and for the final field to contain "everything
else", even if it includes the delimiter.

This patch introduces strbuf_split_max, which provides a
"max number of fields" parameter; it behaves similarly to
perl's "split" with a 3rd field.

The existing 2-argument form of strbuf_split is retained for
compatibility and ease-of-use.

Signed-off-by: Jeff King <peff@peff.net>
---
I am tempted to just call this new one strbuf_split and update all
callers. There aren't that many.

 strbuf.c |    7 +++++--
 strbuf.h |    7 ++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 09c43ae..64f6c1e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -103,7 +103,7 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
-struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
+struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
 {
 	int alloc = 2, pos = 0;
 	char *n, *p;
@@ -114,7 +114,10 @@ struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
 	p = n = sb->buf;
 	while (n < sb->buf + sb->len) {
 		int len;
-		n = memchr(n, delim, sb->len - (n - sb->buf));
+		if (max <= 0 || pos + 1 < max)
+			n = memchr(n, delim, sb->len - (n - sb->buf));
+		else
+			n = NULL;
 		if (pos + 1 >= alloc) {
 			alloc = alloc * 2;
 			ret = xrealloc(ret, sizeof(struct strbuf *) * alloc);
diff --git a/strbuf.h b/strbuf.h
index 9e6d9fa..4cf1dcd 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -44,7 +44,12 @@ extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
-extern struct strbuf **strbuf_split(const struct strbuf *, int delim);
+extern struct strbuf **strbuf_split_max(const struct strbuf *,
+					int delim, int max);
+static inline struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
+{
+	return strbuf_split_max(sb, delim, 0);
+}
 extern void strbuf_list_free(struct strbuf **);
 
 /*----- add data in your buffer -----*/
-- 
1.7.6.rc1.36.g91167

[PATCH 02/10] fix "git -c" parsing of values with equals signs

[Jeff King]

If you do something like:

  git -c core.foo="value with = in it" ...

we would split your option on "=" into three fields and
throw away the third one. With this patch we correctly take
everything after the first "=" as the value (keys cannot
have an equals sign in them, so the parsing is unambiguous).

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c               |    2 +-
 t/t1300-repo-config.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index e0b3b80..aa5eb78 100644
--- a/config.c
+++ b/config.c
@@ -45,7 +45,7 @@ static int git_config_parse_parameter(const char *text,
 	struct strbuf tmp = STRBUF_INIT;
 	struct strbuf **pair;
 	strbuf_addstr(&tmp, text);
-	pair = strbuf_split(&tmp, '=');
+	pair = strbuf_split_max(&tmp, '=', 2);
 	if (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=')
 		strbuf_setlen(pair[0], pair[0]->len - 1);
 	strbuf_trim(pair[0]);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3db5626..ca5058e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -904,4 +904,10 @@ test_expect_success 'git -c works with aliases of builtins' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git -c does not split values on equals' '
+	echo "value with = in it" >expect &&
+	git -c core.foo="value with = in it" config core.foo >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.6.rc1.36.g91167

[PATCH 03/10] config: die on error in command-line config

[Jeff King]

The error handling for git_config is somewhat confusing. We
collect errors from running git_config_from_file on the
various config files and carefully pass them back up. But
the two odd things are:

  1. We actually die on most errors in git_config_from_file.
     In fact, the only error we actually pass back up is if
     fopen() fails on the file.

  2. Most callers of git_config do not check the error
     return at all, but will continue if git_config reports
     an error.

When the code for "git -c core.foo=bar" was added, it
dutifully passed errors up the call stack, only for them to
be eventually ignored. This makes it inconsistent with the
file-parsing code, which will die when it sees malformed
config. And it's somewhat unsafe, because it means an error
in parsing a typo like:

  git -c clean.requireforce=ture clean

will continue the command, ignoring the config the user
tried to give.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to just make git_config call die() on error
instead of returning the error code that is ignored everywhere. But I
wasn't sure if anybody was relying on the "fopen failure is silently
ignored" behavior.

 config.c               |    2 +-
 t/t1300-repo-config.sh |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index aa5eb78..ebd404a 100644
--- a/config.c
+++ b/config.c
@@ -856,7 +856,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	switch (git_config_from_parameters(fn, data)) {
 	case -1: /* error */
-		ret--;
+		die("unable to parse command-line config");
 		break;
 	case 0: /* found nothing */
 		break;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index ca5058e..584e956 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -910,4 +910,12 @@ test_expect_success 'git -c does not split values on equals' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git -c dies on bogus config' '
+	test_must_fail git -c core.bare=foo rev-parse
+'
+
+test_expect_success 'git -c complains about empty key' '
+	test_must_fail git -c "=foo" rev-parse
+'
+
 test_done
-- 
1.7.6.rc1.36.g91167

[PATCH 04/10] config: avoid segfault when parsing command-line config

[Jeff King]

We already check for an empty key on the left side of an
equals, but we would segfault if there was no content at
all.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c               |    2 ++
 t/t1300-repo-config.sh |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/config.c b/config.c
index ebd404a..2517b62 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,8 @@ static int git_config_parse_parameter(const char *text,
 	struct strbuf **pair;
 	strbuf_addstr(&tmp, text);
 	pair = strbuf_split_max(&tmp, '=', 2);
+	if (!pair[0])
+		return error("bogus config parameter: %s", text);
 	if (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=')
 		strbuf_setlen(pair[0], pair[0]->len - 1);
 	strbuf_trim(pair[0]);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 584e956..3e140c1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -918,4 +918,8 @@ test_expect_success 'git -c complains about empty key' '
 	test_must_fail git -c "=foo" rev-parse
 '
 
+test_expect_success 'git -c complains about empty key and value' '
+	test_must_fail git -c "" rev-parse
+'
+
 test_done
-- 
1.7.6.rc1.36.g91167

[PATCH 05/10] strbuf: allow strbuf_split to work on non-strbufs

[Jeff King]

The strbuf_split function takes a strbuf as input, and
outputs a list of strbufs. However, there is no reason that
the input has to be a strbuf, and not an arbitrary buffer.

This patch adds strbuf_split_buf for a length-delimited
buffer, and strbuf_split_str for NUL-terminated strings.

Signed-off-by: Jeff King <peff@peff.net>
---
This makes the naming convention slightly different than "add", which
uses:

  add - add data by pointer and length
  addstr - add data by pointer to NUL-terminated string
  addbuf - add strbuf

but here we have:

  strbuf_split - split a strbuf
  strbuf_split_str - split a NUL-terminated string
  strbuf_split_buf - split pointer and length

It's that way because strbuf_split was introduced first with the current
semantics. I don't know if it is worth fixing the inconsistency.

 strbuf.c |   12 ++++++------
 strbuf.h |   12 +++++++++++-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 64f6c1e..1a7df12 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -103,19 +103,19 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
-struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
+struct strbuf **strbuf_split_buf(const char *str, size_t slen, int delim, int max)
 {
 	int alloc = 2, pos = 0;
-	char *n, *p;
+	const char *n, *p;
 	struct strbuf **ret;
 	struct strbuf *t;
 
 	ret = xcalloc(alloc, sizeof(struct strbuf *));
-	p = n = sb->buf;
-	while (n < sb->buf + sb->len) {
+	p = n = str;
+	while (n < str + slen) {
 		int len;
 		if (max <= 0 || pos + 1 < max)
-			n = memchr(n, delim, sb->len - (n - sb->buf));
+			n = memchr(n, delim, slen - (n - str));
 		else
 			n = NULL;
 		if (pos + 1 >= alloc) {
@@ -123,7 +123,7 @@ struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
 			ret = xrealloc(ret, sizeof(struct strbuf *) * alloc);
 		}
 		if (!n)
-			n = sb->buf + sb->len - 1;
+			n = str + slen - 1;
 		len = n - p + 1;
 		t = xmalloc(sizeof(struct strbuf));
 		strbuf_init(t, len);
diff --git a/strbuf.h b/strbuf.h
index 4cf1dcd..46a33f8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -44,8 +44,18 @@ extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
-extern struct strbuf **strbuf_split_max(const struct strbuf *,
+extern struct strbuf **strbuf_split_buf(const char *, size_t,
 					int delim, int max);
+static inline struct strbuf **strbuf_split_str(const char *str,
+					       int delim, int max)
+{
+	return strbuf_split_buf(str, strlen(str), delim, max);
+}
+static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
+						int delim, int max)
+{
+	return strbuf_split_buf(sb->buf, sb->len, delim, max);
+}
 static inline struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
 {
 	return strbuf_split_max(sb, delim, 0);
-- 
1.7.6.rc1.36.g91167

[PATCH 06/10] config: use strbuf_split_str instead of a temporary strbuf

[Jeff King]

This saves an allocation and copy, and also fixes a minor
memory leak.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 2517b62..189b766 100644
--- a/config.c
+++ b/config.c
@@ -42,10 +42,8 @@ void git_config_push_parameter(const char *text)
 static int git_config_parse_parameter(const char *text,
 				      config_fn_t fn, void *data)
 {
-	struct strbuf tmp = STRBUF_INIT;
 	struct strbuf **pair;
-	strbuf_addstr(&tmp, text);
-	pair = strbuf_split_max(&tmp, '=', 2);
+	pair = strbuf_split_str(text, '=', 2);
 	if (!pair[0])
 		return error("bogus config parameter: %s", text);
 	if (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=')
-- 
1.7.6.rc1.36.g91167

[PATCH 07/10] parse-options: add OPT_STRING_LIST helper

[Jeff King]

This just adds repeated invocations of an option to a list
of strings. Using the "--no-<var>" form will reset the list
to empty.

Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options.c          |   17 +++++++++++++++++
 parse-options.h          |    4 ++++
 t/t0040-parse-options.sh |   17 +++++++++++++++++
 test-parse-options.c     |    6 ++++++
 4 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 73bd28a..879ea82 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
+#include "string-list.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -687,3 +688,19 @@ int parse_options_concat(struct option *dst, size_t dst_size, struct option *src
 	}
 	return -1;
 }
+
+int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
+{
+	struct string_list *v = opt->value;
+
+	if (unset) {
+		string_list_clear(v, 0);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	string_list_append(v, xstrdup(arg));
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index d1b12fe..05eb09b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -130,6 +130,9 @@ struct option {
 				      (h), PARSE_OPT_NOARG, NULL, (p) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), "n", (h) }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
+#define OPT_STRING_LIST(s, l, v, a, h) \
+				    { OPTION_CALLBACK, (s), (l), (v), (a), \
+				      (h), 0, &parse_opt_string_list }
 #define OPT_UYN(s, l, v, h)         { OPTION_CALLBACK, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, &parse_opt_tertiary }
 #define OPT_DATE(s, l, v, h) \
@@ -204,6 +207,7 @@ extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
+extern int parse_opt_string_list(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_BOOLEAN('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_BOOLEAN('q', "quiet",   (var), (h))
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae26614..007f39d 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -28,6 +28,7 @@ String options
     --st <st>             get another string (pervert ordering)
     -o <str>              get another string
     --default-string      set string to default
+    --list <str>          add str to list
 
 Magic arguments
     --quux                means --quux
@@ -337,4 +338,20 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
 	test_cmp expect output
 '
 
+cat >>expect <<'EOF'
+list: foo
+list: bar
+list: baz
+EOF
+test_expect_success '--list keeps list of strings' '
+	test-parse-options --list foo --list=bar --list=baz >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--no-list resets list' '
+	test-parse-options --list=other --list=irrelevant --list=options \
+		--no-list --list=foo --list=bar --list=baz >output &&
+	test_cmp expect output
+'
+
 test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 4e3710b..91a5701 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "parse-options.h"
+#include "string-list.h"
 
 static int boolean = 0;
 static int integer = 0;
@@ -9,6 +10,7 @@ static int verbose = 0, dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
+static struct string_list list;
 
 static int length_callback(const struct option *opt, const char *arg, int unset)
 {
@@ -54,6 +56,7 @@ int main(int argc, const char **argv)
 		OPT_STRING('o', NULL, &string, "str", "get another string"),
 		OPT_SET_PTR(0, "default-string", &string,
 			"set string to default", (unsigned long)"default"),
+		OPT_STRING_LIST(0, "list", &list, "str", "add str to list"),
 		OPT_GROUP("Magic arguments"),
 		OPT_ARGUMENT("quux", "means --quux"),
 		OPT_NUMBER_CALLBACK(&integer, "set integer to NUM",
@@ -85,6 +88,9 @@ int main(int argc, const char **argv)
 	printf("dry run: %s\n", dry_run ? "yes" : "no");
 	printf("file: %s\n", file ? file : "(not set)");
 
+	for (i = 0; i < list.nr; i++)
+		printf("list: %s\n", list.items[i].string);
+
 	for (i = 0; i < argc; i++)
 		printf("arg %02d: %s\n", i, argv[i]);
 
-- 
1.7.6.rc1.36.g91167

[PATCH 08/10] remote: use new OPT_STRING_LIST

[Jeff King]

This saves us having our own callback function.

Signed-off-by: Jeff King <peff@peff.net>
---
This isn't technically related to the rest of the series, but is a
cleanup we can do because of the previous commit.

 builtin/remote.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 9ff1cac..05b1f5b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -88,16 +88,6 @@ static inline int postfixcmp(const char *string, const char *postfix)
 	return strcmp(string + len1 - len2, postfix);
 }
 
-static int opt_parse_track(const struct option *opt, const char *arg, int not)
-{
-	struct string_list *list = opt->value;
-	if (not)
-		string_list_clear(list, 0);
-	else
-		string_list_append(list, arg);
-	return 0;
-}
-
 static int fetch_remote(const char *name)
 {
 	const char *argv[] = { "fetch", name, NULL, NULL };
@@ -176,8 +166,8 @@ static int add(int argc, const char **argv)
 			    TAGS_SET),
 		OPT_SET_INT(0, NULL, &fetch_tags,
 			    "or do not fetch any tag at all (--no-tags)", TAGS_UNSET),
-		OPT_CALLBACK('t', "track", &track, "branch",
-			"branch(es) to track", opt_parse_track),
+		OPT_STRING_LIST('t', "track", &track, "branch",
+				"branch(es) to track"),
 		OPT_STRING('m', "master", &master, "branch", "master branch"),
 		{ OPTION_CALLBACK, 0, "mirror", &mirror, "push|fetch",
 			"set up remote as a mirror to push to or fetch from",
-- 
1.7.6.rc1.36.g91167

[PATCH 09/10] config: make git_config_parse_parameter a public function

[Jeff King]

We use this internally to parse "git -c core.foo=bar", but
the general format of "key=value" is useful for other
places.

Signed-off-by: Jeff King <peff@peff.net>
---
This could perhaps have a better name than "parameter". I dunno.

 cache.h  |    2 ++
 config.c |    4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index e11cf6a..933be72 100644
--- a/cache.h
+++ b/cache.h
@@ -1063,6 +1063,8 @@ extern int config_error_nonbool(const char *);
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
+extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+
 extern const char *config_exclusive_filename;
 
 #define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index 189b766..25d998c 100644
--- a/config.c
+++ b/config.c
@@ -39,8 +39,8 @@ void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
-static int git_config_parse_parameter(const char *text,
-				      config_fn_t fn, void *data)
+int git_config_parse_parameter(const char *text,
+			       config_fn_t fn, void *data)
 {
 	struct strbuf **pair;
 	pair = strbuf_split_str(text, '=', 2);
-- 
1.7.6.rc1.36.g91167

[PATCH 10/10] clone: accept config options on the command line

[Jeff King]

Clone does all of init, "remote add", fetch, and checkout
without giving the user a chance to intervene and set any
configuration. This patch allows you to set config options
in the newly created repository after the clone, but before
we do any other operations.

In many cases, this is a minor convenience over something
like:

  git clone git://...
  git config core.whatever true

But in some cases, it can bring extra efficiency by changing
how the fetch or checkout work. For example, setting
line-ending config before the checkout avoids having to
re-checkout all of the contents with the correct line
endings.

It also provides a mechanism for passing information to remote
helpers during a clone; the helpers may read the git config
to influence how they operate.

Signed-off-by: Jeff King <peff@peff.net>
---
Yay, the actual patch. Even if we don't end up using this for transport
helpers, I think it's a sane thing to allow (e.g., see the final test
below).

 Documentation/git-clone.txt |    9 +++++++++
 builtin/clone.c             |   21 ++++++++++++++++++++-
 t/t5707-clone-config.sh     |   40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletions(-)
 create mode 100755 t/t5707-clone-config.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index b093e45..3e24064 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -159,6 +159,15 @@ objects from the source repository into a pack in the cloned repository.
 	Specify the directory from which templates will be used;
 	(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
 
+--config <key>=<value>::
+-c <key>=<value>::
+	Set a configuration variable in the newly-cloned repository. The
+	key is in the same as expected by linkgit:git-config[1] (e.g.,
+	`core.eol=true`). If multiple values are given for the same key,
+	each value will be written to the config file. This makes it
+	safe, for example, to add additional fetch refspecs to the
+	origin remote.
+
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
 	specified number of revisions.  A shallow repository has a
diff --git a/builtin/clone.c b/builtin/clone.c
index f579794..a15784a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -46,6 +46,7 @@ static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
 static int option_progress;
+static struct string_list option_config;
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
@@ -83,7 +84,8 @@ static struct option builtin_clone_options[] = {
 		    "create a shallow clone of that depth"),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, "gitdir",
 		   "separate git dir from working tree"),
-
+	OPT_STRING_LIST('c', "config", &option_config, "key=value",
+			"set config inside the new repository"),
 	OPT_END()
 };
 
@@ -364,6 +366,22 @@ static void write_remote_refs(const struct ref *local_refs)
 	clear_extra_refs();
 }
 
+static int write_one_config(const char *key, const char *value, void *data)
+{
+	return git_config_set_multivar(key, value ? value : "true", "^$", 0);
+}
+
+static void write_config(struct string_list *config)
+{
+	int i;
+
+	for (i = 0; i < config->nr; i++) {
+		if (git_config_parse_parameter(config->items[i].string,
+					       write_one_config, NULL) < 0)
+			die("unable to write parameters to config file");
+	}
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -482,6 +500,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			printf(_("Cloning into %s...\n"), dir);
 	}
 	init_db(option_template, INIT_DB_QUIET);
+	write_config(&option_config);
 
 	/*
 	 * At this point, the config exists, so we do not need the
diff --git a/t/t5707-clone-config.sh b/t/t5707-clone-config.sh
new file mode 100755
index 0000000..27d730c
--- /dev/null
+++ b/t/t5707-clone-config.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='tests for git clone -c key=value'
+. ./test-lib.sh
+
+test_expect_success 'clone -c sets config in cloned repo' '
+	rm -rf child &&
+	git clone -c core.foo=bar . child &&
+	echo bar >expect &&
+	git --git-dir=child/.git config core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c can set multi-keys' '
+	rm -rf child &&
+	git clone -c core.foo=bar -c core.foo=baz . child &&
+	{ echo bar; echo baz; } >expect &&
+	git --git-dir=child/.git config --get-all core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c without a value is boolean true' '
+	rm -rf child &&
+	git clone -c core.foo . child &&
+	echo true >expect &&
+	git --git-dir=child/.git config --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c config is available during clone' '
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	rm -rf child &&
+	git clone -c core.autocrlf . child &&
+	printf "content\\r\\n" >expect &&
+	test_cmp expect child/file
+'
+
+test_done
-- 
1.7.6.rc1.36.g91167

Re: [PATCH 10/10] clone: accept config options on the command line

[Bert Wesarg]

On Thu, Jun 9, 2011 at 17:57, Jeff King <peff@peff.net> wrote:
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index b093e45..3e24064 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -159,6 +159,15 @@ objects from the source repository into a pack in the cloned repository.
>        Specify the directory from which templates will be used;
>        (See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
>
> +--config <key>=<value>::
> +-c <key>=<value>::
> +       Set a configuration variable in the newly-cloned repository. The

Shouldn't this be 'newly-created repository', because the
configuration variables are set before the actual cloning, but after
initializing the new repository.

Bert

> +       key is in the same as expected by linkgit:git-config[1] (e.g.,
> +       `core.eol=true`). If multiple values are given for the same key,
> +       each value will be written to the config file. This makes it
> +       safe, for example, to add additional fetch refspecs to the
> +       origin remote.
> +
>  --depth <depth>::
>        Create a 'shallow' clone with a history truncated to the
>        specified number of revisions.  A shallow repository has a

Re: [PATCH 10/10] clone: accept config options on the command line

[Jeff King]

On Thu, Jun 09, 2011 at 07:10:33PM +0200, Bert Wesarg wrote:

> On Thu, Jun 9, 2011 at 17:57, Jeff King <peff@peff.net> wrote:
> > diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> > index b093e45..3e24064 100644
> > --- a/Documentation/git-clone.txt
> > +++ b/Documentation/git-clone.txt
> > @@ -159,6 +159,15 @@ objects from the source repository into a pack in the cloned repository.
> >        Specify the directory from which templates will be used;
> >        (See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
> >
> > +--config <key>=<value>::
> > +-c <key>=<value>::
> > +       Set a configuration variable in the newly-cloned repository. The
> 
> Shouldn't this be 'newly-created repository', because the
> configuration variables are set before the actual cloning, but after
> initializing the new repository.

Yeah, I think your wording is better. It might also make sense to
explicitly say that the config takes effect during the fetch and
checkout phases of the clone (there's otherwise no point, of course, but
it never hurts to make the documentation clear).

-Peff

Re: [PATCH 10/10] clone: accept config options on the command line

[Jeff King]

On Thu, Jun 09, 2011 at 01:12:14PM -0400, Jeff King wrote:

> > > +-c <key>=<value>::
> > > +       Set a configuration variable in the newly-cloned repository. The
> > 
> > Shouldn't this be 'newly-created repository', because the
> > configuration variables are set before the actual cloning, but after
> > initializing the new repository.
> 
> Yeah, I think your wording is better. It might also make sense to
> explicitly say that the config takes effect during the fetch and
> checkout phases of the clone (there's otherwise no point, of course, but
> it never hurts to make the documentation clear).

There was also an unrelated typo in the newly-added documentation. So
here's a replacement 10/10 with the fixed documentation.

-- >8 --
Subject: [PATCH] clone: accept config options on the command line

Clone does all of init, "remote add", fetch, and checkout
without giving the user a chance to intervene and set any
configuration. This patch allows you to set config options
in the newly created repository after the clone, but before
we do any other operations.

In many cases, this is a minor convenience over something
like:

  git clone git://...
  git config core.whatever true

But in some cases, it can bring extra efficiency by changing
how the fetch or checkout work. For example, setting
line-ending config before the checkout avoids having to
re-checkout all of the contents with the correct line
endings.

It also provides a mechanism for passing information to remote
helpers during a clone; the helpers may read the git config
to influence how they operate.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-clone.txt |   11 +++++++++++
 builtin/clone.c             |   21 ++++++++++++++++++++-
 t/t5707-clone-config.sh     |   40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100755 t/t5707-clone-config.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index b093e45..4b8b26b 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -159,6 +159,17 @@ objects from the source repository into a pack in the cloned repository.
 	Specify the directory from which templates will be used;
 	(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
 
+--config <key>=<value>::
+-c <key>=<value>::
+	Set a configuration variable in the newly-created repository;
+	this takes effect immediately after the repository is
+	initialized, but before the remote history is fetched or any
+	files checked out.  The key is in the same format as expected by
+	linkgit:git-config[1] (e.g., `core.eol=true`). If multiple
+	values are given for the same key, each value will be written to
+	the config file. This makes it safe, for example, to add
+	additional fetch refspecs to the origin remote.
+
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
 	specified number of revisions.  A shallow repository has a
diff --git a/builtin/clone.c b/builtin/clone.c
index f579794..a15784a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -46,6 +46,7 @@ static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
 static int option_progress;
+static struct string_list option_config;
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
@@ -83,7 +84,8 @@ static struct option builtin_clone_options[] = {
 		    "create a shallow clone of that depth"),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, "gitdir",
 		   "separate git dir from working tree"),
-
+	OPT_STRING_LIST('c', "config", &option_config, "key=value",
+			"set config inside the new repository"),
 	OPT_END()
 };
 
@@ -364,6 +366,22 @@ static void write_remote_refs(const struct ref *local_refs)
 	clear_extra_refs();
 }
 
+static int write_one_config(const char *key, const char *value, void *data)
+{
+	return git_config_set_multivar(key, value ? value : "true", "^$", 0);
+}
+
+static void write_config(struct string_list *config)
+{
+	int i;
+
+	for (i = 0; i < config->nr; i++) {
+		if (git_config_parse_parameter(config->items[i].string,
+					       write_one_config, NULL) < 0)
+			die("unable to write parameters to config file");
+	}
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -482,6 +500,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			printf(_("Cloning into %s...\n"), dir);
 	}
 	init_db(option_template, INIT_DB_QUIET);
+	write_config(&option_config);
 
 	/*
 	 * At this point, the config exists, so we do not need the
diff --git a/t/t5707-clone-config.sh b/t/t5707-clone-config.sh
new file mode 100755
index 0000000..27d730c
--- /dev/null
+++ b/t/t5707-clone-config.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='tests for git clone -c key=value'
+. ./test-lib.sh
+
+test_expect_success 'clone -c sets config in cloned repo' '
+	rm -rf child &&
+	git clone -c core.foo=bar . child &&
+	echo bar >expect &&
+	git --git-dir=child/.git config core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c can set multi-keys' '
+	rm -rf child &&
+	git clone -c core.foo=bar -c core.foo=baz . child &&
+	{ echo bar; echo baz; } >expect &&
+	git --git-dir=child/.git config --get-all core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c without a value is boolean true' '
+	rm -rf child &&
+	git clone -c core.foo . child &&
+	echo true >expect &&
+	git --git-dir=child/.git config --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c config is available during clone' '
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	rm -rf child &&
+	git clone -c core.autocrlf . child &&
+	printf "content\\r\\n" >expect &&
+	test_cmp expect child/file
+'
+
+test_done
-- 
1.7.6.rc1.36.g91167

Re: [PATCH 10/10] clone: accept config options on the command line

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> Yay, the actual patch. Even if we don't end up using this for transport
> helpers, I think it's a sane thing to allow (e.g., see the final test
> below).

Ack.

I reviewed quickly the serie, which seems OK. Nice work, amazing serie
of patches to end up with this trivial one ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 01/10] strbuf_split: add a max parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am tempted to just call this new one strbuf_split and update all
> callers. There aren't that many.

Yes, that is indeed tempting, and because we have a new parameter the
compiler will catch any new callers that pop up in a mismerge so that
would be perfectly safe.

> -struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
> +struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
>  {
>  	int alloc = 2, pos = 0;
>  	char *n, *p;
> @@ -114,7 +114,10 @@ struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
>  	p = n = sb->buf;
>  	while (n < sb->buf + sb->len) {
>  		int len;
> -		n = memchr(n, delim, sb->len - (n - sb->buf));
> +		if (max <= 0 || pos + 1 < max)
> +			n = memchr(n, delim, sb->len - (n - sb->buf));
> +		else
> +			n = NULL;
>  		if (pos + 1 >= alloc) {
>  			alloc = alloc * 2;
>  			ret = xrealloc(ret, sizeof(struct strbuf *) * alloc);

Hmm, even when we know the value of max, we go exponential, and even do so
by hand without using ALLOC_GROW(). Somewhat sad.

Also do we currently rely on the bug that strbuf_split() returns (NULL,)
instead of ("", NULL) when given an empty string?  If not, perhaps...

 strbuf.c |   50 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 09c43ae..7a8ee3a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -103,31 +103,43 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
-struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
+struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
 {
-	int alloc = 2, pos = 0;
 	char *n, *p;
-	struct strbuf **ret;
+	struct strbuf **ret = NULL;
 	struct strbuf *t;
-
-	ret = xcalloc(alloc, sizeof(struct strbuf *));
-	p = n = sb->buf;
-	while (n < sb->buf + sb->len) {
-		int len;
-		n = memchr(n, delim, sb->len - (n - sb->buf));
-		if (pos + 1 >= alloc) {
-			alloc = alloc * 2;
-			ret = xrealloc(ret, sizeof(struct strbuf *) * alloc);
+	int pass, count = 0;
+
+	for (pass = 0; pass < 2; pass++) {
+		/* First pass counts, second pass allocates and fills */
+		if (pass)
+			ret = xcalloc(count + 1 + !count, sizeof(struct strbuf *));
+		count = 0;
+		n = sb->buf;
+		while (n < sb->buf + sb->len) {
+			p = n;
+			if (max <= 0 || count + 1 < max)
+				n = memchr(n, delim, sb->len - (n - sb->buf));
+			else
+				n = NULL;
+			if (!n)
+				n = sb->buf + sb->len - 1;
+
+			if (pass) {
+				int len = n - p + 1;
+				t = xmalloc(sizeof(struct strbuf));
+				strbuf_init(t, len);
+				strbuf_add(t, p, len);
+				ret[count] = t;
+			}
+			count++;
+			n++;
 		}
-		if (!n)
-			n = sb->buf + sb->len - 1;
-		len = n - p + 1;
+	}
+	if (!count) {
 		t = xmalloc(sizeof(struct strbuf));
-		strbuf_init(t, len);
-		strbuf_add(t, p, len);
-		ret[pos] = t;
-		ret[++pos] = NULL;
-		p = ++n;
+		strbuf_init(t, 0);
+		ret[0] = t;
 	}
 	return ret;
 }

Re: [PATCH 04/10] config: avoid segfault when parsing command-line config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We already check for an empty key on the left side of an
> equals, but we would segfault if there was no content at
> all.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  config.c               |    2 ++
>  t/t1300-repo-config.sh |    4 ++++
>  2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/config.c b/config.c
> index ebd404a..2517b62 100644
> --- a/config.c
> +++ b/config.c
> @@ -46,6 +46,8 @@ static int git_config_parse_parameter(const char *text,
>  	struct strbuf **pair;
>  	strbuf_addstr(&tmp, text);
>  	pair = strbuf_split_max(&tmp, '=', 2);
> +	if (!pair[0])
> +		return error("bogus config parameter: %s", text);

This feels wrong.

Asking strbuf_split() to split a string "foo" with "=" delimiter would
give you one element array ("foo", NULL), a string "fo" would give you
("fo", NULL), and a string "f" would give you ("f", NULL).  Shouldn't we
get ("", NULL) if we ask it to split ""?

Re: [PATCH 01/10] strbuf_split: add a max parameter

[Jeff King]

On Mon, Jun 13, 2011 at 10:30:07AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I am tempted to just call this new one strbuf_split and update all
> > callers. There aren't that many.
> 
> Yes, that is indeed tempting, and because we have a new parameter the
> compiler will catch any new callers that pop up in a mismerge so that
> would be perfectly safe.

Should we also change the naming later in the series to remain
consistent with strbuf_add. IOW, to end up at:

  struct strbuf **strbuf_split(const char *buf, int len, int delim, int max);
  struct strbuf **strbuf_split_str(const char *s, int delim, int max);
  struct strbuf **strbuf_split_buf(const struct strbuf *, int delim, int max);

(though I think consistency would also dictate "splitstr" and "splitbuf"
without the extra underscore. Personally I find it a bit unreadable).

> > -struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
> > +struct strbuf **strbuf_split_max(const struct strbuf *sb, int delim, int max)
> >  {
> >  	int alloc = 2, pos = 0;
> >  	char *n, *p;
> > @@ -114,7 +114,10 @@ struct strbuf **strbuf_split(const struct strbuf *sb, int delim)
> >  	p = n = sb->buf;
> >  	while (n < sb->buf + sb->len) {
> >  		int len;
> > -		n = memchr(n, delim, sb->len - (n - sb->buf));
> > +		if (max <= 0 || pos + 1 < max)
> > +			n = memchr(n, delim, sb->len - (n - sb->buf));
> > +		else
> > +			n = NULL;
> >  		if (pos + 1 >= alloc) {
> >  			alloc = alloc * 2;
> >  			ret = xrealloc(ret, sizeof(struct strbuf *) * alloc);
> 
> Hmm, even when we know the value of max, we go exponential, and even do so
> by hand without using ALLOC_GROW(). Somewhat sad.

Thanks for reminding me. I noticed it wasn't using ALLOC_GROW, but
decided not to change it because I wanted to introduce an optimization
later on not to grow beyond max. But then I forgot. :)

The optimization I was going to do was to simply allocate "max" slots at
the beginning (if it's defined). You know you can't grow beyond that,
and in most splits with a max, the caller is expecting all of them to be
filled.

But your two-pass patch below is also reasonable.

> Also do we currently rely on the bug that strbuf_split() returns (NULL,)
> instead of ("", NULL) when given an empty string?  If not, perhaps...

I assumed that behavior was not a bug (and even had to avoid a segfault
with it in a later series, as you saw). But thinking on it more, it
really is one; splitting even a single character without delimiter ends
up with a non-NULL portion, and I think the empty string should do the
same.

>  strbuf.c |   50 +++++++++++++++++++++++++++++++-------------------
>  1 files changed, 31 insertions(+), 19 deletions(-)

I think your patch looks reasonable. In theory doing two passes over a
very large buffer (e.g., splitting lines from a large commit message)
might be slightly less efficient, but I imagine it is drowned out in the
noise of malloc'ing strbufs.

> +	for (pass = 0; pass < 2; pass++) {
> +		/* First pass counts, second pass allocates and fills */

Maybe it is just me, but I tend not to like writing multi-pass stuff
like this as a for-loop, but instead to factor it into a function with
an "actually allocate" parameter. I find it makes the code much more
obvious.

> +	if (!count) {
>  		t = xmalloc(sizeof(struct strbuf));
> -		strbuf_init(t, len);
> -		strbuf_add(t, p, len);
> -		ret[pos] = t;
> -		ret[++pos] = NULL;
> -		p = ++n;
> +		strbuf_init(t, 0);
> +		ret[0] = t;
>  	}

I think my test in 4/10 (which avoids the segfault by checking
explicitly for NULL in the caller) should go with this part, and then
4/10 can go away.

-Peff

Re: [PATCH 04/10] config: avoid segfault when parsing command-line config

[Jeff King]

On Mon, Jun 13, 2011 at 10:30:22AM -0700, Junio C Hamano wrote:

> > +	if (!pair[0])
> > +		return error("bogus config parameter: %s", text);
> 
> This feels wrong.
> 
> Asking strbuf_split() to split a string "foo" with "=" delimiter would
> give you one element array ("foo", NULL), a string "fo" would give you
> ("fo", NULL), and a string "f" would give you ("f", NULL).  Shouldn't we
> get ("", NULL) if we ask it to split ""?

Yeah, I was making the assumption that strbuf_split was not bugging, and
coding to its output. But I think you are right, that it is simply
returning bogus output.

-Peff