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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 15, 2015, 16:52 UTC
Message-ID: <vpqzizkysyh.fsf@grenoble-inp.fr>
In-Reply-To: <1444911524-14504-3-git-send-email-tklauser@distanz.ch>

Tobias Klauser <tklauser@distanz.ch> writes:

Show 8 quoted lines

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

Show 5 quoted lines

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