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

From: ZheNing Hu <adlternative@gmail.com>
Date: May 7, 2021, 04:32 UTC
Message-ID: <CAOLTT8QWadP9ovrUC_7m86dKh1PsawUyZ=msK5tx1utTpuCXMQ@mail.gmail.com>
In-Reply-To: <xmqq8s4ry2ye.fsf@gitster.g>

Show 8 quoted lines

> Hmph, I do no think that would be futureproof at all.  If a new atom
> "pushe" gets added, it is not all that unlikely that it would add
> its own member to the same union.  name here would be "pushe" and
> starts with "push", and upon parsing "pushe", its own member in the
> union, atom->u.X, would have been assigned to, but the code we see
> here still accesses atom->u.remote_ref.*, so you still have the same
> problem you started to solve, no?
>

Well, this "pushe" has `atom->u.X` which is different from "push" and "push:". This is indeed worth worrying about.

Show 9 quoted lines

> The check we use in remote_ref_atom_parser() to see if the atom is
> about pushing, i.e.
>
>         if (!strcmp(atom->name, "push") || starts_with(atom->name, "push:"))
>
> is unlikely to be invalidated in future changes, as this is very
> much end-user facing and changing the condition will break existing
> scripts, so that is what I was expecting you to use instead.
>

But I am afraid that the cost we paid for string matching here is too high, Since the string matching here is to illustrate that we use one of the members of the union, why can't we directly add a "union_type" member to used_atom? if we have this, we can just switch the "union_type" flag and also make the program correct. Perhaps this would be better than the large number of string matches have done here.

>
>

Thanks. -- ZheNing Hu