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

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

From: ZheNing Hu <redacted>

Johannes Schindelin seems to have introduced a bug in
cc72385f(for-each-ref: let upstream/push optionally
report the remote name), it use `atom->u.remote_ref.option`
which is a member of enumeration in the judgment statement.

Sorry but I am not sure if our readers would understand what "a
member of enumeration in the judgment statement" is (I certainly do
not), and even more importantly, "bugs caused by enumeration" on the
title does not hint much about what problem the patch is trying to
solve.

When we use other members in the enumeration `used_atom.u`,
and it happened to fill in `remote_ref.push`, this judgment
may still be established and produces errors. So replace the
judgment statement with `starts_with(name, "push")` to fix
the error.

And this paragraph does not enlighten all that much, unfortunately.

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.

Junio C Hamano [off-list ref] 于2021年5月6日周四 上午9:53写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

From: ZheNing Hu <redacted>

Johannes Schindelin seems to have introduced a bug in
cc72385f(for-each-ref: let upstream/push optionally
report the remote name), it use `atom->u.remote_ref.option`
which is a member of enumeration in the judgment statement.

Sorry but I am not sure if our readers would understand what "a
member of enumeration in the judgment statement" is (I certainly do
not), and even more importantly, "bugs caused by enumeration" on the
title does not hint much about what problem the patch is trying to
solve.

quoted

When we use other members in the enumeration `used_atom.u`,
and it happened to fill in `remote_ref.push`, this judgment
may still be established and produces errors. So replace the
judgment statement with `starts_with(name, "push")` to fix
the error.

And this paragraph does not enlighten all that much, unfortunately.

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?

Thanks.
--
ZheNing Hu