threads / patch / 54847

patchtest: bisect-porcelain: fix location of files

Subject: [PATCH] test: bisect-porcelain: fix location of files

## tl;dr

5 messages between Dec 18, 2020 and Dec 21, 2020. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Felipe Contreras· Dec 18, 2020, 15:14 UTC · lore

Commit ba7eafe146 (t6030: explicitly test for bisection cleanup, 2017-09-29) introduced checks for files in the $GIT_DIR directory, but that variable is not always defined, and in this test file it's not.

Therefore these checks always passed regardless of the presence of these files (unless the user has some /BISECT_LOG file, for some reason).

Let's check the files in the correct location.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Show changes to t/t6030-bisect-porcelain.sh +8 −8

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 34099160ed..6d5440d704 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect bad $HASH4 &&
 	git bisect reset &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
-	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
-	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
-	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
-	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
-	test_path_is_missing "$GIT_DIR/head-name" &&
-	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
-	test_path_is_missing "$GIT_DIR/BISECT_START"
+	test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+	test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
+	test_path_is_missing ".git/BISECT_LOG" &&
+	test_path_is_missing ".git/BISECT_RUN" &&
+	test_path_is_missing ".git/BISECT_TERMS" &&
+	test_path_is_missing ".git/head-name" &&
+	test_path_is_missing ".git/BISECT_HEAD" &&
+	test_path_is_missing ".git/BISECT_START"
 '
 
 test_done

-- 
2.30.0.rc0

Christian Couder· Dec 18, 2020, 15:36 UTC · re: Felipe Contreras · lore

Re: [PATCH] test: bisect-porcelain: fix location of files

On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 9 quoted lines

>
> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,
> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but
> that variable is not always defined, and in this test file it's not.
>
> Therefore these checks always passed regardless of the presence of these
> files (unless the user has some /BISECT_LOG file, for some reason).
>
> Let's check the files in the correct location.

This looks good to me, but...

Show 5 quoted lines

> index 34099160ed..6d5440d704 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
>         git bisect bad $HASH4 &&

...if we wanted this kind of bug not to happen again, we could add a test here, before `git bisect reset`, to check that here one of the files below actually exists.

Show 19 quoted lines

>         git bisect reset &&
>         test -z "$(git for-each-ref "refs/bisect/*")" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
> -       test_path_is_missing "$GIT_DIR/head-name" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
> -       test_path_is_missing "$GIT_DIR/BISECT_START"
> +       test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
> +       test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
> +       test_path_is_missing ".git/BISECT_LOG" &&
> +       test_path_is_missing ".git/BISECT_RUN" &&
> +       test_path_is_missing ".git/BISECT_TERMS" &&
> +       test_path_is_missing ".git/head-name" &&
> +       test_path_is_missing ".git/BISECT_HEAD" &&
> +       test_path_is_missing ".git/BISECT_START"
>  '

Thanks, Christian.

Felipe Contreras· Dec 18, 2020, 15:50 UTC · re: Christian Couder · lore

Re: [PATCH] test: bisect-porcelain: fix location of files

Christian Couder wrote:

> On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> ...if we wanted this kind of bug not to happen again, we could add a
> test here, before `git bisect reset`, to check that here one of the
> files below actually exists.

Yeah, but that's like testing the test-case.

Unless the .git directory is renamed by default (like s/master/main), there would never be a need to update this in the same way.

Cheers.

-- 
Felipe Contreras

Junio C Hamano· Dec 21, 2020, 20:10 UTC · re: Christian Couder · lore

Re: [PATCH] test: bisect-porcelain: fix location of files

Christian Couder <christian.couder@gmail.com> writes:

Show 23 quoted lines

> On Fri, Dec 18, 2020 at 4:14 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,
>> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but
>> that variable is not always defined, and in this test file it's not.
>>
>> Therefore these checks always passed regardless of the presence of these
>> files (unless the user has some /BISECT_LOG file, for some reason).
>>
>> Let's check the files in the correct location.
>
> This looks good to me, but...
>
>> index 34099160ed..6d5440d704 100755
>> --- a/t/t6030-bisect-porcelain.sh
>> +++ b/t/t6030-bisect-porcelain.sh
>> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
>>         git bisect bad $HASH4 &&
>
> ...if we wanted this kind of bug not to happen again, we could add a
> test here, before `git bisect reset`, to check that here one of the
> files below actually exists.

It may make sense in the larger context of making sure "git bisect" keeps the log in expected location.

If we do not have the basic test to do so, we probably should add one separately. But "git bisect reset cleans" test should be able to assume that these files are stored in their correct place, and test only their removal.

An alternative and possibly a better way may be, before reset, to ask "git bisect" command itself to see if it can retrieve what is stored in one of these files. Perhaps "git bisect terms" would work only when .git/BISECT_TERMS is there. You need to cover other files in a similar way.

Such a testing strategy would make sure what we truly care about will keep working, no matter how internal implementation changes in the future. In a sense, we do not really care the log file is called BISECT_LOG or stored directly under .git/ directory ---we care that the information is kept somewhere to allow "git bisect" to work with, and upon "git bisect reset", the information is wiped away (so you'd test that, after "git bisect reset", "git bisect terms" would behave differently without the BISECT_TERMS file).

Hmm?

For now, the hunk I see below looks like a pure improvement, but I do not have the original patch handy, so...

Show 19 quoted lines

>>         git bisect reset &&
>>         test -z "$(git for-each-ref "refs/bisect/*")" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
>> -       test_path_is_missing "$GIT_DIR/head-name" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
>> -       test_path_is_missing "$GIT_DIR/BISECT_START"
>> +       test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
>> +       test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
>> +       test_path_is_missing ".git/BISECT_LOG" &&
>> +       test_path_is_missing ".git/BISECT_RUN" &&
>> +       test_path_is_missing ".git/BISECT_TERMS" &&
>> +       test_path_is_missing ".git/head-name" &&
>> +       test_path_is_missing ".git/BISECT_HEAD" &&
>> +       test_path_is_missing ".git/BISECT_START"
>>  '

Thanks.

Junio C Hamano· Dec 21, 2020, 21:08 UTC · re: Felipe Contreras · lore

Re: [PATCH] test: bisect-porcelain: fix location of files

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Commit ba7eafe146 (t6030: explicitly test for bisection cleanup,
> 2017-09-29) introduced checks for files in the $GIT_DIR directory, but
> that variable is not always defined, and in this test file it's not.

It is somewhat surprising that nobody caught it for 3 years, but perhaps nobody cared very deeply (and it must have helped that the feature it purports to test is not broken).

Will queue.

Show 10 quoted lines

>
> Therefore these checks always passed regardless of the presence of these
> files (unless the user has some /BISECT_LOG file, for some reason).
>
> Let's check the files in the correct location.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t6030-bisect-porcelain.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Show 28 quoted lines

>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 34099160ed..6d5440d704 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -926,14 +926,14 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
>  	git bisect bad $HASH4 &&
>  	git bisect reset &&
>  	test -z "$(git for-each-ref "refs/bisect/*")" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
> -	test_path_is_missing "$GIT_DIR/head-name" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
> -	test_path_is_missing "$GIT_DIR/BISECT_START"
> +	test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
> +	test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
> +	test_path_is_missing ".git/BISECT_LOG" &&
> +	test_path_is_missing ".git/BISECT_RUN" &&
> +	test_path_is_missing ".git/BISECT_TERMS" &&
> +	test_path_is_missing ".git/head-name" &&
> +	test_path_is_missing ".git/BISECT_HEAD" &&
> +	test_path_is_missing ".git/BISECT_START"
>  '
>  
>  test_done