{"thread":{"id":"40567","subject":"[PATCH 3/3] git rebase -i: Use newly added --count-lines option for stripspace","startedAt":"2015-10-15T12:18:41Z","lastAt":"2015-10-17T10:44:25Z","messageCount":19,"participants":["Tobias Klauser","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"271731","messageId":"1444911524-14504-1-git-send-email-tklauser@distanz.ch","threadId":"40567","inReplyTo":null,"subject":"[PATCH 0/3] stripspace: Implement and use --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-15T12:18:41Z","receivedAt":"2015-10-15T12:18:41Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"This patch set implements some of the project ideas around git stripspace\nsuggested on [1].\n\n[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas\n\nThe first patch moves the stripspace() function to the\nstrbuf module (adding a prefix and changing all users accordingly, also\na wrapper is introduced in case any topic branches still depend on the\nold name). The second patch introduces option --count-lines to git\nstripspace and also adds documentation and tests accordingly, Finally,\nthe third patch changes git-rebase--interactive.sh to replace commands\nlike:\n\n\tgit stripspace ... | wc -l\n\nwith:\n\n\tgit stripspace --count-lines ...\n\nTobias Klauser (3):\n  strbuf: make stripspace() part of strbuf\n  stripspace: Implement --count-lines option\n  git rebase -i: Use newly added --count-lines option for stripspace\n\n Documentation/git-stripspace.txt |  13 ++++-\n builtin/am.c                     |   2 +-\n builtin/branch.c                 |   2 +-\n builtin/commit.c                 |   6 +-\n builtin/merge.c                  |   2 +-\n builtin/notes.c                  |   6 +-\n builtin/stripspace.c             | 122 ++++++++++-----------------------------\n builtin/tag.c                    |   2 +-\n git-rebase--interactive.sh       |   6 +-\n strbuf.c                         |  72 +++++++++++++++++++++++\n strbuf.h                         |  14 ++++-\n t/t0030-stripspace.sh            |  36 ++++++++++++\n 12 files changed, 177 insertions(+), 106 deletions(-)\n\n-- \n2.6.1.145.gb27dacc\n"},{"id":"271730","messageId":"1444911524-14504-2-git-send-email-tklauser@distanz.ch","threadId":"40567","inReplyTo":"1444911524-14504-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-15T12:18:42Z","receivedAt":"2015-10-15T12:18:42Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Rename stripspace() to strbuf_stripspace() and move it to the strbuf\nmodule as suggested in [1].\n\nAlso switch all current users of stripspace() to the new function name\nand  keep a temporary wrapper inline function for topic branches still\nusing stripspace().\n\n[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\n builtin/am.c         |  2 +-\n builtin/branch.c     |  2 +-\n builtin/commit.c     |  6 ++---\n builtin/merge.c      |  2 +-\n builtin/notes.c      |  6 ++---\n builtin/stripspace.c | 69 ++--------------------------------------------------\n builtin/tag.c        |  2 +-\n strbuf.c             | 66 +++++++++++++++++++++++++++++++++++++++++++++++++\n strbuf.h             | 11 ++++++++-\n 9 files changed, 88 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4f77e07..fbe9152 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1343,7 +1343,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstrbuf_addstr(&msg, \"\\n\\n\");\n \tif (strbuf_read_file(&msg, am_path(state, \"msg\"), 0) < 0)\n \t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n-\tstripspace(&msg, 0);\n+\tstrbuf_stripspace(&msg, 0);\n \n \tif (state->signoff)\n \t\tam_signoff(&msg);\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3ba4d1b..3f48746 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -794,7 +794,7 @@ static int edit_branch_description(const char *branch_name)\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n \t}\n-\tstripspace(&buf, 1);\n+\tstrbuf_stripspace(&buf, 1);\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n \tstatus = git_config_set(name.buf, buf.len ? buf.buf : NULL);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 63772d0..dca09e2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -775,7 +775,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \ts->hints = 0;\n \n \tif (clean_message_contents)\n-\t\tstripspace(&sb, 0);\n+\t\tstrbuf_stripspace(&sb, 0);\n \n \tif (signoff)\n \t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n@@ -1014,7 +1014,7 @@ static int template_untouched(struct strbuf *sb)\n \tif (!template_file || strbuf_read_file(&tmpl, template_file, 0) <= 0)\n \t\treturn 0;\n \n-\tstripspace(&tmpl, cleanup_mode == CLEANUP_ALL);\n+\tstrbuf_stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);\n \tif (!skip_prefix(sb->buf, tmpl.buf, &start))\n \t\tstart = sb->buf;\n \tstrbuf_release(&tmpl);\n@@ -1726,7 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\twt_status_truncate_message_at_cut_line(&sb);\n \n \tif (cleanup_mode != CLEANUP_NONE)\n-\t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n+\t\tstrbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);\n \tif (template_untouched(&sb) && !allow_empty_message) {\n \t\trollback_index_files();\n \t\tfprintf(stderr, _(\"Aborting commit; you did not edit the message.\\n\"));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a0edaca..e6741f3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -806,7 +806,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n \tread_merge_msg(&msg);\n-\tstripspace(&msg, 0 < option_edit);\n+\tstrbuf_stripspace(&msg, 0 < option_edit);\n \tif (!msg.len)\n \t\tabort_commit(remoteheads, _(\"Empty commit message.\"));\n \tstrbuf_release(&merge_msg);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 3608c64..bb23d55 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -192,7 +192,7 @@ static void prepare_note_data(const unsigned char *object, struct note_data *d,\n \t\tif (launch_editor(d->edit_path, &d->buf, NULL)) {\n \t\t\tdie(_(\"Please supply the note contents using either -m or -F option\"));\n \t\t}\n-\t\tstripspace(&d->buf, 1);\n+\t\tstrbuf_stripspace(&d->buf, 1);\n \t}\n }\n \n@@ -215,7 +215,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \tif (d->buf.len)\n \t\tstrbuf_addch(&d->buf, '\\n');\n \tstrbuf_addstr(&d->buf, arg);\n-\tstripspace(&d->buf, 0);\n+\tstrbuf_stripspace(&d->buf, 0);\n \n \td->given = 1;\n \treturn 0;\n@@ -232,7 +232,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n \t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n-\tstripspace(&d->buf, 0);\n+\tstrbuf_stripspace(&d->buf, 0);\n \n \td->given = 1;\n \treturn 0;\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1259ed7..f677093 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -1,71 +1,6 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n-\n-/*\n- * Returns the length of a line, without trailing spaces.\n- *\n- * If the line ends with newline, it will be removed too.\n- */\n-static size_t cleanup(char *line, size_t len)\n-{\n-\twhile (len) {\n-\t\tunsigned char c = line[len - 1];\n-\t\tif (!isspace(c))\n-\t\t\tbreak;\n-\t\tlen--;\n-\t}\n-\n-\treturn len;\n-}\n-\n-/*\n- * Remove empty lines from the beginning and end\n- * and also trailing spaces from every line.\n- *\n- * Turn multiple consecutive empty lines between paragraphs\n- * into just one empty line.\n- *\n- * If the input has only empty lines and spaces,\n- * no output will be produced.\n- *\n- * If last line does not have a newline at the end, one is added.\n- *\n- * Enable skip_comments to skip every line starting with comment\n- * character.\n- */\n-void stripspace(struct strbuf *sb, int skip_comments)\n-{\n-\tint empties = 0;\n-\tsize_t i, j, len, newlen;\n-\tchar *eol;\n-\n-\t/* We may have to add a newline. */\n-\tstrbuf_grow(sb, 1);\n-\n-\tfor (i = j = 0; i < sb->len; i += len, j += newlen) {\n-\t\teol = memchr(sb->buf + i, '\\n', sb->len - i);\n-\t\tlen = eol ? eol - (sb->buf + i) + 1 : sb->len - i;\n-\n-\t\tif (skip_comments && len && sb->buf[i] == comment_line_char) {\n-\t\t\tnewlen = 0;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tnewlen = cleanup(sb->buf + i, len);\n-\n-\t\t/* Not just an empty line? */\n-\t\tif (newlen) {\n-\t\t\tif (empties > 0 && j > 0)\n-\t\t\t\tsb->buf[j++] = '\\n';\n-\t\t\tempties = 0;\n-\t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n-\t\t\tsb->buf[newlen + j++] = '\\n';\n-\t\t} else {\n-\t\t\tempties++;\n-\t\t}\n-\t}\n-\n-\tstrbuf_setlen(sb, j);\n-}\n+#include \"strbuf.h\"\n \n static void comment_lines(struct strbuf *buf)\n {\n@@ -111,7 +46,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \t\tdie_errno(\"could not read the input\");\n \n \tif (mode == STRIP_SPACE)\n-\t\tstripspace(&buf, strip_comments);\n+\t\tstrbuf_stripspace(&buf, strip_comments);\n \telse\n \t\tcomment_lines(&buf);\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 9e17dca..5660787 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -268,7 +268,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t}\n \n \tif (opt->cleanup_mode != CLEANUP_NONE)\n-\t\tstripspace(buf, opt->cleanup_mode == CLEANUP_ALL);\n+\t\tstrbuf_stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);\n \n \tif (!opt->message_given && !buf->len)\n \t\tdie(_(\"no tag message?\"));\ndiff --git a/strbuf.c b/strbuf.c\nindex 29df55b..9583875 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -743,3 +743,69 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n \t}\n \tstrbuf_setlen(sb, sb->len + len);\n }\n+\n+/*\n+ * Returns the length of a line, without trailing spaces.\n+ *\n+ * If the line ends with newline, it will be removed too.\n+ */\n+static size_t cleanup(char *line, size_t len)\n+{\n+\twhile (len) {\n+\t\tunsigned char c = line[len - 1];\n+\t\tif (!isspace(c))\n+\t\t\tbreak;\n+\t\tlen--;\n+\t}\n+\n+\treturn len;\n+}\n+\n+/*\n+ * Remove empty lines from the beginning and end\n+ * and also trailing spaces from every line.\n+ *\n+ * Turn multiple consecutive empty lines between paragraphs\n+ * into just one empty line.\n+ *\n+ * If the input has only empty lines and spaces,\n+ * no output will be produced.\n+ *\n+ * If last line does not have a newline at the end, one is added.\n+ *\n+ * Enable skip_comments to skip every line starting with comment\n+ * character.\n+ */\n+void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n+{\n+\tint empties = 0;\n+\tsize_t i, j, len, newlen;\n+\tchar *eol;\n+\n+\t/* We may have to add a newline. */\n+\tstrbuf_grow(sb, 1);\n+\n+\tfor (i = j = 0; i < sb->len; i += len, j += newlen) {\n+\t\teol = memchr(sb->buf + i, '\\n', sb->len - i);\n+\t\tlen = eol ? eol - (sb->buf + i) + 1 : sb->len - i;\n+\n+\t\tif (skip_comments && len && sb->buf[i] == comment_line_char) {\n+\t\t\tnewlen = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tnewlen = cleanup(sb->buf + i, len);\n+\n+\t\t/* Not just an empty line? */\n+\t\tif (newlen) {\n+\t\t\tif (empties > 0 && j > 0)\n+\t\t\t\tsb->buf[j++] = '\\n';\n+\t\t\tempties = 0;\n+\t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n+\t\t\tsb->buf[newlen + j++] = '\\n';\n+\t\t} else {\n+\t\t\tempties++;\n+\t\t}\n+\t}\n+\n+\tstrbuf_setlen(sb, j);\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex aef2794..5397d91 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -418,7 +418,16 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n  * Strip whitespace from a buffer. The second parameter controls if\n  * comments are considered contents to be removed or not.\n  */\n-extern void stripspace(struct strbuf *buf, int skip_comments);\n+extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);\n+\n+/**\n+ * Temporary alias until all topic branches have switched to use\n+ * strbuf_stripspace directly.\n+ */\n+static inline void stripspace(struct strbuf *buf, int skip_comments)\n+{\n+\tstrbuf_stripspace(buf, skip_comments);\n+}\n \n static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n {\n-- \n2.6.1.145.gb27dacc\n"},{"id":"271732","messageId":"1444911524-14504-3-git-send-email-tklauser@distanz.ch","threadId":"40567","inReplyTo":"1444911524-14504-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-15T12:18:43Z","receivedAt":"2015-10-15T12:18:43Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"As suggested in the small project ideas [1], implement a --count-lines\noptions for git stripspace to be able to omit calling\n\n  git stripspace --strip-comments < infile | wc -l\n\ne.g. in git-rebase--interactive.sh. The above command can now be\nreplaced by:\n\n  git stripspace --strip-comments --count-lines < infile\n\nThis will also make it easier to port git-rebase--interactive.sh to C\nlater on.\n\nFurthermore, add the corresponding documentation and tests.\n\n[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\n Documentation/git-stripspace.txt | 13 ++++++++-\n builtin/stripspace.c             | 57 ++++++++++++++++++++++------------------\n strbuf.c                         | 12 ++++++---\n strbuf.h                         |  5 ++--\n t/t0030-stripspace.sh            | 36 +++++++++++++++++++++++++\n 5 files changed, 92 insertions(+), 31 deletions(-)\n\ndiff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt\nindex 60328d5..411e17c 100644\n--- a/Documentation/git-stripspace.txt\n+++ b/Documentation/git-stripspace.txt\n@@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace\n SYNOPSIS\n --------\n [verse]\n-'git stripspace' [-s | --strip-comments] < input\n+'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input\n 'git stripspace' [-c | --comment-lines] < input\n \n DESCRIPTION\n@@ -44,6 +44,11 @@ OPTIONS\n \tbe terminated with a newline. On empty lines, only the comment character\n \twill be prepended.\n \n+-C::\n+--count-lines::\n+\tOutput the number of resulting lines after stripping. This is equivalent\n+\tto calling 'git stripspace | wc -l'.\n+\n EXAMPLES\n --------\n \n@@ -88,6 +93,12 @@ Use 'git stripspace --strip-comments' to obtain:\n |The end.$\n ---------\n \n+Use 'git stripspace --count-lines' to obtain:\n+\n+---------\n+|5$\n+---------\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex f677093..7882edd 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n #include \"strbuf.h\"\n \n static void comment_lines(struct strbuf *buf)\n@@ -12,45 +13,51 @@ static void comment_lines(struct strbuf *buf)\n \tfree(msg);\n }\n \n-static const char *usage_msg = \"\\n\"\n-\"  git stripspace [-s | --strip-comments] < input\\n\"\n-\"  git stripspace [-c | --comment-lines] < input\";\n+static const char * const usage_msg[] = {\n+\tN_(\"git stripspace [-s | --strip-comments] [-C | --count-lines] < input\"),\n+\tN_(\"git stripspace [-c | --comment-lines] < input\"),\n+\tNULL\n+};\n \n int cmd_stripspace(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint strip_comments = 0;\n-\tenum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;\n-\n-\tif (argc == 2) {\n-\t\tif (!strcmp(argv[1], \"-s\") ||\n-\t\t    !strcmp(argv[1], \"--strip-comments\")) {\n-\t\t\tstrip_comments = 1;\n-\t\t} else if (!strcmp(argv[1], \"-c\") ||\n-\t\t\t   !strcmp(argv[1], \"--comment-lines\")) {\n-\t\t\tmode = COMMENT_LINES;\n-\t\t} else {\n-\t\t\tmode = INVAL;\n-\t\t}\n-\t} else if (argc > 1) {\n-\t\tmode = INVAL;\n-\t}\n+\tint comment_mode = 0, count_lines = 0, strip_comments = 0;\n+\tsize_t lines = 0;\n+\n+\tconst struct option options[] = {\n+\t\tOPT_BOOL('s', \"strip-comments\", &strip_comments,\n+\t\t\t N_(\"skip and remove all lines starting with comment character\")),\n+\t\tOPT_BOOL('c', \"comment-lines\", &comment_mode,\n+\t\t\t N_(\"prepend comment character and blank to each line\")),\n+\t\tOPT_BOOL('C', \"count-lines\", &count_lines, N_(\"print line count\")),\n+\t\tOPT_END()\n+\t};\n \n-\tif (mode == INVAL)\n-\t\tusage(usage_msg);\n+\targc = parse_options(argc, argv, prefix, options, usage_msg,\n+\t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n-\tif (strip_comments || mode == COMMENT_LINES)\n+\tif (comment_mode && (count_lines || strip_comments))\n+\t\tusage_with_options(usage_msg, options);\n+\n+\tif (strip_comments || comment_mode)\n \t\tgit_config(git_default_config, NULL);\n \n \tif (strbuf_read(&buf, 0, 1024) < 0)\n \t\tdie_errno(\"could not read the input\");\n \n-\tif (mode == STRIP_SPACE)\n-\t\tstrbuf_stripspace(&buf, strip_comments);\n+\tif (!comment_mode)\n+\t\tlines = strbuf_stripspace(&buf, strip_comments);\n \telse\n \t\tcomment_lines(&buf);\n \n-\twrite_or_die(1, buf.buf, buf.len);\n+\tif (!count_lines)\n+\t\twrite_or_die(1, buf.buf, buf.len);\n+\telse {\n+\t\tstruct strbuf tmp = STRBUF_INIT;\n+\t\tstrbuf_addf(&tmp, \"%zu\\n\", lines);\n+\t\twrite_or_die(1, tmp.buf, tmp.len);\n+\t}\n \tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/strbuf.c b/strbuf.c\nindex 9583875..45ea933 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -775,11 +775,13 @@ static size_t cleanup(char *line, size_t len)\n  *\n  * Enable skip_comments to skip every line starting with comment\n  * character.\n+ *\n+ * Returns the number of lines in the resulting strbuf.\n  */\n-void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n+size_t strbuf_stripspace(struct strbuf *sb, int skip_comments)\n {\n \tint empties = 0;\n-\tsize_t i, j, len, newlen;\n+\tsize_t i, j, len, newlen, lines = 0;\n \tchar *eol;\n \n \t/* We may have to add a newline. */\n@@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n \n \t\t/* Not just an empty line? */\n \t\tif (newlen) {\n-\t\t\tif (empties > 0 && j > 0)\n+\t\t\tif (empties > 0 && j > 0) {\n \t\t\t\tsb->buf[j++] = '\\n';\n+\t\t\t\tlines++;\n+\t\t\t}\n \t\t\tempties = 0;\n \t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n \t\t\tsb->buf[newlen + j++] = '\\n';\n+\t\t\tlines++;\n \t\t} else {\n \t\t\tempties++;\n \t\t}\n \t}\n \n \tstrbuf_setlen(sb, j);\n+\treturn lines;\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex 5397d91..7d33f39 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -416,9 +416,10 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n /**\n  * Strip whitespace from a buffer. The second parameter controls if\n- * comments are considered contents to be removed or not.\n+ * comments are considered contents to be removed or not. Returns the\n+ * number of lines in the resulting buffer.\n  */\n-extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);\n+extern size_t strbuf_stripspace(struct strbuf *buf, int skip_comments);\n \n /**\n  * Temporary alias until all topic branches have switched to use\ndiff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\nindex 29e91d8..5471a5a 100755\n--- a/t/t0030-stripspace.sh\n+++ b/t/t0030-stripspace.sh\n@@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented line' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--count-lines with newline only' '\n+\tprintf \"0\\n\" >expect &&\n+\tprintf \"\\n\" | git stripspace -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--count-lines with single line' '\n+\tprintf \"1\\n\" >expect &&\n+\tprintf \"foo\\n\" | git stripspace -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--count-lines with single line preceeded by empty line' '\n+\tprintf \"1\\n\" >expect &&\n+\tprintf \"\\nfoo\" | git stripspace -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--count-lines with single line followed by empty line' '\n+\tprintf \"1\\n\" >expect &&\n+\tprintf \"foo\\n\\n\" | git stripspace -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--count-lines with multiple lines and consecutive newlines' '\n+\tprintf \"5\\n\" >expect &&\n+\tprintf \"\\none\\n\\n\\nthree\\nfour\\nfive\\n\" | git stripspace -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--count-lines combined with --strip-comments' '\n+\tprintf \"5\\n\" >expect &&\n+\tprintf \"\\n# stripped\\none\\n#stripped\\n\\nthree\\nfour\\nfive\\n\" | git stripspace -s -C >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.1.145.gb27dacc\n"},{"id":"271729","messageId":"1444911524-14504-4-git-send-email-tklauser@distanz.ch","threadId":"40567","inReplyTo":"1444911524-14504-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH 3/3] git rebase -i: Use newly added --count-lines option for stripspace","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-15T12:18:44Z","receivedAt":"2015-10-15T12:18:44Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Use the newly added --count-lines option for 'git stripspace' to count\nlines instead of piping the entire output to 'wc -l'.\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\n git-rebase--interactive.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f01637b..c40ca90 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -120,9 +120,9 @@ mark_action_done () {\n \tsed -e 1q < \"$todo\" >> \"$done\"\n \tsed -e 1d < \"$todo\" >> \"$todo\".new\n \tmv -f \"$todo\".new \"$todo\"\n-\tnew_count=$(git stripspace --strip-comments <\"$done\" | wc -l)\n+\tnew_count=$(git stripspace --strip-comments --count-lines <\"$done\")\n \techo $new_count >\"$msgnum\"\n-\ttotal=$(($new_count + $(git stripspace --strip-comments <\"$todo\" | wc -l)))\n+\ttotal=$(($new_count + $(git stripspace --strip-comments --count-lines <\"$todo\")))\n \techo $total >\"$end\"\n \tif test \"$last_count\" != \"$new_count\"\n \tthen\n@@ -1247,7 +1247,7 @@ test -s \"$todo\" || echo noop >> \"$todo\"\n test -n \"$autosquash\" && rearrange_squash \"$todo\"\n test -n \"$cmd\" && add_exec_commands \"$todo\"\n \n-todocount=$(git stripspace --strip-comments <\"$todo\" | wc -l)\n+todocount=$(git stripspace --strip-comments --count-lines <\"$todo\")\n todocount=${todocount##* }\n \n cat >>\"$todo\" <<EOF\n-- \n2.6.1.145.gb27dacc\n"},{"id":"271738","messageId":"vpqio6813tc.fsf@grenoble-inp.fr","threadId":"40567","inReplyTo":"1444911524-14504-2-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T16:42:23Z","receivedAt":"2015-10-15T16:42:23Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> Also switch all current users of stripspace() to the new function name\n> and  keep a temporary wrapper inline function for topic branches still\n> using stripspace().\n\nSince you have this temporary wrapper, it would have made sense to split\nthe patch into 1) move and rename the function, and 2) change the\ncallsites to strbuf_stripspace. This way 2) would be absolutely trivial\nto review.\n\nOTOH, this patch is already easy to review, so you may consider it's OK\nlike this.\n\nReviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271739","messageId":"vpqeggw13r3.fsf@grenoble-inp.fr","threadId":"40567","inReplyTo":"1444911524-14504-2-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T16:43:44Z","receivedAt":"2015-10-15T16:43:44Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf\n\nI don't think we want such references in the commit message. It does\nmake sense in a \"below ---\" comment, but commit messages are here to\nstay forever, while the SmallProjectsIdeas entries are meant to be\ndeleted when they are completed.\n\n(Same in other patches)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271740","messageId":"vpqzizkysyh.fsf@grenoble-inp.fr","threadId":"40567","inReplyTo":"1444911524-14504-3-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T16:52:54Z","receivedAt":"2015-10-15T16:52:54Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> --- a/Documentation/git-stripspace.txt\n> +++ b/Documentation/git-stripspace.txt\n> @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git stripspace' [-s | --strip-comments] < input\n> +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input\n\nI'm not sure it's a good idea to introduce a one-letter shortcut (-C).\nIn scripts, --count-lines is self-explanatory hence more readable than\n-C (which is even more confusing since \"git -C foo stripspace\" and \"git\nstripspace -C\" have totally different meanings. Outside scripts, I'm not\nsure the command would be useful. I'd rather avoid polluting the\none-letter-option namespace.\n\n> +Use 'git stripspace --count-lines' to obtain:\n> +\n> +---------\n> +|5$\n> +---------\n\nIn the examples above, I read the | as part of the input (unlike $ which\nis used only to show the end of line). So the | should not be here. I\ndon't think you need the $ either, the --count-lines option is no longer\nabout trailing whitespaces.\n\n> +static const char * const usage_msg[] = {\n\nStick the * to usage_msg please.\n\nNo time to review the rest for now sorry.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271742","messageId":"xmqqsi5cghke.fsf@gitster.mtv.corp.google.com","threadId":"40567","inReplyTo":"1444911524-14504-2-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-15T17:36:17Z","receivedAt":"2015-10-15T17:36:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> Rename stripspace() to strbuf_stripspace() and move it to the strbuf\n> module as suggested in [1].\n>\n> Also switch all current users of stripspace() to the new function name\n> and  keep a temporary wrapper inline function for topic branches still\n> using stripspace().\n>\n> [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf\n\nI think Matthieu already mentioned this, but please explain why it\nis a good idea to do this in your own words here, without forcing\nreaders to go to other places to find out.  Giving credit to an\nexternal page for giving you an inspiration to do something is good,\nbut the proposed log message needs to justify itself.  In other\nwords, when you are challenged to defend this change, you are not\nallowed to say \"SmallProjectIdeas page said it is a good thing to\ndo.  I just did it without questioning it.\" ;-)  Instead you are\nexpected to justify it yourself.\n\n> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>\n> ---\n\nA good rule of thumb to see if it is a good time to do this kind of\nchange is to do this:\n\n\t$ git log -p maint..pu | grep stripspace\n\nwhich shows only one mention in a context, so this change may cause\ntextual conflict with something else somewhere nearby but won't hurt\nother topics in flight.\n\nThe patch itself looks OK.\n\nThanks.\n\n>  builtin/am.c         |  2 +-\n>  builtin/branch.c     |  2 +-\n>  builtin/commit.c     |  6 ++---\n>  builtin/merge.c      |  2 +-\n>  builtin/notes.c      |  6 ++---\n>  builtin/stripspace.c | 69 ++--------------------------------------------------\n>  builtin/tag.c        |  2 +-\n>  strbuf.c             | 66 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  strbuf.h             | 11 ++++++++-\n>  9 files changed, 88 insertions(+), 78 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 4f77e07..fbe9152 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1343,7 +1343,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n>  \tstrbuf_addstr(&msg, \"\\n\\n\");\n>  \tif (strbuf_read_file(&msg, am_path(state, \"msg\"), 0) < 0)\n>  \t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n> -\tstripspace(&msg, 0);\n> +\tstrbuf_stripspace(&msg, 0);\n>  \n>  \tif (state->signoff)\n>  \t\tam_signoff(&msg);\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 3ba4d1b..3f48746 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -794,7 +794,7 @@ static int edit_branch_description(const char *branch_name)\n>  \t\tstrbuf_release(&buf);\n>  \t\treturn -1;\n>  \t}\n> -\tstripspace(&buf, 1);\n> +\tstrbuf_stripspace(&buf, 1);\n>  \n>  \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n>  \tstatus = git_config_set(name.buf, buf.len ? buf.buf : NULL);\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 63772d0..dca09e2 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -775,7 +775,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \ts->hints = 0;\n>  \n>  \tif (clean_message_contents)\n> -\t\tstripspace(&sb, 0);\n> +\t\tstrbuf_stripspace(&sb, 0);\n>  \n>  \tif (signoff)\n>  \t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n> @@ -1014,7 +1014,7 @@ static int template_untouched(struct strbuf *sb)\n>  \tif (!template_file || strbuf_read_file(&tmpl, template_file, 0) <= 0)\n>  \t\treturn 0;\n>  \n> -\tstripspace(&tmpl, cleanup_mode == CLEANUP_ALL);\n> +\tstrbuf_stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);\n>  \tif (!skip_prefix(sb->buf, tmpl.buf, &start))\n>  \t\tstart = sb->buf;\n>  \tstrbuf_release(&tmpl);\n> @@ -1726,7 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\twt_status_truncate_message_at_cut_line(&sb);\n>  \n>  \tif (cleanup_mode != CLEANUP_NONE)\n> -\t\tstripspace(&sb, cleanup_mode == CLEANUP_ALL);\n> +\t\tstrbuf_stripspace(&sb, cleanup_mode == CLEANUP_ALL);\n>  \tif (template_untouched(&sb) && !allow_empty_message) {\n>  \t\trollback_index_files();\n>  \t\tfprintf(stderr, _(\"Aborting commit; you did not edit the message.\\n\"));\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a0edaca..e6741f3 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -806,7 +806,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n>  \t\t\tabort_commit(remoteheads, NULL);\n>  \t}\n>  \tread_merge_msg(&msg);\n> -\tstripspace(&msg, 0 < option_edit);\n> +\tstrbuf_stripspace(&msg, 0 < option_edit);\n>  \tif (!msg.len)\n>  \t\tabort_commit(remoteheads, _(\"Empty commit message.\"));\n>  \tstrbuf_release(&merge_msg);\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 3608c64..bb23d55 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -192,7 +192,7 @@ static void prepare_note_data(const unsigned char *object, struct note_data *d,\n>  \t\tif (launch_editor(d->edit_path, &d->buf, NULL)) {\n>  \t\t\tdie(_(\"Please supply the note contents using either -m or -F option\"));\n>  \t\t}\n> -\t\tstripspace(&d->buf, 1);\n> +\t\tstrbuf_stripspace(&d->buf, 1);\n>  \t}\n>  }\n>  \n> @@ -215,7 +215,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n>  \tif (d->buf.len)\n>  \t\tstrbuf_addch(&d->buf, '\\n');\n>  \tstrbuf_addstr(&d->buf, arg);\n> -\tstripspace(&d->buf, 0);\n> +\tstrbuf_stripspace(&d->buf, 0);\n>  \n>  \td->given = 1;\n>  \treturn 0;\n> @@ -232,7 +232,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n>  \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n>  \t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n>  \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n> -\tstripspace(&d->buf, 0);\n> +\tstrbuf_stripspace(&d->buf, 0);\n>  \n>  \td->given = 1;\n>  \treturn 0;\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 1259ed7..f677093 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -1,71 +1,6 @@\n>  #include \"builtin.h\"\n>  #include \"cache.h\"\n> -\n> -/*\n> - * Returns the length of a line, without trailing spaces.\n> - *\n> - * If the line ends with newline, it will be removed too.\n> - */\n> -static size_t cleanup(char *line, size_t len)\n> -{\n> -\twhile (len) {\n> -\t\tunsigned char c = line[len - 1];\n> -\t\tif (!isspace(c))\n> -\t\t\tbreak;\n> -\t\tlen--;\n> -\t}\n> -\n> -\treturn len;\n> -}\n> -\n> -/*\n> - * Remove empty lines from the beginning and end\n> - * and also trailing spaces from every line.\n> - *\n> - * Turn multiple consecutive empty lines between paragraphs\n> - * into just one empty line.\n> - *\n> - * If the input has only empty lines and spaces,\n> - * no output will be produced.\n> - *\n> - * If last line does not have a newline at the end, one is added.\n> - *\n> - * Enable skip_comments to skip every line starting with comment\n> - * character.\n> - */\n> -void stripspace(struct strbuf *sb, int skip_comments)\n> -{\n> -\tint empties = 0;\n> -\tsize_t i, j, len, newlen;\n> -\tchar *eol;\n> -\n> -\t/* We may have to add a newline. */\n> -\tstrbuf_grow(sb, 1);\n> -\n> -\tfor (i = j = 0; i < sb->len; i += len, j += newlen) {\n> -\t\teol = memchr(sb->buf + i, '\\n', sb->len - i);\n> -\t\tlen = eol ? eol - (sb->buf + i) + 1 : sb->len - i;\n> -\n> -\t\tif (skip_comments && len && sb->buf[i] == comment_line_char) {\n> -\t\t\tnewlen = 0;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tnewlen = cleanup(sb->buf + i, len);\n> -\n> -\t\t/* Not just an empty line? */\n> -\t\tif (newlen) {\n> -\t\t\tif (empties > 0 && j > 0)\n> -\t\t\t\tsb->buf[j++] = '\\n';\n> -\t\t\tempties = 0;\n> -\t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n> -\t\t\tsb->buf[newlen + j++] = '\\n';\n> -\t\t} else {\n> -\t\t\tempties++;\n> -\t\t}\n> -\t}\n> -\n> -\tstrbuf_setlen(sb, j);\n> -}\n> +#include \"strbuf.h\"\n>  \n>  static void comment_lines(struct strbuf *buf)\n>  {\n> @@ -111,7 +46,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n>  \t\tdie_errno(\"could not read the input\");\n>  \n>  \tif (mode == STRIP_SPACE)\n> -\t\tstripspace(&buf, strip_comments);\n> +\t\tstrbuf_stripspace(&buf, strip_comments);\n>  \telse\n>  \t\tcomment_lines(&buf);\n>  \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 9e17dca..5660787 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -268,7 +268,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n>  \t}\n>  \n>  \tif (opt->cleanup_mode != CLEANUP_NONE)\n> -\t\tstripspace(buf, opt->cleanup_mode == CLEANUP_ALL);\n> +\t\tstrbuf_stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);\n>  \n>  \tif (!opt->message_given && !buf->len)\n>  \t\tdie(_(\"no tag message?\"));\n> diff --git a/strbuf.c b/strbuf.c\n> index 29df55b..9583875 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -743,3 +743,69 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)\n>  \t}\n>  \tstrbuf_setlen(sb, sb->len + len);\n>  }\n> +\n> +/*\n> + * Returns the length of a line, without trailing spaces.\n> + *\n> + * If the line ends with newline, it will be removed too.\n> + */\n> +static size_t cleanup(char *line, size_t len)\n> +{\n> +\twhile (len) {\n> +\t\tunsigned char c = line[len - 1];\n> +\t\tif (!isspace(c))\n> +\t\t\tbreak;\n> +\t\tlen--;\n> +\t}\n> +\n> +\treturn len;\n> +}\n> +\n> +/*\n> + * Remove empty lines from the beginning and end\n> + * and also trailing spaces from every line.\n> + *\n> + * Turn multiple consecutive empty lines between paragraphs\n> + * into just one empty line.\n> + *\n> + * If the input has only empty lines and spaces,\n> + * no output will be produced.\n> + *\n> + * If last line does not have a newline at the end, one is added.\n> + *\n> + * Enable skip_comments to skip every line starting with comment\n> + * character.\n> + */\n> +void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n> +{\n> +\tint empties = 0;\n> +\tsize_t i, j, len, newlen;\n> +\tchar *eol;\n> +\n> +\t/* We may have to add a newline. */\n> +\tstrbuf_grow(sb, 1);\n> +\n> +\tfor (i = j = 0; i < sb->len; i += len, j += newlen) {\n> +\t\teol = memchr(sb->buf + i, '\\n', sb->len - i);\n> +\t\tlen = eol ? eol - (sb->buf + i) + 1 : sb->len - i;\n> +\n> +\t\tif (skip_comments && len && sb->buf[i] == comment_line_char) {\n> +\t\t\tnewlen = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tnewlen = cleanup(sb->buf + i, len);\n> +\n> +\t\t/* Not just an empty line? */\n> +\t\tif (newlen) {\n> +\t\t\tif (empties > 0 && j > 0)\n> +\t\t\t\tsb->buf[j++] = '\\n';\n> +\t\t\tempties = 0;\n> +\t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n> +\t\t\tsb->buf[newlen + j++] = '\\n';\n> +\t\t} else {\n> +\t\t\tempties++;\n> +\t\t}\n> +\t}\n> +\n> +\tstrbuf_setlen(sb, j);\n> +}\n> diff --git a/strbuf.h b/strbuf.h\n> index aef2794..5397d91 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -418,7 +418,16 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n>   * Strip whitespace from a buffer. The second parameter controls if\n>   * comments are considered contents to be removed or not.\n>   */\n> -extern void stripspace(struct strbuf *buf, int skip_comments);\n> +extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);\n> +\n> +/**\n> + * Temporary alias until all topic branches have switched to use\n> + * strbuf_stripspace directly.\n> + */\n> +static inline void stripspace(struct strbuf *buf, int skip_comments)\n> +{\n> +\tstrbuf_stripspace(buf, skip_comments);\n> +}\n>  \n>  static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n>  {\n"},{"id":"271743","messageId":"xmqqoag0ggjh.fsf@gitster.mtv.corp.google.com","threadId":"40567","inReplyTo":"1444911524-14504-3-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-15T17:58:26Z","receivedAt":"2015-10-15T17:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt\n> index 60328d5..411e17c 100644\n> --- a/Documentation/git-stripspace.txt\n> +++ b/Documentation/git-stripspace.txt\n> @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git stripspace' [-s | --strip-comments] < input\n> +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input\n>  'git stripspace' [-c | --comment-lines] < input\n>  \n>  DESCRIPTION\n> @@ -44,6 +44,11 @@ OPTIONS\n>  \tbe terminated with a newline. On empty lines, only the comment character\n>  \twill be prepended.\n>  \n> +-C::\n> +--count-lines::\n> +\tOutput the number of resulting lines after stripping. This is equivalent\n> +\tto calling 'git stripspace | wc -l'.\n\nI agree with Matthieu that squatting on a short-and-sweet -C is\nunwanted here.\n\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index f677093..7882edd 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -1,5 +1,6 @@\n>  #include \"builtin.h\"\n>  #include \"cache.h\"\n> +#include \"parse-options.h\"\n>  #include \"strbuf.h\"\n>  \n>  static void comment_lines(struct strbuf *buf)\n> @@ -12,45 +13,51 @@ static void comment_lines(struct strbuf *buf)\n>  \tfree(msg);\n>  }\n>  \n> -static const char *usage_msg = \"\\n\"\n> -\"  git stripspace [-s | --strip-comments] < input\\n\"\n> -\"  git stripspace [-c | --comment-lines] < input\";\n> +static const char * const usage_msg[] = {\n> +\tN_(\"git stripspace [-s | --strip-comments] [-C | --count-lines] < input\"),\n> +\tN_(\"git stripspace [-c | --comment-lines] < input\"),\n> +\tNULL\n> +};\n>  \n>  int cmd_stripspace(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n> -\tint strip_comments = 0;\n> -\tenum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;\n> -\n> -\tif (argc == 2) {\n> -\t\tif (!strcmp(argv[1], \"-s\") ||\n> -\t\t    !strcmp(argv[1], \"--strip-comments\")) {\n> -\t\t\tstrip_comments = 1;\n> -\t\t} else if (!strcmp(argv[1], \"-c\") ||\n> -\t\t\t   !strcmp(argv[1], \"--comment-lines\")) {\n> -\t\t\tmode = COMMENT_LINES;\n> -\t\t} else {\n> -\t\t\tmode = INVAL;\n> -\t\t}\n> -\t} else if (argc > 1) {\n> -\t\tmode = INVAL;\n> -\t}\n> +\tint comment_mode = 0, count_lines = 0, strip_comments = 0;\n> +\tsize_t lines = 0;\n> +\n> +\tconst struct option options[] = {\n> +\t\tOPT_BOOL('s', \"strip-comments\", &strip_comments,\n> +\t\t\t N_(\"skip and remove all lines starting with comment character\")),\n> +\t\tOPT_BOOL('c', \"comment-lines\", &comment_mode,\n> +\t\t\t N_(\"prepend comment character and blank to each line\")),\n> +\t\tOPT_BOOL('C', \"count-lines\", &count_lines, N_(\"print line count\")),\n> +\t\tOPT_END()\n> +\t};\n\nI think -s and -c are incompatible, so OPT_CMDMODE() might be a\nbetter fit for them if you are going to switch the command line\nparser to use parse-options.\n\nWhich makes me strongly suspect that this should be done in at least\ntwo separate steps.  (1) Use parse-options to parse command line\nwithout adding the counting at all, followed by (2) Add counting.\n\n> +\tif (!count_lines)\n> +\t\twrite_or_die(1, buf.buf, buf.len);\n> +\telse {\n> +\t\tstruct strbuf tmp = STRBUF_INIT;\n> +\t\tstrbuf_addf(&tmp, \"%zu\\n\", lines);\n> +\t\twrite_or_die(1, tmp.buf, tmp.len);\n> +\t}\n\nSo this is your output code, which gives only the number of lines\nwithout the cleaned up result.\n\n> @@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n>  \n>  \t\t/* Not just an empty line? */\n>  \t\tif (newlen) {\n> -\t\t\tif (empties > 0 && j > 0)\n> +\t\t\tif (empties > 0 && j > 0) {\n>  \t\t\t\tsb->buf[j++] = '\\n';\n> +\t\t\t\tlines++;\n> +\t\t\t}\n>  \t\t\tempties = 0;\n>  \t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n>  \t\t\tsb->buf[newlen + j++] = '\\n';\n> +\t\t\tlines++;\n>  \t\t} else {\n>  \t\t\tempties++;\n>  \t\t}\n>  \t}\n\nI cannot say that the above is one of the better possible\nimplementations of this feature I would think of.\n\n (1) If this is performance sensitive, then you do not want to do\n     memmove() etc. to actually touch the contents of the *sb and\n     only increment lines++, because you are not going to show that\n     in the output anyway.\n\n (2) If this feature is not performance sensitive, then the best\n     implementation would be not to touch strbuf_stripspace() at all\n     to realize this change, and count the number of '\\n' in the\n     cmd_stripspace() just before you use printf(\"%d\\n\", lines).\n     That is best from maintainability's point-of-view, because it\n     makes it infinitely less error-prone for future changes of\n     strbuf_stripspace() to forget incrementing lines++ when it adds\n     a newline to the output.\n\n (3) If you are going to still munge *sb, even if you are not going\n     to show it at the end, perhaps because that would make it\n     easier to keep track of where the code is scanning to find when\n     you need to increment lines++, then at least the patch should\n     devise a mechanism to make it less likely that the future\n     changes to strbuf_stripspace() would make 'lines' and the\n     number of '\\n' in the *sb out-of-sync (hint: perhaps a macro\n     called APPEND_LF(sb, line) or something).  It is easy to append\n     '\\n' to sb->buf without incrementing lines++ as the code stands\n     with this patch applied---the patch makes the code less\n     maintainable.\n\nMy gut feeling is that you should do (2), which is the cleanest from\nthe maintainability's point-of-view.\n"},{"id":"271745","messageId":"vpq4mhsx7hq.fsf@grenoble-inp.fr","threadId":"40567","inReplyTo":"1444911524-14504-3-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T19:21:53Z","receivedAt":"2015-10-15T19:21:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> + * comments are considered contents to be removed or not. Returns the\n> + * number of lines in the resulting buffer.\n\nWe write comments at imperative tone, hence \"Return\", not \"Returns\".\n\nOther than that, I agree with Junio:\n\n* A preparatory patch introducing parse-options would make the actual\n  patch much easier to review.\n\n* Just running strbuf_stripspace and counting the number of lines in the\n  result is much simpler. We use stripspace only on user-provided input\n  which are never really big so maintainability is more important than\n  performance.\n\nCheers,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271799","messageId":"20151016070953.GJ11304@distanz.ch","threadId":"40567","inReplyTo":"xmqqsi5cghke.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T07:09:54Z","receivedAt":"2015-10-16T07:09:54Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Thanks for the review.\n\nOn 2015-10-15 at 19:36:17 +0200, Junio C Hamano <gitster@pobox.com> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> > Rename stripspace() to strbuf_stripspace() and move it to the strbuf\n> > module as suggested in [1].\n> >\n> > Also switch all current users of stripspace() to the new function name\n> > and  keep a temporary wrapper inline function for topic branches still\n> > using stripspace().\n> >\n> > [1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf\n> \n> I think Matthieu already mentioned this, but please explain why it\n> is a good idea to do this in your own words here, without forcing\n> readers to go to other places to find out.  Giving credit to an\n> external page for giving you an inspiration to do something is good,\n> but the proposed log message needs to justify itself.  In other\n> words, when you are challenged to defend this change, you are not\n> allowed to say \"SmallProjectIdeas page said it is a good thing to\n> do.  I just did it without questioning it.\" ;-)  Instead you are\n> expected to justify it yourself.\n\nYes, makes sense. I'll adjust the commit message for v2 to justify the\nchange for itself and move the link below --- as Matthieu suggested.\n\n> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>\n> > ---\n> \n> A good rule of thumb to see if it is a good time to do this kind of\n> change is to do this:\n> \n> \t$ git log -p maint..pu | grep stripspace\n> \n> which shows only one mention in a context, so this change may cause\n> textual conflict with something else somewhere nearby but won't hurt\n> other topics in flight.\n\nOk, thanks for the hint. I'll check again before resubmitting v2.\n\nThanks\n"},{"id":"271800","messageId":"20151016071406.GK11304@distanz.ch","threadId":"40567","inReplyTo":"vpqio6813tc.fsf@grenoble-inp.fr","subject":"Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T07:14:07Z","receivedAt":"2015-10-16T07:14:07Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-15 at 18:42:23 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> > Also switch all current users of stripspace() to the new function name\n> > and  keep a temporary wrapper inline function for topic branches still\n> > using stripspace().\n> \n> Since you have this temporary wrapper, it would have made sense to split\n> the patch into 1) move and rename the function, and 2) change the\n> callsites to strbuf_stripspace. This way 2) would be absolutely trivial\n> to review.\n> \n> OTOH, this patch is already easy to review, so you may consider it's OK\n> like this.\n\nOk, in this case will keep the patch as is for v2, but with the adjusted\ncommit message as indicated in your and Junio's review.\n\n> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\nWill add it to v2, thanks!\n"},{"id":"271801","messageId":"20151016072126.GL11304@distanz.ch","threadId":"40567","inReplyTo":"vpqzizkysyh.fsf@grenoble-inp.fr","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T07:21:27Z","receivedAt":"2015-10-16T07:21:27Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> > --- a/Documentation/git-stripspace.txt\n> > +++ b/Documentation/git-stripspace.txt\n> > @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace\n> >  SYNOPSIS\n> >  --------\n> >  [verse]\n> > -'git stripspace' [-s | --strip-comments] < input\n> > +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input\n> \n> I'm not sure it's a good idea to introduce a one-letter shortcut (-C).\n> In scripts, --count-lines is self-explanatory hence more readable than\n> -C (which is even more confusing since \"git -C foo stripspace\" and \"git\n> stripspace -C\" have totally different meanings. Outside scripts, I'm not\n> sure the command would be useful. I'd rather avoid polluting the\n> one-letter-option namespace.\n\nOk, I'll drop the -C. Didn't consider the `git -C stripspace' case, so\nthat's definitely unwanted.\n\n> > +Use 'git stripspace --count-lines' to obtain:\n> > +\n> > +---------\n> > +|5$\n> > +---------\n> \n> In the examples above, I read the | as part of the input (unlike $ which\n> is used only to show the end of line). So the | should not be here. I\n> don't think you need the $ either, the --count-lines option is no longer\n> about trailing whitespaces.\n\nWill drop both | and $. Seems I didn't check the output careful enough,\nboth don't make sense for this option.\n\n> > +static const char * const usage_msg[] = {\n> \n> Stick the * to usage_msg please.\n\nWill change in v2.\n"},{"id":"271803","messageId":"20151016075134.GM11304@distanz.ch","threadId":"40567","inReplyTo":"xmqqoag0ggjh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T07:51:35Z","receivedAt":"2015-10-16T07:51:35Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-15 at 19:58:26 +0200, Junio C Hamano <gitster@pobox.com> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> > diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt\n> > index 60328d5..411e17c 100644\n> > --- a/Documentation/git-stripspace.txt\n> > +++ b/Documentation/git-stripspace.txt\n> > @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace\n> >  SYNOPSIS\n> >  --------\n> >  [verse]\n> > -'git stripspace' [-s | --strip-comments] < input\n> > +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input\n> >  'git stripspace' [-c | --comment-lines] < input\n> >  \n> >  DESCRIPTION\n> > @@ -44,6 +44,11 @@ OPTIONS\n> >  \tbe terminated with a newline. On empty lines, only the comment character\n> >  \twill be prepended.\n> >  \n> > +-C::\n> > +--count-lines::\n> > +\tOutput the number of resulting lines after stripping. This is equivalent\n> > +\tto calling 'git stripspace | wc -l'.\n> \n> I agree with Matthieu that squatting on a short-and-sweet -C is\n> unwanted here.\n\nWill drop it in v2.\n\n> > diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> > index f677093..7882edd 100644\n> > --- a/builtin/stripspace.c\n> > +++ b/builtin/stripspace.c\n> > @@ -1,5 +1,6 @@\n> >  #include \"builtin.h\"\n> >  #include \"cache.h\"\n> > +#include \"parse-options.h\"\n> >  #include \"strbuf.h\"\n> >  \n> >  static void comment_lines(struct strbuf *buf)\n> > @@ -12,45 +13,51 @@ static void comment_lines(struct strbuf *buf)\n> >  \tfree(msg);\n> >  }\n> >  \n> > -static const char *usage_msg = \"\\n\"\n> > -\"  git stripspace [-s | --strip-comments] < input\\n\"\n> > -\"  git stripspace [-c | --comment-lines] < input\";\n> > +static const char * const usage_msg[] = {\n> > +\tN_(\"git stripspace [-s | --strip-comments] [-C | --count-lines] < input\"),\n> > +\tN_(\"git stripspace [-c | --comment-lines] < input\"),\n> > +\tNULL\n> > +};\n> >  \n> >  int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> >  {\n> >  \tstruct strbuf buf = STRBUF_INIT;\n> > -\tint strip_comments = 0;\n> > -\tenum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = STRIP_SPACE;\n> > -\n> > -\tif (argc == 2) {\n> > -\t\tif (!strcmp(argv[1], \"-s\") ||\n> > -\t\t    !strcmp(argv[1], \"--strip-comments\")) {\n> > -\t\t\tstrip_comments = 1;\n> > -\t\t} else if (!strcmp(argv[1], \"-c\") ||\n> > -\t\t\t   !strcmp(argv[1], \"--comment-lines\")) {\n> > -\t\t\tmode = COMMENT_LINES;\n> > -\t\t} else {\n> > -\t\t\tmode = INVAL;\n> > -\t\t}\n> > -\t} else if (argc > 1) {\n> > -\t\tmode = INVAL;\n> > -\t}\n> > +\tint comment_mode = 0, count_lines = 0, strip_comments = 0;\n> > +\tsize_t lines = 0;\n> > +\n> > +\tconst struct option options[] = {\n> > +\t\tOPT_BOOL('s', \"strip-comments\", &strip_comments,\n> > +\t\t\t N_(\"skip and remove all lines starting with comment character\")),\n> > +\t\tOPT_BOOL('c', \"comment-lines\", &comment_mode,\n> > +\t\t\t N_(\"prepend comment character and blank to each line\")),\n> > +\t\tOPT_BOOL('C', \"count-lines\", &count_lines, N_(\"print line count\")),\n> > +\t\tOPT_END()\n> > +\t};\n> \n> I think -s and -c are incompatible, so OPT_CMDMODE() might be a\n> better fit for them if you are going to switch the command line\n> parser to use parse-options.\n\nOk, will change to OPT_CMDMODE()\n\n> Which makes me strongly suspect that this should be done in at least\n> two separate steps.  (1) Use parse-options to parse command line\n> without adding the counting at all, followed by (2) Add counting.\n\nOk, will split the patch as you suggest for v2 (or more if it makes it\neasier to review).\n\n> > +\tif (!count_lines)\n> > +\t\twrite_or_die(1, buf.buf, buf.len);\n> > +\telse {\n> > +\t\tstruct strbuf tmp = STRBUF_INIT;\n> > +\t\tstrbuf_addf(&tmp, \"%zu\\n\", lines);\n> > +\t\twrite_or_die(1, tmp.buf, tmp.len);\n> > +\t}\n> \n> So this is your output code, which gives only the number of lines\n> without the cleaned up result.\n\nThis should better be a simple printf(\"%zu\\n\", lines) I guess?\n\n> > @@ -797,15 +799,19 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n> >  \n> >  \t\t/* Not just an empty line? */\n> >  \t\tif (newlen) {\n> > -\t\t\tif (empties > 0 && j > 0)\n> > +\t\t\tif (empties > 0 && j > 0) {\n> >  \t\t\t\tsb->buf[j++] = '\\n';\n> > +\t\t\t\tlines++;\n> > +\t\t\t}\n> >  \t\t\tempties = 0;\n> >  \t\t\tmemmove(sb->buf + j, sb->buf + i, newlen);\n> >  \t\t\tsb->buf[newlen + j++] = '\\n';\n> > +\t\t\tlines++;\n> >  \t\t} else {\n> >  \t\t\tempties++;\n> >  \t\t}\n> >  \t}\n> \n> I cannot say that the above is one of the better possible\n> implementations of this feature I would think of.\n> \n>  (1) If this is performance sensitive, then you do not want to do\n>      memmove() etc. to actually touch the contents of the *sb and\n>      only increment lines++, because you are not going to show that\n>      in the output anyway.\n> \n>  (2) If this feature is not performance sensitive, then the best\n>      implementation would be not to touch strbuf_stripspace() at all\n>      to realize this change, and count the number of '\\n' in the\n>      cmd_stripspace() just before you use printf(\"%d\\n\", lines).\n>      That is best from maintainability's point-of-view, because it\n>      makes it infinitely less error-prone for future changes of\n>      strbuf_stripspace() to forget incrementing lines++ when it adds\n>      a newline to the output.\n> \n>  (3) If you are going to still munge *sb, even if you are not going\n>      to show it at the end, perhaps because that would make it\n>      easier to keep track of where the code is scanning to find when\n>      you need to increment lines++, then at least the patch should\n>      devise a mechanism to make it less likely that the future\n>      changes to strbuf_stripspace() would make 'lines' and the\n>      number of '\\n' in the *sb out-of-sync (hint: perhaps a macro\n>      called APPEND_LF(sb, line) or something).  It is easy to append\n>      '\\n' to sb->buf without incrementing lines++ as the code stands\n>      with this patch applied---the patch makes the code less\n>      maintainable.\n> \n> My gut feeling is that you should do (2), which is the cleanest from\n> the maintainability's point-of-view.\n\nThank you for outlining and assessing these possible implementations. I\nagree that my suggested implementation is probably the most naïve one :)\nand think that (2) would less intrusive and better to maintain. Will\nchange the patch accordingly.\n> \n"},{"id":"271804","messageId":"20151016075413.GN11304@distanz.ch","threadId":"40567","inReplyTo":"vpq4mhsx7hq.fsf@grenoble-inp.fr","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T07:54:13Z","receivedAt":"2015-10-16T07:54:13Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-15 at 21:21:53 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> > + * comments are considered contents to be removed or not. Returns the\n> > + * number of lines in the resulting buffer.\n> \n> We write comments at imperative tone, hence \"Return\", not \"Returns\".\n\nThe other comments in strbuf.h used \"Returns\", so I went for it for\nconsistency reasons. But the comment will be obsolete anyhow, as\nstrbuf_stripspace() will not be changed and the functionality\nimplemented in cmd_stripspace() as Junio suggested.\n\n> Other than that, I agree with Junio:\n> \n> * A preparatory patch introducing parse-options would make the actual\n>   patch much easier to review.\n\nWill do.\n\n> \n> * Just running strbuf_stripspace and counting the number of lines in the\n>   result is much simpler. We use stripspace only on user-provided input\n>   which are never really big so maintainability is more important than\n>   performance.\n\nDitto.\n\nThanks\n"},{"id":"271806","messageId":"20151016084018.GO11304@distanz.ch","threadId":"40567","inReplyTo":"vpqzizkysyh.fsf@grenoble-inp.fr","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-16T08:40:19Z","receivedAt":"2015-10-16T08:40:19Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> > +static const char * const usage_msg[] = {\n> \n> Stick the * to usage_msg please.\n\nJust noticed while looking at how other sub-commands define this, the vast\nmajority use \"const char * const\" and not \"const char const *\".\n\nAlso it would change the meaning. The following wouldn't produce a\ncompiler warning:\n\n\tstatic const char const *usage_msg[] = { ... };\n\t...\n\tusage_msg[0] = \"Foobar\"\n\nwhile\n\n\tstatic const char * const usage_msg[] = { ... };\n\t...\n\tusage_msg[0] = \"Foobar\"\n\nwould produce one:\n\n\tbuiltin/stripspace.c:36:2: error: assignment of read-only location ‘usage_msg[0]’\n\nEven though I don't expect anyone to modify usage_msg at runtime I think\nit'd be better to stick with the current version.\n"},{"id":"271807","messageId":"vpqwpun89yv.fsf@grenoble-inp.fr","threadId":"40567","inReplyTo":"20151016084018.GO11304@distanz.ch","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-16T08:59:52Z","receivedAt":"2015-10-16T08:59:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n> On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n>> Tobias Klauser <tklauser@distanz.ch> writes:\n>> > +static const char * const usage_msg[] = {\n>> \n>> Stick the * to usage_msg please.\n>\n> Just noticed while looking at how other sub-commands define this, the vast\n> majority use \"const char * const\" and not \"const char const *\".\n\nOops, I read your code too quickly. We stick the * to variable names\nwhen it follows the star, but I didn't see the \"const\", sorry.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271825","messageId":"xmqq8u72g1hm.fsf@gitster.mtv.corp.google.com","threadId":"40567","inReplyTo":"20151016075134.GM11304@distanz.ch","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-16T17:35:49Z","receivedAt":"2015-10-16T17:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser <tklauser@distanz.ch> writes:\n\n>> So this is your output code, which gives only the number of lines\n>> without the cleaned up result.\n>\n> This should better be a simple printf(\"%zu\\n\", lines) I guess?\n\nI think we actively avoid using %z conversion that is only C99.\n\nIf you really want to, you could count in size_t and use %lu with\nappropriate casting, which I think is what we do in the rest of the\ncodebase.\n\nFor this one, I think it is sufficient to just count in int and\nprint as int with %d, though.\n"},{"id":"271852","messageId":"20151017104425.GF2468@distanz.ch","threadId":"40567","inReplyTo":"xmqq8u72g1hm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: Implement --count-lines option","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-17T10:44:25Z","receivedAt":"2015-10-17T10:44:25Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-16 at 19:35:49 +0200, Junio C Hamano <gitster@pobox.com> wrote:\n> Tobias Klauser <tklauser@distanz.ch> writes:\n> \n> >> So this is your output code, which gives only the number of lines\n> >> without the cleaned up result.\n> >\n> > This should better be a simple printf(\"%zu\\n\", lines) I guess?\n> \n> I think we actively avoid using %z conversion that is only C99.\n> \n> If you really want to, you could count in size_t and use %lu with\n> appropriate casting, which I think is what we do in the rest of the\n> codebase.\n> \n> For this one, I think it is sufficient to just count in int and\n> print as int with %d, though.\n\nOk, will use an int to count and printf(\"%d\\n\").\n"}]}