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

From: Tobias Klauser <tklauser@distanz.ch>
Date: Oct 19, 2015, 13:31 UTC
Message-ID: <20151019133150.GK2468@distanz.ch>
In-Reply-To: <CAPig+cQ=8FO8yFY4sHUwr0mYuyvMu4d-eizHZeadE9f0BgpXpQ@mail.gmail.com>

On 2015-10-18 at 01:57:57 +0200, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 13 quoted lines

> On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser <tklauser@distanz.ch> wrote:
> > Implement the --count-lines options for git stripspace [...]
> >
> > This will make it easier to port git-rebase--interactive.sh to C later
> > on.
> 
> Is there any application beyond git-rebase--interactive where a
> --count-lines options is expected to be useful? It's not obvious from
> the commit message that this change is necessarily a win for later
> porting of git-rebase--interactive to C since the amount of extra code
> and support material added by this patch probably outweighs the amount
> of code a C version of git-rebase--interactive would need to count the
> lines itself.

Agreed, it doesn't make much sense anymore in the current form. An strbuf helper function implementing the line counting would probably be the better way. But I guess this should only be introduced once someone decides to write a C version of git-rebase--interactive (or any other use for line counting appears).

> Stated differently, are the two or three instances of piping through
> 'wc' in git-rebase--interactive sufficient justification for
> introducing extra complexity into git-stripspace and its documentation
> and tests?

IMO it doesn't add a lot of complexity, but on the other hand it also doesn't provide a large benefit apart from getting rid of a few calls to an external program in a code path which is not performance critical.

So I suggest I'll drop patches 3/4 and 4/4 for v3. Once someone really needs the line counting functionality in C, an strbuf helper can still be added.

Show 23 quoted lines

> More below.
> 
> > Furthermore, add the corresponding documentation and tests.
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> > index 29e91d8..9c00cb9 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 --count-lines >actual &&
> > +       test_cmp expect actual
> > +'
> 
> What is the expected behavior when the input is an empty file, a file
> with content but no newline, a file with one or more lines but lacking
> a newline on the final line? Should these cases be tested, as well?

Not really sure. For the implementation I followed the behavior of 'wc -l' which doesn't consider the final line if it lacks a newline. Should this be different for git's purposes? In any case, I agree that these cases should excplicitely be tested/documented.