[PATCH 0/6] post-strbuf_getline cleanups

DORMANTno replies

11 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/6] post-strbuf_getline cleanups

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

As promised, here are the "how about this on top" patches that came out
of the discussion for the "strbuf_getline" series in:

  http://thread.gmane.org/gmane.comp.version-control.msysgit/21773/focus=284001

As I looked further at some of the option-parsing cleanups, I realized
that some of them are more than cleanups; we're actually fixing bizarre
behavior and segfaults.

  [1/6]: give "nbuf" strbuf a more meaningful name
  [2/6]: checkout-index: simplify "-z" option parsing
  [3/6]: checkout-index: handle "--no-prefix" option
  [4/6]: checkout-index: handle "--no-index" option
  [5/6]: checkout-index: disallow "--no-stage" option
  [6/6]: apply, ls-files: simplify "-z" parsing

-Peff

[PATCH 1/6] give "nbuf" strbuf a more meaningful name

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

It's a common pattern in our code to read paths from stdin,
separated either by newlines or NULs, and unquote as
necessary. In each of these five cases we use "nbuf" to
temporarily store the unquoted value. Let's give it the more
meaningful name "unquoted", which makes it easier to
understand the purpose of the variable.

While we're at it, let's also static-initialize all of our
strbufs. It's not wrong to call strbuf_init, but it
increases the cognitive load on the reader, who might wonder
"do we sometimes avoid initializing them?  why?".

Signed-off-by: Jeff King <redacted>
---
For those seeing this patch for the first time, I'll copy my
regrets from the earlier posting:

It's a shame that we can't just factor out this common
code, but I don't think it's quite long enough to merit
the boilerplate. The interesting part of each function
happens inside the loop. If C had lambdas, we could do
something like:

  foreach_path_from(stdin, nul_term_line) {
        /* now do something interesting with "buf"
           and some other local variables */
  }

but using function pointers for this kind of iteration is
pretty awful (define the two-line loop body as a separate
function, stuff any local variables into a struct and pass
it as "void *", etc). You can overcome that with macros
and make the above code work if you don't mind creating an
undebuggable mess. :)

 builtin/check-attr.c     | 13 ++++++-------
 builtin/check-ignore.c   | 13 ++++++-------
 builtin/checkout-index.c | 11 ++++++-----
 builtin/hash-object.c    | 11 ++++++-----
 builtin/update-index.c   | 11 ++++++-----
 5 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 087325e..53a5a18 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c

@@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt,
 static void check_attr_stdin_paths(const char *prefix, int cnt,
 	struct git_attr_check *check)
 {
-	struct strbuf buf, nbuf;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf unquoted = STRBUF_INIT;
 	strbuf_getline_fn getline_fn;
 
 	getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
-	strbuf_init(&buf, 0);
-	strbuf_init(&nbuf, 0);
 	while (getline_fn(&buf, stdin) != EOF) {
 		if (!nul_term_line && buf.buf[0] == '"') {
-			strbuf_reset(&nbuf);
-			if (unquote_c_style(&nbuf, buf.buf, NULL))
+			strbuf_reset(&unquoted);
+			if (unquote_c_style(&unquoted, buf.buf, NULL))
 				die("line is badly quoted");
-			strbuf_swap(&buf, &nbuf);
+			strbuf_swap(&buf, &unquoted);
 		}
 		check_attr(prefix, cnt, check, buf.buf);
 		maybe_flush_or_die(stdout, "attribute to stdout");
 	}
 	strbuf_release(&buf);
-	strbuf_release(&nbuf);
+	strbuf_release(&unquoted);
 }
 
 static NORETURN void error_with_usage(const char *msg)

diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 4f0b09e..1d73d3c 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c

@@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir,
 
 static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)
 {
-	struct strbuf buf, nbuf;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf unquoted = STRBUF_INIT;
 	char *pathspec[2] = { NULL, NULL };
 	strbuf_getline_fn getline_fn;
 	int num_ignored = 0;
 
 	getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
-	strbuf_init(&buf, 0);
-	strbuf_init(&nbuf, 0);
 	while (getline_fn(&buf, stdin) != EOF) {
 		if (!nul_term_line && buf.buf[0] == '"') {
-			strbuf_reset(&nbuf);
-			if (unquote_c_style(&nbuf, buf.buf, NULL))
+			strbuf_reset(&unquoted);
+			if (unquote_c_style(&unquoted, buf.buf, NULL))
 				die("line is badly quoted");
-			strbuf_swap(&buf, &nbuf);
+			strbuf_swap(&buf, &unquoted);
 		}
 		pathspec[0] = buf.buf;
 		num_ignored += check_ignore(dir, prefix,

@@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)
 		maybe_flush_or_die(stdout, "check-ignore to stdout");
 	}
 	strbuf_release(&buf);
-	strbuf_release(&nbuf);
+	strbuf_release(&unquoted);
 	return num_ignored;
 }

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index ed888a5..d8d7bd3 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_from_stdin) {
-		struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
+		struct strbuf unquoted = STRBUF_INIT;
 		strbuf_getline_fn getline_fn;
 
 		if (all)

@@ -261,16 +262,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		while (getline_fn(&buf, stdin) != EOF) {
 			char *p;
 			if (!nul_term_line && buf.buf[0] == '"') {
-				strbuf_reset(&nbuf);
-				if (unquote_c_style(&nbuf, buf.buf, NULL))
+				strbuf_reset(&unquoted);
+				if (unquote_c_style(&unquoted, buf.buf, NULL))
 					die("line is badly quoted");
-				strbuf_swap(&buf, &nbuf);
+				strbuf_swap(&buf, &unquoted);
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			checkout_file(p, prefix);
 			free(p);
 		}
-		strbuf_release(&nbuf);
+		strbuf_release(&unquoted);
 		strbuf_release(&buf);
 	}

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 3bc5ec1..d3cb4e5 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c

@@ -58,20 +58,21 @@ static void hash_object(const char *path, const char *type, const char *vpath,
 static void hash_stdin_paths(const char *type, int no_filters, unsigned flags,
 			     int literally)
 {
-	struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf unquoted = STRBUF_INIT;
 
 	while (strbuf_getline_lf(&buf, stdin) != EOF) {
 		if (buf.buf[0] == '"') {
-			strbuf_reset(&nbuf);
-			if (unquote_c_style(&nbuf, buf.buf, NULL))
+			strbuf_reset(&unquoted);
+			if (unquote_c_style(&unquoted, buf.buf, NULL))
 				die("line is badly quoted");
-			strbuf_swap(&buf, &nbuf);
+			strbuf_swap(&buf, &unquoted);
 		}
 		hash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags,
 			    literally);
 	}
 	strbuf_release(&buf);
-	strbuf_release(&nbuf);
+	strbuf_release(&unquoted);
 }
 
 int cmd_hash_object(int argc, const char **argv, const char *prefix)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7c5c143..f052faf 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c

@@ -1075,16 +1075,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_from_stdin) {
-		struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
+		struct strbuf buf = STRBUF_INIT;
+		struct strbuf unquoted = STRBUF_INIT;
 
 		setup_work_tree();
 		while (getline_fn(&buf, stdin) != EOF) {
 			char *p;
 			if (!nul_term_line && buf.buf[0] == '"') {
-				strbuf_reset(&nbuf);
-				if (unquote_c_style(&nbuf, buf.buf, NULL))
+				strbuf_reset(&unquoted);
+				if (unquote_c_style(&unquoted, buf.buf, NULL))
 					die("line is badly quoted");
-				strbuf_swap(&buf, &nbuf);
+				strbuf_swap(&buf, &unquoted);
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			update_one(p);

@@ -1092,7 +1093,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 				chmod_path(set_executable_bit, p);
 			free(p);
 		}
-		strbuf_release(&nbuf);
+		strbuf_release(&unquoted);
 		strbuf_release(&buf);
 	}

-- 
2.7.0.489.g6faad84

[PATCH 2/6] checkout-index: simplify "-z" option parsing

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

Now that we act as a simple bool, there's no need to use a
custom callback.

Signed-off-by: Jeff King <redacted>
---
 builtin/checkout-index.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index d8d7bd3..3b913d1 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_z(const struct option *opt,
-			  const char *arg, int unset)
-{
-	nul_term_line = !unset;
-	return 0;
-}
-
 static int option_parse_prefix(const struct option *opt,
 			       const char *arg, int unset)
 {

@@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
 			N_("update stat information in the index file"),
 			PARSE_OPT_NOARG, option_parse_u },
-		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-			N_("paths are separated with NUL character"),
-			PARSE_OPT_NOARG, option_parse_z },
+		OPT_BOOL('z', NULL, &nul_term_line,
+			N_("paths are separated with NUL character")),
 		OPT_BOOL(0, "stdin", &read_from_stdin,
 			N_("read list of paths from the standard input")),
 		OPT_BOOL(0, "temp", &to_tempfile,

-- 
2.7.0.489.g6faad84

[PATCH 3/6] checkout-index: handle "--no-prefix" option

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

We use a custom callback to parse "--prefix", but it does
not handle the "unset" case. As a result, passing
"--no-prefix" will cause a segfault.

We can fix this by switching it to an OPT_STRING, which
makes "--no-prefix" counteract a previous "--prefix". Note
that this assigns NULL, so we bump our default-case
initialization to lower in the main function.

Signed-off-by: Jeff King <redacted>
---
 builtin/checkout-index.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 3b913d1..43bedde 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -142,14 +142,6 @@ static int option_parse_u(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_prefix(const struct option *opt,
-			       const char *arg, int unset)
-{
-	state.base_dir = arg;
-	state.base_dir_len = strlen(arg);
-	return 0;
-}
-
 static int option_parse_stage(const struct option *opt,
 			      const char *arg, int unset)
 {

@@ -191,9 +183,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			N_("read list of paths from the standard input")),
 		OPT_BOOL(0, "temp", &to_tempfile,
 			N_("write the content to temporary files")),
-		OPT_CALLBACK(0, "prefix", NULL, N_("string"),
-			N_("when creating files, prepend <string>"),
-			option_parse_prefix),
+		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
+			N_("when creating files, prepend <string>")),
 		OPT_CALLBACK(0, "stage", NULL, NULL,
 			N_("copy out the files from named stage"),
 			option_parse_stage),

@@ -204,7 +195,6 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_checkout_index_usage,
 				   builtin_checkout_index_options);
 	git_config(git_default_config, NULL);
-	state.base_dir = "";
 	prefix_length = prefix ? strlen(prefix) : 0;
 
 	if (read_cache() < 0) {

@@ -217,6 +207,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	state.quiet = quiet;
 	state.not_new = not_new;
 
+	if (!state.base_dir)
+		state.base_dir = "";
+	state.base_dir_len = strlen(state.base_dir);
+
 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
 		 * want to update cache.

-- 
2.7.0.489.g6faad84

[PATCH 4/6] checkout-index: handle "--no-index" option

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

The parsing of "--index" is done in a callback, but it does
not handle an "unset" option. We don't necessarily expect
anyone to use this, but the current behavior is to treat it
exactly like "--index", which would probably be surprising.

Instead, let's just turn it into an OPT_BOOL, and handle it
after we're done parsing. This makes "--no-index" just work
(it cancels a previous "--index").

As a bonus, this makes the logic easier to follow. The old
code opened the index during the option parsing, leaving the
reader to wonder if there was some timing issue (there
isn't; none of the other options care that we've opened it).
And then if we found that "--prefix" had been given, we had
to rollback the index. Now we can simply avoid opening it in
the first place.

Note that it might make more sense for checkout-index to
complain when "--index --prefix=foo" is given (rather than
silently ignoring "--index"), but since it has been that way
since 415e96c ([PATCH] Implement git-checkout-cache -u to
update stat information in the cache., 2005-05-15), it's
safer to leave it as-is.

Signed-off-by: Jeff King <redacted>
---
I also reformatted the comment that violated our style
guidelines, but I am not sure if it is all that helpful. It
seems we also cancel "--index" with "--temp" or
"--stage=all", but I do not know why. I left the content
as-is, but if somebody knows enough to elaborate, it might
be worth doing.

 builtin/checkout-index.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 43bedde..f8179a7 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] = {
 
 static struct lock_file lock_file;
 
-static int option_parse_u(const struct option *opt,
-			      const char *arg, int unset)
-{
-	int *newfd = opt->value;
-
-	state.refresh_cache = 1;
-	state.istate = &the_index;
-	if (*newfd < 0)
-		*newfd = hold_locked_index(&lock_file, 1);
-	return 0;
-}
-
 static int option_parse_stage(const struct option *opt,
 			      const char *arg, int unset)
 {

@@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int read_from_stdin = 0;
 	int prefix_length;
 	int force = 0, quiet = 0, not_new = 0;
+	int index_opt = 0;
 	struct option builtin_checkout_index_options[] = {
 		OPT_BOOL('a', "all", &all,
 			N_("check out all files in the index")),

@@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			N_("no warning for existing files and files not in index")),
 		OPT_BOOL('n', "no-create", &not_new,
 			N_("don't checkout new files")),
-		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
-			N_("update stat information in the index file"),
-			PARSE_OPT_NOARG, option_parse_u },
+		OPT_BOOL('u', "index", &index_opt,
+			 N_("update stat information in the index file")),
 		OPT_BOOL('z', NULL, &nul_term_line,
 			N_("paths are separated with NUL character")),
 		OPT_BOOL(0, "stdin", &read_from_stdin,

@@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		state.base_dir = "";
 	state.base_dir_len = strlen(state.base_dir);
 
-	if (state.base_dir_len || to_tempfile) {
-		/* when --prefix is specified we do not
-		 * want to update cache.
-		 */
-		if (state.refresh_cache) {
-			rollback_lock_file(&lock_file);
-			newfd = -1;
-		}
-		state.refresh_cache = 0;
+	/*
+	 * when --prefix is specified we do not want to update cache.
+	 */
+	if (index_opt && !state.base_dir_len && !to_tempfile) {
+		state.refresh_cache = 1;
+		state.istate = &the_index;
+		newfd = hold_locked_index(&lock_file, 1);
 	}
 
 	/* Check out named files first */

-- 
2.7.0.489.g6faad84

[PATCH 5/6] checkout-index: disallow "--no-stage" option

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

We do not really expect people to use "--no-stage", but if
they do, git currently segfaults. We could instead have it
undo the effects of a previous "--stage", but this gets
tricky around the "to_tempfile" flag. We cannot simply reset
it to 0, because we don't know if it was set by a previous
"--stage=all" or an explicit "--temp" option.

We could solve this by setting a flag and resolving
to_tempfile later, but it's not worth the effort. Nobody
actually wants to use "--no-stage"; we are just trying to
fix a potential segfault here.

While we're in the area adding a translated string, let's
mark the other possible error message in this function as
translatable.

Signed-off-by: Jeff King <redacted>
---
 builtin/checkout-index.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..7a9b561 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -133,6 +133,8 @@ static struct lock_file lock_file;
 static int option_parse_stage(const struct option *opt,
 			      const char *arg, int unset)
 {
+	if (unset)
+		return error(_("--stage cannot be negated"));
 	if (!strcmp(arg, "all")) {
 		to_tempfile = 1;
 		checkout_stage = CHECKOUT_ALL;

@@ -141,7 +143,7 @@ static int option_parse_stage(const struct option *opt,
 		if ('1' <= ch && ch <= '3')
 			checkout_stage = arg[0] - '0';
 		else
-			die("stage should be between 1 and 3 or all");
+			die(_("stage should be between 1 and 3 or all"));
 	}
 	return 0;
 }

-- 
2.7.0.489.g6faad84

[PATCH 6/6] apply, ls-files: simplify "-z" parsing

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

As a short option, we cannot handle negation. Thus a
callback handling "unset" is overkill, and we can just use
OPT_SET_INT instead to handle setting the option.

Signed-off-by: Jeff King <redacted>
---
I left this one for last, because it's the most questionable. Unlike the
previous "-z" case, we're setting the actual character, so the logic is
inverted: turning on the option sets it to 0, and turning it off restore
'\n'.

This means OPT_SET_INT would do the wrong thing for the "unset" case, as
it would put a "0" into the option. You can't trigger that now, but if
somebody were to add a long option (e.g., "--nul"), then "--no-nul"
would do the wrong thing.

I'm on the fence on whether the simplification is worth it, or if we
should leave the callbacks as future-proofing.

 builtin/apply.c    | 15 ++-------------
 builtin/ls-files.c | 13 ++-----------
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index deb1364..565f3fd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_z(const struct option *opt,
-			  const char *arg, int unset)
-{
-	if (unset)
-		line_termination = '\n';
-	else
-		line_termination = 0;
-	return 0;
-}
-
 static int option_parse_space_change(const struct option *opt,
 			  const char *arg, int unset)
 {

@@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			 N_( "attempt three-way merge if a patch does not apply")),
 		OPT_FILENAME(0, "build-fake-ancestor", &fake_ancestor,
 			N_("build a temporary index based on embedded index information")),
-		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-			N_("paths are separated with NUL character"),
-			PARSE_OPT_NOARG, option_parse_z },
+		OPT_SET_INT('z', NULL, &line_termination,
+			N_("paths are separated with NUL character"), '\0'),
 		OPT_INTEGER('C', NULL, &p_context,
 				N_("ensure at least <n> lines of context match")),
 		{ OPTION_CALLBACK, 0, "whitespace", &whitespace_option, N_("action"),

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b6a7cb0..59bad9b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = {
 	NULL
 };
 
-static int option_parse_z(const struct option *opt,
-			  const char *arg, int unset)
-{
-	line_terminator = unset ? '\n' : '\0';
-
-	return 0;
-}
-
 static int option_parse_exclude(const struct option *opt,
 				const char *arg, int unset)
 {

@@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	struct exclude_list *el;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct option builtin_ls_files_options[] = {
-		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
-			N_("paths are separated with NUL character"),
-			PARSE_OPT_NOARG, option_parse_z },
+		OPT_SET_INT('z', NULL, &line_terminator,
+			N_("paths are separated with NUL character"), '\0'),
 		OPT_BOOL('t', NULL, &show_tag,
 			N_("identify the file status with tags")),
 		OPT_BOOL('v', NULL, &show_valid_bit,

-- 
2.7.0.489.g6faad84

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:08:02

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

quoted

It's a shame that we can't just factor out this common
code, but I don't think it's quite long enough to merit
the boilerplate. The interesting part of each function
happens inside the loop. If C had lambdas, we could do
something like:

  foreach_path_from(stdin, nul_term_line) {
        /* now do something interesting with "buf"
           and some other local variables */
  }

Technically, we do not have to do lambdas for that paradigm, we could
introduce a new data type and a reader, i.e. something like this:

struct path_reader {
	FILE *in;
	int nul_term_line;
	struct strbuf path;
};
#define PATH_READER_INIT { NULL, STRBUF_INIT };

int read_next_path(struct path_reader *reader, FILE *in, int nul_term_line)
{
	if (!reader->in) {
		... [initialize] ...
	}

	... [read and possibly unquote path] ...
}

void cleanup_path_reader(struct path_reader *reader)
{
	if (reader->in) {
		fclose(reader->in);
		reader->in = NULL;
	}

	strbuf_release(&reader->buf);
}

And then the repeated code could be replaced by something like this:

	struct path_reader path_reader = PATH_READER_INIT;

	while (read_next_path(&reader, stdin, 1)) {
		... [work with reader->path.buf] ...
	}

	cleanup_path_reader();

Probably this is actually not limited to path names, so the functions
should be renamed...

(totally untested, of course...)

Ciao,
Dscho

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

From: Jeff King <hidden>
Date: 2016-06-15 23:08:02

On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

quoted

It's a shame that we can't just factor out this common
code, but I don't think it's quite long enough to merit
the boilerplate. The interesting part of each function
happens inside the loop. If C had lambdas, we could do
something like:

  foreach_path_from(stdin, nul_term_line) {
        /* now do something interesting with "buf"
           and some other local variables */
  }

Technically, we do not have to do lambdas for that paradigm, we could
introduce a new data type and a reader, i.e. something like this:
[...]
And then the repeated code could be replaced by something like this:

	struct path_reader path_reader = PATH_READER_INIT;

	while (read_next_path(&reader, stdin, 1)) {
		... [work with reader->path.buf] ...
	}

	cleanup_path_reader();

Yeah, you're right. I was thinking of lifting the loop completely out of
the call-sites, but simplifying it to a single line loop condition is
just as good.

I still think this crosses my line of "too much boilerplate to be worth
it", though.

-Peff

Re: [PATCH 6/6] apply, ls-files: simplify "-z" parsing

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:08:02

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

I left this one for last, because it's the most questionable.

I like the simplification... ;-)

The other patches in this series look fine to me.

Ciao,
Dscho

Re: [PATCH 1/6] give "nbuf" strbuf a more meaningful name

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:08:02

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:

quoted

On Sun, 31 Jan 2016, Jeff King wrote:

quoted

It's a shame that we can't just factor out this common
code, but I don't think it's quite long enough to merit
the boilerplate. The interesting part of each function
happens inside the loop. If C had lambdas, we could do
something like:

  foreach_path_from(stdin, nul_term_line) {
        /* now do something interesting with "buf"
           and some other local variables */
  }

Technically, we do not have to do lambdas for that paradigm, we could
introduce a new data type and a reader, i.e. something like this:
[...]
And then the repeated code could be replaced by something like this:

	struct path_reader path_reader = PATH_READER_INIT;

	while (read_next_path(&reader, stdin, 1)) {
		... [work with reader->path.buf] ...
	}

	cleanup_path_reader();

Yeah, you're right. I was thinking of lifting the loop completely out of
the call-sites, but simplifying it to a single line loop condition is
just as good.

I still think this crosses my line of "too much boilerplate to be worth
it", though.

Oh, I totally agree. Just wanted to point out that there are other
options.

Ciao,
Dscho

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help