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

ZheNing Hu [off-list ref] writes:

quoted

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

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.

quoted

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

Junio C Hamano [off-list ref] 于2021年6月4日周五 上午5:35写道：

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

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.

Alright, I got it: we can perform related check in verify_ref_format(),
after parse_ref_filter_atom(), It is a good checkpoint.

quoted

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?

You are right. ;-)

Thanks.
--
ZheNing Hu