Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2021, 05:35 UTC
Message-ID: <xmqqk0oczao3.fsf@gitster.g>
In-Reply-To: <CAOLTT8SmvRaohV+v2C9eFXyc8O+di5PfZJeWNinmm8X=Ckdveg@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 20 quoted lines

>> Is it that a check refers to one member of a union without making
>> sure that member is the one in effect in the union?  I am most
>> puzzled by the mention of "enumeration" when there does not appear
>> to be any enum involved.
>
> Sorry, I didn't make it clear. I re-describe the problem first, and then
> modify the commit messages.
>
> Suppose we are dealing with "%(notes)", the name member of our
> `used_atom` item at this time is "notes", and its member union `u` uses
> a `struct notes_option`, we fill some values in `used_atom.u.notes_option`,
>
> When we traverse in `used_atom` array in `populate_value()` and previous
> judgement like "if (starts_with(name, "refname"))" will failed, because we
> are dealing with atom "notes", but in judgement "else if
> (atom->u.remote_ref.push)",
> The value we fill in `used_atom.u.notes_option` just makes
> `used_atom.u.remote_ref.push` non-zero. This leads us into the wrong case.
>
> Is this clearer?

This time you avoided the word enumeration, and it made it clearer. The word commonly used is "condition" where you said "judgment", I think, and wit that it would probably be even more clear.

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.

So the fix was to see what atom it is by checking its name member? Is starts_with() a reliable test? A fictitious atom "pushe" will be different from "push" or "push:<something>", but will still pass that test, so from the point of view of future-proofing the tests, shouldn't it do the same check as the one at the beginning of remote_ref_atom_parser()?

Thanks.