{"thread":{"id":"34518","subject":"[PATCH 0/5] range-set and line-log bug fixes","startedAt":"2013-07-23T14:28:03Z","lastAt":"2013-07-25T13:09:19Z","messageCount":17,"participants":["Eric Sunshine","Thomas Rast","SZEDER Gábor","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"223979","messageId":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":null,"subject":"[PATCH 0/5] range-set and line-log bug fixes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:03Z","receivedAt":"2013-07-23T14:28:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"While implementing multiple -L support for git-blame, I encountered\nseveral bugs in range-set and line-log resulting in crashes. This\nseries fixes those bugs.\n\nEric Sunshine (5):\n range-set: fix sort_and_merge_range_set() corner case bug\n t4211: demonstrate empty -L range crash\n range-set: satisfy non-empty ranges invariant\n t4211: demonstrate crash when first -L encountered is empty range\n line-log: fix \"log -LN\" crash when N is last line of file\n\n line-log.c | 9 ++++++---\n t/t4211-line-log.sh | 13 +++++++++++++\n 2 files changed, 19 insertions(+), 3 deletions(-)\n\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223980","messageId":"1374589688-27751-2-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 1/5] range-set: fix sort_and_merge_range_set() corner case bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:04Z","receivedAt":"2013-07-23T14:28:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When handed an empty range_set (range_set.nr == 0),\nsort_and_merge_range_set() incorrectly sets range_set.nr to 1 at exit.\nSubsequent range_set functions then access the bogus range at element\nzero and crash or throw an assertion failure. Fix this bug.\n\nSigned-off-by: Eric Sunshine \n---\n\nThis bug could have been fixed with a simple conditional rather than\nchanging the for() loop to start at 0. I did it this way, however,\nbecause the next fix (range-set: satisfy non-empty ranges invariant)\nalso needs the for() loop starting at 0. Thus, by changing the for()\nloop here, the subsequent fix becomes much more obvious and easy to\nread.\n\n line-log.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 8cc29a0..5234879 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -110,12 +110,12 @@ static void range_set_check_invariants(struct range_set *rs)\n static void sort_and_merge_range_set(struct range_set *rs)\n {\n \tint i;\n-\tint o = 1; /* output cursor */\n+\tint o = 0; /* output cursor */\n \n \tqsort(rs->ranges, rs->nr, sizeof(struct range), range_cmp);\n \n-\tfor (i = 1; i < rs->nr; i++) {\n-\t\tif (rs->ranges[i].start <= rs->ranges[o-1].end) {\n+\tfor (i = 0; i < rs->nr; i++) {\n+\t\tif (o > 0 && rs->ranges[i].start <= rs->ranges[o-1].end) {\n \t\t\tif (rs->ranges[o-1].end < rs->ranges[i].end)\n \t\t\t\trs->ranges[o-1].end = rs->ranges[i].end;\n \t\t} else {\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223981","messageId":"1374589688-27751-3-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 2/5] t4211: demonstrate empty -L range crash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:05Z","receivedAt":"2013-07-23T14:28:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Signed-off-by: Eric Sunshine \n---\n t/t4211-line-log.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 7776f93..1db1edd 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -64,4 +64,12 @@ test_bad_opts \"-L 1,1000:b.c\" \"has only.*lines\"\n test_bad_opts \"-L :b.c\" \"argument.*not of the form\"\n test_bad_opts \"-L :foo:b.c\" \"no match\"\n \n+# There is a separate bug when an empty -L range is the first -L encountered,\n+# thus to demonstrate this particular bug, the empty -L range must follow a\n+# non-empty -L range.\n+test_expect_failure '-L {empty-range} (any -L)' '\n+\tn=$(expr $(cat b.c | wc -l) + 1) &&\n+\tgit log -L1,1:b.c -L$n:b.c\n+'\n+\n test_done\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223983","messageId":"1374589688-27751-4-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 3/5] range-set: satisfy non-empty ranges invariant","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:06Z","receivedAt":"2013-07-23T14:28:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"range-set invariants are: ranges must be (1) non-empty, (2) disjoint,\n(3) sorted in ascending order.\n\nDuring processing, various range-set utility functions break the\ninvariants (for instance, by adding empty ranges), with the\nexpectation that a finalizing sort_and_merge_range_set() will restore\nsanity.\n\nsort_and_merge_range_set(), however, neglects to fold out empty\nranges, thus it fails to satisfy the non-empty constraint. Subsequent\nrange-set functions crash or throw an assertion failure upon\nencountering such an anomaly. Rectify the situation by having\nsort_and_merge_range_set() fold out empty ranges.\n\nSigned-off-by: Eric Sunshine \n---\n line-log.c | 2 ++\n t/t4211-line-log.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 5234879..6f94d56 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -115,6 +115,8 @@ static void sort_and_merge_range_set(struct range_set *rs)\n \tqsort(rs->ranges, rs->nr, sizeof(struct range), range_cmp);\n \n \tfor (i = 0; i < rs->nr; i++) {\n+\t\tif (rs->ranges[i].start == rs->ranges[i].end)\n+\t\t\tcontinue;\n \t\tif (o > 0 && rs->ranges[i].start <= rs->ranges[o-1].end) {\n \t\t\tif (rs->ranges[o-1].end < rs->ranges[i].end)\n \t\t\t\trs->ranges[o-1].end = rs->ranges[i].end;\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 1db1edd..12814c0 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -67,7 +67,7 @@ test_bad_opts \"-L :foo:b.c\" \"no match\"\n # There is a separate bug when an empty -L range is the first -L encountered,\n # thus to demonstrate this particular bug, the empty -L range must follow a\n # non-empty -L range.\n-test_expect_failure '-L {empty-range} (any -L)' '\n+test_expect_success '-L {empty-range} (any -L)' '\n \tn=$(expr $(cat b.c | wc -l) + 1) &&\n \tgit log -L1,1:b.c -L$n:b.c\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223982","messageId":"1374589688-27751-5-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 4/5] t4211: demonstrate crash when first -L encountered is empty range","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:07Z","receivedAt":"2013-07-23T14:28:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Signed-off-by: Eric Sunshine \n---\n t/t4211-line-log.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 12814c0..7616365 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -72,4 +72,9 @@ test_expect_success '-L {empty-range} (any -L)' '\n \tgit log -L1,1:b.c -L$n:b.c\n '\n \n+test_expect_failure '-L {empty-range} (first -L)' '\n+\tn=$(expr $(cat b.c | wc -l) + 1) &&\n+\tgit log -L$n:b.c\n+'\n+\n test_done\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223984","messageId":"1374589688-27751-6-git-send-email-sunshine@sunshineco.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 5/5] line-log: fix \"log -LN\" crash when N is last line of file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T14:28:08Z","receivedAt":"2013-07-23T14:28:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"range-set invariants are: ranges must be (1) non-empty, (2) disjoint,\n(3) sorted in ascending order.\n\nline_log_data_insert() breaks the non-empty invariant under the\nfollowing conditions: the incoming range is empty and the pathname\nattached to the range has not yet been encountered. In this case,\nline_log_data_insert() assigns the empty range to a new line_log_data\nrecord without taking any action to ensure that the empty range is\neventually folded out. Subsequent range-set functions crash or throw an\nassertion failure upon encountering such an anomaly. Fix this bug.\n\nSigned-off-by: Eric Sunshine \n---\n line-log.c | 1 +\n t/t4211-line-log.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 6f94d56..c2d01dc 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -299,6 +299,7 @@ static void line_log_data_insert(struct line_log_data **list,\n \tp = xcalloc(1, sizeof(struct line_log_data));\n \tp->path = path;\n \trange_set_append(&p->ranges, begin, end);\n+\tsort_and_merge_range_set(&p->ranges);\n \tif (ip) {\n \t\tp->next = ip->next;\n \t\tip->next = p;\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 7616365..94267d7 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -72,7 +72,7 @@ test_expect_success '-L {empty-range} (any -L)' '\n \tgit log -L1,1:b.c -L$n:b.c\n '\n \n-test_expect_failure '-L {empty-range} (first -L)' '\n+test_expect_success '-L {empty-range} (first -L)' '\n \tn=$(expr $(cat b.c | wc -l) + 1) &&\n \tgit log -L$n:b.c\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"223987","messageId":"87ehapparc.fsf@linux-k42r.v.cablecom.net","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 0/5] range-set and line-log bug fixes","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-23T15:16:23Z","receivedAt":"2013-07-23T15:16:23Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Eric Sunshine writes:\n\n> While implementing multiple -L support for git-blame, I encountered\n> several bugs in range-set and line-log resulting in crashes. This\n> series fixes those bugs.\n\nAcked-by: Thomas Rast \n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223990","messageId":"20130723175942.GA21019@goldbirke","threadId":"34518","inReplyTo":"1374589688-27751-3-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 2/5] t4211: demonstrate empty -L range crash","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-07-23T17:59:42Z","receivedAt":"2013-07-23T17:59:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 23, 2013 at 10:28:05AM -0400, Eric Sunshine wrote:\n> Signed-off-by: Eric Sunshine \n> ---\n> t/t4211-line-log.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\n> index 7776f93..1db1edd 100755\n> --- a/t/t4211-line-log.sh\n> +++ b/t/t4211-line-log.sh\n> @@ -64,4 +64,12 @@ test_bad_opts \"-L 1,1000:b.c\" \"has only.*lines\"\n> test_bad_opts \"-L :b.c\" \"argument.*not of the form\"\n> test_bad_opts \"-L :foo:b.c\" \"no match\"\n> \n> +# There is a separate bug when an empty -L range is the first -L encountered,\n> +# thus to demonstrate this particular bug, the empty -L range must follow a\n> +# non-empty -L range.\n> +test_expect_failure '-L {empty-range} (any -L)' '\n> +\tn=$(expr $(cat b.c | wc -l) + 1) &&\n\nYou could avoid the 'cat' here and patch in 4/5 by doing $(wc -l writes:\n\n> You could avoid the 'cat' here and patch in 4/5 by doing $(wc -l (Side question: the test suite is full with similar constructs, i.e.\n> redirecting file contents to stdin, but why not just use wc -l b.c,\n> i.e. let wc open the file?)\n\n\"wc -l SZEDER Gábor writes:\n> > (Side question: the test suite is full with similar constructs, i.e.\n> > redirecting file contents to stdin, but why not just use wc -l b.c,\n> > i.e. let wc open the file?)\n> \n> \"wc -l otherwise you will see the filename in the output, and you need to\n> somehow strip it if you are only interested in the line count.\n\nFacepalm, that should have been obvious...\n"},{"id":"224013","messageId":"CAPig+cTPcFJ_kfQ7zveE-Uf-itRRw2O99vQ61Or0QHvkKnxc_g@mail.gmail.com","threadId":"34518","inReplyTo":"7vvc416qvq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] t4211: demonstrate empty -L range crash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-23T23:15:24Z","receivedAt":"2013-07-23T23:15:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 23, 2013 at 3:03 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>> You could avoid the 'cat' here and patch in 4/5 by doing $(wc -l Correct.\n\nThanks, I like that better.\n\nUnfortunately, what actually got queued on 'next', after applying this\nfix-up and re-ordering the patch series, is slightly bogus. The diff\nfor f8395edc (range-set: satisfy non-empty ranges invariant) looks\nlike this:\n\n@@ -67,7 +67,8 @@ test_bad_opts \"-L :foo:b.c\" \"no match\"\n # There is a separate bug when an empty -L range is the first -L encountered,\n # thus to demonstrate this particular bug, the empty -L range must follow a\n # non-empty -L range.\n-test_expect_failure '-L {empty-range} (any -L)' '\n+test_expect_success '-L {empty-range} (any -L)' '\n+ n=$(expr $(cat b.c | wc -l) + 1) &&\n n=$(expr $(wc -l writes:\n\n> On Tue, Jul 23, 2013 at 3:03 PM, Junio C Hamano wrote:\n>> SZEDER Gábor writes:\n>>> You could avoid the 'cat' here and patch in 4/5 by doing $(wc -l > Correct.\n>\n> Thanks, I like that better.\n>\n> Unfortunately, what actually got queued on 'next', after applying this\n> fix-up and re-ordering the patch series, is slightly bogus.\n\nThe lesson is that one should not rebase while waiting for a flight\nin a hurry X-<.\n\nWill queue the following on top.\n\nThanks for spotting; really appreciated.\n\n-- >8 --\nSubject: t4211: fix incorrect rebase at f8395edc (range-set: satisfy non-empty ranges invariant)\n\nWnen I rewrote \"cat b.c | wc -l\" into \"wc -l \n---\n t/t4211-line-log.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 00a850d..7665d67 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -68,7 +68,6 @@ test_bad_opts \"-L :foo:b.c\" \"no match\"\n # thus to demonstrate this particular bug, the empty -L range must follow a\n # non-empty -L range.\n test_expect_success '-L {empty-range} (any -L)' '\n-\tn=$(expr $(cat b.c | wc -l) + 1) &&\n \tn=$(expr $(wc -l wrote:\n> Eric Sunshine writes:\n>\n>> On Tue, Jul 23, 2013 at 3:03 PM, Junio C Hamano wrote:\n>>> SZEDER Gábor writes:\n>>>> You could avoid the 'cat' here and patch in 4/5 by doing $(wc -l >> Correct.\n>>\n>> Thanks, I like that better.\n>>\n>> Unfortunately, what actually got queued on 'next', after applying this\n>> fix-up and re-ordering the patch series, is slightly bogus.\n>\n> The lesson is that one should not rebase while waiting for a flight\n> in a hurry X-<.\n>\n> Will queue the following on top.\n\nThanks, that looks fine.\n"},{"id":"224068","messageId":"CAPig+cRWaKv6UN5pZLwonXBpHnFE7+4ELXTnE+2nLr8h83bX6Q@mail.gmail.com","threadId":"34518","inReplyTo":"1374589688-27751-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 0/5] range-set and line-log bug fixes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-25T08:03:35Z","receivedAt":"2013-07-25T08:03:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 23, 2013 at 10:28 AM, Eric Sunshine wrote:\n> While implementing multiple -L support for git-blame, I encountered\n> several bugs in range-set and line-log resulting in crashes. This\n> series fixes those bugs.\n>\n> Eric Sunshine (5):\n> range-set: fix sort_and_merge_range_set() corner case bug\n> t4211: demonstrate empty -L range crash\n> range-set: satisfy non-empty ranges invariant\n> t4211: demonstrate crash when first -L encountered is empty range\n> line-log: fix \"log -LN\" crash when N is last line of file\n\nI've run into a bit of a conundrum relating to the tests added by this\nseries, for which I could use some input.\n\nThe tests in this series identify real bugs in dealing with empty\nranges, which the subsequent patches fix. The test are possible\nbecause one can specify an empty range via blame/log -L, however, I\nnow realize that the ability for -L to create empty ranges was never\nintended or part of the design, but is in fact itself a bug. An\nexample manifestation of this bug, given a 5-line file:\n\n % git blame -L5 foo # OK, blames line 5\n % git blame -L6 foo # accepted, no error, no output, huh?\n % git blame -L7 foo # error: \"fatal: file foo has only 5 lines\"\n\nI believe that it is correct to fix this bug (and already have a fix\nlocally). The example -L6 should error out just like -L7 rather than\ncreating an empty range.\n\nFixing this bug closes the empty-range-creation loophole which is used\nby the t4211 tests added in this series and, unfortunately, there is\nno other way to create an empty range, hence no way to keep these\ntests operational. What to do?\n\n* Should we drop these new t4211 tests which guard against real potential bugs?\n\n* Should we add custom C code to the test suite to make the\nempty-range testing possible?\n\n* Should we introduce another (undocumented) loophole just for the\nsake of the tests?\n\nFor the last point, I was considering -Lfoo,+0. It is currently\nundocumented and its behavior undefined. In fact, -Lfoo,+0 and\n-Lfoo,-0 presently are interpreted as -Lfoo,+2 (definitely undefined\nbehavior). It would be possible to make -Lfoo,+0 mean empty-range and\nkeep it undocumented, which would create the loophole these tests\nrequire.\n\nI personally dislike this idea for several reasons: (1) we should be\nclosing loopholes rather than creating them intentionally; (2)\ngenerally, an empty range has no useful meaning in conjunction with\n-L; (3) if not an empty range, then -Lfoo,+0 conveys nothing and\nshould be reported as an error.\n\nThe only possible minor advantage I can see to interpreting -Lfoo,+0\nas an empty range is that it could provide an anchor for relative\n-L/RE/ searches once blame accepts multiple -L options. For example,\n\"blame -L42,+0 -L/^struct/,/^}/ foo.c\" would blame the first struct\nstarting at line 42, without also showing blame for line 42. I don't\nreally consider this a good argument in favor of -Lfoo,+0 representing\nan empty range, and it's a very poor substitute for Michael Haggerty's\nmore expressive proposal [1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/229755/focus=230967\n"},{"id":"224069","messageId":"CAPig+cQkoaCi1SHTDbHFy2Fjx09_WcOquaUti45_Z3UxOJQ33Q@mail.gmail.com","threadId":"34518","inReplyTo":"CAPig+cRWaKv6UN5pZLwonXBpHnFE7+4ELXTnE+2nLr8h83bX6Q@mail.gmail.com","subject":"Re: [PATCH 0/5] range-set and line-log bug fixes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-25T08:09:50Z","receivedAt":"2013-07-25T08:09:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 25, 2013 at 4:03 AM, Eric Sunshine wrote:\n> I don't\n> really consider this a good argument in favor of -Lfoo,+0 representing\n> an empty range, and it's a very poor substitute for Michael Haggerty's\n> more expressive proposal [1].\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/229755/focus=230967\n\nAnd here's the correct link to Michael's proposal:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/229755/focus=229995\n"},{"id":"224070","messageId":"51F0EBF5.80105@viscovery.net","threadId":"34518","inReplyTo":"CAPig+cRWaKv6UN5pZLwonXBpHnFE7+4ELXTnE+2nLr8h83bX6Q@mail.gmail.com","subject":"Re: [PATCH 0/5] range-set and line-log bug fixes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-07-25T09:12:21Z","receivedAt":"2013-07-25T09:12:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/25/2013 10:03, schrieb Eric Sunshine:\n> The tests in this series identify real bugs in dealing with empty\n> ranges, which the subsequent patches fix. The test are possible\n> because one can specify an empty range via blame/log -L, however, I\n> now realize that the ability for -L to create empty ranges was never\n> intended or part of the design, but is in fact itself a bug.\n...\n> * Should we drop these new t4211 tests which guard against real potential bugs?\n> \n> * Should we add custom C code to the test suite to make the\n> empty-range testing possible?\n> \n> * Should we introduce another (undocumented) loophole just for the\n> sake of the tests?\n\nIIUC, the tests you added are protecting the *implementation* of range-set\nfunctions. For tests of the implementation, we usually write test-foo\nprograms that call the functions directly.\n\nTests invoking git should test the observable behavior. Therefore, if\ncalling a git utility with \"-Lfoo,+0\" should be an error, then the test\nsuite should mark such a call with test_must_fail. I guess this rules out\nthe loophole approach.\n\n-- Hannes\n"},{"id":"224075","messageId":"CAPig+cToEX1b+3q-Mhw_ukvZzqSDnRR7bJymC962t+qkZE25fw@mail.gmail.com","threadId":"34518","inReplyTo":"51F0EBF5.80105@viscovery.net","subject":"Re: [PATCH 0/5] range-set and line-log bug fixes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-25T13:09:19Z","receivedAt":"2013-07-25T13:09:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 25, 2013 at 5:12 AM, Johannes Sixt wrote:\n> Am 7/25/2013 10:03, schrieb Eric Sunshine:\n>> The tests in this series identify real bugs in dealing with empty\n>> ranges, which the subsequent patches fix. The test are possible\n>> because one can specify an empty range via blame/log -L, however, I\n>> now realize that the ability for -L to create empty ranges was never\n>> intended or part of the design, but is in fact itself a bug.\n> ...\n>> * Should we drop these new t4211 tests which guard against real potential bugs?\n>>\n>> * Should we add custom C code to the test suite to make the\n>> empty-range testing possible?\n>>\n>> * Should we introduce another (undocumented) loophole just for the\n>> sake of the tests?\n>\n> IIUC, the tests you added are protecting the *implementation* of range-set\n> functions. For tests of the implementation, we usually write test-foo\n> programs that call the functions directly.\n\nYou understand correctly. The added t4211 tests check range-set and\nline-log functionality.\n\nrange-set is an implementation detail of git-log's -L and is entirely\nprivate (static to the implementation file), so there's no API to test\nvia a test-foo program. It is sufficiently generic that its API could\n(some day) be published, thus allowing a test-foo program, however,\ndoing so would involve writing documentation and covering its entire\nAPI with tests: a large enough task in itself, and quite orthogonal to\nfixing the log/blame -L loophole.\n\nline-log is partially public, however, the code in which the bug was\ndiscovered is private (static) and likely always will be since it is\nnot generic. Moreover, once the -L loophole is closed, there will be\nno way to trigger the case under consideration via its public API, so\nagain there is no opportunity for a test-foo program.\n\nThus, the question remains: What to do with these two tests once the\n-L loophole is closed? Remove them?\n\n> Tests invoking git should test the observable behavior. Therefore, if\n> calling a git utility with \"-Lfoo,+0\" should be an error, then the test\n> suite should mark such a call with test_must_fail. I guess this rules out\n> the loophole approach.\n\nIndeed, nicely stated.\n"}]}