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

[Junio C Hamano <gitster@pobox.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?

> 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.

>  	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
>  
>  '