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

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

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);

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

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

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.

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

Junio C Hamano [off-list ref] 于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

ZheNing Hu [off-list ref] 于2021年6月3日周四 下午1:36写道：

Junio C Hamano [off-list ref] 于2021年6月3日周四 上午10:38写道：

quoted

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.

Now I am building %(raw:textconv) and %(raw:filter), the code will be
very difficult to write:

        if (format->quote_style && !strncmp(sp, "raw", 3)
                                && ((!arg) || (!strncmp(arg,
":textconv", 9)) || (!strncmp(arg, ":filter", 7))))
                return strbuf_addf_ret(err, -1, _("--format=%.*s
cannot be used with"
                                "--python, --shell, --tcl, --perl"),
(int)(ep-atom), atom);

Is there any good way?

Thanks.
--
ZheNing Hu