Junio C Hamano <gitster@xxxxxxxxx> 于2021年6月3日周四 上午10:38写道：
>
>
> 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.
> - 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.
> 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.
> 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
