{"thread":{"id":"54847","subject":"[PATCH] test: bisect-porcelain: fix location of files","startedAt":"2020-12-18T15:15:15Z","lastAt":"2020-12-21T21:09:32Z","messageCount":5,"participants":["Felipe Contreras","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"412566","messageId":"20201218151406.1193792-1-felipe.contreras@gmail.com","threadId":"54847","inReplyTo":null,"subject":"[PATCH] test: bisect-porcelain: fix location of files","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-18T15:14:06Z","receivedAt":"2020-12-18T15:15:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,\n2017-09-29) introduced checks for files in the $GIT_DIR directory, but\nthat variable is not always defined, and in this test file it's not.\n\nTherefore these checks always passed regardless of the presence of these\nfiles (unless the user has some /BISECT_LOG file, for some reason).\n\nLet's check the files in the correct location.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t6030-bisect-porcelain.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 34099160ed..6d5440d704 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \tgit bisect bad $HASH4 &&\n \tgit bisect reset &&\n \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_LOG\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_RUN\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_TERMS\" &&\n-\ttest_path_is_missing \"$GIT_DIR/head-name\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_HEAD\" &&\n-\ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n+\ttest_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n+\ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n+\ttest_path_is_missing \".git/BISECT_LOG\" &&\n+\ttest_path_is_missing \".git/BISECT_RUN\" &&\n+\ttest_path_is_missing \".git/BISECT_TERMS\" &&\n+\ttest_path_is_missing \".git/head-name\" &&\n+\ttest_path_is_missing \".git/BISECT_HEAD\" &&\n+\ttest_path_is_missing \".git/BISECT_START\"\n '\n \n test_done\n-- \n2.30.0.rc0\n\n"},{"id":"412568","messageId":"CAP8UFD0X5HR67T6msOhJwgAGKQfgW4JxH+pJprseBi9XAZ2rCw@mail.gmail.com","threadId":"54847","inReplyTo":"20201218151406.1193792-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: bisect-porcelain: fix location of files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-18T15:36:40Z","receivedAt":"2020-12-18T15:37:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,\n> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but\n> that variable is not always defined, and in this test file it's not.\n>\n> Therefore these checks always passed regardless of the presence of these\n> files (unless the user has some /BISECT_LOG file, for some reason).\n>\n> Let's check the files in the correct location.\n\nThis looks good to me, but...\n\n> index 34099160ed..6d5440d704 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n>         git bisect bad $HASH4 &&\n\n...if we wanted this kind of bug not to happen again, we could add a\ntest here, before `git bisect reset`, to check that here one of the\nfiles below actually exists.\n\n>         git bisect reset &&\n>         test -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_LOG\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_RUN\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_TERMS\" &&\n> -       test_path_is_missing \"$GIT_DIR/head-name\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_HEAD\" &&\n> -       test_path_is_missing \"$GIT_DIR/BISECT_START\"\n> +       test_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n> +       test_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n> +       test_path_is_missing \".git/BISECT_LOG\" &&\n> +       test_path_is_missing \".git/BISECT_RUN\" &&\n> +       test_path_is_missing \".git/BISECT_TERMS\" &&\n> +       test_path_is_missing \".git/head-name\" &&\n> +       test_path_is_missing \".git/BISECT_HEAD\" &&\n> +       test_path_is_missing \".git/BISECT_START\"\n>  '\n\nThanks,\nChristian.\n"},{"id":"412569","messageId":"5fdccfb8179d9_127502208c4@natae.notmuch","threadId":"54847","inReplyTo":"CAP8UFD0X5HR67T6msOhJwgAGKQfgW4JxH+pJprseBi9XAZ2rCw@mail.gmail.com","subject":"Re: [PATCH] test: bisect-porcelain: fix location of files","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-18T15:50:16Z","receivedAt":"2020-12-18T15:51:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Christian Couder wrote:\n> On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n\n> ...if we wanted this kind of bug not to happen again, we could add a\n> test here, before `git bisect reset`, to check that here one of the\n> files below actually exists.\n\nYeah, but that's like testing the test-case.\n\nUnless the .git directory is renamed by default (like s/master/main),\nthere would never be a need to update this in the same way.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412770","messageId":"xmqqo8imly3p.fsf@gitster.c.googlers.com","threadId":"54847","inReplyTo":"CAP8UFD0X5HR67T6msOhJwgAGKQfgW4JxH+pJprseBi9XAZ2rCw@mail.gmail.com","subject":"Re: [PATCH] test: bisect-porcelain: fix location of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-21T20:10:02Z","receivedAt":"2020-12-21T20:10:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n>>\n>> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,\n>> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but\n>> that variable is not always defined, and in this test file it's not.\n>>\n>> Therefore these checks always passed regardless of the presence of these\n>> files (unless the user has some /BISECT_LOG file, for some reason).\n>>\n>> Let's check the files in the correct location.\n>\n> This looks good to me, but...\n>\n>> index 34099160ed..6d5440d704 100755\n>> --- a/t/t6030-bisect-porcelain.sh\n>> +++ b/t/t6030-bisect-porcelain.sh\n>> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n>>         git bisect bad $HASH4 &&\n>\n> ...if we wanted this kind of bug not to happen again, we could add a\n> test here, before `git bisect reset`, to check that here one of the\n> files below actually exists.\n\nIt may make sense in the larger context of making sure \"git bisect\"\nkeeps the log in expected location.\n\nIf we do not have the basic test to do so, we probably should add\none separately.  But \"git bisect reset cleans\" test should be able\nto assume that these files are stored in their correct place, and\ntest only their removal.\n\nAn alternative and possibly a better way may be, before reset, to\nask \"git bisect\" command itself to see if it can retrieve what is\nstored in one of these files.  Perhaps \"git bisect terms\" would work\nonly when .git/BISECT_TERMS is there.  You need to cover other files\nin a similar way.\n\nSuch a testing strategy would make sure what we truly care about\nwill keep working, no matter how internal implementation changes in\nthe future.  In a sense, we do not really care the log file is\ncalled BISECT_LOG or stored directly under .git/ directory ---we\ncare that the information is kept somewhere to allow \"git bisect\" to\nwork with, and upon \"git bisect reset\", the information is wiped\naway (so you'd test that, after \"git bisect reset\", \"git bisect\nterms\" would behave differently without the BISECT_TERMS file).\n\nHmm?\n\nFor now, the hunk I see below looks like a pure improvement, but I\ndo not have the original patch handy, so... \n\n>>         git bisect reset &&\n>>         test -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_LOG\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_RUN\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_TERMS\" &&\n>> -       test_path_is_missing \"$GIT_DIR/head-name\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_HEAD\" &&\n>> -       test_path_is_missing \"$GIT_DIR/BISECT_START\"\n>> +       test_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n>> +       test_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n>> +       test_path_is_missing \".git/BISECT_LOG\" &&\n>> +       test_path_is_missing \".git/BISECT_RUN\" &&\n>> +       test_path_is_missing \".git/BISECT_TERMS\" &&\n>> +       test_path_is_missing \".git/head-name\" &&\n>> +       test_path_is_missing \".git/BISECT_HEAD\" &&\n>> +       test_path_is_missing \".git/BISECT_START\"\n>>  '\n\nThanks.\n"},{"id":"412774","messageId":"xmqqk0talve7.fsf@gitster.c.googlers.com","threadId":"54847","inReplyTo":"20201218151406.1193792-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: bisect-porcelain: fix location of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-21T21:08:32Z","receivedAt":"2020-12-21T21:09:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,\n> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but\n> that variable is not always defined, and in this test file it's not.\n\nIt is somewhat surprising that nobody caught it for 3 years, but\nperhaps nobody cared very deeply (and it must have helped that the\nfeature it purports to test is not broken).\n\nWill queue.\n\n>\n> Therefore these checks always passed regardless of the presence of these\n> files (unless the user has some /BISECT_LOG file, for some reason).\n>\n> Let's check the files in the correct location.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  t/t6030-bisect-porcelain.sh | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n\n\n\n>\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index 34099160ed..6d5440d704 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n>  \tgit bisect bad $HASH4 &&\n>  \tgit bisect reset &&\n>  \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_LOG\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_RUN\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_TERMS\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/head-name\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_HEAD\" &&\n> -\ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n> +\ttest_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n> +\ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n> +\ttest_path_is_missing \".git/BISECT_LOG\" &&\n> +\ttest_path_is_missing \".git/BISECT_RUN\" &&\n> +\ttest_path_is_missing \".git/BISECT_TERMS\" &&\n> +\ttest_path_is_missing \".git/head-name\" &&\n> +\ttest_path_is_missing \".git/BISECT_HEAD\" &&\n> +\ttest_path_is_missing \".git/BISECT_START\"\n>  '\n>  \n>  test_done\n"}]}