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

From: Tobias Klauser <tklauser@distanz.ch>
Date: Oct 16, 2015, 07:21 UTC
Message-ID: <20151016072126.GL11304@distanz.ch>
In-Reply-To: <vpqzizkysyh.fsf@grenoble-inp.fr>

On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 17 quoted lines

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

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

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.