| Message ID | 20210215154427.32693-12-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | userdiff: refactor + test + doc + misc improvements | expand |
Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason: > Simplify the test added in 9466e3809d (blame: enable funcname blaming > with userdiff driver, 2020-11-01) to use the --author support recently > added in 999cfc4f45 (test-lib functions: add --author support to > test_commit, 2021-01-12). > > We also did not need the full fortran-external-function content, let's > cut it down to just the important parts, and further modify it to > demonstrate that the fortran-specific userdiff function is in effect > by adding "WRONG" lines surrounding the "RIGHT" one. > > The test also left behind a .gitattributes files, let's clean it up > with "test_when_finished". > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/annotate-tests.sh | 36 +++++++++++++++--------------------- > 1 file changed, 15 insertions(+), 21 deletions(-) > > diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh > index 04a2c58594..4a86e0f349 100644 > --- a/t/annotate-tests.sh > +++ b/t/annotate-tests.sh > @@ -479,32 +479,26 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' ' > check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1 > ' > > -test_expect_success 'setup -L :funcname with userdiff driver' ' > - echo "fortran-* diff=fortran" >.gitattributes && > - fortran_file=fortran-external-function && > - cat >$fortran_file <<-\EOF && > +test_expect_success 'blame -L :funcname with userdiff driver' ' > + cat >file.template <<-\EOF && > + def WRONG begin end > function RIGHT(a, b) result(c) > + int WRONG(void) {} > > integer, intent(in) :: ChangeMe > - integer, intent(in) :: b > - integer, intent(out) :: c > - > - c = a+b > - > - end function RIGHT > EOF > - git add "$fortran_file" && > - GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \ > - git commit -m "add fortran file" && > - sed -e "s/ChangeMe/IWasChanged/" <"$fortran_file" >"$fortran_file".tmp && > - mv "$fortran_file".tmp "$fortran_file" && > - git add "$fortran_file" && > - GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \ > - git commit -m "change fortran file" > -' > > -test_expect_success 'blame -L :funcname with userdiff driver' ' > - check_count -f fortran-external-function -L:RIGHT A 7 B 1 > + fortran_file=file.f03 && > + test_when_finished "rm .gitattributes" && > + echo "$fortran_file diff=fortran" >.gitattributes && > + > + test_commit --author "A <A@test.git>" \ > + "add" $fortran_file \ > + "$(cat file.template)" && > + test_commit --author "B <B@test.git>" \ > + "change" $fortran_file \ > + "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" && > + check_count -f $fortran_file -L:RIGHT A 3 B 1 > ' > > test_expect_success 'setup incremental' ' > I don't get the point. What do you need the tokens "WRONG" for when they are not checked anywhere? Instead of adding unrelated lines (that do not even look like Fortran), couldn't you just not remove some of the others? In particular, the last one that contains "RIGHT" as well may be useful to keep in order to show that the code is not confused by it. Please place "$fortran_file" in dquotes on the check_count line. -- Hannes
On Mon, Feb 15 2021, Johannes Sixt wrote: > Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason: >> Simplify the test added in 9466e3809d (blame: enable funcname blaming >> with userdiff driver, 2020-11-01) to use the --author support recently >> added in 999cfc4f45 (test-lib functions: add --author support to >> test_commit, 2021-01-12). >> We also did not need the full fortran-external-function content, >> let's >> cut it down to just the important parts, and further modify it to >> demonstrate that the fortran-specific userdiff function is in effect >> by adding "WRONG" lines surrounding the "RIGHT" one. >> The test also left behind a .gitattributes files, let's clean it up >> with "test_when_finished". >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> t/annotate-tests.sh | 36 +++++++++++++++--------------------- >> 1 file changed, 15 insertions(+), 21 deletions(-) >> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh >> index 04a2c58594..4a86e0f349 100644 >> --- a/t/annotate-tests.sh >> +++ b/t/annotate-tests.sh >> @@ -479,32 +479,26 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' ' >> check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1 >> ' >> -test_expect_success 'setup -L :funcname with userdiff driver' ' >> - echo "fortran-* diff=fortran" >.gitattributes && >> - fortran_file=fortran-external-function && >> - cat >$fortran_file <<-\EOF && >> +test_expect_success 'blame -L :funcname with userdiff driver' ' >> + cat >file.template <<-\EOF && >> + def WRONG begin end >> function RIGHT(a, b) result(c) >> + int WRONG(void) {} >> integer, intent(in) :: ChangeMe >> - integer, intent(in) :: b >> - integer, intent(out) :: c >> - >> - c = a+b >> - >> - end function RIGHT >> EOF >> - git add "$fortran_file" && >> - GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \ >> - git commit -m "add fortran file" && >> - sed -e "s/ChangeMe/IWasChanged/" <"$fortran_file" >"$fortran_file".tmp && >> - mv "$fortran_file".tmp "$fortran_file" && >> - git add "$fortran_file" && >> - GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \ >> - git commit -m "change fortran file" >> -' >> -test_expect_success 'blame -L :funcname with userdiff driver' ' >> - check_count -f fortran-external-function -L:RIGHT A 7 B 1 >> + fortran_file=file.f03 && >> + test_when_finished "rm .gitattributes" && >> + echo "$fortran_file diff=fortran" >.gitattributes && >> + >> + test_commit --author "A <A@test.git>" \ >> + "add" $fortran_file \ >> + "$(cat file.template)" && >> + test_commit --author "B <B@test.git>" \ >> + "change" $fortran_file \ >> + "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" && >> + check_count -f $fortran_file -L:RIGHT A 3 B 1 >> ' >> test_expect_success 'setup incremental' ' >> > > I don't get the point. What do you need the tokens "WRONG" for when > they are not checked anywhere? Instead of adding unrelated lines (that > do not even look like Fortran), couldn't you just not remove some of > the others? In particular, the last one that contains "RIGHT" as well > may be useful to keep in order to show that the code is not confused > by it. Isn't the point of the test to assert that we're using a userdiff driver over the built-in xdiff rules here? We can imagine that a change to its default heuristics would be to find the first non-whitespace line, but it jumping over non-whitespace lines in search of a fortran-looking line doesn't seem like it would ever happen. Hence the WRONG lines. > Please place "$fortran_file" in dquotes on the check_count line. Why do we need to dquote a convenience variable defined in the test itself that'll never contain spaces or other funny things we'd get if we had $(pwd) or whatever in there? It wouldn't hurt, but maybe I'm missing some reason for why it's necessary or desired here.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> + fortran_file=file.f03 && >>> + test_when_finished "rm .gitattributes" && >>> + echo "$fortran_file diff=fortran" >.gitattributes && >>> + >>> + test_commit --author "A <A@test.git>" \ >>> + "add" $fortran_file \ >>> + "$(cat file.template)" && >>> + test_commit --author "B <B@test.git>" \ >>> + "change" $fortran_file \ >>> + "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" && >>> + check_count -f $fortran_file -L:RIGHT A 3 B 1 > ... >> Please place "$fortran_file" in dquotes on the check_count line. > > Why do we need to dquote a convenience variable defined in the test > itself that'll never contain spaces or other funny things we'd get if we > had $(pwd) or whatever in there? It wouldn't hurt, but maybe I'm missing > some reason for why it's necessary or desired here. Always dquoting when the code does not depend on splitting at IFS whitespace reduces cognitive load. The person who looks at the variable reference, $fortran_file, has to wonder if the unquoted form is used to take advantage of being split at IFS whitespaces, or the test author saved two keystrokes because the filename does not have any such whitespace, and go back to the assignment to check.
Am 17.02.21 um 02:33 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Feb 15 2021, Johannes Sixt wrote: > >> Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason: >>> Simplify the test added in 9466e3809d (blame: enable funcname blaming >>> with userdiff driver, 2020-11-01) to use the --author support recently >>> added in 999cfc4f45 (test-lib functions: add --author support to >>> test_commit, 2021-01-12). >>> We also did not need the full fortran-external-function content, >>> let's >>> cut it down to just the important parts, and further modify it to >>> demonstrate that the fortran-specific userdiff function is in effect >>> by adding "WRONG" lines surrounding the "RIGHT" one. >>> The test also left behind a .gitattributes files, let's clean it up >>> with "test_when_finished". >>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >>> --- >>> t/annotate-tests.sh | 36 +++++++++++++++--------------------- >>> 1 file changed, 15 insertions(+), 21 deletions(-) >>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh >>> index 04a2c58594..4a86e0f349 100644 >>> --- a/t/annotate-tests.sh >>> +++ b/t/annotate-tests.sh >>> @@ -479,32 +479,26 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' ' >>> check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1 >>> ' >>> -test_expect_success 'setup -L :funcname with userdiff driver' ' >>> - echo "fortran-* diff=fortran" >.gitattributes && >>> - fortran_file=fortran-external-function && >>> - cat >$fortran_file <<-\EOF && >>> +test_expect_success 'blame -L :funcname with userdiff driver' ' >>> + cat >file.template <<-\EOF && >>> + def WRONG begin end >>> function RIGHT(a, b) result(c) >>> + int WRONG(void) {} >>> integer, intent(in) :: ChangeMe >>> - integer, intent(in) :: b >>> - integer, intent(out) :: c >>> - >>> - c = a+b >>> - >>> - end function RIGHT >>> EOF >>> - git add "$fortran_file" && >>> - GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \ >>> - git commit -m "add fortran file" && >>> - sed -e "s/ChangeMe/IWasChanged/" <"$fortran_file" >"$fortran_file".tmp && >>> - mv "$fortran_file".tmp "$fortran_file" && >>> - git add "$fortran_file" && >>> - GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \ >>> - git commit -m "change fortran file" >>> -' >>> -test_expect_success 'blame -L :funcname with userdiff driver' ' >>> - check_count -f fortran-external-function -L:RIGHT A 7 B 1 >>> + fortran_file=file.f03 && >>> + test_when_finished "rm .gitattributes" && >>> + echo "$fortran_file diff=fortran" >.gitattributes && >>> + >>> + test_commit --author "A <A@test.git>" \ >>> + "add" $fortran_file \ >>> + "$(cat file.template)" && >>> + test_commit --author "B <B@test.git>" \ >>> + "change" $fortran_file \ >>> + "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" && >>> + check_count -f $fortran_file -L:RIGHT A 3 B 1 >>> ' >>> test_expect_success 'setup incremental' ' >>> >> >> I don't get the point. What do you need the tokens "WRONG" for when >> they are not checked anywhere? Instead of adding unrelated lines (that >> do not even look like Fortran), couldn't you just not remove some of >> the others? In particular, the last one that contains "RIGHT" as well >> may be useful to keep in order to show that the code is not confused >> by it. > > Isn't the point of the test to assert that we're using a userdiff driver > over the built-in xdiff rules here? > > We can imagine that a change to its default heuristics would be to find > the first non-whitespace line, but it jumping over non-whitespace lines > in search of a fortran-looking line doesn't seem like it would ever > happen. Hence the WRONG lines. Fair enough. A justification along these lines either as comment or in the commit message would have helped; the existing text in the commit message was not sufficient to get the point across. I was distracted by the capitalized "WRONG" token because we use a capitalized "RIGHT" token and check for it, so I expected that we also should check for "WRONG". Wouldn't it then just be sufficient to remove the blank lines from the exiting Fortran content? >> Please place "$fortran_file" in dquotes on the check_count line. I just noticed that there are more unquoted expansions above that line. (I'm just mentioning it for completeness.) -- Hannes
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 04a2c58594..4a86e0f349 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -479,32 +479,26 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' ' check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1 ' -test_expect_success 'setup -L :funcname with userdiff driver' ' - echo "fortran-* diff=fortran" >.gitattributes && - fortran_file=fortran-external-function && - cat >$fortran_file <<-\EOF && +test_expect_success 'blame -L :funcname with userdiff driver' ' + cat >file.template <<-\EOF && + def WRONG begin end function RIGHT(a, b) result(c) + int WRONG(void) {} integer, intent(in) :: ChangeMe - integer, intent(in) :: b - integer, intent(out) :: c - - c = a+b - - end function RIGHT EOF - git add "$fortran_file" && - GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \ - git commit -m "add fortran file" && - sed -e "s/ChangeMe/IWasChanged/" <"$fortran_file" >"$fortran_file".tmp && - mv "$fortran_file".tmp "$fortran_file" && - git add "$fortran_file" && - GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \ - git commit -m "change fortran file" -' -test_expect_success 'blame -L :funcname with userdiff driver' ' - check_count -f fortran-external-function -L:RIGHT A 7 B 1 + fortran_file=file.f03 && + test_when_finished "rm .gitattributes" && + echo "$fortran_file diff=fortran" >.gitattributes && + + test_commit --author "A <A@test.git>" \ + "add" $fortran_file \ + "$(cat file.template)" && + test_commit --author "B <B@test.git>" \ + "change" $fortran_file \ + "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" && + check_count -f $fortran_file -L:RIGHT A 3 B 1 ' test_expect_success 'setup incremental' '
Simplify the test added in 9466e3809d (blame: enable funcname blaming with userdiff driver, 2020-11-01) to use the --author support recently added in 999cfc4f45 (test-lib functions: add --author support to test_commit, 2021-01-12). We also did not need the full fortran-external-function content, let's cut it down to just the important parts, and further modify it to demonstrate that the fortran-specific userdiff function is in effect by adding "WRONG" lines surrounding the "RIGHT" one. The test also left behind a .gitattributes files, let's clean it up with "test_when_finished". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/annotate-tests.sh | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)