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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2021, 21:35 UTC
Message-ID: <xmqq35ty6391.fsf@gitster.g>
In-Reply-To: <CAOLTT8TiyAtJfcrPoBF6SECfaKOTKt24NSOtTmf84JwHrU8g1A@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 16 quoted lines

>>  * 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;
>        }
> -------------------

The point is that you can piggyback existing string comparison (which is called "parsing") and use the parsed result (i.e. if you can compare with ATOM_RAW instead of adding another strcmp(), that can be a better solution).

Show 10 quoted lines

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

That only says using parse_sorting_atom() and relying on the check in the function is still too early, and does not necessarily support the posted patch that redundantly runs strcmp().

After parsing all the command line options, we have used_atom[] fully populated and we know what host language we are quoting the result for---and that makes a good place to check for comflicting requests.

Show 22 quoted lines

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

That is something you can fix to make the code easier to follow, no?