Re: [PATCH 04/20] cat-file: make valid_atoms as a function parameter
From: Junio C Hamano <hidden>
Date: 2018-01-09 22:16:40
Olga Telezhnaya [off-list ref] writes:
Make valid_atoms as a function parameter, there could be another variable further. Need that for further reusing of formatting logic in cat-file.c. Signed-off-by: Olga Telezhnaia <redacted> Mentored-by: Christian Couder [off-list ref] Mentored by: Jeff King [off-list ref] --- ref-filter.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
Please be a bit more careful about your title. This change does not seem to have anything to do with "cat-file". I was not sure what you meant by "make X as a function parameter" after reading the proposed log message twice, but I am guessing that you are allowing these functions to operate on not just the global singleton but a different instance of array. I also suspect that this step may make the references to the valid_atom[] array somewhat inconsistent in that the functions that are touched by this patch would refer to the passed-in array while the remainder of the existing code still works on the global singleton. For example, parse_ref_filter_atom() is called by verify_ref_format(), but this patch does not make _it_ take the array as a parameter, and instead uses the global singleton, so anybody who wants to use verify_ref_format() with different valid_atom[] are SOL. I am not saying that this inconsistency will be a problem, but that the patch (including the proposed log message) does not explain why it is not---and it should. Thanks.
quoted hunk
diff --git a/ref-filter.c b/ref-filter.c index 1426f0c28bce7..2d6e81fe1ab00 100644 --- a/ref-filter.c +++ b/ref-filter.c@@ -396,6 +396,7 @@ struct atom_value { * Used to parse format string and sort specifiers */ static int parse_ref_filter_atom(const struct ref_format *format, + const struct valid_atom *valid_atoms, int n_atoms, const char *atom, const char *ep) { const char *sp;@@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct ref_format *format, atom_len = (arg ? arg : ep) - sp; /* Is the atom a valid one? */ - for (i = 0; i < ARRAY_SIZE(valid_atoms); i++) { + for (i = 0; i < n_atoms; i++) { int len = strlen(valid_atoms[i].name); if (len == atom_len && !memcmp(valid_atoms[i].name, sp, len)) break; } - if (ARRAY_SIZE(valid_atoms) <= i) + if (n_atoms <= i) die(_("unknown field name: %.*s"), (int)(ep-atom), atom); /* Add it in, including the deref prefix */@@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format) if (!ep) return error(_("malformed format string %s"), sp); /* sp points at "%(" and ep points at the closing ")" */ - at = parse_ref_filter_atom(format, sp + 2, ep); + at = parse_ref_filter_atom(format, valid_atoms, + ARRAY_SIZE(valid_atoms), sp + 2, ep); cp = ep + 1; if (skip_prefix(used_atoms[at].name, "color:", &color))@@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info, if (cp < sp) append_literal(cp, sp, &state); get_ref_atom_value(info, - parse_ref_filter_atom(format, sp + 2, ep), + parse_ref_filter_atom(format, valid_atoms, + ARRAY_SIZE(valid_atoms), + sp + 2, ep), &atomv); atomv->handler(atomv, &state); }@@ -2187,7 +2191,8 @@ static int parse_sorting_atom(const char *atom) */ struct ref_format dummy = REF_FORMAT_INIT; const char *end = atom + strlen(atom); - return parse_ref_filter_atom(&dummy, atom, end); + return parse_ref_filter_atom(&dummy, valid_atoms, + ARRAY_SIZE(valid_atoms), atom, end); } /* If no sorting option is given, use refname to sort as default */ --https://github.com/git/git/pull/450