Re: [PATCH 2/2] [GSOC] ref-filter: add %(raw) atom

From: ZheNing Hu <adlternative@gmail.com>
Date: Jun 3, 2021, 05:36 UTC
Message-ID: <CAOLTT8TiyAtJfcrPoBF6SECfaKOTKt24NSOtTmf84JwHrU8g1A@mail.gmail.com>
In-Reply-To: <xmqqsg1z65b8.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年6月3日周四 上午10:38写道：

Show 10 quoted lines

>
>
> These two hunks
>
>  - hoists up the code that sets 'arg' to optional string after
>    "<atom>:" and counts how long the "<atom>" is in 'atom_len'; the
>    change causes the counting done even when the same placeholder is
>    already used elsewhere (in which case we do not have to do such
>    counting);
>

I admit that I am doing repetitive work here.

Show 15 quoted lines

>  - inserts the early return to reject use of "raw" atom when
>    language specific quoting is used.
>
> It probably makes it easier to understand if the former is split
> into a separate commit, but at the same time a series with too many
> small steps is harder to manage, so let's keep them in a single
> change.
>
> But I do not think we want to add the new change at this location,
> at least for two reasons:
>
>  * The posted patch checks '!arg' to avoid rejecting "raw:size",
>    which would not scale at all.  What if you wanted to later add
>    "raw:upcase", which you must reject?
>

Yeah, the code here makes "raw" lack of scalability. Especially we want to add "%(raw:textconv)" and "%(raw:filter)" later.

>  * We do have enumerated constants for each atom types, but this
>    early check and return does string comparison.
>

Note that at this time we must compare strings... parse_ref_filter_atom() passes string form of the atom. Code block A also requires comparing strings.

------------------- Code block A:

        for (i = 0; i < used_atom_cnt; i++) {
               int len = strlen(used_atom[i].name);
               if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
                       return i;
       }
-------------------

All the following replies are based on such a fact: We will reuse used atoms as much as possible.

Think about this situation:

$ git for-each-ref --format="%(raw)" --sort="raw" --python

Since we specified --sort="raw",`parse_sorting_atom()` will be called in parse_opt_ref_sorting(), but at this time we haven't parsed --<lang> yet. So format->quote_style == 0, we cannot refuse --<lang> at this time, and a "%(raw)" atom item will be added to used_atom, when we use `verify_ref_format()` to call `parse_sorting_atom()` for the second time, we already have raw atom item in used_atom, in Code Block A we directly returned, We can't refuse --<lang> too after Code Block A. So as a last solution, we refuse --<lang> with "%(raw)" before Code Block A.

Show 11 quoted lines

> Where it belongs is either after "Is the atom a valid one?" loop
> where 'atom_len' is used to locate the placeholder's atom in the
> table of valid atoms [*], or inside raw_atom_parser().
>
>     Side note: If you read the original code, you would notice that
>     there already is a similar "this is a valid atom that appear in
>     the valid_atom[] table, but unallowed in this situation" check
>     done with .source != SOURCE_NONE conditional.  One downside is
>     that until calling raw_atom_parser(), you do not know if
>     RAW_BARE or RAW_LENGTH is requested.
>

Yes, but we need to pay attention to it is below Code Block A.

Show 18 quoted lines

> If we do inside raw_atom_parser(), it would probably look like this:
>
> +static int raw_atom_parser(const struct ref_format *format, struct used_atom *atom,
> +                               const char *arg, struct strbuf *err)
> +{
> +       if (!arg)
> +               atom->u.raw_data.option = RAW_BARE;
> +       else if (!strcmp(arg, "size"))
> +               atom->u.raw_data.option = RAW_LENGTH;
> +       else
> +               return strbuf_addf_ret(err, -1, _("unrecognized %%(raw) argument: %s"), arg);
> +
> +       if (atom->u.raw_data.option == RAW_BARE && format->quote_style)
> +               return strbuf_addf_ret(err, -1,
> +                                      _("--format=%%(raw) cannot be used with ...")...);
> +
> +       return 0;
> +}

It's same, "*.parser()" is below Code Block A.

After all, the reason why this must be done here is the ref-filter original logic has not considered rejecting a format atom and an option.

Thanks. -- ZheNing Hu