From: ZheNing Hu Date: Fri, 28 May 2021 15:04:59 GMT Subject: Re: [PATCH 1/2] [GSOC] ref-filter: add %(raw) atom Message-ID: In-Reply-To: Junio C Hamano 于2021年5月28日周五 上午11:04写道： > > "ZheNing Hu via GitGitGadget" writes: > > > The raw data of blob, tree objects may contain '\0', but most of > > the logic in `ref-filter` depands on the output of the atom being > > a structured string (end with '\0'). > > Text, yes, string is also OK. But structured? Probably not. > > ... being text (specifically, no embedded NULs in it). > OK. > > Beyond, `--format=%(raw)` should not combine with `--python`, `--shell`, > > `--tcl`, `--perl` because if our binary raw data is passed to a variable > > in the host language, the host languages may cause escape errors. > > OK. I think at least --perl and possibly --python should be able to > express NULs in the "string" type we use from the host language, but > I am perfectly fine with the decision to leave it to later updates. > Yes, for the time being, unified processing -- will be easier. > > +Note that `--format=%(raw)` should not combine with `--python`, `--shell`, `--tcl`, > > "should not combine" -> "cannot be used" would make it read more > naturally (ditto for the phrase used in the proposed log message). > OK, so the wording is better. > > > > struct atom_value { > > const char *s; > > + size_t s_size; > > OK, so everything used to be a C-string that cannot hold NULs in it, > but now it is a counted string. Good. > This suddenly reminded me of strbuf... I don't know if it is worth replacing all s, s_size with strbuf. > > @@ -588,6 +607,10 @@ static int parse_ref_filter_atom(const struct ref_format *format, > > return strbuf_addf_ret(err, -1, _("malformed field name: %.*s"), > > (int)(ep-atom), atom); > > > > + if (format->quote_style && starts_with(sp, "raw")) > > + return strbuf_addf_ret(err, -1, _("--format=%.*s should not combine with" > > + "--python, --shell, --tcl, --perl"), (int)(ep-atom), atom); > > Don't we want to allow "raw:size" that would be a plain text? > I am not sure if this check belongs here in the first place. > Shouldn't it be done in raw_atom_parser() instead? > You are right: "raw:size" should be keep, but I can't this check to raw_atom_parser(), becase "if I move it to raw_atom_parser(), when we use: `git ref-filter --format=%raw --sort=raw --python` Git will continue to run instead of die because parse_sorting_atom() will use a dummy ref_format and don't remember --language details, next time format_ref_array_item() will reuse the used_atom entry of sorting atom in parse_ref_filter_atom(), this will skip the check in raw_atom_parser()." > Another idea is to teach a more generic rule to quote_formatting() > to detect NULs in v->s[0..v->s_size] at runtime and barf, i.e. a > plain-text blob object can be used with "--shell --format=%(raw)" > just fine. > The cost of such a check is not small. Maybe can add an option such as "--only-text" to do it. > > @@ -652,11 +675,14 @@ static int parse_ref_filter_atom(const struct ref_format *format, > > return at; > > } > > > > -static void quote_formatting(struct strbuf *s, const char *str, int quote_style) > > +static void quote_formatting(struct strbuf *s, const char *str, size_t len, int quote_style) > > { > > switch (quote_style) { > > case QUOTE_NONE: > > - strbuf_addstr(s, str); > > + if (len != ATOM_VALUE_S_SIZE_INIT) > > + strbuf_add(s, str, len); > > + else > > + strbuf_addstr(s, str); > > It probably is a good idea to invent a C preprocessor macro for a > named constant to be used when a structure is initialized, but it > would be easier to read if the rule is "len field is negative when > the value is a C-string", e.g. > > if (len < 0) > I do not recognize such an approach because we are deal with "size_t s_size", if (len < 0) will never be established. I use -1 is because it's equal to 18446744073709551615 and it's impossible to have such a large file in Git. > > @@ -785,14 +814,16 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state > > return 0; > > } > > > > -static int is_empty(const char *s) > > +static int is_empty(struct strbuf *buf) > > { > > - while (*s != '\0') { > > - if (!isspace(*s)) > > - return 0; > > + const char *s = buf->buf; > > + size_t cur_len = 0; > > + > > + while ((cur_len != buf->len) && (isspace(*s) || *s == '\0')) { > > s++; > > + cur_len++; > > } > > - return 1; > > + return cur_len == buf->len; > > } > > Assuming that we do want to treat NULs the same way as whitespaces, > the updated code works as intended, which is good. But I have no > reason to support that design decision. I do not have a strong > reason to support a design decision that goes the opposite way to > treat a NUL just like we treat an 'X', but at least I can understand > it (i.e. "because we have no reason to special case NUL any more > than 'X' when trying to see if a buffer is 'empty', we don't"). > This code on the other hand must be supported with "because we need > to special case NUL for such and such reasons for the purpose of > determining if a buffer is 'empty', we treat them the same way as > whitespaces". > Something like "\0abc", from the perspective of the string, it is empty; from the perspective of the memory, it is not empty; I don't know any absolutely good solutions here. > Can the change in this commit violate the invariant that > if_then_else->str cannot be NULL, which seems to have been the case > forever as we see an unchecked strcmp() done in the original? > > If so, perhaps you can check the condition upfront, where you > compute str_len above, e.g. > > if (!if_then_else->str) { > if (if_then_else->cmp_status == COMPARE_EQUAL || > if_then_else->cmp_status == COMPARE_UNEQUAL) > BUG(...); > } else > str_len = strlen(...); > > If not, then I do not see the point of adding this (and later) check > with BUG to this code. > > Or is the invariant that .str must not be NULL could have been > violated without this patch (i.e. the original was buggy in running > strcmp() on .str without checking)? If so, please make it a separate > preliminary change to add such an assert. > The BUG() here actually acts as an "assert()". ".str must not be NULL" is right, it point to "xxx" in "%(if:equals=xxx)", so it seems that these BUG() are somewhat redundant, I will remove them. > > /* See grab_values */ > > -static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) > > +static void grab_raw_data(struct atom_value *val, int deref, void *buf, unsigned long buf_size, struct object *obj) > > { > > int i; > > const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL; > > @@ -1307,10 +1349,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) > > continue; > > if (deref) > > name++; > > - if (strcmp(name, "body") && > > - !starts_with(name, "subject") && > > - !starts_with(name, "trailers") && > > - !starts_with(name, "contents")) > > + > > + if (starts_with(name, "raw")) { > > + if (atom->u.raw_data.option == RAW_BARE) { > > + v->s = xmemdupz(buf, buf_size); > > + v->s_size = buf_size; > > + } else if (atom->u.raw_data.option == RAW_LENGTH) > > + v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size); > > + continue; > > + } > > I can understand that "raw[:]" handling has been inserted > above the existing "from here on, we only deal with log message > components" check. But > > > + if ((obj->type != OBJ_TAG && > > + obj->type != OBJ_COMMIT) || > > I do not see why these new conditions are added. The change is not > justified in the proposed log message, the original did not need > these conditions, and this does not concern the primary point of the > change, which is to start supporting %(raw[: