[PATCH v2 0/4] stripspace: Implement and use --count-lines option

[PATCH v2 0/4] stripspace: Implement and use --count-lines option

[Tobias Klauser]

(1) Move the stripspace() function to the strbuf module adding a prefix
    and changing all users accordingly. Also introduce a wrapper in case
    any topic branches still depend on the old name.

(2) Switch git stripspace to use parse-options in order to simplify
    introducing new command line options (as in the following patch). In
    v1 this was folded into patch (3) and is now split out for v2.

(3) Introduce option --count-lines to git stripspace and add the
    corresponding documentation and tests.

(4) Change git-rebase--interactive.sh to replace commands like:

	git stripspace ... | wc -l

    with:

	git stripspace --count-lines ...

This patch set implements some of the project ideas around git stripspace
suggested on https://git.wiki.kernel.org/index.php/SmallProjectsIdeas

v1 -> v2:

  - Thanks to Junio and Matthieu for the review.
  - Split patch 2/3 into two patches: patch 2/4 switches git stripspace
    to use parse-options and patch 3/4 introduces the new option.
  - Implement line counting in cmd_stripbuf() instead of (ab-)using
    strbuf_stripspace() for it.
  - Drop -C short option
  - Correct example command output in documentation.
  - Adjust commit messages to not include links to the wiki, fully
    describe the motivation in the commit message instead.

Tobias Klauser (4):
  strbuf: make stripspace() part of strbuf
  stripspace: Use parse-options for command-line parsing
  stripspace: Implement --count-lines option
  git rebase -i: Use newly added --count-lines option for stripspace

 Documentation/git-stripspace.txt |  14 +++-
 builtin/am.c                     |   2 +-
 builtin/branch.c                 |   2 +-
 builtin/commit.c                 |   6 +-
 builtin/merge.c                  |   2 +-
 builtin/notes.c                  |   6 +-
 builtin/stripspace.c             | 137 +++++++++++++--------------------------
 builtin/tag.c                    |   2 +-
 git-rebase--interactive.sh       |   6 +-
 strbuf.c                         |  66 +++++++++++++++++++
 strbuf.h                         |  11 +++-
 t/t0030-stripspace.sh            |  36 ++++++++++
 12 files changed, 181 insertions(+), 109 deletions(-)

-- 
2.6.1.148.g7927db1

[PATCH v2 1/4] strbuf: make stripspace() part of strbuf

[Tobias Klauser]

Rename stripspace() to strbuf_stripspace() and move it to the strbuf
module. The function is also used in other builtins than stripspace, so
it makes sense to have it in a more generic place. Since it operates on
an strbuf and the function is declared in strbuf.h, move it to strbuf.c
and add the corresponding prefix to its name.

Also switch all current users of stripspace() to the new function name
and keep a temporary wrapper inline function for any topic branches
still using stripspace().

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

 builtin/am.c         |  2 +-
 builtin/branch.c     |  2 +-
 builtin/commit.c     |  6 ++---
 builtin/merge.c      |  2 +-
 builtin/notes.c      |  6 ++---
 builtin/stripspace.c | 69 ++--------------------------------------------------
 builtin/tag.c        |  2 +-
 strbuf.c             | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 strbuf.h             | 11 ++++++++-
 9 files changed, 88 insertions(+), 78 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3bd4fd7..7b8e11e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1343,7 +1343,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 	strbuf_addstr(&msg, "\n\n");
 	if (strbuf_read_file(&msg, am_path(state, "msg"), 0) < 0)
 		die_errno(_("could not read '%s'"), am_path(state, "msg"));
-	stripspace(&msg, 0);
+	strbuf_stripspace(&msg, 0);
 
 	if (state->signoff)
 		am_signoff(&msg);
diff --git a/builtin/branch.c b/builtin/branch.c
index 01f9530..b99a436 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -592,7 +592,7 @@ static int edit_branch_description(const char *branch_name)
 		strbuf_release(&buf);
 		return -1;
 	}
-	stripspace(&buf, 1);
+	strbuf_stripspace(&buf, 1);
 
 	strbuf_addf(&name, "branch.%s.description", branch_name);
 	status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 63772d0..dca09e2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -775,7 +775,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	s->hints = 0;
 
 	if (clean_message_contents)
-		stripspace(&sb, 0);
+		strbuf_stripspace(&sb, 0);
 
 	if (signoff)
 		append_signoff(&sb, ignore_non_trailer(&sb), 0);
@@ -1014,7 +1014,7 @@ static int template_untouched(struct strbuf *sb)
 	if (!template_file || strbuf_read_file(&tmpl, template_file, 0) <= 0)
 		return 0;
 
-	stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
+	strbuf_stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
 	if (!skip_prefix(sb->buf, tmpl.buf, &start))
 		start = sb->buf;
 	strbuf_release(&tmpl);
@@ -1726,7 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		wt_status_truncate_message_at_cut_line(&sb);
 
 	if (cleanup_mode != CLEANUP_NONE)
-		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
+		strbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (template_untouched(&sb) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
diff --git a/builtin/merge.c b/builtin/merge.c
index a0edaca..e6741f3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -806,7 +806,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 	read_merge_msg(&msg);
-	stripspace(&msg, 0 < option_edit);
+	strbuf_stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
 		abort_commit(remoteheads, _("Empty commit message."));
 	strbuf_release(&merge_msg);
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..bb23d55 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -192,7 +192,7 @@ static void prepare_note_data(const unsigned char *object, struct note_data *d,
 		if (launch_editor(d->edit_path, &d->buf, NULL)) {
 			die(_("Please supply the note contents using either -m or -F option"));
 		}
-		stripspace(&d->buf, 1);
+		strbuf_stripspace(&d->buf, 1);
 	}
 }
 
@@ -215,7 +215,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 	if (d->buf.len)
 		strbuf_addch(&d->buf, '\n');
 	strbuf_addstr(&d->buf, arg);
-	stripspace(&d->buf, 0);
+	strbuf_stripspace(&d->buf, 0);
 
 	d->given = 1;
 	return 0;
@@ -232,7 +232,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
 			die_errno(_("cannot read '%s'"), arg);
 	} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)
 		die_errno(_("could not open or read '%s'"), arg);
-	stripspace(&d->buf, 0);
+	strbuf_stripspace(&d->buf, 0);
 
 	d->given = 1;
 	return 0;
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..f677093 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -1,71 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
-
-/*
- * Returns the length of a line, without trailing spaces.
- *
- * If the line ends with newline, it will be removed too.
- */
-static size_t cleanup(char *line, size_t len)
-{
-	while (len) {
-		unsigned char c = line[len - 1];
-		if (!isspace(c))
-			break;
-		len--;
-	}
-
-	return len;
-}
-
-/*
- * Remove empty lines from the beginning and end
- * and also trailing spaces from every line.
- *
- * Turn multiple consecutive empty lines between paragraphs
- * into just one empty line.
- *
- * If the input has only empty lines and spaces,
- * no output will be produced.
- *
- * If last line does not have a newline at the end, one is added.
- *
- * Enable skip_comments to skip every line starting with comment
- * character.
- */
-void stripspace(struct strbuf *sb, int skip_comments)
-{
-	int empties = 0;
-	size_t i, j, len, newlen;
-	char *eol;
-
-	/* We may have to add a newline. */
-	strbuf_grow(sb, 1);
-
-	for (i = j = 0; i < sb->len; i += len, j += newlen) {
-		eol = memchr(sb->buf + i, '\n', sb->len - i);
-		len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
-
-		if (skip_comments && len && sb->buf[i] == comment_line_char) {
-			newlen = 0;
-			continue;
-		}
-		newlen = cleanup(sb->buf + i, len);
-
-		/* Not just an empty line? */
-		if (newlen) {
-			if (empties > 0 && j > 0)
-				sb->buf[j++] = '\n';
-			empties = 0;
-			memmove(sb->buf + j, sb->buf + i, newlen);
-			sb->buf[newlen + j++] = '\n';
-		} else {
-			empties++;
-		}
-	}
-
-	strbuf_setlen(sb, j);
-}
+#include "strbuf.h"
 
 static void comment_lines(struct strbuf *buf)
 {
@@ -111,7 +46,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 		die_errno("could not read the input");
 
 	if (mode == STRIP_SPACE)
-		stripspace(&buf, strip_comments);
+		strbuf_stripspace(&buf, strip_comments);
 	else
 		comment_lines(&buf);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 9e17dca..5660787 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -268,7 +268,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 
 	if (opt->cleanup_mode != CLEANUP_NONE)
-		stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
+		strbuf_stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
 
 	if (!opt->message_given && !buf->len)
 		die(_("no tag message?"));
diff --git a/strbuf.c b/strbuf.c
index 29df55b..9583875 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -743,3 +743,69 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 	}
 	strbuf_setlen(sb, sb->len + len);
 }
+
+/*
+ * Returns the length of a line, without trailing spaces.
+ *
+ * If the line ends with newline, it will be removed too.
+ */
+static size_t cleanup(char *line, size_t len)
+{
+	while (len) {
+		unsigned char c = line[len - 1];
+		if (!isspace(c))
+			break;
+		len--;
+	}
+
+	return len;
+}
+
+/*
+ * Remove empty lines from the beginning and end
+ * and also trailing spaces from every line.
+ *
+ * Turn multiple consecutive empty lines between paragraphs
+ * into just one empty line.
+ *
+ * If the input has only empty lines and spaces,
+ * no output will be produced.
+ *
+ * If last line does not have a newline at the end, one is added.
+ *
+ * Enable skip_comments to skip every line starting with comment
+ * character.
+ */
+void strbuf_stripspace(struct strbuf *sb, int skip_comments)
+{
+	int empties = 0;
+	size_t i, j, len, newlen;
+	char *eol;
+
+	/* We may have to add a newline. */
+	strbuf_grow(sb, 1);
+
+	for (i = j = 0; i < sb->len; i += len, j += newlen) {
+		eol = memchr(sb->buf + i, '\n', sb->len - i);
+		len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
+
+		if (skip_comments && len && sb->buf[i] == comment_line_char) {
+			newlen = 0;
+			continue;
+		}
+		newlen = cleanup(sb->buf + i, len);
+
+		/* Not just an empty line? */
+		if (newlen) {
+			if (empties > 0 && j > 0)
+				sb->buf[j++] = '\n';
+			empties = 0;
+			memmove(sb->buf + j, sb->buf + i, newlen);
+			sb->buf[newlen + j++] = '\n';
+		} else {
+			empties++;
+		}
+	}
+
+	strbuf_setlen(sb, j);
+}
diff --git a/strbuf.h b/strbuf.h
index aef2794..5397d91 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -418,7 +418,16 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
  * Strip whitespace from a buffer. The second parameter controls if
  * comments are considered contents to be removed or not.
  */
-extern void stripspace(struct strbuf *buf, int skip_comments);
+extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);
+
+/**
+ * Temporary alias until all topic branches have switched to use
+ * strbuf_stripspace directly.
+ */
+static inline void stripspace(struct strbuf *buf, int skip_comments)
+{
+	strbuf_stripspace(buf, skip_comments);
+}
 
 static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 {
-- 
2.6.1.148.g7927db1

[PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

Use parse-options to parse command-line options instead of a
hand-crafted implementation.

This is a preparatory patch to simplify the introduction of the
--count-lines option in a follow-up patch.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 builtin/stripspace.c | 56 ++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index f677093..ac1ab3d 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "strbuf.h"
 
 static void comment_lines(struct strbuf *buf)
@@ -12,41 +13,44 @@ static void comment_lines(struct strbuf *buf)
 	free(msg);
 }
 
-static const char *usage_msg = "\n"
-"  git stripspace [-s | --strip-comments] < input\n"
-"  git stripspace [-c | --comment-lines] < input";
+static const char * const stripspace_usage[] = {
+	N_("git stripspace [-s | --strip-comments] < input"),
+	N_("git stripspace [-c | --comment-lines] < input"),
+	NULL
+};
+
+enum stripspace_mode {
+	STRIP_DEFAULT = 0,
+	STRIP_COMMENTS,
+	COMMENT_LINES
+};
 
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int strip_comments = 0;
-	enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;
-
-	if (argc == 2) {
-		if (!strcmp(argv[1], "-s") ||
-		    !strcmp(argv[1], "--strip-comments")) {
-			strip_comments = 1;
-		} else if (!strcmp(argv[1], "-c") ||
-			   !strcmp(argv[1], "--comment-lines")) {
-			mode = COMMENT_LINES;
-		} else {
-			mode = INVAL;
-		}
-	} else if (argc > 1) {
-		mode = INVAL;
-	}
-
-	if (mode == INVAL)
-		usage(usage_msg);
-
-	if (strip_comments || mode == COMMENT_LINES)
+	enum stripspace_mode mode = STRIP_DEFAULT;
+
+	const struct option options[] = {
+		OPT_CMDMODE('s', "strip-comments", &mode,
+			    N_("skip and remove all lines starting with comment character"),
+			    STRIP_COMMENTS),
+		OPT_CMDMODE('c', "comment-lines", &mode,
+			    N_("prepend comment character and blank to each line"),
+			    COMMENT_LINES),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, stripspace_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
 		git_config(git_default_config, NULL);
 
 	if (strbuf_read(&buf, 0, 1024) < 0)
 		die_errno("could not read the input");
 
-	if (mode == STRIP_SPACE)
-		strbuf_stripspace(&buf, strip_comments);
+	if (mode == STRIP_DEFAULT || mode == STRIP_COMMENTS)
+		strbuf_stripspace(&buf, mode == STRIP_COMMENTS);
 	else
 		comment_lines(&buf);
 
-- 
2.6.1.148.g7927db1

[PATCH v2 3/4] stripspace: Implement --count-lines option

[Tobias Klauser]

Implement the --count-lines options for git stripspace to be able to
omit calling:

  git stripspace --strip-comments < infile | wc -l

e.g. in git-rebase--interactive.sh. The above command can now be
replaced by:

  git stripspace --strip-comments --count-lines < infile

This will make it easier to port git-rebase--interactive.sh to C later
on.

Furthermore, add the corresponding documentation and tests.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

 Documentation/git-stripspace.txt | 14 ++++++++++++--
 builtin/stripspace.c             | 18 +++++++++++++++---
 t/t0030-stripspace.sh            | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index 60328d5..79900b8 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -9,8 +9,8 @@ git-stripspace - Remove unnecessary whitespace
 SYNOPSIS
 --------
 [verse]
-'git stripspace' [-s | --strip-comments] < input
-'git stripspace' [-c | --comment-lines] < input
+'git stripspace' [-s | --strip-comments] [--count-lines] < input
+'git stripspace' [-c | --comment-lines] [--count-lines] < input
 
 DESCRIPTION
 -----------
@@ -44,6 +44,10 @@ OPTIONS
 	be terminated with a newline. On empty lines, only the comment character
 	will be prepended.
 
+--count-lines::
+	Output the number of resulting lines after stripping. This is equivalent
+	to calling 'git stripspace | wc -l'.
+
 EXAMPLES
 --------
 
@@ -88,6 +92,12 @@ Use 'git stripspace --strip-comments' to obtain:
 |The end.$
 ---------
 
+Use 'git stripspace --count-lines' to obtain:
+
+---------
+5
+---------
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index ac1ab3d..487523f 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -14,8 +14,8 @@ static void comment_lines(struct strbuf *buf)
 }
 
 static const char * const stripspace_usage[] = {
-	N_("git stripspace [-s | --strip-comments] < input"),
-	N_("git stripspace [-c | --comment-lines] < input"),
+	N_("git stripspace [-s | --strip-comments] [--count-lines] < input"),
+	N_("git stripspace [-c | --comment-lines] [--count-lines] < input"),
 	NULL
 };
 
@@ -29,6 +29,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	enum stripspace_mode mode = STRIP_DEFAULT;
+	int count_lines = 0;
 
 	const struct option options[] = {
 		OPT_CMDMODE('s', "strip-comments", &mode,
@@ -37,6 +38,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE('c', "comment-lines", &mode,
 			    N_("prepend comment character and blank to each line"),
 			    COMMENT_LINES),
+		OPT_BOOL(0, "count-lines", &count_lines, N_("print line count")),
 		OPT_END()
 	};
 
@@ -54,7 +56,17 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 	else
 		comment_lines(&buf);
 
-	write_or_die(1, buf.buf, buf.len);
+	if (!count_lines)
+		write_or_die(1, buf.buf, buf.len);
+	else {
+		size_t i, lines;
+
+		for (i = lines = 0; i < buf.len; i++) {
+			if (buf.buf[i] == '\n')
+				lines++;
+		}
+		printf("%zu\n", lines);
+	}
 	strbuf_release(&buf);
 	return 0;
 }
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..9c00cb9 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented line' '
 	test_cmp expect actual
 '
 
+test_expect_success '--count-lines with newline only' '
+	printf "0\n" >expect &&
+	printf "\n" | git stripspace --count-lines >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line' '
+	printf "1\n" >expect &&
+	printf "foo\n" | git stripspace --count-lines >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line preceeded by empty line' '
+	printf "1\n" >expect &&
+	printf "\nfoo" | git stripspace --count-lines >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line followed by empty line' '
+	printf "1\n" >expect &&
+	printf "foo\n\n" | git stripspace --count-lines >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines with multiple lines and consecutive newlines' '
+	printf "5\n" >expect &&
+	printf "\none\n\n\nthree\nfour\nfive\n" | git stripspace --count-lines >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines combined with --strip-comments' '
+	printf "5\n" >expect &&
+	printf "\n# stripped\none\n#stripped\n\nthree\nfour\nfive\n" | git stripspace -s --count-lines >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.1.148.g7927db1

[PATCH v2 4/4] git rebase -i: Use newly added --count-lines option for stripspace

[Tobias Klauser]

Use the newly added --count-lines option for 'git stripspace' to count
lines instead of piping the entire output to 'wc -l'.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

 git-rebase--interactive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..f80da30 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -120,9 +120,9 @@ mark_action_done () {
 	sed -e 1q < "$todo" >> "$done"
 	sed -e 1d < "$todo" >> "$todo".new
 	mv -f "$todo".new "$todo"
-	new_count=$(git stripspace --strip-comments <"$done" | wc -l)
+	new_count=$(git stripspace --strip-comments --count-lines <"$done")
 	echo $new_count >"$msgnum"
-	total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l)))
+	total=$(($new_count + $(git stripspace --strip-comments --count-lines <"$todo")))
 	echo $total >"$end"
 	if test "$last_count" != "$new_count"
 	then
@@ -1243,7 +1243,7 @@ test -s "$todo" || echo noop >> "$todo"
 test -n "$autosquash" && rearrange_squash "$todo"
 test -n "$cmd" && add_exec_commands "$todo"
 
-todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
+todocount=$(git stripspace --strip-comments --count-lines <"$todo")
 todocount=${todocount##* }
 
 cat >>"$todo" <<EOF
-- 
2.6.1.148.g7927db1

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

Be consistent with the subjects, please.

>   strbuf: make stripspace() part of strbuf

s/make/make/ ;-)

>   stripspace: Use parse-options for command-line parsing

s/Use/use/

>   stripspace: Implement --count-lines option

s/Implement/implement/

>   git rebase -i: Use newly added --count-lines option for stripspace

s/Use/use/


>  Documentation/git-stripspace.txt |  14 +++-
>  builtin/am.c                     |   2 +-
>  builtin/branch.c                 |   2 +-
>  builtin/commit.c                 |   6 +-
>  builtin/merge.c                  |   2 +-
>  builtin/notes.c                  |   6 +-
>  builtin/stripspace.c             | 137 +++++++++++++--------------------------
>  builtin/tag.c                    |   2 +-
>  git-rebase--interactive.sh       |   6 +-
>  strbuf.c                         |  66 +++++++++++++++++++
>  strbuf.h                         |  11 +++-
>  t/t0030-stripspace.sh            |  36 ++++++++++
>  12 files changed, 181 insertions(+), 109 deletions(-)

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

>   - Split patch 2/3 into two patches: patch 2/4 switches git stripspace
>     to use parse-options and patch 3/4 introduces the new option.

Much better now.

>   - Implement line counting in cmd_stripbuf() instead of (ab-)using
>     strbuf_stripspace() for it.

Also short and sweet, I like it.

>   - Drop -C short option
>   - Correct example command output in documentation.
>   - Adjust commit messages to not include links to the wiki, fully
>     describe the motivation in the commit message instead.

Good.

I read the patches again, and the whole series is now

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

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

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> Use parse-options to parse command-line options instead of a
> hand-crafted implementation.
>
> This is a preparatory patch to simplify the introduction of the
> --count-lines option in a follow-up patch.

The second paragraph is probably of much lessor importance than one
thing you forgot to mention: the users can now use a unique prefix
of the option and say "stripspace --comment".

> +enum stripspace_mode {
> +	STRIP_DEFAULT = 0,
> +	STRIP_COMMENTS,
> +	COMMENT_LINES
> +};
>  
>  int cmd_stripspace(int argc, const char **argv, const char *prefix)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	int strip_comments = 0;
> -	enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;
> -
> -	if (argc == 2) {
> -		if (!strcmp(argv[1], "-s") ||
> -		    !strcmp(argv[1], "--strip-comments")) {
> -			strip_comments = 1;
> -		} else if (!strcmp(argv[1], "-c") ||
> -			   !strcmp(argv[1], "--comment-lines")) {
> -			mode = COMMENT_LINES;
> -		} else {
> -			mode = INVAL;
> -		}
> -	} else if (argc > 1) {
> -		mode = INVAL;
> -	}
> -
> -	if (mode == INVAL)
> -		usage(usage_msg);

When given "git stripspace -s blorg", we used to set mode to INVAL
and then showed the correct usage.  But we no longer have a check
that corresponds to the old INVAL thing, do we?  Perhaps check argc
to detect presence of an otherwise ignored non-option argument
immediately after parse_options() returns?

> -	if (strip_comments || mode == COMMENT_LINES)
> +	enum stripspace_mode mode = STRIP_DEFAULT;
> +
> +	const struct option options[] = {
> +		OPT_CMDMODE('s', "strip-comments", &mode,
> +			    N_("skip and remove all lines starting with comment character"),
> +			    STRIP_COMMENTS),
> +		OPT_CMDMODE('c', "comment-lines", &mode,
> +			    N_("prepend comment character and blank to each line"),
> +			    COMMENT_LINES),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);

What is the point of keep-dashdash here?

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

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

>> -	if (mode == INVAL)
>> -		usage(usage_msg);
>
> When given "git stripspace -s blorg", we used to set mode to INVAL
> and then showed the correct usage.  But we no longer have a check
> that corresponds to the old INVAL thing, do we?  Perhaps check argc
> to detect presence of an otherwise ignored non-option argument
> immediately after parse_options() returns?

Perhaps like this.

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index ac1ab3d..a8b7a93 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -40,8 +40,9 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	argc = parse_options(argc, argv, prefix, options, stripspace_usage,
-			     PARSE_OPT_KEEP_DASHDASH);
+	argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);
+	if (argc)
+		usage_with_options(stripspace_usage, options);
 
 	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
 		git_config(git_default_config, NULL);

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

[Tobias Klauser]

On 2015-10-16 at 18:41:31 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> Be consistent with the subjects, please.
> 
> >   strbuf: make stripspace() part of strbuf
> 
> s/make/make/ ;-)
> 
> >   stripspace: Use parse-options for command-line parsing
> 
> s/Use/use/
> 
> >   stripspace: Implement --count-lines option
> 
> s/Implement/implement/
> 
> >   git rebase -i: Use newly added --count-lines option for stripspace
> 
> s/Use/use/

Will adjust all of them to lowercase for v3. Thanks.

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

[Tobias Klauser]

On 2015-10-16 at 18:54:45 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> >   - Split patch 2/3 into two patches: patch 2/4 switches git stripspace
> >     to use parse-options and patch 3/4 introduces the new option.
> 
> Much better now.
> 
> >   - Implement line counting in cmd_stripbuf() instead of (ab-)using
> >     strbuf_stripspace() for it.
> 
> Also short and sweet, I like it.
> 
> >   - Drop -C short option
> >   - Correct example command output in documentation.
> >   - Adjust commit messages to not include links to the wiki, fully
> >     describe the motivation in the commit message instead.
> 
> Good.
> 
> I read the patches again, and the whole series is now
> 
> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Thank you for the review!

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-16 at 19:07:34 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > Use parse-options to parse command-line options instead of a
> > hand-crafted implementation.
> >
> > This is a preparatory patch to simplify the introduction of the
> > --count-lines option in a follow-up patch.
> 
> The second paragraph is probably of much lessor importance than one
> thing you forgot to mention: the users can now use a unique prefix
> of the option and say "stripspace --comment".

I didn't even know about that feature, but now that you've mentioned it
I will certainly make use of it more in the future :) And of course,
I'll adjust the commit message accordingly for v3.

> > +enum stripspace_mode {
> > +	STRIP_DEFAULT = 0,
> > +	STRIP_COMMENTS,
> > +	COMMENT_LINES
> > +};
> >  
> >  int cmd_stripspace(int argc, const char **argv, const char *prefix)
> >  {
> >  	struct strbuf buf = STRBUF_INIT;
> > -	int strip_comments = 0;
> > -	enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;
> > -
> > -	if (argc == 2) {
> > -		if (!strcmp(argv[1], "-s") ||
> > -		    !strcmp(argv[1], "--strip-comments")) {
> > -			strip_comments = 1;
> > -		} else if (!strcmp(argv[1], "-c") ||
> > -			   !strcmp(argv[1], "--comment-lines")) {
> > -			mode = COMMENT_LINES;
> > -		} else {
> > -			mode = INVAL;
> > -		}
> > -	} else if (argc > 1) {
> > -		mode = INVAL;
> > -	}
> > -
> > -	if (mode == INVAL)
> > -		usage(usage_msg);
> 
> When given "git stripspace -s blorg", we used to set mode to INVAL
> and then showed the correct usage.  But we no longer have a check
> that corresponds to the old INVAL thing, do we?  Perhaps check argc
> to detect presence of an otherwise ignored non-option argument
> immediately after parse_options() returns?

Agreed, we should check it. I'll go with the implementation you
suggested in the follow-up message.

> > -	if (strip_comments || mode == COMMENT_LINES)
> > +	enum stripspace_mode mode = STRIP_DEFAULT;
> > +
> > +	const struct option options[] = {
> > +		OPT_CMDMODE('s', "strip-comments", &mode,
> > +			    N_("skip and remove all lines starting with comment character"),
> > +			    STRIP_COMMENTS),
> > +		OPT_CMDMODE('c', "comment-lines", &mode,
> > +			    N_("prepend comment character and blank to each line"),
> > +			    COMMENT_LINES),
> > +		OPT_END()
> > +	};
> > +
> > +	argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> > +			     PARSE_OPT_KEEP_DASHDASH);
> 
> What is the point of keep-dashdash here?

Likewise, it shouldn't be there as in your follow-up patch.

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-16 at 19:29:35 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> -	if (mode == INVAL)
> >> -		usage(usage_msg);
> >
> > When given "git stripspace -s blorg", we used to set mode to INVAL
> > and then showed the correct usage.  But we no longer have a check
> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> > to detect presence of an otherwise ignored non-option argument
> > immediately after parse_options() returns?
> 
> Perhaps like this.

Thanks. I'll fold it into v3.

> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index ac1ab3d..a8b7a93 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -40,8 +40,9 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> -			     PARSE_OPT_KEEP_DASHDASH);
> +	argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);
> +	if (argc)
> +		usage_with_options(stripspace_usage, options);
>  
>  	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
>  		git_config(git_default_config, NULL);
> 

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> On 2015-10-16 at 19:29:35 +0200, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> >> -	if (mode == INVAL)
>> >> -		usage(usage_msg);
>> >
>> > When given "git stripspace -s blorg", we used to set mode to INVAL
>> > and then showed the correct usage.  But we no longer have a check
>> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
>> > to detect presence of an otherwise ignored non-option argument
>> > immediately after parse_options() returns?
>> 
>> Perhaps like this.
>
> Thanks. I'll fold it into v3.

Before starting v3, please fetch from me and check what is queued on
'pu'.  It may turn out that the fix-ups I did while queuing this
round is sufficient, in which case you can just say that instead ;-)

Thanks.

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Eric Sunshine]

On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser <tklauser@distanz.ch> wrote:
> Implement the --count-lines options for git stripspace [...]
>
> This will make it easier to port git-rebase--interactive.sh to C later
> on.

Is there any application beyond git-rebase--interactive where a
--count-lines options is expected to be useful? It's not obvious from
the commit message that this change is necessarily a win for later
porting of git-rebase--interactive to C since the amount of extra code
and support material added by this patch probably outweighs the amount
of code a C version of git-rebase--interactive would need to count the
lines itself.

Stated differently, are the two or three instances of piping through
'wc' in git-rebase--interactive sufficient justification for
introducing extra complexity into git-stripspace and its documentation
and tests?

More below.

> Furthermore, add the corresponding documentation and tests.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d8..9c00cb9 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented line' '
>         test_cmp expect actual
>  '
>
> +test_expect_success '--count-lines with newline only' '
> +       printf "0\n" >expect &&
> +       printf "\n" | git stripspace --count-lines >actual &&
> +       test_cmp expect actual
> +'

What is the expected behavior when the input is an empty file, a file
with content but no newline, a file with one or more lines but lacking
a newline on the final line? Should these cases be tested, as well?

> +test_expect_success '--count-lines with single line' '
> +       printf "1\n" >expect &&
> +       printf "foo\n" | git stripspace --count-lines >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '--count-lines with single line preceeded by empty line' '
> +       printf "1\n" >expect &&
> +       printf "\nfoo" | git stripspace --count-lines >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '--count-lines with single line followed by empty line' '
> +       printf "1\n" >expect &&
> +       printf "foo\n\n" | git stripspace --count-lines >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '--count-lines with multiple lines and consecutive newlines' '
> +       printf "5\n" >expect &&
> +       printf "\none\n\n\nthree\nfour\nfive\n" | git stripspace --count-lines >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '--count-lines combined with --strip-comments' '
> +       printf "5\n" >expect &&
> +       printf "\n# stripped\none\n#stripped\n\nthree\nfour\nfive\n" | git stripspace -s --count-lines >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.6.1.148.g7927db1

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Is there any application beyond git-rebase--interactive where a
> --count-lines options is expected to be useful? It's not obvious from
> the commit message that this change is necessarily a win for later
> porting of git-rebase--interactive to C since the amount of extra code
> and support material added by this patch probably outweighs the amount
> of code a C version of git-rebase--interactive would need to count the
> lines itself.
>
> Stated differently, are the two or three instances of piping through
> 'wc' in git-rebase--interactive sufficient justification for
> introducing extra complexity into git-stripspace and its documentation
> and tests?

Interesting thought.  When somebody rewrites "rebase -i" in C,
nobody needs to count lines in "stripspace" output.  The rewritten
"rebase -i" would internally run strbuf_stripspace() and the question
becomes what is the best way to let that code find out how many lines
the result contains.

When viewed from that angle, I agree that "stripspace --count" does
not add anything to further the goal of helping "rebase -i" to move
to C.  Adding strbuf_count_lines() that counts the number of lines
in the given strbuf (if there is no such helper yet; I didn't check),
though.

>> +test_expect_success '--count-lines with newline only' '
>> +       printf "0\n" >expect &&
>> +       printf "\n" | git stripspace --count-lines >actual &&
>> +       test_cmp expect actual
>> +'
>
> What is the expected behavior when the input is an empty file, a file
> with content but no newline, a file with one or more lines but lacking
> a newline on the final line? Should these cases be tested, as well?

Good point here, too.  If we were to add strbuf_count_lines()
helper, whoever adds that function needs to take a possible
incomplete line at the end into account.

Thanks for your comments.

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Tobias Klauser]

On 2015-10-18 at 01:57:57 +0200, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser <tklauser@distanz.ch> wrote:
> > Implement the --count-lines options for git stripspace [...]
> >
> > This will make it easier to port git-rebase--interactive.sh to C later
> > on.
> 
> Is there any application beyond git-rebase--interactive where a
> --count-lines options is expected to be useful? It's not obvious from
> the commit message that this change is necessarily a win for later
> porting of git-rebase--interactive to C since the amount of extra code
> and support material added by this patch probably outweighs the amount
> of code a C version of git-rebase--interactive would need to count the
> lines itself.

Agreed, it doesn't make much sense anymore in the current form. An
strbuf helper function implementing the line counting would probably be
the better way. But I guess this should only be introduced once someone
decides to write a C version of git-rebase--interactive (or any other
use for line counting appears).

> Stated differently, are the two or three instances of piping through
> 'wc' in git-rebase--interactive sufficient justification for
> introducing extra complexity into git-stripspace and its documentation
> and tests?

IMO it doesn't add a lot of complexity, but on the other hand it also
doesn't provide a large benefit apart from getting rid of a few
calls to an external program in a code path which is not performance
critical.

So I suggest I'll drop patches 3/4 and 4/4 for v3. Once someone really
needs the line counting functionality in C, an strbuf helper can still
be added.

> More below.
> 
> > Furthermore, add the corresponding documentation and tests.
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> > index 29e91d8..9c00cb9 100755
> > --- a/t/t0030-stripspace.sh
> > +++ b/t/t0030-stripspace.sh
> > @@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented line' '
> >         test_cmp expect actual
> >  '
> >
> > +test_expect_success '--count-lines with newline only' '
> > +       printf "0\n" >expect &&
> > +       printf "\n" | git stripspace --count-lines >actual &&
> > +       test_cmp expect actual
> > +'
> 
> What is the expected behavior when the input is an empty file, a file
> with content but no newline, a file with one or more lines but lacking
> a newline on the final line? Should these cases be tested, as well?

Not really sure. For the implementation I followed the behavior of 'wc
-l' which doesn't consider the final line if it lacks a newline. Should
this be different for git's purposes? In any case, I agree that these
cases should excplicitely be tested/documented.

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Tobias Klauser]

On 2015-10-18 at 19:18:53 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > Is there any application beyond git-rebase--interactive where a
> > --count-lines options is expected to be useful? It's not obvious from
> > the commit message that this change is necessarily a win for later
> > porting of git-rebase--interactive to C since the amount of extra code
> > and support material added by this patch probably outweighs the amount
> > of code a C version of git-rebase--interactive would need to count the
> > lines itself.
> >
> > Stated differently, are the two or three instances of piping through
> > 'wc' in git-rebase--interactive sufficient justification for
> > introducing extra complexity into git-stripspace and its documentation
> > and tests?
> 
> Interesting thought.  When somebody rewrites "rebase -i" in C,
> nobody needs to count lines in "stripspace" output.  The rewritten
> "rebase -i" would internally run strbuf_stripspace() and the question
> becomes what is the best way to let that code find out how many lines
> the result contains.
> 
> When viewed from that angle, I agree that "stripspace --count" does
> not add anything to further the goal of helping "rebase -i" to move
> to C.  Adding strbuf_count_lines() that counts the number of lines
> in the given strbuf (if there is no such helper yet; I didn't check),
> though.

I check before implementing this series and didn't find any helper. I
also didn't find any other uses of line counting in the code.

After considering your and Eric's reply, I'll drop these patches for
now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to
Eric).

> >> +test_expect_success '--count-lines with newline only' '
> >> +       printf "0\n" >expect &&
> >> +       printf "\n" | git stripspace --count-lines >actual &&
> >> +       test_cmp expect actual
> >> +'
> >
> > What is the expected behavior when the input is an empty file, a file
> > with content but no newline, a file with one or more lines but lacking
> > a newline on the final line? Should these cases be tested, as well?
> 
> Good point here, too.  If we were to add strbuf_count_lines()
> helper, whoever adds that function needs to take a possible
> incomplete line at the end into account.

Yes, makes more sense like this (even though it doesn't correspond to
what 'wc -l' does).

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Christian Couder]

On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser <tklauser@distanz.ch> wrote:
> On 2015-10-18 at 19:18:53 +0200, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>> > Is there any application beyond git-rebase--interactive where a
>> > --count-lines options is expected to be useful? It's not obvious from
>> > the commit message that this change is necessarily a win for later
>> > porting of git-rebase--interactive to C since the amount of extra code
>> > and support material added by this patch probably outweighs the amount
>> > of code a C version of git-rebase--interactive would need to count the
>> > lines itself.
>> >
>> > Stated differently, are the two or three instances of piping through
>> > 'wc' in git-rebase--interactive sufficient justification for
>> > introducing extra complexity into git-stripspace and its documentation
>> > and tests?
>>
>> Interesting thought.  When somebody rewrites "rebase -i" in C,
>> nobody needs to count lines in "stripspace" output.  The rewritten
>> "rebase -i" would internally run strbuf_stripspace() and the question
>> becomes what is the best way to let that code find out how many lines
>> the result contains.
>>
>> When viewed from that angle, I agree that "stripspace --count" does
>> not add anything to further the goal of helping "rebase -i" to move
>> to C.  Adding strbuf_count_lines() that counts the number of lines
>> in the given strbuf (if there is no such helper yet; I didn't check),
>> though.
>
> I check before implementing this series and didn't find any helper. I
> also didn't find any other uses of line counting in the code.

This shows that implementing "git stripspace --count-lines" could
indirectly help porting "git rebase -i" to C as you could implement
strbuf_count_lines() for the former and it could then be reused in the
latter.

> After considering your and Eric's reply, I'll drop these patches for
> now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to
> Eric).

It would be sad in my opinion.

>> >> +test_expect_success '--count-lines with newline only' '
>> >> +       printf "0\n" >expect &&
>> >> +       printf "\n" | git stripspace --count-lines >actual &&
>> >> +       test_cmp expect actual
>> >> +'
>> >
>> > What is the expected behavior when the input is an empty file, a file
>> > with content but no newline, a file with one or more lines but lacking
>> > a newline on the final line? Should these cases be tested, as well?
>>
>> Good point here, too.  If we were to add strbuf_count_lines()
>> helper, whoever adds that function needs to take a possible
>> incomplete line at the end into account.
>
> Yes, makes more sense like this (even though it doesn't correspond to
> what 'wc -l' does).

Tests for "git stripspace --count-lines" would test
strbuf_count_lines() which would also help when porting git rebase -i
to C.

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Eric Sunshine]

On Mon, Oct 19, 2015 at 1:03 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser <tklauser@distanz.ch> wrote:
>> On 2015-10-18 at 19:18:53 +0200, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> > Is there any application beyond git-rebase--interactive where a
>>> > --count-lines options is expected to be useful? It's not obvious from
>>> > the commit message that this change is necessarily a win for later
>>> > porting of git-rebase--interactive to C since the amount of extra code
>>> > and support material added by this patch probably outweighs the amount
>>> > of code a C version of git-rebase--interactive would need to count the
>>> > lines itself.
>>> >
>>> > Stated differently, are the two or three instances of piping through
>>> > 'wc' in git-rebase--interactive sufficient justification for
>>> > introducing extra complexity into git-stripspace and its documentation
>>> > and tests?
>>>
>>> Interesting thought.  When somebody rewrites "rebase -i" in C,
>>> nobody needs to count lines in "stripspace" output.  The rewritten
>>> "rebase -i" would internally run strbuf_stripspace() and the question
>>> becomes what is the best way to let that code find out how many lines
>>> the result contains.
>>>
>>> When viewed from that angle, I agree that "stripspace --count" does
>>> not add anything to further the goal of helping "rebase -i" to move
>>> to C.  Adding strbuf_count_lines() that counts the number of lines
>>> in the given strbuf (if there is no such helper yet; I didn't check),
>>> though.
>>
>> I check before implementing this series and didn't find any helper. I
>> also didn't find any other uses of line counting in the code.
>
> This shows that implementing "git stripspace --count-lines" could
> indirectly help porting "git rebase -i" to C as you could implement
> strbuf_count_lines() for the former and it could then be reused in the
> latter.

In this project, where all user-facing functionality must be supported
for the life of the project, each new command, command-line option,
configuration setting, and environment variable exacts additional
costs beyond the initial implementation cost. With this in mind, my
question was also indirectly asking whether there was sufficient
justification of the long-term cost of a --count-lines option. The
argument that --count-lines would help test a proposed
strbuf_count_lines() likely does not outweigh that cost.

>>> >> +test_expect_success '--count-lines with newline only' '
>>> >> +       printf "0\n" >expect &&
>>> >> +       printf "\n" | git stripspace --count-lines >actual &&
>>> >> +       test_cmp expect actual
>>> >> +'
>>> >
>>> > What is the expected behavior when the input is an empty file, a file
>>> > with content but no newline, a file with one or more lines but lacking
>>> > a newline on the final line? Should these cases be tested, as well?
>>>
>>> Good point here, too.  If we were to add strbuf_count_lines()
>>> helper, whoever adds that function needs to take a possible
>>> incomplete line at the end into account.
>>
>> Yes, makes more sense like this (even though it doesn't correspond to
>> what 'wc -l' does).
>
> Tests for "git stripspace --count-lines" would test
> strbuf_count_lines() which would also help when porting git rebase -i
> to C.

Rather than saddling the project with the cost of a new user-facing,
but otherwise unneeded option, a more direct way to test the proposed
strbuf_count_lines() would be to add a test-strbuf program, akin to
test-config, test-string-list, etc. This has the added benefit of
providing a home for strbuf-based tests beyond line counting.

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> With this in mind, my
> question was also indirectly asking whether there was sufficient
> justification of the long-term cost of a --count-lines option. The
> argument that --count-lines would help test a proposed
> strbuf_count_lines() likely does not outweigh that cost.

I agree. If we expect users to call --count-lines outside
rebase-interactive.sh and our own tests, then the actual use should be
justified in the commit message.

If not, then at least the --count-lines option should be hidden and not
documented in the public doc. But I agree that introducing test-strbuf
would be even better.

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

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Tobias Klauser]

On 2015-10-17 at 23:24:13 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > On 2015-10-16 at 19:29:35 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >> 
> >> >> -	if (mode == INVAL)
> >> >> -		usage(usage_msg);
> >> >
> >> > When given "git stripspace -s blorg", we used to set mode to INVAL
> >> > and then showed the correct usage.  But we no longer have a check
> >> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> >> > to detect presence of an otherwise ignored non-option argument
> >> > immediately after parse_options() returns?
> >> 
> >> Perhaps like this.
> >
> > Thanks. I'll fold it into v3.
> 
> Before starting v3, please fetch from me and check what is queued on
> 'pu'.  It may turn out that the fix-ups I did while queuing this
> round is sufficient, in which case you can just say that instead ;-)

Now that patches 3 and 4 will be dropped and no changes being necessary
for patches 1 and 2 (except for your changes that I see are already
folded into 'pu'), do you want me to submit a v3 of the series? Or is it
enough if I ask you to drop patches 3 (stripspace: implement
--count-lines option) and 4 (rebase -i: use "stripspace --count-lines"
when counting todo items)?

Thanks

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> On 2015-10-17 at 23:24:13 +0200, Junio C Hamano <gitster@pobox.com> wrote:
>> Before starting v3, please fetch from me and check what is queued on
>> 'pu'.  It may turn out that the fix-ups I did while queuing this
>> round is sufficient, in which case you can just say that instead ;-)
>
> Now that patches 3 and 4 will be dropped and no changes being necessary
> for patches 1 and 2 (except for your changes that I see are already
> folded into 'pu'), do you want me to submit a v3 of the series? Or is it
> enough if I ask you to drop patches 3 (stripspace: implement
> --count-lines option) and 4 (rebase -i: use "stripspace --count-lines"
> when counting todo items)?

Yes, the latter is sufficient and preferrable (less work for both of
us, without losing anything to help other people to review and
discuss who may want to chime in).

Thanks.