On 2015-10-18 at 01:57:57 +0200, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser <tklauser@xxxxxxxxxx> 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. > More below. > > > Furthermore, add the corresponding documentation and tests. > > > > Signed-off-by: Tobias Klauser <tklauser@xxxxxxxxxx> > > --- > > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html