Re: [PATCH v2 3/3] t3501: test cherry-pick revert with oids

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2022, 18:24 UTC
Message-ID: <xmqq7d88wtjw.fsf@gitster.g>
In-Reply-To: <20220331191525.17927-4-khalid.masum.92@gmail.com>

Labnann <khalid.masum.92@gmail.com> writes:

>  we did a revert of a file we had so that it's the same as in
> "initial", but now it's at a different path, which we can exhaustively
> do by checking the blob OIDs

Don't indent the first line by a space, and capitalize the first letter of the full sentence as usual. End the sentence with a full stop.

I did not quite get the "blob OIDs" reference; does that refer to the changes in both hunks?

Show 17 quoted lines

> Signed-off-by: Labnann <khalid.masum.92@gmail.com>
> ---
>  t/t3501-revert-cherry-pick.sh | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index bd19c272d6..08103923ab 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -72,8 +72,7 @@ test_expect_success 'cherry-pick after renaming branch' '
>  
>  	git checkout rename2 &&
>  	git cherry-pick added &&
> -	test_cmp_rev_parse HEAD^ rename2 &&
> -	test_path_is_file opos &&
> +	test_cmp_rev_parse rename2 HEAD^ &&
>  	grep "Add extra line at the end" opos &&

This change is not quite explained anywhere. Why do we have to swap HEAD^ and rename2? I agree that we do not have to make sure that opos exists since we are going to run "grep" on it in a later step anyway, and the lack of that file will be detected as a failure, but these two changes deserve mention in the proposed log message.

Show 11 quoted lines

>  	git reflog -1 | grep cherry-pick
>  
> @@ -83,9 +82,8 @@ test_expect_success 'revert after renaming branch' '
>  
>  	git checkout rename1 &&
>  	git revert added &&
> -	test_cmp_rev_parse HEAD^ rename1 &&
> -	test_path_is_file spoo &&
> -	! grep "Add extra line at the end" spoo &&
> +	test_cmp_rev_parse rename1 HEAD^ &&
> +	test_cmp_rev_parse initial:oops HEAD:spoo &&

Again, did we need to swap and if so why?

So instead of allowing spoo to be some random garbage as long as it does not have the "Add extra line" message, we can exactly predict what the contents of the correct result, which is to end up with the identical contents in oops of the initial commit. OK, that makes sense.

    Swap the order of revisions being compared in two tests for such
    and such reasons.  In one test, stop checking for the presence
    of a file (opos) because we are going to "grep" in it in the
    same test and the lack of it will be noticed as a failure
    anyway.  In the other test, instead of allowing any random
    contents as long as a known phrase is not there in it, we can
    expect the exact outcome---after the successful revert of
    "added", the contents of file "spoo" should become identical to
    what was in file "oops" in the "initial" commit.

or something like that, perhaps.

Strictly speaking, this used to check the working tree files but now it checks the contents of the resulting commit. I wonder if we need to ensure that the HEAD, the index and the working tree are all updated the same way, or if that is too basic, obvious, and (most importantly) already tested elsewhere?

Thanks.

>  	git reflog -1 | grep revert
>  
>  '