Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
Eric Sunshine writes: > With this in mind, my > question was also indirectly asking whether there was sufficient > justification of the long-term cost of a --count-lines option. The > argument that --count-lines would help test a proposed > strbuf_count_lines() likely does not outweigh that cost. I agree. If we expect users to call --count-lines outside rebase-interactive.sh and our own tests, then the actual use should be justified in the commit message. If not, then at least the --count-lines option should be hidden and not documented in the public doc. But I agree that introducing test-strbuf would be even better. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
On Mon, Oct 19, 2015 at 1:03 PM, Christian Couder wrote: > On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser wrote: >> On 2015-10-18 at 19:18:53 +0200, Junio C Hamano wrote: >>> Eric Sunshine writes: >>> > 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. >>> > >>> > 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? >>> >>> Interesting thought. When somebody rewrites "rebase -i" in C, >>> nobody needs to count lines in "stripspace" output. The rewritten >>> "rebase -i" would internally run strbuf_stripspace() and the question >>> becomes what is the best way to let that code find out how many lines >>> the result contains. >>> >>> When viewed from that angle, I agree that "stripspace --count" does >>> not add anything to further the goal of helping "rebase -i" to move >>> to C. Adding strbuf_count_lines() that counts the number of lines >>> in the given strbuf (if there is no such helper yet; I didn't check), >>> though. >> >> I check before implementing this series and didn't find any helper. I >> also didn't find any other uses of line counting in the code. > > This shows that implementing "git stripspace --count-lines" could > indirectly help porting "git rebase -i" to C as you could implement > strbuf_count_lines() for the former and it could then be reused in the > latter. In this project, where all user-facing functionality must be supported for the life of the project, each new command, command-line option, configuration setting, and environment variable exacts additional costs beyond the initial implementation cost. With this in mind, my question was also indirectly asking whether there was sufficient justification of the long-term cost of a --count-lines option. The argument that --count-lines would help test a proposed strbuf_count_lines() likely does not outweigh that cost. >>> >> +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? >>> >>> Good point here, too. If we were to add strbuf_count_lines() >>> helper, whoever adds that function needs to take a possible >>> incomplete line at the end into account. >> >> Yes, makes more sense like this (even though it doesn't correspond to >> what 'wc -l' does). > > Tests for "git stripspace --count-lines" would test > strbuf_count_lines() which would also help when porting git rebase -i > to C. Rather than saddling the project with the cost of a new user-facing, but otherwise unneeded option, a more direct way to test the proposed strbuf_count_lines() would be to add a test-strbuf program, akin to test-config, test-string-list, etc. This has the added benefit of providing a home for strbuf-based tests beyond line counting. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser wrote: > On 2015-10-18 at 19:18:53 +0200, Junio C Hamano wrote: >> Eric Sunshine writes: >> >> > 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. >> > >> > 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? >> >> Interesting thought. When somebody rewrites "rebase -i" in C, >> nobody needs to count lines in "stripspace" output. The rewritten >> "rebase -i" would internally run strbuf_stripspace() and the question >> becomes what is the best way to let that code find out how many lines >> the result contains. >> >> When viewed from that angle, I agree that "stripspace --count" does >> not add anything to further the goal of helping "rebase -i" to move >> to C. Adding strbuf_count_lines() that counts the number of lines >> in the given strbuf (if there is no such helper yet; I didn't check), >> though. > > I check before implementing this series and didn't find any helper. I > also didn't find any other uses of line counting in the code. This shows that implementing "git stripspace --count-lines" could indirectly help porting "git rebase -i" to C as you could implement strbuf_count_lines() for the former and it could then be reused in the latter. > After considering your and Eric's reply, I'll drop these patches for > now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to > Eric). It would be sad in my opinion. >> >> +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? >> >> Good point here, too. If we were to add strbuf_count_lines() >> helper, whoever adds that function needs to take a possible >> incomplete line at the end into account. > > Yes, makes more sense like this (even though it doesn't correspond to > what 'wc -l' does). Tests for "git stripspace --count-lines" would test strbuf_count_lines() which would also help when porting git rebase -i to C. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
On 2015-10-18 at 19:18:53 +0200, Junio C Hamano wrote: > Eric Sunshine writes: > > > 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. > > > > 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? > > Interesting thought. When somebody rewrites "rebase -i" in C, > nobody needs to count lines in "stripspace" output. The rewritten > "rebase -i" would internally run strbuf_stripspace() and the question > becomes what is the best way to let that code find out how many lines > the result contains. > > When viewed from that angle, I agree that "stripspace --count" does > not add anything to further the goal of helping "rebase -i" to move > to C. Adding strbuf_count_lines() that counts the number of lines > in the given strbuf (if there is no such helper yet; I didn't check), > though. I check before implementing this series and didn't find any helper. I also didn't find any other uses of line counting in the code. After considering your and Eric's reply, I'll drop these patches for now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to Eric). > >> +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? > > Good point here, too. If we were to add strbuf_count_lines() > helper, whoever adds that function needs to take a possible > incomplete line at the end into account. Yes, makes more sense like this (even though it doesn't correspond to what 'wc -l' does). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
On 2015-10-18 at 01:57:57 +0200, Eric Sunshine wrote: > On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser 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 > > --- > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
Eric Sunshine writes: > 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. > > 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? Interesting thought. When somebody rewrites "rebase -i" in C, nobody needs to count lines in "stripspace" output. The rewritten "rebase -i" would internally run strbuf_stripspace() and the question becomes what is the best way to let that code find out how many lines the result contains. When viewed from that angle, I agree that "stripspace --count" does not add anything to further the goal of helping "rebase -i" to move to C. Adding strbuf_count_lines() that counts the number of lines in the given strbuf (if there is no such helper yet; I didn't check), though. >> +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? Good point here, too. If we were to add strbuf_count_lines() helper, whoever adds that function needs to take a possible incomplete line at the end into account. Thanks for your comments. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] stripspace: Implement --count-lines option
On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser 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. 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? More below. > Furthermore, add the corresponding documentation and tests. > > Signed-off-by: Tobias Klauser > --- > 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? > +test_expect_success '--count-lines with single line' ' > + printf "1\n" >expect && > + printf "foo\n" | git stripspace --count-lines >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--count-lines with single line preceeded by empty line' > ' > + printf "1\n" >expect && > + printf "\nfoo" | git stripspace --count-lines >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--count-lines with single line followed by empty line' ' > + printf "1\n" >expect && > + printf "foo\n\n" | git stripspace --count-lines >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--count-lines with multiple lines and consecutive > newlines' ' > + printf "5\n" >expect && > + printf "\none\n\n\nthree\nfour\nfive\n" | git stripspace > --count-lines >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--count-lines combined with --strip-comments' ' > + printf "5\n" >expect && > + printf "\n# stripped\none\n#stripped\n\nthree\nfour\nfive\n" | git > stripspace -s --count-lines >actual && > + test_cmp expect actual > +' > + > test_done > -- > 2.6.1.148.g7927db1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
