Re: [PATCH 2/3] stripspace: Implement --count-lines option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 15, 2015, 17:58 UTC
- Message-ID
- <xmqqoag0ggjh.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1444911524-14504-3-git-send-email-tklauser@distanz.ch>
Tobias Klauser <tklauser@distanz.ch> writes:
Show 21 quoted lines
> 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.
Show 54 quoted lines
> 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.
Show 7 quoted lines
> + 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.
Show 17 quoted lines
> @@ -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.