> 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.
>
OK.
> 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.
>
Yes, that's what it means. I got it wrong before, of course used_atom.u
is not a enum, it's union.
> 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()?
>
I think it's not necesssary. Before we call `populate_value()`,
`parse_ref_filter_atom()` only allow user use atom which match
"valid_atom" entry fully, something like "pushe" will be rejected
and process die with "fatal: unknown field name: pushe", so it didn't
pass this "starts_with()" test.
/* Is the atom a valid one? */
for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
int len = strlen(valid_atom[i].name);
if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
break;
}
if (ARRAY_SIZE(valid_atom) <= i)
return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
(int)(ep-atom), atom);
> Thanks.
Thanks.
--
ZheNing Hu
