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

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

[Tobias Klauser]

This patch set implements some of the project ideas around git stripspace
suggested on [1].

[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas

The first patch moves the stripspace() function to the
strbuf module (adding a prefix and changing all users accordingly, also
a wrapper is introduced in case any topic branches still depend on the
old name). The second patch introduces option --count-lines to git
stripspace and also adds documentation and tests accordingly, Finally,
the third patch changes git-rebase--interactive.sh to replace commands
like:

	git stripspace ... | wc -l

with:

	git stripspace --count-lines ...

Tobias Klauser (3):
  strbuf: make stripspace() part of strbuf
  stripspace: Implement --count-lines option
  git rebase -i: Use newly added --count-lines option for stripspace

 Documentation/git-stripspace.txt |  13 ++++-
 builtin/am.c                     |   2 +-
 builtin/branch.c                 |   2 +-
 builtin/commit.c                 |   6 +-
 builtin/merge.c                  |   2 +-
 builtin/notes.c                  |   6 +-
 builtin/stripspace.c             | 122 ++++++++++-----------------------------
 builtin/tag.c                    |   2 +-
 git-rebase--interactive.sh       |   6 +-
 strbuf.c                         |  72 +++++++++++++++++++++++
 strbuf.h                         |  14 ++++-
 t/t0030-stripspace.sh            |  36 ++++++++++++
 12 files changed, 177 insertions(+), 106 deletions(-)

-- 
2.6.1.145.gb27dacc

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

[Tobias Klauser]

Rename stripspace() to strbuf_stripspace() and move it to the strbuf
module as suggested in [1].

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

[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 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 4f77e07..fbe9152 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 3ba4d1b..3f48746 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -794,7 +794,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.145.gb27dacc

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

[Tobias Klauser]

As suggested in the small project ideas [1], implement a --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 also make it easier to port git-rebase--interactive.sh to C
later on.

Furthermore, add the corresponding documentation and tests.

[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 Documentation/git-stripspace.txt | 13 ++++++++-
 builtin/stripspace.c             | 57 ++++++++++++++++++++++------------------
 strbuf.c                         | 12 ++++++---
 strbuf.h                         |  5 ++--
 t/t0030-stripspace.sh            | 36 +++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 31 deletions(-)

diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index 60328d5..411e17c 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
 SYNOPSIS
 --------
 [verse]
-'git stripspace' [-s | --strip-comments] < input
+'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
 'git stripspace' [-c | --comment-lines] < input
 
 DESCRIPTION
@@ -44,6 +44,11 @@ OPTIONS
 	be terminated with a newline. On empty lines, only the comment character
 	will be prepended.
 
+-C::
+--count-lines::
+	Output the number of resulting lines after stripping. This is equivalent
+	to calling 'git stripspace | wc -l'.
+
 EXAMPLES
 --------
 
@@ -88,6 +93,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 f677093..7882edd 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,45 +13,51 @@ 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 usage_msg[] = {
+	N_("git stripspace [-s | --strip-comments] [-C | --count-lines] < input"),
+	N_("git stripspace [-c | --comment-lines] < input"),
+	NULL
+};
 
 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;
-	}
+	int comment_mode = 0, count_lines = 0, strip_comments = 0;
+	size_t lines = 0;
+
+	const struct option options[] = {
+		OPT_BOOL('s', "strip-comments", &strip_comments,
+			 N_("skip and remove all lines starting with comment character")),
+		OPT_BOOL('c', "comment-lines", &comment_mode,
+			 N_("prepend comment character and blank to each line")),
+		OPT_BOOL('C', "count-lines", &count_lines, N_("print line count")),
+		OPT_END()
+	};
 
-	if (mode == INVAL)
-		usage(usage_msg);
+	argc = parse_options(argc, argv, prefix, options, usage_msg,
+			     PARSE_OPT_KEEP_DASHDASH);
 
-	if (strip_comments || mode == COMMENT_LINES)
+	if (comment_mode && (count_lines || strip_comments))
+		usage_with_options(usage_msg, options);
+
+	if (strip_comments || comment_mode)
 		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 (!comment_mode)
+		lines = strbuf_stripspace(&buf, strip_comments);
 	else
 		comment_lines(&buf);
 
-	write_or_die(1, buf.buf, buf.len);
+	if (!count_lines)
+		write_or_die(1, buf.buf, buf.len);
+	else {
+		struct strbuf tmp = STRBUF_INIT;
+		strbuf_addf(&tmp, "%zu\n", lines);
+		write_or_die(1, tmp.buf, tmp.len);
+	}
 	strbuf_release(&buf);
 	return 0;
 }
diff --git a/strbuf.c b/strbuf.c
index 9583875..45ea933 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -775,11 +775,13 @@ static size_t cleanup(char *line, size_t len)
  *
  * Enable skip_comments to skip every line starting with comment
  * character.
+ *
+ * Returns the number of lines in the resulting strbuf.
  */
-void strbuf_stripspace(struct strbuf *sb, int skip_comments)
+size_t strbuf_stripspace(struct strbuf *sb, int skip_comments)
 {
 	int empties = 0;
-	size_t i, j, len, newlen;
+	size_t i, j, len, newlen, lines = 0;
 	char *eol;
 
 	/* We may have to add a newline. */
@@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
 
 		/* Not just an empty line? */
 		if (newlen) {
-			if (empties > 0 && j > 0)
+			if (empties > 0 && j > 0) {
 				sb->buf[j++] = '\n';
+				lines++;
+			}
 			empties = 0;
 			memmove(sb->buf + j, sb->buf + i, newlen);
 			sb->buf[newlen + j++] = '\n';
+			lines++;
 		} else {
 			empties++;
 		}
 	}
 
 	strbuf_setlen(sb, j);
+	return lines;
 }
diff --git a/strbuf.h b/strbuf.h
index 5397d91..7d33f39 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -416,9 +416,10 @@ 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.
+ * comments are considered contents to be removed or not. Returns the
+ * number of lines in the resulting buffer.
  */
-extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);
+extern size_t strbuf_stripspace(struct strbuf *buf, int skip_comments);
 
 /**
  * Temporary alias until all topic branches have switched to use
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..5471a5a 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 -C >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line' '
+	printf "1\n" >expect &&
+	printf "foo\n" | git stripspace -C >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 -C >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 -C >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 -C >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 -C >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.1.145.gb27dacc

[PATCH 3/3] 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>
---
 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 f01637b..c40ca90 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
@@ -1247,7 +1247,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.145.gb27dacc

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

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

Since you have this temporary wrapper, it would have made sense to split
the patch into 1) move and rename the function, and 2) change the
callsites to strbuf_stripspace. This way 2) would be absolutely trivial
to review.

OTOH, this patch is already easy to review, so you may consider it's OK
like this.

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

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

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

> [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

I don't think we want such references in the commit message. It does
make sense in a "below ---" comment, but commit messages are here to
stay forever, while the SmallProjectsIdeas entries are meant to be
deleted when they are completed.

(Same in other patches)

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

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

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

> --- a/Documentation/git-stripspace.txt
> +++ b/Documentation/git-stripspace.txt
> @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
>  SYNOPSIS
>  --------
>  [verse]
> -'git stripspace' [-s | --strip-comments] < input
> +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input

I'm not sure it's a good idea to introduce a one-letter shortcut (-C).
In scripts, --count-lines is self-explanatory hence more readable than
-C (which is even more confusing since "git -C foo stripspace" and "git
stripspace -C" have totally different meanings. Outside scripts, I'm not
sure the command would be useful. I'd rather avoid polluting the
one-letter-option namespace.

> +Use 'git stripspace --count-lines' to obtain:
> +
> +---------
> +|5$
> +---------

In the examples above, I read the | as part of the input (unlike $ which
is used only to show the end of line). So the | should not be here. I
don't think you need the $ either, the --count-lines option is no longer
about trailing whitespaces.

> +static const char * const usage_msg[] = {

Stick the * to usage_msg please.

No time to review the rest for now sorry.

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

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> Rename stripspace() to strbuf_stripspace() and move it to the strbuf
> module as suggested in [1].
>
> Also switch all current users of stripspace() to the new function name
> and  keep a temporary wrapper inline function for topic branches still
> using stripspace().
>
> [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

I think Matthieu already mentioned this, but please explain why it
is a good idea to do this in your own words here, without forcing
readers to go to other places to find out.  Giving credit to an
external page for giving you an inspiration to do something is good,
but the proposed log message needs to justify itself.  In other
words, when you are challenged to defend this change, you are not
allowed to say "SmallProjectIdeas page said it is a good thing to
do.  I just did it without questioning it." ;-)  Instead you are
expected to justify it yourself.

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

A good rule of thumb to see if it is a good time to do this kind of
change is to do this:

	$ git log -p maint..pu | grep stripspace

which shows only one mention in a context, so this change may cause
textual conflict with something else somewhere nearby but won't hurt
other topics in flight.

The patch itself looks OK.

Thanks.

>  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 4f77e07..fbe9152 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 3ba4d1b..3f48746 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -794,7 +794,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)
>  {

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

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
> index 60328d5..411e17c 100644
> --- a/Documentation/git-stripspace.txt
> +++ b/Documentation/git-stripspace.txt
> @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
>  SYNOPSIS
>  --------
>  [verse]
> -'git stripspace' [-s | --strip-comments] < input
> +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
>  'git stripspace' [-c | --comment-lines] < input
>  
>  DESCRIPTION
> @@ -44,6 +44,11 @@ OPTIONS
>  	be terminated with a newline. On empty lines, only the comment character
>  	will be prepended.
>  
> +-C::
> +--count-lines::
> +	Output the number of resulting lines after stripping. This is equivalent
> +	to calling 'git stripspace | wc -l'.

I agree with Matthieu that squatting on a short-and-sweet -C is
unwanted here.

> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index f677093..7882edd 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,45 +13,51 @@ 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 usage_msg[] = {
> +	N_("git stripspace [-s | --strip-comments] [-C | --count-lines] < input"),
> +	N_("git stripspace [-c | --comment-lines] < input"),
> +	NULL
> +};
>  
>  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;
> -	}
> +	int comment_mode = 0, count_lines = 0, strip_comments = 0;
> +	size_t lines = 0;
> +
> +	const struct option options[] = {
> +		OPT_BOOL('s', "strip-comments", &strip_comments,
> +			 N_("skip and remove all lines starting with comment character")),
> +		OPT_BOOL('c', "comment-lines", &comment_mode,
> +			 N_("prepend comment character and blank to each line")),
> +		OPT_BOOL('C', "count-lines", &count_lines, N_("print line count")),
> +		OPT_END()
> +	};

I think -s and -c are incompatible, so OPT_CMDMODE() might be a
better fit for them if you are going to switch the command line
parser to use parse-options.

Which makes me strongly suspect that this should be done in at least
two separate steps.  (1) Use parse-options to parse command line
without adding the counting at all, followed by (2) Add counting.

> +	if (!count_lines)
> +		write_or_die(1, buf.buf, buf.len);
> +	else {
> +		struct strbuf tmp = STRBUF_INIT;
> +		strbuf_addf(&tmp, "%zu\n", lines);
> +		write_or_die(1, tmp.buf, tmp.len);
> +	}

So this is your output code, which gives only the number of lines
without the cleaned up result.

> @@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
>  
>  		/* Not just an empty line? */
>  		if (newlen) {
> -			if (empties > 0 && j > 0)
> +			if (empties > 0 && j > 0) {
>  				sb->buf[j++] = '\n';
> +				lines++;
> +			}
>  			empties = 0;
>  			memmove(sb->buf + j, sb->buf + i, newlen);
>  			sb->buf[newlen + j++] = '\n';
> +			lines++;
>  		} else {
>  			empties++;
>  		}
>  	}

I cannot say that the above is one of the better possible
implementations of this feature I would think of.

 (1) If this is performance sensitive, then you do not want to do
     memmove() etc. to actually touch the contents of the *sb and
     only increment lines++, because you are not going to show that
     in the output anyway.

 (2) If this feature is not performance sensitive, then the best
     implementation would be not to touch strbuf_stripspace() at all
     to realize this change, and count the number of '\n' in the
     cmd_stripspace() just before you use printf("%d\n", lines).
     That is best from maintainability's point-of-view, because it
     makes it infinitely less error-prone for future changes of
     strbuf_stripspace() to forget incrementing lines++ when it adds
     a newline to the output.

 (3) If you are going to still munge *sb, even if you are not going
     to show it at the end, perhaps because that would make it
     easier to keep track of where the code is scanning to find when
     you need to increment lines++, then at least the patch should
     devise a mechanism to make it less likely that the future
     changes to strbuf_stripspace() would make 'lines' and the
     number of '\n' in the *sb out-of-sync (hint: perhaps a macro
     called APPEND_LF(sb, line) or something).  It is easy to append
     '\n' to sb->buf without incrementing lines++ as the code stands
     with this patch applied---the patch makes the code less
     maintainable.

My gut feeling is that you should do (2), which is the cleanest from
the maintainability's point-of-view.

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

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

> + * comments are considered contents to be removed or not. Returns the
> + * number of lines in the resulting buffer.

We write comments at imperative tone, hence "Return", not "Returns".

Other than that, I agree with Junio:

* A preparatory patch introducing parse-options would make the actual
  patch much easier to review.

* Just running strbuf_stripspace and counting the number of lines in the
  result is much simpler. We use stripspace only on user-provided input
  which are never really big so maintainability is more important than
  performance.

Cheers,

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

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

[Tobias Klauser]

Thanks for the review.

On 2015-10-15 at 19:36:17 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > Rename stripspace() to strbuf_stripspace() and move it to the strbuf
> > module as suggested in [1].
> >
> > Also switch all current users of stripspace() to the new function name
> > and  keep a temporary wrapper inline function for topic branches still
> > using stripspace().
> >
> > [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf
> 
> I think Matthieu already mentioned this, but please explain why it
> is a good idea to do this in your own words here, without forcing
> readers to go to other places to find out.  Giving credit to an
> external page for giving you an inspiration to do something is good,
> but the proposed log message needs to justify itself.  In other
> words, when you are challenged to defend this change, you are not
> allowed to say "SmallProjectIdeas page said it is a good thing to
> do.  I just did it without questioning it." ;-)  Instead you are
> expected to justify it yourself.

Yes, makes sense. I'll adjust the commit message for v2 to justify the
change for itself and move the link below --- as Matthieu suggested.

> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> 
> A good rule of thumb to see if it is a good time to do this kind of
> change is to do this:
> 
> 	$ git log -p maint..pu | grep stripspace
> 
> which shows only one mention in a context, so this change may cause
> textual conflict with something else somewhere nearby but won't hurt
> other topics in flight.

Ok, thanks for the hint. I'll check again before resubmitting v2.

Thanks

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

[Tobias Klauser]

On 2015-10-15 at 18:42:23 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > Also switch all current users of stripspace() to the new function name
> > and  keep a temporary wrapper inline function for topic branches still
> > using stripspace().
> 
> Since you have this temporary wrapper, it would have made sense to split
> the patch into 1) move and rename the function, and 2) change the
> callsites to strbuf_stripspace. This way 2) would be absolutely trivial
> to review.
> 
> OTOH, this patch is already easy to review, so you may consider it's OK
> like this.

Ok, in this case will keep the patch as is for v2, but with the adjusted
commit message as indicated in your and Junio's review.

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

Will add it to v2, thanks!

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

[Tobias Klauser]

On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > --- a/Documentation/git-stripspace.txt
> > +++ b/Documentation/git-stripspace.txt
> > @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git stripspace' [-s | --strip-comments] < input
> > +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
> 
> I'm not sure it's a good idea to introduce a one-letter shortcut (-C).
> In scripts, --count-lines is self-explanatory hence more readable than
> -C (which is even more confusing since "git -C foo stripspace" and "git
> stripspace -C" have totally different meanings. Outside scripts, I'm not
> sure the command would be useful. I'd rather avoid polluting the
> one-letter-option namespace.

Ok, I'll drop the -C. Didn't consider the `git -C stripspace' case, so
that's definitely unwanted.

> > +Use 'git stripspace --count-lines' to obtain:
> > +
> > +---------
> > +|5$
> > +---------
> 
> In the examples above, I read the | as part of the input (unlike $ which
> is used only to show the end of line). So the | should not be here. I
> don't think you need the $ either, the --count-lines option is no longer
> about trailing whitespaces.

Will drop both | and $. Seems I didn't check the output careful enough,
both don't make sense for this option.

> > +static const char * const usage_msg[] = {
> 
> Stick the * to usage_msg please.

Will change in v2.

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

[Tobias Klauser]

On 2015-10-15 at 19:58:26 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
> > index 60328d5..411e17c 100644
> > --- a/Documentation/git-stripspace.txt
> > +++ b/Documentation/git-stripspace.txt
> > @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git stripspace' [-s | --strip-comments] < input
> > +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
> >  'git stripspace' [-c | --comment-lines] < input
> >  
> >  DESCRIPTION
> > @@ -44,6 +44,11 @@ OPTIONS
> >  	be terminated with a newline. On empty lines, only the comment character
> >  	will be prepended.
> >  
> > +-C::
> > +--count-lines::
> > +	Output the number of resulting lines after stripping. This is equivalent
> > +	to calling 'git stripspace | wc -l'.
> 
> I agree with Matthieu that squatting on a short-and-sweet -C is
> unwanted here.

Will drop it in v2.

> > diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> > index f677093..7882edd 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,45 +13,51 @@ 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 usage_msg[] = {
> > +	N_("git stripspace [-s | --strip-comments] [-C | --count-lines] < input"),
> > +	N_("git stripspace [-c | --comment-lines] < input"),
> > +	NULL
> > +};
> >  
> >  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;
> > -	}
> > +	int comment_mode = 0, count_lines = 0, strip_comments = 0;
> > +	size_t lines = 0;
> > +
> > +	const struct option options[] = {
> > +		OPT_BOOL('s', "strip-comments", &strip_comments,
> > +			 N_("skip and remove all lines starting with comment character")),
> > +		OPT_BOOL('c', "comment-lines", &comment_mode,
> > +			 N_("prepend comment character and blank to each line")),
> > +		OPT_BOOL('C', "count-lines", &count_lines, N_("print line count")),
> > +		OPT_END()
> > +	};
> 
> I think -s and -c are incompatible, so OPT_CMDMODE() might be a
> better fit for them if you are going to switch the command line
> parser to use parse-options.

Ok, will change to OPT_CMDMODE()

> Which makes me strongly suspect that this should be done in at least
> two separate steps.  (1) Use parse-options to parse command line
> without adding the counting at all, followed by (2) Add counting.

Ok, will split the patch as you suggest for v2 (or more if it makes it
easier to review).

> > +	if (!count_lines)
> > +		write_or_die(1, buf.buf, buf.len);
> > +	else {
> > +		struct strbuf tmp = STRBUF_INIT;
> > +		strbuf_addf(&tmp, "%zu\n", lines);
> > +		write_or_die(1, tmp.buf, tmp.len);
> > +	}
> 
> So this is your output code, which gives only the number of lines
> without the cleaned up result.

This should better be a simple printf("%zu\n", lines) I guess?

> > @@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
> >  
> >  		/* Not just an empty line? */
> >  		if (newlen) {
> > -			if (empties > 0 && j > 0)
> > +			if (empties > 0 && j > 0) {
> >  				sb->buf[j++] = '\n';
> > +				lines++;
> > +			}
> >  			empties = 0;
> >  			memmove(sb->buf + j, sb->buf + i, newlen);
> >  			sb->buf[newlen + j++] = '\n';
> > +			lines++;
> >  		} else {
> >  			empties++;
> >  		}
> >  	}
> 
> I cannot say that the above is one of the better possible
> implementations of this feature I would think of.
> 
>  (1) If this is performance sensitive, then you do not want to do
>      memmove() etc. to actually touch the contents of the *sb and
>      only increment lines++, because you are not going to show that
>      in the output anyway.
> 
>  (2) If this feature is not performance sensitive, then the best
>      implementation would be not to touch strbuf_stripspace() at all
>      to realize this change, and count the number of '\n' in the
>      cmd_stripspace() just before you use printf("%d\n", lines).
>      That is best from maintainability's point-of-view, because it
>      makes it infinitely less error-prone for future changes of
>      strbuf_stripspace() to forget incrementing lines++ when it adds
>      a newline to the output.
> 
>  (3) If you are going to still munge *sb, even if you are not going
>      to show it at the end, perhaps because that would make it
>      easier to keep track of where the code is scanning to find when
>      you need to increment lines++, then at least the patch should
>      devise a mechanism to make it less likely that the future
>      changes to strbuf_stripspace() would make 'lines' and the
>      number of '\n' in the *sb out-of-sync (hint: perhaps a macro
>      called APPEND_LF(sb, line) or something).  It is easy to append
>      '\n' to sb->buf without incrementing lines++ as the code stands
>      with this patch applied---the patch makes the code less
>      maintainable.
> 
> My gut feeling is that you should do (2), which is the cleanest from
> the maintainability's point-of-view.

Thank you for outlining and assessing these possible implementations. I
agree that my suggested implementation is probably the most naïve one :)
and think that (2) would less intrusive and better to maintain. Will
change the patch accordingly.
> 

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

[Tobias Klauser]

On 2015-10-15 at 21:21:53 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > + * comments are considered contents to be removed or not. Returns the
> > + * number of lines in the resulting buffer.
> 
> We write comments at imperative tone, hence "Return", not "Returns".

The other comments in strbuf.h used "Returns", so I went for it for
consistency reasons. But the comment will be obsolete anyhow, as
strbuf_stripspace() will not be changed and the functionality
implemented in cmd_stripspace() as Junio suggested.

> Other than that, I agree with Junio:
> 
> * A preparatory patch introducing parse-options would make the actual
>   patch much easier to review.

Will do.

> 
> * Just running strbuf_stripspace and counting the number of lines in the
>   result is much simpler. We use stripspace only on user-provided input
>   which are never really big so maintainability is more important than
>   performance.

Ditto.

Thanks

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

[Tobias Klauser]

On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> > +static const char * const usage_msg[] = {
> 
> Stick the * to usage_msg please.

Just noticed while looking at how other sub-commands define this, the vast
majority use "const char * const" and not "const char const *".

Also it would change the meaning. The following wouldn't produce a
compiler warning:

	static const char const *usage_msg[] = { ... };
	...
	usage_msg[0] = "Foobar"

while

	static const char * const usage_msg[] = { ... };
	...
	usage_msg[0] = "Foobar"

would produce one:

	builtin/stripspace.c:36:2: error: assignment of read-only location ‘usage_msg[0]’

Even though I don't expect anyone to modify usage_msg at runtime I think
it'd be better to stick with the current version.

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

[Matthieu Moy]

Tobias Klauser <tklauser@distanz.ch> writes:

> On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Tobias Klauser <tklauser@distanz.ch> writes:
>> > +static const char * const usage_msg[] = {
>> 
>> Stick the * to usage_msg please.
>
> Just noticed while looking at how other sub-commands define this, the vast
> majority use "const char * const" and not "const char const *".

Oops, I read your code too quickly. We stick the * to variable names
when it follows the star, but I didn't see the "const", sorry.

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

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

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

>> So this is your output code, which gives only the number of lines
>> without the cleaned up result.
>
> This should better be a simple printf("%zu\n", lines) I guess?

I think we actively avoid using %z conversion that is only C99.

If you really want to, you could count in size_t and use %lu with
appropriate casting, which I think is what we do in the rest of the
codebase.

For this one, I think it is sufficient to just count in int and
print as int with %d, though.

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

[Tobias Klauser]

On 2015-10-16 at 19:35:49 +0200, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> >> So this is your output code, which gives only the number of lines
> >> without the cleaned up result.
> >
> > This should better be a simple printf("%zu\n", lines) I guess?
> 
> I think we actively avoid using %z conversion that is only C99.
> 
> If you really want to, you could count in size_t and use %lu with
> appropriate casting, which I think is what we do in the rest of the
> codebase.
> 
> For this one, I think it is sufficient to just count in int and
> print as int with %d, though.

Ok, will use an int to count and printf("%d\n").