Re: [PATCH v2 02/10] t4207: test coloring of grafted decorations

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, me@ttaylorr.com,
	vdye@github.com, steadmon@google.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 02/10] t4207: test coloring of grafted decorations
Date: Wed, 03 Aug 2022 08:25:30 +0200	[thread overview]
Message-ID: <220803.86y1w5g7r0.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <b5eb110958baa80b72a345b3c850f1dfceabf076.1659122979.git.gitgitgadget@gmail.com>

On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The color.decorate.<slot> config option added the 'grafted' slot in
> 09c4ba410b0f (log-tree: allow to customize 'grafted' color, 2018-05-26)
> but included no tests for this behavior. When modifying some logic
> around decorations, this ref namespace was ignored and could have been
> lost as a default namespace for 'git log' decorations by default.
>
> Add two tests to t4207 that check that the replaced objects are
> correctly decorated. Use "black" as the color since it is distinct from
> the other colors already in the test. The first test uses regular
> replace-objects while the second creates a commit graft.
>
> Be sure to test both modes with GIT_REPLACE_REF_BASE unset and set to an
> alternative base.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/t4207-log-decoration-colors.sh | 59 ++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
> index 36ac6aff1e4..69f8ac602d6 100755
> --- a/t/t4207-log-decoration-colors.sh
> +++ b/t/t4207-log-decoration-colors.sh
> @@ -18,6 +18,7 @@ test_expect_success setup '
>  	git config color.decorate.tag "reverse bold yellow" &&
>  	git config color.decorate.stash magenta &&
>  	git config color.decorate.HEAD cyan &&
> +	git config color.decorate.grafted black &&
>  
>  	c_reset="<RESET>" &&
>  
> @@ -27,6 +28,7 @@ test_expect_success setup '
>  	c_tag="<BOLD;REVERSE;YELLOW>" &&
>  	c_stash="<MAGENTA>" &&
>  	c_HEAD="<CYAN>" &&
> +	c_grafted="<BLACK>" &&
>  
>  	test_commit A &&
>  	git clone . other &&
> @@ -63,4 +65,61 @@ test_expect_success 'Commit Decorations Colored Correctly' '
>  	test_cmp expected out
>  '
>  
> +cat >expected <<EOF
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\
> + ${c_reset}${c_branch}main${c_reset}${c_commit},\
> + ${c_reset}${c_tag}tag: D${c_reset}${c_commit})${c_reset} D
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: C${c_reset}${c_commit},\
> + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
> +EOF

I see this is used by (one) existing test, but for new test code let's
add such setup in the test_expect_success block.

A small issue, but e.g. if the partition our trash directories is on
fills up this sort of thing outside of test_expect_success hides the
source of the "first" error.

More generally (just eyeballing this v.s. the existing one) maybe we can
combine them a bit & share some of these lines?

> +test_expect_success 'test coloring with replace-objects' '
> +	test_when_finished rm -rf .git/refs/replace* &&
> +	test_commit C &&
> +	test_commit D &&
> +
> +	git replace HEAD~1 HEAD~2 &&
> +	git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD |

This hides segfaults, abort() etc. in git log, let's use an intermediate
file rather than git on the LHS of a pipe.

> +	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |

{10,10} in a regex is just {10}, no?

> +	test_decode_color >out &&
> +	test_cmp expected out &&
> +	git replace -d HEAD~1 &&
> +
> +	GIT_REPLACE_REF_BASE=refs/replace2/ git replace HEAD~1 HEAD~2 &&
> +	GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \
> +		--decorate --oneline --color=always HEAD |

Ditto LHS.

> +	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |

Ditto RX, but at this point I see it's copy/pasted from the existing
test in the file, might be worth starting by factoring the duplicate
bits out into a function...

> +	test_decode_color >out &&
> +	test_cmp expected out
> +'
> +
> +cat >expected <<EOF
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD ->\
> + ${c_reset}${c_branch}main${c_reset}${c_commit},\
> + ${c_reset}${c_tag}tag: D${c_reset}${c_commit},\
> + ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit},\
> + ${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B
> +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
> +EOF
> +
> +test_expect_success 'test coloring with grafted commit' '
> +	test_when_finished rm -rf .git/refs/replace* &&
> +
> +	git replace --graft HEAD HEAD~2 &&
> +	git log --first-parent --abbrev=10 --decorate --oneline --color=always HEAD |
> +	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> +	test_decode_color >out &&
> +	test_cmp expected out &&
> +	git replace -d HEAD &&
> +
> +	GIT_REPLACE_REF_BASE=refs/replace2/ git replace --graft HEAD HEAD~2 &&
> +	GIT_REPLACE_REF_BASE=refs/replace2/ git log --first-parent --abbrev=10 \
> +		--decorate --oneline --color=always HEAD |
> +	sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> +	test_decode_color >out &&
> +	test_cmp expected out
> +'
> +
>  test_done

...as we had one of these, and now have 3x...