Re: [PATCH v5] [GSOC] ref-filter: fix read invalid union member bug

From: Junio C Hamano <gitster@pobox.com>
To: "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	ZheNing Hu <adlternative@gmail.com>
Subject: Re: [PATCH v5] [GSOC] ref-filter: fix read invalid union member bug
Date: Wed, 12 May 2021 10:36:03 +0900	[thread overview]
Message-ID: <xmqq35usivi4.fsf@gitster.g> (raw)
In-Reply-To: pull.949.v5.git.1620747320947.gitgitgadget@gmail.com

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> used_atom.u is an union, and it has different members depending on
> what atom the auxiliary data the union part of the "struct
> used_atom" wants to record. At most only one of the members can be
> valid at any one time. Since the code checks u.remote_ref without
> even making sure if the atom is "push" or "push:" (which are only
> two cases that u.remote_ref.push becomes valid), but u.remote_ref
> shares the same storage for other members of the union, the check
> was reading from an invalid member, which was the bug.
>
> Modify the condition here to check whether the atom name
> equals to "push" or starts with "push:", to avoid reading the
> value of invalid member of the union.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---

Thanks.

> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 9866b1b57368..309cf699506f 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' '
>  	test_must_fail git for-each-ref --format="%(color)%(refname)"
>  '
>  
> +test_expect_success '%(color:#aa22ac) must successed' '

"succeed".

> +	test_when_finished "cd .. && rm -rf ./test" &&

Not a very good practice to chdir around, even if you have
when-finished clean-up.  Not worth risking "rm -rf" at random
places when for example somebody breaks when-finished.

Instead...

> +	mkdir test &&

Place everything below ...

> +	cd test &&
> +	git init &&
> +	cat >expect <<-\EOF &&
> +	refs/heads/main
> +	EOF
> +	git add . &&
> +	git branch -M main &&

This is in a freshly created repository without commit.  Does
"branch -M" work for such an unborn branch?  Perhaps start this
block like so:

	git init test &&
	(
		cd test &&
		test_commit initial &&
		git branch -M main &&
		cat >expect <<-\EOF &&
		refs/heads/main
                refs/tags/initial
		EOF
		git remote add origin nowhere &&
		...

> +	git commit -m "test" &&
> +	git remote add origin nowhere &&
> +	git config branch.main.remote origin &&
> +	git config branch.main.merge refs/heads/main &&
> +	git for-each-ref --format="%(color:#aa22ac)%(refname)" >actual &&
> +	test_cmp expect actual

... up to here inside a (subshell).

By the way, your use of "git branch -M" makes the test work whether
the default initial branch name is still 'master' or already 'main'
by forcing the branch used for testing to be 'main'.  Clever ;-).

Will queue with all of the above suggestions squashed in; please see
if the result is good when it is pushed out later today.

Thanks.

> +'
> +
>  test_expect_success 'left alignment is default' '
>  	cat >expect <<-\EOF &&
>  	refname is refs/heads/main    |refs/heads/main
>
> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a