Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-02-15 8:23 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format() because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> Hmm. So I see where you're going here, but I think in the end we'd want
> to have a single valid_atom list again, and we wouldn't need this.
>
> And indeed, it looks like this goes away in patch 17. Can we
> reorganize/rebase the series so that we avoid dead-end directions like
> this?

I tried to do that, but in my opinion it's easier to understand
everything in current version. I need to squash too many items so that
commits do not look atomic, and it's really hard to understand what is
going on. Now we have that helper commit that will be cancelled later,
and other logic is more clear for readers.

>
> -Peff

Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter

[Jeff King]

On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:

> Make valid_atom as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
> 
> We do not need to allow users to pass their own valid_atom variable in
> global functions like verify_ref_format() because in the end we want to
> have same set of valid atoms for all commands. But, as a first step
> of migrating, I create further another version of valid_atom
> for cat-file.

Hmm. So I see where you're going here, but I think in the end we'd want
to have a single valid_atom list again, and we wouldn't need this.

And indeed, it looks like this goes away in patch 17. Can we
reorganize/rebase the series so that we avoid dead-end directions like
this?

-Peff

[PATCH v3 04/23] ref-filter: make valid_atom as function parameter

[Olga Telezhnaya]

Make valid_atom as a function parameter,
there could be another variable further.
Need that for further reusing of formatting logic in cat-file.c.

We do not need to allow users to pass their own valid_atom variable in
global functions like verify_ref_format() because in the end we want to
have same set of valid atoms for all commands. But, as a first step
of migrating, I create further another version of valid_atom
for cat-file.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 9ed5e66066a7a..5e7ed0f338490 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -325,7 +325,7 @@ static void head_atom_parser(const struct ref_format 
*format, struct used_atom *
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
 }
 
-static struct {
+static struct valid_atom {
const char *name;
cmp_type cmp_type;
void (*parser)(const struct ref_format *format, struct used_atom *atom, 
const char *arg);
@@ -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_atom, 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_atom); i++) {
+   for (i = 0; i < n_atoms; i++) {
int len = strlen(valid_atom[i].name);
if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
break;
}
 
-   if (ARRAY_SIZE(valid_atom) <= 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_atom,
+  ARRAY_SIZE(valid_atom), sp + 2, ep);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
@@ -2145,7 +2147,9 @@ int format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, );
if (get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, 
ep),
+  parse_ref_filter_atom(format, valid_atom,
+
ARRAY_SIZE(valid_atom),
+sp + 2, ep),
   ))
return -1;
atomv->handler(atomv, );
@@ -2198,7 +2202,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(, atom, end);
+   return parse_ref_filter_atom(, valid_atom,
+ARRAY_SIZE(valid_atom), atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/452