[PATCH 0/6] post-strbuf_getline cleanups

[PATCH 0/6] post-strbuf_getline cleanups

[Jeff King]

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

[Jeff King]

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 <peff@peff.net>
---
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

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 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

[Jeff King]

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 <peff@peff.net>
---
 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

[Jeff King]

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 <peff@peff.net>
---
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

[Jeff King]

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 <peff@peff.net>
---
 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

[Jeff King]

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 <peff@peff.net>
---
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

[Johannes Schindelin]

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

> > 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 6/6] apply, ls-files: simplify "-z" parsing

[Johannes Schindelin]

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

[Jeff King]

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

> Hi Peff,
> 
> On Sun, 31 Jan 2016, Jeff King wrote:
> 
> > > 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 1/6] give "nbuf" strbuf a more meaningful name

[Johannes Schindelin]

Hi Peff,

On Sun, 31 Jan 2016, Jeff King wrote:

> On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:
> 
> > On Sun, 31 Jan 2016, Jeff King wrote:
> > 
> > > > 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

Re: [PATCH 0/6] post-strbuf_getline cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Thanks for following up.

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Thanks.  That's worth fixing and I agree with the decision that it
is the best way to go to not support '--no-stage'.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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"));

Hmm, it is surprising that there is no parse-options flag that says
"this 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;
>  }

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Isn't the --index about updating the cached stat information, to
allow us to then say "the working tree file hasn't been modified
since we wrote it out"?  After writing a file out to somewhere that
is not its natural location (i.e. using prefix, stage or temp would
all write the contents of F to somewhere that is not F), the next
"diff-files" would not compare the index entry with the contents
held in that relocated location, but with its natural location.

Admittedly, even if we update the cached stat info from that
relocated place, the next "diff-files" would certainly not match
(not just mtime but i-num would be different), but if the real
working tree file were clean when --temp checkout happened, that
would mean we would force another round of update-index --refresh,
so...

>  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 */

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

[Jeff King]

On Sun, Jan 31, 2016 at 06:18:39PM -0800, Junio C Hamano wrote:

> >  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"));
> 
> Hmm, it is surprising that there is no parse-options flag that says
> "this cannot be negated".

There is, but you have to stop using the nice OPT_CALLBACK macro and do
a full-on "{}" struct literal in the options. Since we have a callback,
I figured doing this is less ugly to look at. But it does mean making up
our own message.

I didn't actually try it yesterday; having just done so, it's a lot less
bad than I expected. And I also noticed yet another problem with it. :-/

How about this as a replacement patch?

-- >8 --
Subject: [PATCH] checkout-index: disallow "--no-stage" option

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, let's improve the user-facing
messages for this option. The error string should be
translatable, and we should give some hint in the "-h"
output about what can go in the argument field.

Signed-off-by: Jeff King <peff@peff.net>
---
I also notice that you cannot use "--stage=0" to reset a previous
"--stage=1". It probably would not hurt to allow that, but I left it out
of this patch.

 builtin/checkout-index.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..92c6967 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -141,7 +141,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;
 }
@@ -173,9 +173,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			N_("write the content to temporary files")),
 		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
 			N_("when creating files, prepend <string>")),
-		OPT_CALLBACK(0, "stage", NULL, NULL,
+		{ OPTION_CALLBACK, 0, "stage", NULL, "1-3|all",
 			N_("copy out the files from named stage"),
-			option_parse_stage),
+			PARSE_OPT_NONEG, option_parse_stage },
 		OPT_END()
 	};
 
-- 
2.7.0.489.g6faad84

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

[Jeff King]

On Sun, Jan 31, 2016 at 06:25:22PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Isn't the --index about updating the cached stat information, to
> allow us to then say "the working tree file hasn't been modified
> since we wrote it out"?  After writing a file out to somewhere that
> is not its natural location (i.e. using prefix, stage or temp would
> all write the contents of F to somewhere that is not F), the next
> "diff-files" would not compare the index entry with the contents
> held in that relocated location, but with its natural location.

Yeah, that makes sense to me. I should have said "...but I do not know
why, and I did not really look into it" in my original.

That probably makes it OK to silently ignore (as opposed to complaining
that "--prefix" is used with "--index"). It is not so much "these
options are incompatible" as the fact that there is no entry to update
in the case of a prefix or tempfile. So "--index" is still in effect, it
just has no work to do. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 <peff@peff.net>
> ---
> 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.

If done as two patch series where one does this and the other flips
polarity of the variable (!!line_termination === !nul_term_line),
would that make the result both simpler to read and future-proof?

Of course, a patch adding a "--nul" can be the one that does the
polarity flipping, so in that sense, this simplification is probably
OK, as long as there is some comment that warns a time-bomb you just
planted here ;-)

>  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,

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Of course, a patch adding a "--nul" can be the one that does the
> polarity flipping, so in that sense, this simplification is probably
> OK, as long as there is some comment that warns a time-bomb you just
> planted here ;-)

I'll queue it with this tweak for now.

The idea is to have them run "blame" to find the last paragraph of
the commit log message.

Thanks.

    From: Jeff King <peff@peff.net>
    Date: Sun, 31 Jan 2016 06:35:46 -0500
    Subject: [PATCH] apply, ls-files: simplify "-z" parsing

    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.

 +  Anybody who adds "--nul" synonym to this later would need to be
 +  careful not to break "--no-nul", which should mean that lines are
 +  terminated with LF at the end.

    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/apply.c b/builtin/apply.c
index 565f3fd..d61ac65 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4536,6 +4536,7 @@ 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")),
+		/* Think twice before adding "--nul" synonym to this */
 		OPT_SET_INT('z', NULL, &line_termination,
 			N_("paths are separated with NUL character"), '\0'),
 		OPT_INTEGER('C', NULL, &p_context,
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 59bad9b..467699b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -400,6 +400,7 @@ 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[] = {
+		/* Think twice before adding "--nul" synonym to this */
 		OPT_SET_INT('z', NULL, &line_terminator,
 			N_("paths are separated with NUL character"), '\0'),
 		OPT_BOOL('t', NULL, &show_tag,

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

[Jeff King]

On Mon, Feb 01, 2016 at 02:52:30PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Of course, a patch adding a "--nul" can be the one that does the
> > polarity flipping, so in that sense, this simplification is probably
> > OK, as long as there is some comment that warns a time-bomb you just
> > planted here ;-)
> 
> I'll queue it with this tweak for now.
> 
> The idea is to have them run "blame" to find the last paragraph of
> the commit log message.

That looks like a good compromise. Thanks.

-Peff