ref-filter: introduce enum atom_type

RFC PATCH Superseded ZheNing Hu via GitGitGadget 4 participants 27 messages May 8, 2021 – May 13, 2021

Share & tools

ZheNing Hu via GitGitGadget

author

2021-05-08 15:22 UTC #0 lore raw

cover ref-filter: introduce enum atom_type

Firstly, let the union used_atom.u add a member "objectsize",which learn to record the attributes of "objectsize", this will bring an extension line for atom "%(objectsize)". Next patch will base on this feature to better support its functions.

Secondly, In
https://lore.kernel.org/git/CAOLTT8RhVZQJX8z1gY5UM1jv0imZ4K9UnD14MgJFfvqBBiAZQg@mail.gmail.com/
I and Junio discussed the use of enum atom_type in used_atom to represent the specific type of union used_atom.u, which can correctly distinguish atom types and reduce the overhead of string matching.

But after my attempts, I found that our used_atom is better using enum to remember the type of valid_atom item instead of remember type of used_atom.u, which can be used throughout the ref-filter, easily distinguish different types of atoms through a used_atom[i].atom_type, and reduce the overhead of string matching.

What's left: some function like grab_oid() or grab_person(), they can also take advantage of this feature, but require some additional modifications.

What's for future: Let used_atom entry can index directly by enum atom_type, In order to reduce the very many loops in ref-filter.

E.g.

static struct used_atom_cache { enum atom_type atom_type; struct used_atom *used_atom; } used_atom_cache[] = { {ATOM_REFNAME, NULL}, {ATOM_OBJECTTYPE, NULL}, ... };

If want check whether used_atom entries have a atom "%(refname)", we can just check if
used_atom_cache[ATOM_REFNAME].used_atom is equal to NULL, the time complexity is O(1) and the complexity of the original method is O(n). This will once again get a performance boost for ref-filter.

ZheNing Hu (2):
[GSOC] ref-filter: add objectsize to used_atom [GSOC][RFC] ref-filter: introduce enum atom_type

 ref-filter.c | 209 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 135 insertions(+), 74 deletions(-)

base-commit: 7e391989789db82983665667013a46eabc6fc570
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-951%2Fadlternative%2Fref-filter-atom-type-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-951/adlternative/ref-filter-atom-type-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/951

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-08 15:22 UTC #1 lore raw

PATCH 1/2 ref-filter: add objectsize to used_atom

From: ZheNing Hu <[email protected]>

Since "objectsize:size" is composed of two parts, "type:attribute". However, the original implementation did not decouple the two parts "type" and "attribute" well, we still need to judge separately whether the atom is "objectsize" or "objectsize:disk" in grab_common_values().

Add a new member objectsize to the union used_atom.u, so that we can separate the judgment of atom type from the judgment of atom attribute, This will bring scalability to atom %(objectsize).

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

ref-filter.c +13 -6

index a0adb4551d87..f420bae6e5ba 100644
@@ -146,6 +146,9 @@ static struct used_atom {
 			enum { O_FULL, O_LENGTH, O_SHORT } option;
 			unsigned int length;
 		} oid;
+		struct {
+			enum { O_SIZE, O_SIZE_DISK } option;
+		} objectsize;
 		struct email_option {
 			enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
 		} email_option;

@@ -269,11 +272,13 @@ static int objectsize_atom_parser(const struct ref_format *format, struct used_a
 				  const char *arg, struct strbuf *err)
 {
 	if (!arg) {
+		atom->u.objectsize.option = O_SIZE;
 		if (*atom->name == '*')
 			oi_deref.info.sizep = &oi_deref.size;
 		else
 			oi.info.sizep = &oi.size;
 	} else if (!strcmp(arg, "disk")) {
+		atom->u.objectsize.option = O_SIZE_DISK;
 		if (*atom->name == '*')
 			oi_deref.info.disk_sizep = &oi_deref.disk_size;
 		else

@@ -967,12 +972,14 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
-		else if (!strcmp(name, "objectsize:disk")) {
-			v->value = oi->disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
-		} else if (!strcmp(name, "objectsize")) {
-			v->value = oi->size;
-			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+		else if (starts_with(name, "objectsize")) {
+			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
+				v->value = oi->disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+			} else if (used_atom[i].u.objectsize.option == O_SIZE) {
+				v->value = oi->size;
+				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+			}
 		} else if (!strcmp(name, "deltabase"))
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
 		else if (deref)

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-08 15:22 UTC #2 lore raw

RFC PATCH 2/2 ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

In the original ref-filter design, it will copy the parsed atom's name and attributes to used_atom[i].name in the atom's parsing step, and use it again for string matching in the later specific ref attributes filling step. It use a lot of string matching to determine which atom we need.

Introduce the enum member valid_atom.atom_type which record type of each valid_atom, in the first step of the atom parsing, used_atom.atom_type will record corresponding enum value from valid_atom.atom_type, and then in specific reference attribute filling step, only need to compare the value of the used_atom.atom_type to judge the atom type.

At the same time, The value of an atom_type is the coordinate of its corresponding valid_atom entry, we can quickly index to the corresponding valid_atom entry by the atom_type value.

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 192 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 123 insertions(+), 69 deletions(-)

ref-filter.c +123 ~~-69~~

index f420bae6e5ba..4ce46e70dc99 100644
@@ -108,6 +108,50 @@ static struct ref_to_worktree_map {
 	struct worktree **worktrees;
 } ref_to_worktree_map;
 
+enum atom_type {
+ATOM_REFNAME,
+ATOM_OBJECTTYPE,
+ATOM_OBJECTSIZE,
+ATOM_OBJECTNAME,
+ATOM_DELTABASE,
+ATOM_TREE,
+ATOM_PARENT,
+ATOM_NUMPARENT,
+ATOM_OBJECT,
+ATOM_TYPE,
+ATOM_TAG,
+ATOM_AUTHOR,
+ATOM_AUTHORNAME,
+ATOM_AUTHOREMAIL,
+ATOM_AUTHORDATE,
+ATOM_COMMITTER,
+ATOM_COMMITTERNAME,
+ATOM_COMMITTEREMAIL,
+ATOM_COMMITTERDATE,
+ATOM_TAGGER,
+ATOM_TAGGERNAME,
+ATOM_TAGGEREMAIL,
+ATOM_TAGGERDATE,
+ATOM_CREATOR,
+ATOM_CREATORDATE,
+ATOM_SUBJECT,
+ATOM_BODY,
+ATOM_TRAILERS,
+ATOM_CONTENTS,
+ATOM_UPSTREAM,
+ATOM_PUSH,
+ATOM_SYMREF,
+ATOM_FLAG,
+ATOM_HEAD,
+ATOM_COLOR,
+ATOM_WORKTREEPATH,
+ATOM_ALIGN,
+ATOM_END,
+ATOM_IF,
+ATOM_THEN,
+ATOM_ELSE,
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -122,6 +166,7 @@ static struct used_atom {
 	const char *name;
 	cmp_type type;
 	info_source source;
+	enum atom_type atom_type;
 	union {
 		char color[COLOR_MAXLEN];
 		struct align align;

@@ -500,53 +545,54 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 }
 
 static struct {
+	enum atom_type atom_type;
 	const char *name;
 	info_source source;
 	cmp_type cmp_type;
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
-	{ "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
-	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
-	{ "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
-	{ "object", SOURCE_OBJ },
-	{ "type", SOURCE_OBJ },
-	{ "tag", SOURCE_OBJ },
-	{ "author", SOURCE_OBJ },
-	{ "authorname", SOURCE_OBJ },
-	{ "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "authordate", SOURCE_OBJ, FIELD_TIME },
-	{ "committer", SOURCE_OBJ },
-	{ "committername", SOURCE_OBJ },
-	{ "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "tagger", SOURCE_OBJ },
-	{ "taggername", SOURCE_OBJ },
-	{ "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "creator", SOURCE_OBJ },
-	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
-	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
-	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
-	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
-	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
-	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "flag", SOURCE_NONE },
-	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
-	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
-	{ "worktreepath", SOURCE_NONE },
-	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
-	{ "end", SOURCE_NONE },
-	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
-	{ "then", SOURCE_NONE },
-	{ "else", SOURCE_NONE },
+	{ ATOM_REFNAME, "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ ATOM_OBJECTTYPE, "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	{ ATOM_OBJECTSIZE, "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
+	{ ATOM_OBJECTNAME, "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
+	{ ATOM_DELTABASE, "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
+	{ ATOM_TREE, "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	{ ATOM_PARENT, "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	{ ATOM_NUMPARENT, "numparent", SOURCE_OBJ, FIELD_ULONG },
+	{ ATOM_OBJECT, "object", SOURCE_OBJ },
+	{ ATOM_TYPE, "type", SOURCE_OBJ },
+	{ ATOM_TAG, "tag", SOURCE_OBJ },
+	{ ATOM_AUTHOR, "author", SOURCE_OBJ },
+	{ ATOM_AUTHORNAME, "authorname", SOURCE_OBJ },
+	{ ATOM_AUTHOREMAIL, "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	{ ATOM_AUTHORDATE, "authordate", SOURCE_OBJ, FIELD_TIME },
+	{ ATOM_COMMITTER, "committer", SOURCE_OBJ },
+	{ ATOM_COMMITTERNAME, "committername", SOURCE_OBJ },
+	{ ATOM_COMMITTEREMAIL, "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	{ ATOM_COMMITTERDATE, "committerdate", SOURCE_OBJ, FIELD_TIME },
+	{ ATOM_TAGGER, "tagger", SOURCE_OBJ },
+	{ ATOM_TAGGERNAME, "taggername", SOURCE_OBJ },
+	{ ATOM_TAGGEREMAIL, "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	{ ATOM_TAGGERDATE, "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	{ ATOM_CREATOR, "creator", SOURCE_OBJ },
+	{ ATOM_CREATORDATE, "creatordate", SOURCE_OBJ, FIELD_TIME },
+	{ ATOM_SUBJECT, "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	{ ATOM_BODY, "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	{ ATOM_TRAILERS, "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	{ ATOM_CONTENTS, "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	{ ATOM_UPSTREAM, "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ ATOM_PUSH, "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ ATOM_SYMREF, "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ ATOM_FLAG, "flag", SOURCE_NONE },
+	{ ATOM_HEAD, "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	{ ATOM_COLOR, "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	{ ATOM_WORKTREEPATH, "worktreepath", SOURCE_NONE },
+	{ ATOM_ALIGN, "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	{ ATOM_END, "end", SOURCE_NONE },
+	{ ATOM_IF, "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	{ ATOM_THEN, "then", SOURCE_NONE },
+	{ ATOM_ELSE, "else", SOURCE_NONE },
 	/*
 	 * Please update $__git_ref_fieldlist in git-completion.bash
 	 * when you add new atoms

@@ -628,6 +674,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	used_atom[at].atom_type = valid_atom[i].atom_type;
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;

@@ -652,7 +699,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (valid_atom[i].atom_type == ATOM_SYMREF)
 		need_symref = 1;
 	return at;
 }

@@ -965,14 +1012,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "objecttype"))
+		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
-		else if (starts_with(name, "objectsize")) {
+		else if (atom_type == ATOM_OBJECTSIZE) {
 			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
 				v->value = oi->disk_size;
 				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

@@ -980,9 +1028,9 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 				v->value = oi->size;
 				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 			}
-		} else if (!strcmp(name, "deltabase"))
+		} else if (atom_type == ATOM_DELTABASE)
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (deref)
+		else if (atom_type == ATOM_OBJECTNAME && deref)
 			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
 	}
 }

@@ -995,16 +1043,17 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "tag"))
+		if (atom_type == ATOM_TAG)
 			v->s = xstrdup(tag->tag);
-		else if (!strcmp(name, "type") && tag->tagged)
+		else if (atom_type == ATOM_TYPE && tag->tagged)
 			v->s = xstrdup(type_name(tag->tagged->type));
-		else if (!strcmp(name, "object") && tag->tagged)
+		else if (atom_type == ATOM_OBJECT && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
 }

@@ -1017,18 +1066,20 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
+		if (atom_type == ATOM_TREE &&
+		    grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
 			continue;
-		if (!strcmp(name, "numparent")) {
+		if (atom_type == ATOM_NUMPARENT) {
 			v->value = commit_list_count(commit->parents);
 			v->s = xstrfmt("%lu", (unsigned long)v->value);
 		}
-		else if (starts_with(name, "parent")) {
+		else if (atom_type == ATOM_PARENT) {
 			struct commit_list *parents;
 			struct strbuf s = STRBUF_INIT;
 			for (parents = commit->parents; parents; parents = parents->next) {

@@ -1208,15 +1259,16 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 
-		if (starts_with(name, "creatordate"))
+		if (atom_type == ATOM_CREATORDATE)
 			grab_date(wholine, v, name);
-		else if (!strcmp(name, "creator"))
+		else if (atom_type == ATOM_CREATOR)
 			v->s = copy_line(wholine);
 	}
 }

@@ -1696,6 +1748,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
+		enum atom_type atom_type = atom->atom_type;
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;

@@ -1710,18 +1763,18 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (atom_type == ATOM_REFNAME)
 			refname = get_refname(atom, ref);
-		else if (!strcmp(name, "worktreepath")) {
+		else if (atom_type == ATOM_WORKTREEPATH) {
 			if (ref->kind == FILTER_REFS_BRANCHES)
 				v->s = get_worktree_path(atom, ref);
 			else
 				v->s = xstrdup("");
 			continue;
 		}
-		else if (starts_with(name, "symref"))
+		else if (atom_type == ATOM_SYMREF)
 			refname = get_symref(atom, ref);
-		else if (starts_with(name, "upstream")) {
+		else if (atom_type == ATOM_UPSTREAM) {
 			const char *branch_name;
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1737,7 +1790,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			else
 				v->s = xstrdup("");
 			continue;
-		} else if (atom->u.remote_ref.push) {
+		} else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) {
 			const char *branch_name;
 			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1756,10 +1809,10 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (atom_type == ATOM_COLOR) {
 			v->s = xstrdup(atom->u.color);
 			continue;
-		} else if (!strcmp(name, "flag")) {
+		} else if (atom_type == ATOM_FLAG) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");

@@ -1772,23 +1825,24 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) {
-			continue;
-		} else if (!strcmp(name, "HEAD")) {
+		} else if (!deref && atom_type == ATOM_OBJECTNAME &&
+			   grab_oid(name, "objectname", &ref->objectname, v, atom)) {
+				continue;
+		} else if (atom_type == ATOM_HEAD) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
 			else
 				v->s = xstrdup(" ");
 			continue;
-		} else if (starts_with(name, "align")) {
+		} else if (atom_type == ATOM_ALIGN) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "end")) {
+		} else if (atom_type == ATOM_END) {
 			v->handler = end_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (starts_with(name, "if")) {
+		} else if (atom_type == ATOM_IF) {
 			const char *s;
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);

@@ -1796,11 +1850,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
-		} else if (!strcmp(name, "then")) {
+		} else if (atom_type == ATOM_THEN) {
 			v->handler = then_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "else")) {
+		} else if (atom_type == ATOM_ELSE) {
 			v->handler = else_atom_handler;
 			v->s = xstrdup("");
 			continue;

Signature · 1 line

gitgitgadget

Christian Couder

2021-05-09 06:21 UTC #3 lore raw

Quoted 16 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>
In the original ref-filter design, it will copy the parsed atom's name and attributes to used_atom[i].name in the atom's parsing step, and use it again for string matching in the later specific ref attributes filling step. It use a lot of string matching to determine which atom we need.
Introduce the enum member valid_atom.atom_type which record type of each valid_atom, in the first step of the atom parsing, used_atom.atom_type will record corresponding enum value from valid_atom.atom_type, and then in specific reference attribute filling step, only need to compare the value of the used_atom.atom_type to judge the atom type.

Quoted 3 lines

At the same time, The value of an atom_type is the coordinate of its corresponding valid_atom entry, we can quickly index to the corresponding valid_atom entry by the atom_type value.

I am not sure it's worth having an atom_type field for each valid_atom element if the value of that field is already the index of the element, because then one would always be able to replace valid_atom[i].atom_type with just i. Or is it for some kind of type safety issue?

Quoted 10 lines

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 192 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 123 insertions(+), 69 deletions(-)

ref-filter.c +0 -0

index f420bae6e5ba..4ce46e70dc99 100644

ref-filter.c static struct ref_to_worktree_map { · line 112

Quoted 5 lines

ref-filter.c +2 -0

@@ -108,6 +108,50 @@ static struct ref_to_worktree_map {
        struct worktree **worktrees;
 } ref_to_worktree_map;
 
+enum atom_type {
+ATOM_REFNAME,

...
+ATOM_ELSE,
+};

I wonder if the enum should be instead defined like this:

enum atom_type {
ATOM_UNKNOWN = 0,
ATOM_REFNAME,
...
ATOM_ELSE,
ATOM_INVALID, /* should be last */
};

As a struct containing an atom_type would typically be initialized with 0 after being allocated, ATOM_UNKNOWN = 0 could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.

Having ATOM_INVALID as always the last enum value (even if some new ones are added later) could help us iterate over the valid atoms using something like:

for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)

        /* do something with valid_atom[i] */;

Quoted 13 lines

ref-filter.c +2 -0

+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -122,6 +166,7 @@ static struct used_atom {
        const char *name;
        cmp_type type;
        info_source source;
+       enum atom_type atom_type;
        union {
                char color[COLOR_MAXLEN];
                struct align align;

ref-filter.c static int head_atom_parser(const struct ref_format *format, struct used_atom *a · line 551

Quoted 7 lines

ref-filter.c +1 -0

@@ -500,53 +545,54 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 }
 
 static struct {
+       enum atom_type atom_type;
        const char *name;
        info_source source;
        cmp_type cmp_type;

I can see that the fields are already not in the same order as in struct used_atom, but my opinion is that it would be better if they would we as much as possible in the same order. Maybe one day we could even unify these structs in some way.

Also as discussed above we might not even need to add an atom_type to valid_atom[].

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 677

Quoted 9 lines

ref-filter.c +1 -0

        int (*parser)(const struct ref_format *format, struct used_atom *atom,
                      const char *arg, struct strbuf *err);
 } valid_atom[] = {

@@ -628,6 +674,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
        at = used_atom_cnt;
        used_atom_cnt++;
        REALLOC_ARRAY(used_atom, used_atom_cnt);
+       used_atom[at].atom_type = valid_atom[i].atom_type;

As discussed above, if the value of an atom_type is the coordinate of its corresponding valid_atom entry, then here the following would be simpler:

       used_atom[at].atom_type = i;

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 702

Quoted 9 lines

ref-filter.c +1 -1

        used_atom[at].name = xmemdupz(atom, ep - atom);
        used_atom[at].type = valid_atom[i].cmp_type;
        used_atom[at].source = valid_atom[i].source;

@@ -652,7 +699,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
                return -1;
        if (*atom == '*')
                need_tagged = 1;
-       if (!strcmp(valid_atom[i].name, "symref"))
+       if (valid_atom[i].atom_type == ATOM_SYMREF)

In the same way as above, the above line might be replaced with the simpler:

       if (i == ATOM_SYMREF)

Quoted 3 lines

ref-filter.c +0 -0

                need_symref = 1;
        return at;
 }

Junio C Hamano

2021-05-09 08:26 UTC #4 lore raw

Quoted 21 lines · Christian Couder

I am not sure it's worth having an atom_type field for each valid_atom element if the value of that field is already the index of the element, because then one would always be able to replace valid_atom[i].atom_type with just i. Or is it for some kind of type safety issue?
I wonder if the enum should be instead defined like this:
enum atom_type {
ATOM_UNKNOWN = 0,
ATOM_REFNAME,
...
ATOM_ELSE,
ATOM_INVALID, /* should be last */
};
As a struct containing an atom_type would typically be initialized with 0 after being allocated, ATOM_UNKNOWN = 0 could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.

Quoted 6 lines

Having ATOM_INVALID as always the last enum value (even if some new ones are added later) could help us iterate over the valid atoms using something like:
for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)
        /* do something with valid_atom[i] */;

OK.

As to "safety", I think it still makes sense to declare "enum", but I agree that we do not necessarily have to have it in the valid_atom[] struct. We could do something like this instead:

    static struct {
            const char *name;
            info_source source;
            cmp_type cmp_type;
            int (*parser)(const struct ref_format *format, struct used_atom *atom,
                          const char *arg, struct strbuf *err);
    } valid_atom[] = {
           [ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
           [ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype...
           [ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsi...
           ...

ZheNing Hu

2021-05-09 13:40 UTC #6 lore raw

In reply to Christian Couder

Quoted 11 lines · Christian Couder <[email protected]> 于2021年5月9日周日下午2:21写道：

Quoted 4 lines
At the same time, The value of an atom_type is the coordinate of its corresponding valid_atom entry, we can quickly index to the corresponding valid_atom entry by the atom_type value.
I am not sure it's worth having an atom_type field for each valid_atom element if the value of that field is already the index of the element, because then one would always be able to replace valid_atom[i].atom_type with just i. Or is it for some kind of type safety issue?

Well, I think the security issue here is just to allow used_atom and valid_atom to be correctly mapped through atom_type. We don’t want the coder to forget to update "enum atom_type" when adding new atoms to valid_atom in the future. So maybe Junio's suggestion is reasonable, we delete the member atom_type in valid_atom, but maintain the connection between atom_type and valid_atom items by specifying atom_type as array coordinates.

Quoted 15 lines

I wonder if the enum should be instead defined like this:
enum atom_type {
ATOM_UNKNOWN = 0,
ATOM_REFNAME,
...
ATOM_ELSE,
ATOM_INVALID, /* should be last */
};
As a struct containing an atom_type would typically be initialized with 0 after being allocated, ATOM_UNKNOWN = 0 could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.

Quoted 7 lines

Having ATOM_INVALID as always the last enum value (even if some new ones are added later) could help us iterate over the valid atoms using something like:
for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)
        /* do something with valid_atom[i] */;

Thanks, this suggestion is good!

Quoted 15 lines

ref-filter.c +2 -0

+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -122,6 +166,7 @@ static struct used_atom {
        const char *name;
        cmp_type type;
        info_source source;
+       enum atom_type atom_type;
        union {
                char color[COLOR_MAXLEN];
                struct align align;

@@ -500,53 +545,54 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 }

Quoted 11 lines

ref-filter.c static int head_atom_parser(const struct ref_format *format, struct used_atom *a · line 551
Quoted 5 lines
ref-filter.c +1 -0
 static struct {
+       enum atom_type atom_type;
        const char *name;
        info_source source;
        cmp_type cmp_type;
I can see that the fields are already not in the same order as in struct used_atom, but my opinion is that it would be better if they would we as much as possible in the same order. Maybe one day we could even unify these structs in some way.

Yes, atom_value, valid_atom, used_atom, It may be difficult to read for the first time. Maybe unifying them is a good direction for the future.

Quoted 3 lines

Also as discussed above we might not even need to add an atom_type to valid_atom[].

OK.

Quoted 14 lines

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 677
Quoted 9 lines
ref-filter.c +1 -0
        int (*parser)(const struct ref_format *format, struct used_atom *atom,
                      const char *arg, struct strbuf *err);
 } valid_atom[] = {
 
@@ -628,6 +674,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
        at = used_atom_cnt;
        used_atom_cnt++;
        REALLOC_ARRAY(used_atom, used_atom_cnt);
+       used_atom[at].atom_type = valid_atom[i].atom_type;
As discussed above, if the value of an atom_type is the coordinate of its corresponding valid_atom entry, then here the following would be simpler:

Quoted 2 lines

       used_atom[at].atom_type = i;

I agree.

Quoted 12 lines

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 702

Quoted 9 lines

ref-filter.c +1 -1

        used_atom[at].name = xmemdupz(atom, ep - atom);
        used_atom[at].type = valid_atom[i].cmp_type;
        used_atom[at].source = valid_atom[i].source;

@@ -652,7 +699,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
                return -1;
        if (*atom == '*')
                need_tagged = 1;
-       if (!strcmp(valid_atom[i].name, "symref"))
+       if (valid_atom[i].atom_type == ATOM_SYMREF)

In the same way as above, the above line might be replaced with the simpler:

Quoted 5 lines

       if (i == ATOM_SYMREF)

Quoted 3 lines

ref-filter.c +0 -0

                need_symref = 1;
        return at;
 }

Thanks!
--
ZheNing Hu

ZheNing Hu

2021-05-09 13:45 UTC #5 lore raw

In reply to Junio C Hamano

Quoted 37 lines · Junio C Hamano <[email protected]> 于2021年5月9日周日下午4:26写道：

Quoted 21 lines · Christian Couder
I am not sure it's worth having an atom_type field for each valid_atom element if the value of that field is already the index of the element, because then one would always be able to replace valid_atom[i].atom_type with just i. Or is it for some kind of type safety issue?
I wonder if the enum should be instead defined like this:
enum atom_type {
ATOM_UNKNOWN = 0,
ATOM_REFNAME,
...
ATOM_ELSE,
ATOM_INVALID, /* should be last */
};
As a struct containing an atom_type would typically be initialized with 0 after being allocated, ATOM_UNKNOWN = 0 could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.
Quoted 6 lines
Having ATOM_INVALID as always the last enum value (even if some new ones are added later) could help us iterate over the valid atoms using something like:
for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)
        /* do something with valid_atom[i] */;
OK.
As to "safety", I think it still makes sense to declare "enum", but I agree that we do not necessarily have to have it in the valid_atom[] struct. We could do something like this instead:

Quoted 11 lines

    static struct {
            const char *name;
            info_source source;
            cmp_type cmp_type;
            int (*parser)(const struct ref_format *format, struct used_atom *atom,
                          const char *arg, struct strbuf *err);
    } valid_atom[] = {
           [ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
           [ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype...
           [ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsi...
           ...

Thank! Good suggection. We hope that the atom_type and valid_atom items establish a clear connection. Maybe we should add some comments before the definition of the enum atom_type to remind the coder of the connection between atom_type and valid_atom.

--
ZheNing Hu

ZheNing Hu via GitGitGadget

author

2021-05-10 15:24 UTC #7 lore raw

v2 cover ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

Change from last version:

* Remove the atom_type member from valid_atom at the suggestion of Christian and Junio, and use ATOM_* as the coordinates of valid_atom.
* Add ATOM_INVALID and ATOM_UNKNOWN under Christian's suggestion, we can traverse valid_atom through them.
* Add a note about enum atom_type to remind readers of its role.

ZheNing Hu (2):
[GSOC] ref-filter: add objectsize to used_atom
[GSOC] ref-filter: introduce enum atom_type

 ref-filter.c | 231 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 155 insertions(+), 76 deletions(-)

base-commit: 7e391989789db82983665667013a46eabc6fc570
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-951%2Fadlternative%2Fref-filter-atom-type-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-951/adlternative/ref-filter-atom-type-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/951

Range-diff vs v1:

 1:  91ca57c9d04a = 1:  91ca57c9d04a [GSOC] ref-filter: add objectsize to used_atom
 2:  3770df182983 ! 2:  a1f70b39b7ef [GSOC][RFC] ref-filter: introduce enum atom_type

@@ Metadata
Author: ZheNing Hu <[email protected]>

## Commit message ##
- [GSOC][RFC] ref-filter: introduce enum atom_type
+ [GSOC] ref-filter: introduce enum atom_type

In the original ref-filter design, it will copy the parsed atom's name and attributes to used_atom[i].name in the
@@ Commit message
in the later specific ref attributes filling step. It use a lot of string matching to determine which atom we need.

- Introduce the enum member valid_atom.atom_type which
- record type of each valid_atom, in the first step of the
- atom parsing, used_atom.atom_type will record corresponding
- enum value from valid_atom.atom_type, and then in specific
- reference attribute filling step, only need to compare the
- value of the used_atom.atom_type to judge the atom type.
+ Introduce the enum "atom_type", each enum value is named
+ as ATOM_*, which is the index of each corresponding
+ valid_atom entry. In the first step of the atom parsing,
+ used_atom.atom_type will record corresponding enum value
+ from valid_atom entry index, and then in specific reference
+ attribute filling step, only need to compare the value of
+ the used_atom.atom_type to judge the atom type.

- At the same time, The value of an atom_type is the coordinate
- of its corresponding valid_atom entry, we can quickly index
- to the corresponding valid_atom entry by the atom_type value.
+ the enum value of ATOM_UNKNOWN is equals to zero, which
+ could ensure that we can easily distinguish such a struct
+ where the atom_type is known from such a struct where it
+ is unknown yet.
+
+ the enum value of ATOM_INVALID is equals to the size of
+ valid_atom array, which could help us iterate over
+ valid_atom array using something like:
+
+ for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)
+ /* do something with valid_atom[i] */;

Signed-off-by ZheNing Hu <[email protected]>

@@ ref-filter.c: static struct ref_to_worktree_map {
struct worktree **worktrees;
} ref_to_worktree_map;

++/*
++ * The enum atom_type is used as the coordinates of valid_atom entry.
++ * In the atom parsing stage, it will be passed to used_atom.atom_type
++ * as the identifier of the atom type. We can judge the type of used_atom
++ * entry by if (used_atom[i].atom_type == ATOM_*).
++ *
++ * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
++ * atom_type.
++ * ATOM_INVALID equals to the size of valid_atom array, which could help us
++ * iterate over valid_atom array like this:
++ *
++ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
++ * int len = strlen(valid_atom[i].name);
++ * if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
++ * break;
++ * }
++ */
+enum atom_type {
++ATOM_UNKNOWN,
+ATOM_REFNAME,
+ATOM_OBJECTTYPE,
+ATOM_OBJECTSIZE,
@@ ref-filter.c: static struct ref_to_worktree_map {
+ATOM_IF,
+ATOM_THEN,
+ATOM_ELSE,
++ATOM_INVALID,
+};
+
/*
* An atom is a valid field atom listed below, possibly prefixed with
* a "*" to denote deref_tag().
-@@ ref-filter.c: static struct used_atom {
- const char *name;
- cmp_type type;
- info_source source;
-+ enum atom_type atom_type;
- union {
- char color[COLOR_MAXLEN];
- struct align align;
-@@ ref-filter.c: static int head_atom_parser(const struct ref_format *format, struct used_atom *a
- }
-
- static struct {
+@@ ref-filter.c: static struct ref_to_worktree_map {
+ * array.
+ */
+ static struct used_atom {
+ enum atom_type atom_type;
const char *name;
+ cmp_type type;
info_source source;
- cmp_type cmp_type;
+@@ ref-filter.c: static struct {
int (*parser)(const struct ref_format *format, struct used_atom *atom,
const char *arg, struct strbuf *err);
} valid_atom[] = {
@@ ref-filter.c: static int head_atom_parser(const struct ref_format *format, struc
- { "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
- { "then", SOURCE_NONE },
- { "else", SOURCE_NONE },
-+ { ATOM_REFNAME, "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-+ { ATOM_OBJECTTYPE, "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
-+ { ATOM_OBJECTSIZE, "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
-+ { ATOM_OBJECTNAME, "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
-+ { ATOM_DELTABASE, "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
-+ { ATOM_TREE, "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-+ { ATOM_PARENT, "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-+ { ATOM_NUMPARENT, "numparent", SOURCE_OBJ, FIELD_ULONG },
-+ { ATOM_OBJECT, "object", SOURCE_OBJ },
-+ { ATOM_TYPE, "type", SOURCE_OBJ },
-+ { ATOM_TAG, "tag", SOURCE_OBJ },
-+ { ATOM_AUTHOR, "author", SOURCE_OBJ },
-+ { ATOM_AUTHORNAME, "authorname", SOURCE_OBJ },
-+ { ATOM_AUTHOREMAIL, "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-+ { ATOM_AUTHORDATE, "authordate", SOURCE_OBJ, FIELD_TIME },
-+ { ATOM_COMMITTER, "committer", SOURCE_OBJ },
-+ { ATOM_COMMITTERNAME, "committername", SOURCE_OBJ },
-+ { ATOM_COMMITTEREMAIL, "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-+ { ATOM_COMMITTERDATE, "committerdate", SOURCE_OBJ, FIELD_TIME },
-+ { ATOM_TAGGER, "tagger", SOURCE_OBJ },
-+ { ATOM_TAGGERNAME, "taggername", SOURCE_OBJ },
-+ { ATOM_TAGGEREMAIL, "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-+ { ATOM_TAGGERDATE, "taggerdate", SOURCE_OBJ, FIELD_TIME },
-+ { ATOM_CREATOR, "creator", SOURCE_OBJ },
-+ { ATOM_CREATORDATE, "creatordate", SOURCE_OBJ, FIELD_TIME },
-+ { ATOM_SUBJECT, "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
-+ { ATOM_BODY, "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
-+ { ATOM_TRAILERS, "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
-+ { ATOM_CONTENTS, "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
-+ { ATOM_UPSTREAM, "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-+ { ATOM_PUSH, "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-+ { ATOM_SYMREF, "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-+ { ATOM_FLAG, "flag", SOURCE_NONE },
-+ { ATOM_HEAD, "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
-+ { ATOM_COLOR, "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
-+ { ATOM_WORKTREEPATH, "worktreepath", SOURCE_NONE },
-+ { ATOM_ALIGN, "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
-+ { ATOM_END, "end", SOURCE_NONE },
-+ { ATOM_IF, "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
-+ { ATOM_THEN, "then", SOURCE_NONE },
-+ { ATOM_ELSE, "else", SOURCE_NONE },
++ [ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
++ [ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
++ [ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
++ [ATOM_OBJECTNAME] = { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
++ [ATOM_DELTABASE] = { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
++ [ATOM_TREE] = { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
++ [ATOM_PARENT] = { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
++ [ATOM_NUMPARENT] = { "numparent", SOURCE_OBJ, FIELD_ULONG },
++ [ATOM_OBJECT] = { "object", SOURCE_OBJ },
++ [ATOM_TYPE] = { "type", SOURCE_OBJ },
++ [ATOM_TAG] = { "tag", SOURCE_OBJ },
++ [ATOM_AUTHOR] = { "author", SOURCE_OBJ },
++ [ATOM_AUTHORNAME] = { "authorname", SOURCE_OBJ },
++ [ATOM_AUTHOREMAIL] = { "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
++ [ATOM_AUTHORDATE] = { "authordate", SOURCE_OBJ, FIELD_TIME },
++ [ATOM_COMMITTER] = { "committer", SOURCE_OBJ },
++ [ATOM_COMMITTERNAME] = { "committername", SOURCE_OBJ },
++ [ATOM_COMMITTEREMAIL] = { "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
++ [ATOM_COMMITTERDATE] = { "committerdate", SOURCE_OBJ, FIELD_TIME },
++ [ATOM_TAGGER] = { "tagger", SOURCE_OBJ },
++ [ATOM_TAGGERNAME] = { "taggername", SOURCE_OBJ },
++ [ATOM_TAGGEREMAIL] = { "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
++ [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
++ [ATOM_CREATOR] = { "creator", SOURCE_OBJ },
++ [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
++ [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
++ [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
++ [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
++ [ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
++ [ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
++ [ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
++ [ATOM_SYMREF] = { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
++ [ATOM_FLAG] = { "flag", SOURCE_NONE },
++ [ATOM_HEAD] = { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
++ [ATOM_COLOR] = { "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
++ [ATOM_WORKTREEPATH] = { "worktreepath", SOURCE_NONE },
++ [ATOM_ALIGN] = { "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
++ [ATOM_END] = { "end", SOURCE_NONE },
++ [ATOM_IF] = { "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
++ [ATOM_THEN] = { "then", SOURCE_NONE },
++ [ATOM_ELSE] = { "else", SOURCE_NONE },
/*
* Please update $__git_ref_fieldlist in git-completion.bash
* when you add new atoms
+@@ ref-filter.c: 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 = ATOM_UNKNOWN + 1; i < ATOM_INVALID; 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 (i == ATOM_INVALID)
+ return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
+ (int)(ep-atom), atom);
+ if (valid_atom[i].source != SOURCE_NONE && !have_git_dir())
@@ ref-filter.c: static int parse_ref_filter_atom(const struct ref_format *format,
at = used_atom_cnt;
used_atom_cnt++;
REALLOC_ARRAY(used_atom, used_atom_cnt);
-+ used_atom[at].atom_type = valid_atom[i].atom_type;
++ used_atom[at].atom_type = i;
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
used_atom[at].source = valid_atom[i].source;
@@ ref-filter.c: static int parse_ref_filter_atom(const struct ref_format *format,
if (*atom == '*')
need_tagged = 1;
- if (!strcmp(valid_atom[i].name, "symref"))
-+ if (valid_atom[i].atom_type == ATOM_SYMREF)
++ if (i == ATOM_SYMREF)
need_symref = 1;
return at;
}

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-10 15:24 UTC #8 lore raw

PATCH v2 1/2 ref-filter: add objectsize to used_atom

From: ZheNing Hu <[email protected]>

Add a new member objectsize to the union used_atom.u, so that we can separate the judgment of atom type from the judgment of atom attribute, This will bring scalability to atom %(objectsize).

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

ref-filter.c +13 -6

index a0adb4551d87..f420bae6e5ba 100644
@@ -146,6 +146,9 @@ static struct used_atom {
 			enum { O_FULL, O_LENGTH, O_SHORT } option;
 			unsigned int length;
 		} oid;
+		struct {
+			enum { O_SIZE, O_SIZE_DISK } option;
+		} objectsize;
 		struct email_option {
 			enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
 		} email_option;

@@ -269,11 +272,13 @@ static int objectsize_atom_parser(const struct ref_format *format, struct used_a
 				  const char *arg, struct strbuf *err)
 {
 	if (!arg) {
+		atom->u.objectsize.option = O_SIZE;
 		if (*atom->name == '*')
 			oi_deref.info.sizep = &oi_deref.size;
 		else
 			oi.info.sizep = &oi.size;
 	} else if (!strcmp(arg, "disk")) {
+		atom->u.objectsize.option = O_SIZE_DISK;
 		if (*atom->name == '*')
 			oi_deref.info.disk_sizep = &oi_deref.disk_size;
 		else

@@ -967,12 +972,14 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
-		else if (!strcmp(name, "objectsize:disk")) {
-			v->value = oi->disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
-		} else if (!strcmp(name, "objectsize")) {
-			v->value = oi->size;
-			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+		else if (starts_with(name, "objectsize")) {
+			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
+				v->value = oi->disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+			} else if (used_atom[i].u.objectsize.option == O_SIZE) {
+				v->value = oi->size;
+				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+			}
 		} else if (!strcmp(name, "deltabase"))
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
 		else if (deref)

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-10 15:24 UTC #9 lore raw

PATCH v2 2/2 ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Introduce the enum "atom_type", each enum value is named as ATOM_*, which is the index of each corresponding valid_atom entry. In the first step of the atom parsing, used_atom.atom_type will record corresponding enum value from valid_atom entry index, and then in specific reference attribute filling step, only need to compare the value of the used_atom.atom_type to judge the atom type.

the enum value of ATOM_UNKNOWN is equals to zero, which could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.

the enum value of ATOM_INVALID is equals to the size of valid_atom array, which could help us iterate over valid_atom array using something like:

for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)

        /* do something with valid_atom[i] */;

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 214 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 143 insertions(+), 71 deletions(-)

ref-filter.c +143 ~~-71~~

index f420bae6e5ba..e3bf2cd1aaec 100644
@@ -108,6 +108,69 @@ static struct ref_to_worktree_map {
 	struct worktree **worktrees;
 } ref_to_worktree_map;
 
+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom
+ * entry by `if (used_atom[i].atom_type == ATOM_*)`.
+ *
+ * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
+ * atom_type.
+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * 	for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
+ *		int len = strlen(valid_atom[i].name);
+ *		if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
+ *			break;
+ *	}
+ */
+enum atom_type {
+ATOM_UNKNOWN,
+ATOM_REFNAME,
+ATOM_OBJECTTYPE,
+ATOM_OBJECTSIZE,
+ATOM_OBJECTNAME,
+ATOM_DELTABASE,
+ATOM_TREE,
+ATOM_PARENT,
+ATOM_NUMPARENT,
+ATOM_OBJECT,
+ATOM_TYPE,
+ATOM_TAG,
+ATOM_AUTHOR,
+ATOM_AUTHORNAME,
+ATOM_AUTHOREMAIL,
+ATOM_AUTHORDATE,
+ATOM_COMMITTER,
+ATOM_COMMITTERNAME,
+ATOM_COMMITTEREMAIL,
+ATOM_COMMITTERDATE,
+ATOM_TAGGER,
+ATOM_TAGGERNAME,
+ATOM_TAGGEREMAIL,
+ATOM_TAGGERDATE,
+ATOM_CREATOR,
+ATOM_CREATORDATE,
+ATOM_SUBJECT,
+ATOM_BODY,
+ATOM_TRAILERS,
+ATOM_CONTENTS,
+ATOM_UPSTREAM,
+ATOM_PUSH,
+ATOM_SYMREF,
+ATOM_FLAG,
+ATOM_HEAD,
+ATOM_COLOR,
+ATOM_WORKTREEPATH,
+ATOM_ALIGN,
+ATOM_END,
+ATOM_IF,
+ATOM_THEN,
+ATOM_ELSE,
+ATOM_INVALID,
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -119,6 +182,7 @@ static struct ref_to_worktree_map {
  * array.
  */
 static struct used_atom {
+	enum atom_type atom_type;
 	const char *name;
 	cmp_type type;
 	info_source source;

@@ -506,47 +570,47 @@ static struct {
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
-	{ "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
-	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
-	{ "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
-	{ "object", SOURCE_OBJ },
-	{ "type", SOURCE_OBJ },
-	{ "tag", SOURCE_OBJ },
-	{ "author", SOURCE_OBJ },
-	{ "authorname", SOURCE_OBJ },
-	{ "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "authordate", SOURCE_OBJ, FIELD_TIME },
-	{ "committer", SOURCE_OBJ },
-	{ "committername", SOURCE_OBJ },
-	{ "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "tagger", SOURCE_OBJ },
-	{ "taggername", SOURCE_OBJ },
-	{ "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "creator", SOURCE_OBJ },
-	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
-	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
-	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
-	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
-	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
-	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "flag", SOURCE_NONE },
-	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
-	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
-	{ "worktreepath", SOURCE_NONE },
-	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
-	{ "end", SOURCE_NONE },
-	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
-	{ "then", SOURCE_NONE },
-	{ "else", SOURCE_NONE },
+	[ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	[ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
+	[ATOM_OBJECTNAME] = { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
+	[ATOM_DELTABASE] = { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
+	[ATOM_TREE] = { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_PARENT] = { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_NUMPARENT] = { "numparent", SOURCE_OBJ, FIELD_ULONG },
+	[ATOM_OBJECT] = { "object", SOURCE_OBJ },
+	[ATOM_TYPE] = { "type", SOURCE_OBJ },
+	[ATOM_TAG] = { "tag", SOURCE_OBJ },
+	[ATOM_AUTHOR] = { "author", SOURCE_OBJ },
+	[ATOM_AUTHORNAME] = { "authorname", SOURCE_OBJ },
+	[ATOM_AUTHOREMAIL] = { "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_AUTHORDATE] = { "authordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_COMMITTER] = { "committer", SOURCE_OBJ },
+	[ATOM_COMMITTERNAME] = { "committername", SOURCE_OBJ },
+	[ATOM_COMMITTEREMAIL] = { "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_COMMITTERDATE] = { "committerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_TAGGER] = { "tagger", SOURCE_OBJ },
+	[ATOM_TAGGERNAME] = { "taggername", SOURCE_OBJ },
+	[ATOM_TAGGEREMAIL] = { "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
+	[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	[ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	[ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_SYMREF] = { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_FLAG] = { "flag", SOURCE_NONE },
+	[ATOM_HEAD] = { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	[ATOM_COLOR] = { "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	[ATOM_WORKTREEPATH] = { "worktreepath", SOURCE_NONE },
+	[ATOM_ALIGN] = { "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	[ATOM_END] = { "end", SOURCE_NONE },
+	[ATOM_IF] = { "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	[ATOM_THEN] = { "then", SOURCE_NONE },
+	[ATOM_ELSE] = { "else", SOURCE_NONE },
 	/*
 	 * Please update $__git_ref_fieldlist in git-completion.bash
 	 * when you add new atoms

@@ -610,13 +674,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 = ATOM_UNKNOWN + 1; i < ATOM_INVALID; 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 (i == ATOM_INVALID)
 		return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
 				       (int)(ep-atom), atom);
 	if (valid_atom[i].source != SOURCE_NONE && !have_git_dir())

@@ -628,6 +692,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	used_atom[at].atom_type = i;
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;

@@ -652,7 +717,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (i == ATOM_SYMREF)
 		need_symref = 1;
 	return at;
 }

@@ -965,14 +1030,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "objecttype"))
+		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
-		else if (starts_with(name, "objectsize")) {
+		else if (atom_type == ATOM_OBJECTSIZE) {
 			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
 				v->value = oi->disk_size;
 				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

@@ -980,9 +1046,9 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 				v->value = oi->size;
 				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 			}
-		} else if (!strcmp(name, "deltabase"))
+		} else if (atom_type == ATOM_DELTABASE)
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (deref)
+		else if (atom_type == ATOM_OBJECTNAME && deref)
 			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
 	}
 }

@@ -995,16 +1061,17 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "tag"))
+		if (atom_type == ATOM_TAG)
 			v->s = xstrdup(tag->tag);
-		else if (!strcmp(name, "type") && tag->tagged)
+		else if (atom_type == ATOM_TYPE && tag->tagged)
 			v->s = xstrdup(type_name(tag->tagged->type));
-		else if (!strcmp(name, "object") && tag->tagged)
+		else if (atom_type == ATOM_OBJECT && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
 }

@@ -1017,18 +1084,20 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
+		if (atom_type == ATOM_TREE &&
+		    grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
 			continue;
-		if (!strcmp(name, "numparent")) {
+		if (atom_type == ATOM_NUMPARENT) {
 			v->value = commit_list_count(commit->parents);
 			v->s = xstrfmt("%lu", (unsigned long)v->value);
 		}
-		else if (starts_with(name, "parent")) {
+		else if (atom_type == ATOM_PARENT) {
 			struct commit_list *parents;
 			struct strbuf s = STRBUF_INIT;
 			for (parents = commit->parents; parents; parents = parents->next) {

@@ -1208,15 +1277,16 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 
-		if (starts_with(name, "creatordate"))
+		if (atom_type == ATOM_CREATORDATE)
 			grab_date(wholine, v, name);
-		else if (!strcmp(name, "creator"))
+		else if (atom_type == ATOM_CREATOR)
 			v->s = copy_line(wholine);
 	}
 }

@@ -1696,6 +1766,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
+		enum atom_type atom_type = atom->atom_type;
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;

@@ -1710,18 +1781,18 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (atom_type == ATOM_REFNAME)
 			refname = get_refname(atom, ref);
-		else if (!strcmp(name, "worktreepath")) {
+		else if (atom_type == ATOM_WORKTREEPATH) {
 			if (ref->kind == FILTER_REFS_BRANCHES)
 				v->s = get_worktree_path(atom, ref);
 			else
 				v->s = xstrdup("");
 			continue;
 		}
-		else if (starts_with(name, "symref"))
+		else if (atom_type == ATOM_SYMREF)
 			refname = get_symref(atom, ref);
-		else if (starts_with(name, "upstream")) {
+		else if (atom_type == ATOM_UPSTREAM) {
 			const char *branch_name;
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1737,7 +1808,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			else
 				v->s = xstrdup("");
 			continue;
-		} else if (atom->u.remote_ref.push) {
+		} else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) {
 			const char *branch_name;
 			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1756,10 +1827,10 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (atom_type == ATOM_COLOR) {
 			v->s = xstrdup(atom->u.color);
 			continue;
-		} else if (!strcmp(name, "flag")) {
+		} else if (atom_type == ATOM_FLAG) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");

@@ -1772,23 +1843,24 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) {
-			continue;
-		} else if (!strcmp(name, "HEAD")) {
+		} else if (!deref && atom_type == ATOM_OBJECTNAME &&
+			   grab_oid(name, "objectname", &ref->objectname, v, atom)) {
+				continue;
+		} else if (atom_type == ATOM_HEAD) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
 			else
 				v->s = xstrdup(" ");
 			continue;
-		} else if (starts_with(name, "align")) {
+		} else if (atom_type == ATOM_ALIGN) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "end")) {
+		} else if (atom_type == ATOM_END) {
 			v->handler = end_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (starts_with(name, "if")) {
+		} else if (atom_type == ATOM_IF) {
 			const char *s;
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);

@@ -1796,11 +1868,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
-		} else if (!strcmp(name, "then")) {
+		} else if (atom_type == ATOM_THEN) {
 			v->handler = then_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "else")) {
+		} else if (atom_type == ATOM_ELSE) {
 			v->handler = else_atom_handler;
 			v->s = xstrdup("");
 			continue;

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-05-11 02:14 UTC #10 lore raw

Quoted 1 line · ZheNing Hu via GitGitGadget

the enum value of ATOM_UNKNOWN is equals to zero, which

s/the/The/;

Quoted 5 lines

could ensure that we can easily distinguish such a struct where the atom_type is known from such a struct where it is unknown yet.
the enum value of ATOM_INVALID is equals to the size of

Ditto.

Quoted 8 lines

+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom
+ * entry by if (used_atom[i].atom_type == ATOM_*).
+ *
+ * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
+ * atom_type.

Shouldn't it be (-1)?

And I'd assume I am right in the following.

Quoted 4 lines

+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {

I find it far more intuitive to say

	for (i = 0; i < ATOM_INVALID; i++)

than having to say UNKNOWN+1.

In any case, the values should be indented, and a comment should ensure that the final one stays at the end, perhaps like this.

	enum atom_type {
		ATOM_INVALID = -2,
		ATOM_UNKNOWN = -1,
		ATOM_REFNAME,
		...
		ATOM_ELSE,
		ATOM_MAX /* MUST BE AT THE END */
	}

(note that the trailing comma is deliberately omitted).

It would allow people to say

	for (i = 0; i < ATOM_MAX; i++)

instead, which would be even nicer.

Christian Couder

2021-05-11 05:56 UTC #11 lore raw

Quoted 12 lines · Junio C Hamano

Quoted 8 lines · ZheNing Hu via GitGitGadget
+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom
+ * entry by if (used_atom[i].atom_type == ATOM_*).
+ *
+ * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
+ * atom_type.

Quoted 1 line

Shouldn't it be (-1)?

If it's -1 instead of 0, then it might be a bit more complex to initialize structs that contain such a field, as it cannot be done with only xcalloc().

Quoted 7 lines

And I'd assume I am right in the following.
Quoted 4 lines
+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {

Quoted 5 lines

I find it far more intuitive to say
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.

Yeah, that's more intuitive. But in my opinion, using ATOM_UNKNOWN + 1 instead of 0 at least shouldn't often result in more lines of code, and should be a bit easier to get right, compared to having to initialize the field with ATOM_UNKNOWN.

Quoted 10 lines

In any case, the values should be indented, and a comment should ensure that the final one stays at the end, perhaps like this.
        enum atom_type {
                ATOM_INVALID = -2,
                ATOM_UNKNOWN = -1,
                ATOM_REFNAME,
                ...
                ATOM_ELSE,
                ATOM_MAX /* MUST BE AT THE END */

I agree that a comment telling people that it must be at the end is good.

Quoted 3 lines

        }
(note that the trailing comma is deliberately omitted).

Yeah.

Quoted 5 lines

It would allow people to say
        for (i = 0; i < ATOM_MAX; i++)
instead, which would be even nicer.

Yeah, it's also a tradeoff to have the last one called ATOM_MAX instead of ATOM_INVALID, and to have a separate ATOM_INVALID if it's needed.

Junio C Hamano

2021-05-11 06:12 UTC #12 lore raw

Quoted 10 lines · Christian Couder

Quoted 5 lines
I find it far more intuitive to say
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.
Yeah, that's more intuitive. But in my opinion, using ATOM_UNKNOWN + 1 instead of 0 at least shouldn't often result in more lines of code, and should be a bit easier to get right, compared to having to initialize the field with ATOM_UNKNOWN.

Number of lines is not all that important.

But the developers must remember that UNKNOWN is at the bottom end and INVALID is at the top end, which is very taxing. Tying UNKNOWN to the top end and INVALID to the bottom end would equally be plausible and there is no memory aid to help us remember which one is which. Compare it to "array indices begin at 0, and the upper end is MAX". Your scheme is much easier for developers to screw up.

ZheNing Hu

2021-05-11 12:18 UTC #16 lore raw

In reply to Junio C Hamano

Quoted 25 lines

And I'd assume I am right in the following.
Quoted 4 lines
+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
I find it far more intuitive to say
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.
In any case, the values should be indented, and a comment should ensure that the final one stays at the end, perhaps like this.
        enum atom_type {
                ATOM_INVALID = -2,
                ATOM_UNKNOWN = -1,
                ATOM_REFNAME,
                ...
                ATOM_ELSE,
                ATOM_MAX /* MUST BE AT THE END */
        }

Quoted 7 lines

(note that the trailing comma is deliberately omitted).
It would allow people to say
        for (i = 0; i < ATOM_MAX; i++)
instead, which would be even nicer.

I think ATOM_INVALID and ATOM_MAX all will have a
similar effort. Why don't we omit one of them?

For the time being, all the used_atom entry create in parse_ref_filter_atom(), we directly use used_atom[at].atom_type = i; after realloc the used_atom. There is no time for "ATOM_UNKNOWN" at all.

I don’t know if it makes a lot of sense use "ATOM_UNKNOWN" at the moment.

--
ZheNing Hu

ZheNing Hu

2021-05-11 12:38 UTC #14 lore raw

In reply to Christian Couder

Quoted 8 lines · Christian Couder <[email protected]> 于2021年5月11日周二下午1:51写道：

Quoted 3 lines
Shouldn't it be (-1)?
If it's -1 instead of 0, then it might be a bit more complex to initialize structs that contain such a field, as it cannot be done with only xcalloc().

I agree. If the traverse start from 0, an init atom_type will have "ATOM_REFNAME" junk value. If let users manually adjust it to ATOM_UNKNOWN, it seems to be very troublesome.

Quoted 9 lines

Quoted 9 lines
And I'd assume I am right in the following.
Quoted 4 lines
+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
I find it far more intuitive to say

Quoted 9 lines

Quoted 3 lines
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.
Yeah, that's more intuitive. But in my opinion, using ATOM_UNKNOWN + 1 instead of 0 at least shouldn't often result in more lines of code, and should be a bit easier to get right, compared to having to initialize the field with ATOM_UNKNOWN.

There will be a trade-off. Traverse from 0 or does not need to adjust the initialized atom_type = UNKNOWN.

Quoted 10 lines

Quoted 6 lines
It would allow people to say
        for (i = 0; i < ATOM_MAX; i++)
instead, which would be even nicer.
Yeah, it's also a tradeoff to have the last one called ATOM_MAX instead of ATOM_INVALID, and to have a separate ATOM_INVALID if it's needed.

About ATOM_MAX or ATOM_INVALID, I have a idea:

enum atom_type {
ATOM_UNKNOWN,
...
ATOM_ELSE,
ATOM_INVALID,
+ATOM_MAX = ATOM_INVALID
};

This might be able to do both.

Thanks.
--
ZheNing Hu

ZheNing Hu

2021-05-11 12:53 UTC #13 lore raw

In reply to Junio C Hamano

Quoted 14 lines · Junio C Hamano <[email protected]> 于2021年5月11日周二下午2:12写道：

Quoted 10 lines · Christian Couder
Quoted 5 lines
I find it far more intuitive to say
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.
Yeah, that's more intuitive. But in my opinion, using ATOM_UNKNOWN + 1 instead of 0 at least shouldn't often result in more lines of code, and should be a bit easier to get right, compared to having to initialize the field with ATOM_UNKNOWN.

Quoted 9 lines

Number of lines is not all that important.
But the developers must remember that UNKNOWN is at the bottom end and INVALID is at the top end, which is very taxing. Tying UNKNOWN to the top end and INVALID to the bottom end would equally be plausible and there is no memory aid to help us remember which one is which. Compare it to "array indices begin at 0, and the upper end is MAX". Your scheme is much easier for developers to screw up.

Yes, UNKNOWN + 1 is difficult to use. But using UNKNOWN = -1, this means that the coder may indirectly use an init atom_type with junk value "ATOM_REFNAME", they maybe did't notice they need reinitialize the value to UNKNOWN.

I thought that perhaps such a naming would be better:

ATOM_BEGIN = ATOM_UNKNOWN + 1,
ATOM_END = ATOM_INVALID

       for (i = ATOM_BEGIN; i < ATOM_END; i++) {
       }

But ATOM_END has been used...

--
ZheNing Hu

Junio C Hamano

2021-05-11 13:05 UTC #15 lore raw

In reply to Christian Couder

Quoted 3 lines · Christian Couder

If it's -1 instead of 0, then it might be a bit more complex to initialize structs that contain such a field, as it cannot be done with only xcalloc().

In general, yes, but I would have thought that the codepath that allocates used_atom[] elements are pretty isolated---it is not like there are random xcalloc() all over the code. In fact, there is only one "used_atom_cnt++" in the whole file, which means there is only one place that (re)allocates the array.

	...
	at = used_atom_cnt;
	used_atom_cnt++;
	REALLOC_ARRAY(used_atom, used_atom_cnt);
	used_atom[at].name = xmemdupz(atom, ep - atom);
	used_atom[at].type = valid_atom[i].cmp_type;
	used_atom[at].source = valid_atom[i].source;
	...

So, I do not think there is even any need to worry about "initialize to invalid and fill it in as we discover what it really is"; if there were such a use pattern, UNKNOWN would be handy, but that is not what we are dealing with here. In the above snippet, we already have found from which valid_atom[] element to instantiate the new element in used_atom[] array.

Quoted 10 lines

Quoted 10 lines
Quoted 4 lines
+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
+ * iterate over valid_atom array like this:
+ *
+ * for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
I find it far more intuitive to say
        for (i = 0; i < ATOM_INVALID; i++)
than having to say UNKNOWN+1.

And here I was being embarrassingly silly.

As long as we do not waste any entry in valid_atom[] with leading gap, trailing gap or gap in the middle, the way to iterate over such an array is

	for (i = 0; i < ARRAY_SIZE(valid_atom); i++)

hence, there is no need for ATOM_MAX, and no need to burden us to remember that UNKNOWN is near the bottom of the range, and INVALID is near the top of the range.

ZheNing Hu via GitGitGadget

author

2021-05-12 12:11 UTC #17 lore raw

v3 cover ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

Change from last version: Since used_atom obtains the index from valid_atom as its atom_type after init in parse_ref_filter_atom(), here is the only place where used_atom is initialized in the global, ATOM_INVALID and ATOM_UNKNOWN seem to cause unnecessary trouble.

So remove ATOM_INVALID and ATOM_UNKNOWN, Use the original method of traversing valid_atom.

ZheNing Hu (2):
[GSOC] ref-filter: add objectsize to used_atom
[GSOC] ref-filter: introduce enum atom_type

 ref-filter.c | 214 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 140 insertions(+), 74 deletions(-)

base-commit: 7e391989789db82983665667013a46eabc6fc570
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-951%2Fadlternative%2Fref-filter-atom-type-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-951/adlternative/ref-filter-atom-type-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/951

Range-diff vs v2:

 1:  91ca57c9d04a = 1:  91ca57c9d04a [GSOC] ref-filter: add objectsize to used_atom
 2:  a1f70b39b7ef ! 2:  43400cac58e7 [GSOC] ref-filter: introduce enum atom_type
     @@ Commit message
          `used_atom.atom_type` will record corresponding enum value
          from valid_atom entry index, and then in specific reference
          attribute filling step, only need to compare the value of
     -    the `used_atom.atom_type` to judge the atom type.
     -
     -    the enum value of `ATOM_UNKNOWN` is equals to zero, which
     -    could ensure that we can easily distinguish such a struct
     -    where the atom_type is known from such a struct where it
     -    is unknown yet.
     -
     -    the enum value of `ATOM_INVALID` is equals to the size of
     -    valid_atom array, which could help us iterate over
     -    valid_atom array using something like:
     -
     -    for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++)
     -            /* do something with valid_atom[i] */;
     +    the `used_atom[i].atom_type` to judge the atom type.
      
     +    Helped-by: Junio C Hamano <[email protected]>
     +    Helped-by: Christian Couder <[email protected]>
          Signed-off-by: ZheNing Hu <[email protected]>
      
       ## ref-filter.c ##
     @@ ref-filter.c: static struct ref_to_worktree_map {
      + * In the atom parsing stage, it will be passed to used_atom.atom_type
      + * as the identifier of the atom type. We can judge the type of used_atom
      + * entry by `if (used_atom[i].atom_type == ATOM_*)`.
     -+ *
     -+ * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
     -+ * atom_type.
     -+ * ATOM_INVALID equals to the size of valid_atom array, which could help us
     -+ * iterate over valid_atom array like this:
     -+ *
     -+ * 	for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
     -+ *		int len = strlen(valid_atom[i].name);
     -+ *		if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
     -+ *			break;
     -+ *	}
      + */
      +enum atom_type {
     -+ATOM_UNKNOWN,
     -+ATOM_REFNAME,
     -+ATOM_OBJECTTYPE,
     -+ATOM_OBJECTSIZE,
     -+ATOM_OBJECTNAME,
     -+ATOM_DELTABASE,
     -+ATOM_TREE,
     -+ATOM_PARENT,
     -+ATOM_NUMPARENT,
     -+ATOM_OBJECT,
     -+ATOM_TYPE,
     -+ATOM_TAG,
     -+ATOM_AUTHOR,
     -+ATOM_AUTHORNAME,
     -+ATOM_AUTHOREMAIL,
     -+ATOM_AUTHORDATE,
     -+ATOM_COMMITTER,
     -+ATOM_COMMITTERNAME,
     -+ATOM_COMMITTEREMAIL,
     -+ATOM_COMMITTERDATE,
     -+ATOM_TAGGER,
     -+ATOM_TAGGERNAME,
     -+ATOM_TAGGEREMAIL,
     -+ATOM_TAGGERDATE,
     -+ATOM_CREATOR,
     -+ATOM_CREATORDATE,
     -+ATOM_SUBJECT,
     -+ATOM_BODY,
     -+ATOM_TRAILERS,
     -+ATOM_CONTENTS,
     -+ATOM_UPSTREAM,
     -+ATOM_PUSH,
     -+ATOM_SYMREF,
     -+ATOM_FLAG,
     -+ATOM_HEAD,
     -+ATOM_COLOR,
     -+ATOM_WORKTREEPATH,
     -+ATOM_ALIGN,
     -+ATOM_END,
     -+ATOM_IF,
     -+ATOM_THEN,
     -+ATOM_ELSE,
     -+ATOM_INVALID,
     ++	ATOM_REFNAME,
     ++	ATOM_OBJECTTYPE,
     ++	ATOM_OBJECTSIZE,
     ++	ATOM_OBJECTNAME,
     ++	ATOM_DELTABASE,
     ++	ATOM_TREE,
     ++	ATOM_PARENT,
     ++	ATOM_NUMPARENT,
     ++	ATOM_OBJECT,
     ++	ATOM_TYPE,
     ++	ATOM_TAG,
     ++	ATOM_AUTHOR,
     ++	ATOM_AUTHORNAME,
     ++	ATOM_AUTHOREMAIL,
     ++	ATOM_AUTHORDATE,
     ++	ATOM_COMMITTER,
     ++	ATOM_COMMITTERNAME,
     ++	ATOM_COMMITTEREMAIL,
     ++	ATOM_COMMITTERDATE,
     ++	ATOM_TAGGER,
     ++	ATOM_TAGGERNAME,
     ++	ATOM_TAGGEREMAIL,
     ++	ATOM_TAGGERDATE,
     ++	ATOM_CREATOR,
     ++	ATOM_CREATORDATE,
     ++	ATOM_SUBJECT,
     ++	ATOM_BODY,
     ++	ATOM_TRAILERS,
     ++	ATOM_CONTENTS,
     ++	ATOM_UPSTREAM,
     ++	ATOM_PUSH,
     ++	ATOM_SYMREF,
     ++	ATOM_FLAG,
     ++	ATOM_HEAD,
     ++	ATOM_COLOR,
     ++	ATOM_WORKTREEPATH,
     ++	ATOM_ALIGN,
     ++	ATOM_END,
     ++	ATOM_IF,
     ++	ATOM_THEN,
     ++	ATOM_ELSE,
      +};
      +
       /*
     @@ ref-filter.c: static struct {
       	/*
       	 * Please update $__git_ref_fieldlist in git-completion.bash
       	 * when you add new atoms
     -@@ ref-filter.c: 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 = ATOM_UNKNOWN + 1; i < ATOM_INVALID; 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 (i == ATOM_INVALID)
     - 		return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
     - 				       (int)(ep-atom), atom);
     - 	if (valid_atom[i].source != SOURCE_NONE && !have_git_dir())
      @@ ref-filter.c: static int parse_ref_filter_atom(const struct ref_format *format,
       	at = used_atom_cnt;
       	used_atom_cnt++;

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-12 12:11 UTC #18 lore raw

PATCH v3 1/2 ref-filter: add objectsize to used_atom

From: ZheNing Hu <[email protected]>

Add a new member objectsize to the union used_atom.u, so that we can separate the judgment of atom type from the judgment of atom attribute, This will bring scalability to atom %(objectsize).

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

ref-filter.c +13 -6

index a0adb4551d87..f420bae6e5ba 100644
@@ -146,6 +146,9 @@ static struct used_atom {
 			enum { O_FULL, O_LENGTH, O_SHORT } option;
 			unsigned int length;
 		} oid;
+		struct {
+			enum { O_SIZE, O_SIZE_DISK } option;
+		} objectsize;
 		struct email_option {
 			enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
 		} email_option;

@@ -269,11 +272,13 @@ static int objectsize_atom_parser(const struct ref_format *format, struct used_a
 				  const char *arg, struct strbuf *err)
 {
 	if (!arg) {
+		atom->u.objectsize.option = O_SIZE;
 		if (*atom->name == '*')
 			oi_deref.info.sizep = &oi_deref.size;
 		else
 			oi.info.sizep = &oi.size;
 	} else if (!strcmp(arg, "disk")) {
+		atom->u.objectsize.option = O_SIZE_DISK;
 		if (*atom->name == '*')
 			oi_deref.info.disk_sizep = &oi_deref.disk_size;
 		else

@@ -967,12 +972,14 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
-		else if (!strcmp(name, "objectsize:disk")) {
-			v->value = oi->disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
-		} else if (!strcmp(name, "objectsize")) {
-			v->value = oi->size;
-			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+		else if (starts_with(name, "objectsize")) {
+			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
+				v->value = oi->disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+			} else if (used_atom[i].u.objectsize.option == O_SIZE) {
+				v->value = oi->size;
+				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+			}
 		} else if (!strcmp(name, "deltabase"))
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
 		else if (deref)

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-12 12:11 UTC #21 lore raw

PATCH v3 2/2 ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 197 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 128 insertions(+), 69 deletions(-)

ref-filter.c +128 ~~-69~~

index f420bae6e5ba..cf36716b76e3 100644
@@ -108,6 +108,56 @@ static struct ref_to_worktree_map {
 	struct worktree **worktrees;
 } ref_to_worktree_map;
 
+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom
+ * entry by `if (used_atom[i].atom_type == ATOM_*)`.
+ */
+enum atom_type {
+	ATOM_REFNAME,
+	ATOM_OBJECTTYPE,
+	ATOM_OBJECTSIZE,
+	ATOM_OBJECTNAME,
+	ATOM_DELTABASE,
+	ATOM_TREE,
+	ATOM_PARENT,
+	ATOM_NUMPARENT,
+	ATOM_OBJECT,
+	ATOM_TYPE,
+	ATOM_TAG,
+	ATOM_AUTHOR,
+	ATOM_AUTHORNAME,
+	ATOM_AUTHOREMAIL,
+	ATOM_AUTHORDATE,
+	ATOM_COMMITTER,
+	ATOM_COMMITTERNAME,
+	ATOM_COMMITTEREMAIL,
+	ATOM_COMMITTERDATE,
+	ATOM_TAGGER,
+	ATOM_TAGGERNAME,
+	ATOM_TAGGEREMAIL,
+	ATOM_TAGGERDATE,
+	ATOM_CREATOR,
+	ATOM_CREATORDATE,
+	ATOM_SUBJECT,
+	ATOM_BODY,
+	ATOM_TRAILERS,
+	ATOM_CONTENTS,
+	ATOM_UPSTREAM,
+	ATOM_PUSH,
+	ATOM_SYMREF,
+	ATOM_FLAG,
+	ATOM_HEAD,
+	ATOM_COLOR,
+	ATOM_WORKTREEPATH,
+	ATOM_ALIGN,
+	ATOM_END,
+	ATOM_IF,
+	ATOM_THEN,
+	ATOM_ELSE,
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -119,6 +169,7 @@ static struct ref_to_worktree_map {
  * array.
  */
 static struct used_atom {
+	enum atom_type atom_type;
 	const char *name;
 	cmp_type type;
 	info_source source;

@@ -506,47 +557,47 @@ static struct {
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
-	{ "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
-	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
-	{ "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
-	{ "object", SOURCE_OBJ },
-	{ "type", SOURCE_OBJ },
-	{ "tag", SOURCE_OBJ },
-	{ "author", SOURCE_OBJ },
-	{ "authorname", SOURCE_OBJ },
-	{ "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "authordate", SOURCE_OBJ, FIELD_TIME },
-	{ "committer", SOURCE_OBJ },
-	{ "committername", SOURCE_OBJ },
-	{ "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "tagger", SOURCE_OBJ },
-	{ "taggername", SOURCE_OBJ },
-	{ "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "creator", SOURCE_OBJ },
-	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
-	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
-	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
-	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
-	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
-	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "flag", SOURCE_NONE },
-	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
-	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
-	{ "worktreepath", SOURCE_NONE },
-	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
-	{ "end", SOURCE_NONE },
-	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
-	{ "then", SOURCE_NONE },
-	{ "else", SOURCE_NONE },
+	[ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	[ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
+	[ATOM_OBJECTNAME] = { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
+	[ATOM_DELTABASE] = { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
+	[ATOM_TREE] = { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_PARENT] = { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_NUMPARENT] = { "numparent", SOURCE_OBJ, FIELD_ULONG },
+	[ATOM_OBJECT] = { "object", SOURCE_OBJ },
+	[ATOM_TYPE] = { "type", SOURCE_OBJ },
+	[ATOM_TAG] = { "tag", SOURCE_OBJ },
+	[ATOM_AUTHOR] = { "author", SOURCE_OBJ },
+	[ATOM_AUTHORNAME] = { "authorname", SOURCE_OBJ },
+	[ATOM_AUTHOREMAIL] = { "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_AUTHORDATE] = { "authordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_COMMITTER] = { "committer", SOURCE_OBJ },
+	[ATOM_COMMITTERNAME] = { "committername", SOURCE_OBJ },
+	[ATOM_COMMITTEREMAIL] = { "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_COMMITTERDATE] = { "committerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_TAGGER] = { "tagger", SOURCE_OBJ },
+	[ATOM_TAGGERNAME] = { "taggername", SOURCE_OBJ },
+	[ATOM_TAGGEREMAIL] = { "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
+	[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	[ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	[ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_SYMREF] = { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_FLAG] = { "flag", SOURCE_NONE },
+	[ATOM_HEAD] = { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	[ATOM_COLOR] = { "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	[ATOM_WORKTREEPATH] = { "worktreepath", SOURCE_NONE },
+	[ATOM_ALIGN] = { "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	[ATOM_END] = { "end", SOURCE_NONE },
+	[ATOM_IF] = { "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	[ATOM_THEN] = { "then", SOURCE_NONE },
+	[ATOM_ELSE] = { "else", SOURCE_NONE },
 	/*
 	 * Please update $__git_ref_fieldlist in git-completion.bash
 	 * when you add new atoms

@@ -628,6 +679,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	used_atom[at].atom_type = i;
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;

@@ -652,7 +704,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (i == ATOM_SYMREF)
 		need_symref = 1;
 	return at;
 }

@@ -965,14 +1017,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "objecttype"))
+		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
-		else if (starts_with(name, "objectsize")) {
+		else if (atom_type == ATOM_OBJECTSIZE) {
 			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
 				v->value = oi->disk_size;
 				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

@@ -980,9 +1033,9 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 				v->value = oi->size;
 				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 			}
-		} else if (!strcmp(name, "deltabase"))
+		} else if (atom_type == ATOM_DELTABASE)
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (deref)
+		else if (atom_type == ATOM_OBJECTNAME && deref)
 			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
 	}
 }

@@ -995,16 +1048,17 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "tag"))
+		if (atom_type == ATOM_TAG)
 			v->s = xstrdup(tag->tag);
-		else if (!strcmp(name, "type") && tag->tagged)
+		else if (atom_type == ATOM_TYPE && tag->tagged)
 			v->s = xstrdup(type_name(tag->tagged->type));
-		else if (!strcmp(name, "object") && tag->tagged)
+		else if (atom_type == ATOM_OBJECT && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
 }

@@ -1017,18 +1071,20 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
+		if (atom_type == ATOM_TREE &&
+		    grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
 			continue;
-		if (!strcmp(name, "numparent")) {
+		if (atom_type == ATOM_NUMPARENT) {
 			v->value = commit_list_count(commit->parents);
 			v->s = xstrfmt("%lu", (unsigned long)v->value);
 		}
-		else if (starts_with(name, "parent")) {
+		else if (atom_type == ATOM_PARENT) {
 			struct commit_list *parents;
 			struct strbuf s = STRBUF_INIT;
 			for (parents = commit->parents; parents; parents = parents->next) {

@@ -1208,15 +1264,16 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 
-		if (starts_with(name, "creatordate"))
+		if (atom_type == ATOM_CREATORDATE)
 			grab_date(wholine, v, name);
-		else if (!strcmp(name, "creator"))
+		else if (atom_type == ATOM_CREATOR)
 			v->s = copy_line(wholine);
 	}
 }

@@ -1696,6 +1753,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
+		enum atom_type atom_type = atom->atom_type;
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;

@@ -1710,18 +1768,18 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (atom_type == ATOM_REFNAME)
 			refname = get_refname(atom, ref);
-		else if (!strcmp(name, "worktreepath")) {
+		else if (atom_type == ATOM_WORKTREEPATH) {
 			if (ref->kind == FILTER_REFS_BRANCHES)
 				v->s = get_worktree_path(atom, ref);
 			else
 				v->s = xstrdup("");
 			continue;
 		}
-		else if (starts_with(name, "symref"))
+		else if (atom_type == ATOM_SYMREF)
 			refname = get_symref(atom, ref);
-		else if (starts_with(name, "upstream")) {
+		else if (atom_type == ATOM_UPSTREAM) {
 			const char *branch_name;
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1737,7 +1795,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			else
 				v->s = xstrdup("");
 			continue;
-		} else if (atom->u.remote_ref.push) {
+		} else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) {
 			const char *branch_name;
 			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1756,10 +1814,10 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (atom_type == ATOM_COLOR) {
 			v->s = xstrdup(atom->u.color);
 			continue;
-		} else if (!strcmp(name, "flag")) {
+		} else if (atom_type == ATOM_FLAG) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");

@@ -1772,23 +1830,24 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) {
-			continue;
-		} else if (!strcmp(name, "HEAD")) {
+		} else if (!deref && atom_type == ATOM_OBJECTNAME &&
+			   grab_oid(name, "objectname", &ref->objectname, v, atom)) {
+				continue;
+		} else if (atom_type == ATOM_HEAD) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
 			else
 				v->s = xstrdup(" ");
 			continue;
-		} else if (starts_with(name, "align")) {
+		} else if (atom_type == ATOM_ALIGN) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "end")) {
+		} else if (atom_type == ATOM_END) {
 			v->handler = end_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (starts_with(name, "if")) {
+		} else if (atom_type == ATOM_IF) {
 			const char *s;
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);

@@ -1796,11 +1855,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
-		} else if (!strcmp(name, "then")) {
+		} else if (atom_type == ATOM_THEN) {
 			v->handler = then_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "else")) {
+		} else if (atom_type == ATOM_ELSE) {
 			v->handler = else_atom_handler;
 			v->s = xstrdup("");
 			continue;

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-05-12 23:28 UTC #19 lore raw

Re: [PATCH v3 1/2] [GSOC] ref-filter: add objectsize to used_atom

In reply to ZheNing Hu via GitGitGadget

Quoted 4 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>
Since "objectsize:size" is composed of two parts, "type:attribute". However, the original implementation

Y is missing in the above "Since X, Y". Because it is composed of two parts, what?

Dropping "Since " would make the sentence work better. Also, I think it should be "objectsize:disk".

Quoted 3 lines

did not decouple the two parts "type" and "attribute" well, we still need to judge separately whether the atom is "objectsize" or "objectsize:disk" in grab_common_values().

Perhaps

When the support for "objectsize:disk" was bolted onto the existing support for "objectsize", it didn't follow the usual pattern for handling "atomtype:modifier", which reads the <modifier> part just once while parsing the format string, and store the parsed result in the union in the used_atom structure, so that the string form of it does not have to be parsed over and over at runtime (e.g. in grab_common_values()).

Quoted 4 lines

Add a new member objectsize to the union used_atom.u, so that we can separate the judgment of atom type from the judgment of atom attribute, This will bring scalability to atom %(objectsize).

OK.

ref-filter.c static struct used_atom { · line 151

Quoted 3 lines

ref-filter.c +3 -0

+		struct {
+			enum { O_SIZE, O_SIZE_DISK } option;
+		} objectsize;

OK.

ref-filter.c static void grab_common_values(struct atom_value *val, int deref, struct expand_ · line 982

Quoted 12 lines

ref-filter.c +8 -0

@@ -967,12 +972,14 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
+		else if (starts_with(name, "objectsize")) {
+			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
+				v->value = oi->disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+			} else if (used_atom[i].u.objectsize.option == O_SIZE) {
+				v->value = oi->size;
+				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+			}

OK.

Junio C Hamano

2021-05-12 23:32 UTC #22 lore raw

In reply to ZheNing Hu via GitGitGadget

Quoted 2 lines · ZheNing Hu via GitGitGadget

+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.

Usually we do not say "coordinates" when we talk about X of an array element A[X]. "... used as an index in the valid_atom[] array." perhaps.

Quoted 2 lines

+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom

You seem to like the verb "judge" (it was also seen in the proposed log message for 1/2) and tend to overuse it when we use other verbs; in this particular case, probably the right verb is to "check".

Quoted 9 lines

+ * entry by if (used_atom[i].atom_type == ATOM_*).
+ */
ref-filter.c +6 -0
+enum atom_type {
+	ATOM_REFNAME,
+...
+	ATOM_ELSE,
+};
+

ref-filter.c static struct ref_to_worktree_map { · line 175

Quoted 7 lines

ref-filter.c +1 -0

@@ -119,6 +169,7 @@ static struct ref_to_worktree_map {
  * array.
  */
 static struct used_atom {
+	enum atom_type atom_type;
 	const char *name;
 	cmp_type type;
 	info_source source;

OK.

ref-filter.c static struct { · line 600

Quoted 10 lines

ref-filter.c +2 -2

@@ -506,47 +557,47 @@ static struct {
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
...
-	{ "else", SOURCE_NONE },
+	[ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
...
+	[ATOM_ELSE] = { "else", SOURCE_NONE },

Makes sense.

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 682

Quoted 5 lines

ref-filter.c +1 -0

@@ -628,6 +679,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	used_atom[at].atom_type = i;

Makes sense.

ref-filter.c static int parse_ref_filter_atom(const struct ref_format *format, · line 710

Quoted 12 lines

ref-filter.c +1 -1

 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;

@@ -652,7 +704,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (i == ATOM_SYMREF)
 		need_symref = 1;
 	return at;
 }

Nice.

Quoted 8 lines

ref-filter.c +1 -0

@@ -965,14 +1017,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;

ref-filter.c static void grab_common_values(struct atom_value *val, int deref, struct expand_ · line 1031

Quoted 10 lines

ref-filter.c +2 -2

 		if (deref)
 			name++;
-		if (!strcmp(name, "objecttype"))
+		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
-		else if (starts_with(name, "objectsize")) {
+		else if (atom_type == ATOM_OBJECTSIZE) {
 			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
 				v->value = oi->disk_size;
 				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

Replacing !strcmp() with comparison with ATOM_* like the above is the best solution for this step, but I wonder if this part (or any other part that this patch touches) would benefit from using a switch statement on atom_type. Something to think about in the future, after the dust settles.

Thanks.

ZheNing Hu

2021-05-13 09:04 UTC #20 lore raw

Re: [PATCH v3 1/2] [GSOC] ref-filter: add objectsize to used_atom

In reply to Junio C Hamano

Quoted 5 lines · Junio C Hamano <[email protected]> 于2021年5月13日周四上午7:11写道：

Quoted 2 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>

Quoted 9 lines

Quoted 2 lines
Since "objectsize:size" is composed of two parts, "type:attribute". However, the original implementation
Y is missing in the above "Since X, Y". Because it is composed of two parts, what?
Dropping "Since " would make the sentence work better. Also, I think it should be "objectsize:disk".

Oh, here is my typo.

Quoted 6 lines

Quoted 3 lines
did not decouple the two parts "type" and "attribute" well, we still need to judge separately whether the atom is "objectsize" or "objectsize:disk" in grab_common_values().
Perhaps

Quoted 9 lines

When the support for "objectsize:disk" was bolted onto the existing support for "objectsize", it didn't follow the usual pattern for handling "atomtype:modifier", which reads the <modifier> part just once while parsing the format string, and store the parsed result in the union in the used_atom structure, so that the string form of it does not have to be parsed over and over at runtime (e.g. in grab_common_values()).

That’s great.

Thanks.
--
ZheNing Hu

ZheNing Hu

2021-05-13 09:26 UTC #23 lore raw

In reply to Junio C Hamano

Quoted 9 lines · Junio C Hamano <[email protected]> 于2021年5月13日周四上午7:21写道：

Quoted 2 lines · ZheNing Hu via GitGitGadget
+/*
+ * The enum atom_type is used as the coordinates of valid_atom entry.
Usually we do not say "coordinates" when we talk about X of an array element A[X]. "... used as an index in the valid_atom[] array." perhaps.

Ok, use index instead of coordinates.

Quoted 7 lines

Quoted 2 lines
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can judge the type of used_atom
You seem to like the verb "judge" (it was also seen in the proposed log message for 1/2) and tend to overuse it when we use other verbs; in this particular case, probably the right verb is to "check".

Yes, "judge" closer to my personal language expression, I will use "check".

Quoted 25 lines

Quoted 8 lines

ref-filter.c +1 -0

@@ -965,14 +1017,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
      for (i = 0; i < used_atom_cnt; i++) {
              const char *name = used_atom[i].name;
+             enum atom_type atom_type = used_atom[i].atom_type;
              struct atom_value *v = &val[i];
              if (!!deref != (*name == '*'))
                      continue;

ref-filter.c static void grab_common_values(struct atom_value *val, int deref, struct expand_ · line 1031

Quoted 10 lines

ref-filter.c +2 -2

              if (deref)
                      name++;
-             if (!strcmp(name, "objecttype"))
+             if (atom_type == ATOM_OBJECTTYPE)
                      v->s = xstrdup(type_name(oi->type));
-             else if (starts_with(name, "objectsize")) {
+             else if (atom_type == ATOM_OBJECTSIZE) {
                      if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
                              v->value = oi->disk_size;
                              v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

Quoted 1 line

Thanks.

Well, that's right, use switch can increase readability and maintainability. This can be used as a future direction.

Thanks.
--
ZheNing Hu

ZheNing Hu via GitGitGadget

author

2021-05-13 15:15 UTC #24 lore raw

v4 cover ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

Change from last version: fix some typo error and change commit message with Junio's help.

ZheNing Hu (2):
[GSOC] ref-filter: add objectsize to used_atom
[GSOC] ref-filter: introduce enum atom_type

 ref-filter.c | 214 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 140 insertions(+), 74 deletions(-)

base-commit: 7e391989789db82983665667013a46eabc6fc570
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-951%2Fadlternative%2Fref-filter-atom-type-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-951/adlternative/ref-filter-atom-type-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/951

Range-diff vs v3:

1: 91ca57c9d04a ! 1: 23fc04f7eb23 [GSOC] ref-filter: add objectsize to used_atom
@@ Metadata
## Commit message ##
[GSOC] ref-filter: add objectsize to used_atom

- Since "objectsize:size" is composed of two parts,
- "type:attribute". However, the original implementation
- did not decouple the two parts "type" and "attribute" well,
- we still need to judge separately whether the atom is
- "objectsize" or "objectsize:disk" in grab_common_values().
+ When the support for "objectsize:disk" was bolted onto the
+ existing support for "objectsize", it didn't follow the
+ usual pattern for handling "atomtype:modifier", which reads
+ the <modifier> part just once while parsing the format
+ string, and store the parsed result in the union in the
+ used_atom structure, so that the string form of it does not
+ have to be parsed over and over at runtime (e.g. in
+ grab_common_values()).

Add a new member objectsize to the union used_atom.u,
- so that we can separate the judgment of atom type from the
- judgment of atom attribute, This will bring scalability to
- atom %(objectsize).
+ so that we can separate the check of <modifier> from the
+ check of <atomtype>, this will bring scalability to atom
+ %(objectsize).

Signed-off-by ZheNing Hu <[email protected]>

2: 43400cac58e7 ! 2: 50cfe1f0c6c1 [GSOC] ref-filter: introduce enum atom_type
@@ Commit message
used_atom.atom_type will record corresponding enum value from valid_atom entry index, and then in specific reference attribute filling step, only need to compare the value of
- the used_atom[i].atom_type to judge the atom type.
+ the used_atom[i].atom_type to check the atom type.

Helped-by Junio C Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

@@ ref-filter.c: static struct ref_to_worktree_map {
} ref_to_worktree_map;

+/*
-+ * The enum atom_type is used as the coordinates of valid_atom entry. ++ * The enum atom_type is used as the index of valid_atom array.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
-+ * as the identifier of the atom type. We can judge the type of used_atom ++ * as the identifier of the atom type. We can check the type of used_atom
+ * entry by if (used_atom[i].atom_type == ATOM_*).
+ */
+enum atom_type {

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-13 15:15 UTC #25 lore raw

PATCH v4 1/2 ref-filter: add objectsize to used_atom

From: ZheNing Hu <[email protected]>

Add a new member objectsize to the union used_atom.u, so that we can separate the check of <modifier> from the check of <atomtype>, this will bring scalability to atom %(objectsize).

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

ref-filter.c +13 -6

index a0adb4551d87..f420bae6e5ba 100644
@@ -146,6 +146,9 @@ static struct used_atom {
 			enum { O_FULL, O_LENGTH, O_SHORT } option;
 			unsigned int length;
 		} oid;
+		struct {
+			enum { O_SIZE, O_SIZE_DISK } option;
+		} objectsize;
 		struct email_option {
 			enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
 		} email_option;

@@ -269,11 +272,13 @@ static int objectsize_atom_parser(const struct ref_format *format, struct used_a
 				  const char *arg, struct strbuf *err)
 {
 	if (!arg) {
+		atom->u.objectsize.option = O_SIZE;
 		if (*atom->name == '*')
 			oi_deref.info.sizep = &oi_deref.size;
 		else
 			oi.info.sizep = &oi.size;
 	} else if (!strcmp(arg, "disk")) {
+		atom->u.objectsize.option = O_SIZE_DISK;
 		if (*atom->name == '*')
 			oi_deref.info.disk_sizep = &oi_deref.disk_size;
 		else

@@ -967,12 +972,14 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
-		else if (!strcmp(name, "objectsize:disk")) {
-			v->value = oi->disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
-		} else if (!strcmp(name, "objectsize")) {
-			v->value = oi->size;
-			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+		else if (starts_with(name, "objectsize")) {
+			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
+				v->value = oi->disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+			} else if (used_atom[i].u.objectsize.option == O_SIZE) {
+				v->value = oi->size;
+				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
+			}
 		} else if (!strcmp(name, "deltabase"))
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
 		else if (deref)

Signature · 1 line

gitgitgadget

ZheNing Hu via GitGitGadget

author

2021-05-13 15:15 UTC #26 lore raw

PATCH v4 2/2 ref-filter: introduce enum atom_type

In reply to ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Signed-off-by ZheNing Hu <[email protected]>

---
 ref-filter.c | 197 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 128 insertions(+), 69 deletions(-)

ref-filter.c +128 ~~-69~~

index f420bae6e5ba..e7a48d428547 100644
@@ -108,6 +108,56 @@ static struct ref_to_worktree_map {
 	struct worktree **worktrees;
 } ref_to_worktree_map;
 
+/*
+ * The enum atom_type is used as the index of valid_atom array.
+ * In the atom parsing stage, it will be passed to used_atom.atom_type
+ * as the identifier of the atom type. We can check the type of used_atom
+ * entry by `if (used_atom[i].atom_type == ATOM_*)`.
+ */
+enum atom_type {
+	ATOM_REFNAME,
+	ATOM_OBJECTTYPE,
+	ATOM_OBJECTSIZE,
+	ATOM_OBJECTNAME,
+	ATOM_DELTABASE,
+	ATOM_TREE,
+	ATOM_PARENT,
+	ATOM_NUMPARENT,
+	ATOM_OBJECT,
+	ATOM_TYPE,
+	ATOM_TAG,
+	ATOM_AUTHOR,
+	ATOM_AUTHORNAME,
+	ATOM_AUTHOREMAIL,
+	ATOM_AUTHORDATE,
+	ATOM_COMMITTER,
+	ATOM_COMMITTERNAME,
+	ATOM_COMMITTEREMAIL,
+	ATOM_COMMITTERDATE,
+	ATOM_TAGGER,
+	ATOM_TAGGERNAME,
+	ATOM_TAGGEREMAIL,
+	ATOM_TAGGERDATE,
+	ATOM_CREATOR,
+	ATOM_CREATORDATE,
+	ATOM_SUBJECT,
+	ATOM_BODY,
+	ATOM_TRAILERS,
+	ATOM_CONTENTS,
+	ATOM_UPSTREAM,
+	ATOM_PUSH,
+	ATOM_SYMREF,
+	ATOM_FLAG,
+	ATOM_HEAD,
+	ATOM_COLOR,
+	ATOM_WORKTREEPATH,
+	ATOM_ALIGN,
+	ATOM_END,
+	ATOM_IF,
+	ATOM_THEN,
+	ATOM_ELSE,
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().

@@ -119,6 +169,7 @@ static struct ref_to_worktree_map {
  * array.
  */
 static struct used_atom {
+	enum atom_type atom_type;
 	const char *name;
 	cmp_type type;
 	info_source source;

@@ -506,47 +557,47 @@ static struct {
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
-	{ "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
-	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
-	{ "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
-	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
-	{ "object", SOURCE_OBJ },
-	{ "type", SOURCE_OBJ },
-	{ "tag", SOURCE_OBJ },
-	{ "author", SOURCE_OBJ },
-	{ "authorname", SOURCE_OBJ },
-	{ "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "authordate", SOURCE_OBJ, FIELD_TIME },
-	{ "committer", SOURCE_OBJ },
-	{ "committername", SOURCE_OBJ },
-	{ "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "tagger", SOURCE_OBJ },
-	{ "taggername", SOURCE_OBJ },
-	{ "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
-	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
-	{ "creator", SOURCE_OBJ },
-	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
-	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
-	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
-	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
-	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
-	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
-	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "flag", SOURCE_NONE },
-	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
-	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
-	{ "worktreepath", SOURCE_NONE },
-	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
-	{ "end", SOURCE_NONE },
-	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
-	{ "then", SOURCE_NONE },
-	{ "else", SOURCE_NONE },
+	[ATOM_REFNAME] = { "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_OBJECTTYPE] = { "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	[ATOM_OBJECTSIZE] = { "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
+	[ATOM_OBJECTNAME] = { "objectname", SOURCE_OTHER, FIELD_STR, oid_atom_parser },
+	[ATOM_DELTABASE] = { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
+	[ATOM_TREE] = { "tree", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_PARENT] = { "parent", SOURCE_OBJ, FIELD_STR, oid_atom_parser },
+	[ATOM_NUMPARENT] = { "numparent", SOURCE_OBJ, FIELD_ULONG },
+	[ATOM_OBJECT] = { "object", SOURCE_OBJ },
+	[ATOM_TYPE] = { "type", SOURCE_OBJ },
+	[ATOM_TAG] = { "tag", SOURCE_OBJ },
+	[ATOM_AUTHOR] = { "author", SOURCE_OBJ },
+	[ATOM_AUTHORNAME] = { "authorname", SOURCE_OBJ },
+	[ATOM_AUTHOREMAIL] = { "authoremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_AUTHORDATE] = { "authordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_COMMITTER] = { "committer", SOURCE_OBJ },
+	[ATOM_COMMITTERNAME] = { "committername", SOURCE_OBJ },
+	[ATOM_COMMITTEREMAIL] = { "committeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_COMMITTERDATE] = { "committerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_TAGGER] = { "tagger", SOURCE_OBJ },
+	[ATOM_TAGGERNAME] = { "taggername", SOURCE_OBJ },
+	[ATOM_TAGGEREMAIL] = { "taggeremail", SOURCE_OBJ, FIELD_STR, person_email_atom_parser },
+	[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
+	[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+	[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	[ATOM_CONTENTS] = { "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	[ATOM_UPSTREAM] = { "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_PUSH] = { "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	[ATOM_SYMREF] = { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	[ATOM_FLAG] = { "flag", SOURCE_NONE },
+	[ATOM_HEAD] = { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	[ATOM_COLOR] = { "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	[ATOM_WORKTREEPATH] = { "worktreepath", SOURCE_NONE },
+	[ATOM_ALIGN] = { "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	[ATOM_END] = { "end", SOURCE_NONE },
+	[ATOM_IF] = { "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	[ATOM_THEN] = { "then", SOURCE_NONE },
+	[ATOM_ELSE] = { "else", SOURCE_NONE },
 	/*
 	 * Please update $__git_ref_fieldlist in git-completion.bash
 	 * when you add new atoms

@@ -628,6 +679,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	used_atom[at].atom_type = i;
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;

@@ -652,7 +704,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (i == ATOM_SYMREF)
 		need_symref = 1;
 	return at;
 }

@@ -965,14 +1017,15 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "objecttype"))
+		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
-		else if (starts_with(name, "objectsize")) {
+		else if (atom_type == ATOM_OBJECTSIZE) {
 			if (used_atom[i].u.objectsize.option == O_SIZE_DISK) {
 				v->value = oi->disk_size;
 				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);

@@ -980,9 +1033,9 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 				v->value = oi->size;
 				v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 			}
-		} else if (!strcmp(name, "deltabase"))
+		} else if (atom_type == ATOM_DELTABASE)
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (deref)
+		else if (atom_type == ATOM_OBJECTNAME && deref)
 			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
 	}
 }

@@ -995,16 +1048,17 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (!strcmp(name, "tag"))
+		if (atom_type == ATOM_TAG)
 			v->s = xstrdup(tag->tag);
-		else if (!strcmp(name, "type") && tag->tagged)
+		else if (atom_type == ATOM_TYPE && tag->tagged)
 			v->s = xstrdup(type_name(tag->tagged->type));
-		else if (!strcmp(name, "object") && tag->tagged)
+		else if (atom_type == ATOM_OBJECT && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
 }

@@ -1017,18 +1071,20 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
-		if (grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
+		if (atom_type == ATOM_TREE &&
+		    grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
 			continue;
-		if (!strcmp(name, "numparent")) {
+		if (atom_type == ATOM_NUMPARENT) {
 			v->value = commit_list_count(commit->parents);
 			v->s = xstrfmt("%lu", (unsigned long)v->value);
 		}
-		else if (starts_with(name, "parent")) {
+		else if (atom_type == ATOM_PARENT) {
 			struct commit_list *parents;
 			struct strbuf s = STRBUF_INIT;
 			for (parents = commit->parents; parents; parents = parents->next) {

@@ -1208,15 +1264,16 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		enum atom_type atom_type = used_atom[i].atom_type;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 
-		if (starts_with(name, "creatordate"))
+		if (atom_type == ATOM_CREATORDATE)
 			grab_date(wholine, v, name);
-		else if (!strcmp(name, "creator"))
+		else if (atom_type == ATOM_CREATOR)
 			v->s = copy_line(wholine);
 	}
 }

@@ -1696,6 +1753,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
+		enum atom_type atom_type = atom->atom_type;
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;

@@ -1710,18 +1768,18 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (atom_type == ATOM_REFNAME)
 			refname = get_refname(atom, ref);
-		else if (!strcmp(name, "worktreepath")) {
+		else if (atom_type == ATOM_WORKTREEPATH) {
 			if (ref->kind == FILTER_REFS_BRANCHES)
 				v->s = get_worktree_path(atom, ref);
 			else
 				v->s = xstrdup("");
 			continue;
 		}
-		else if (starts_with(name, "symref"))
+		else if (atom_type == ATOM_SYMREF)
 			refname = get_symref(atom, ref);
-		else if (starts_with(name, "upstream")) {
+		else if (atom_type == ATOM_UPSTREAM) {
 			const char *branch_name;
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1737,7 +1795,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			else
 				v->s = xstrdup("");
 			continue;
-		} else if (atom->u.remote_ref.push) {
+		} else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) {
 			const char *branch_name;
 			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",

@@ -1756,10 +1814,10 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
-		} else if (starts_with(name, "color:")) {
+		} else if (atom_type == ATOM_COLOR) {
 			v->s = xstrdup(atom->u.color);
 			continue;
-		} else if (!strcmp(name, "flag")) {
+		} else if (atom_type == ATOM_FLAG) {
 			char buf[256], *cp = buf;
 			if (ref->flag & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");

@@ -1772,23 +1830,24 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_oid(name, "objectname", &ref->objectname, v, atom)) {
-			continue;
-		} else if (!strcmp(name, "HEAD")) {
+		} else if (!deref && atom_type == ATOM_OBJECTNAME &&
+			   grab_oid(name, "objectname", &ref->objectname, v, atom)) {
+				continue;
+		} else if (atom_type == ATOM_HEAD) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
 			else
 				v->s = xstrdup(" ");
 			continue;
-		} else if (starts_with(name, "align")) {
+		} else if (atom_type == ATOM_ALIGN) {
 			v->handler = align_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "end")) {
+		} else if (atom_type == ATOM_END) {
 			v->handler = end_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (starts_with(name, "if")) {
+		} else if (atom_type == ATOM_IF) {
 			const char *s;
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);

@@ -1796,11 +1855,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
-		} else if (!strcmp(name, "then")) {
+		} else if (atom_type == ATOM_THEN) {
 			v->handler = then_atom_handler;
 			v->s = xstrdup("");
 			continue;
-		} else if (!strcmp(name, "else")) {
+		} else if (atom_type == ATOM_ELSE) {
 			v->handler = else_atom_handler;
 			v->s = xstrdup("");
 			continue;

Signature · 1 line

gitgitgadget