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

From: Junio C Hamano <[email-masked]>
Date: Thu, 06 May 2021 14:35:56 +0900

ZheNing Hu <[email-masked]> writes:

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