[PATCH 0/8] [GSOC] ref-filter: eliminate dummy ref_format to make the code cleaner

[PATCH 0/8] [GSOC] ref-filter: eliminate dummy ref_format to make the code cleaner

[ZheNing Hu via GitGitGadget]

Note that there are these disadvantages in the ref-filter code:

 1. Scattered state variables: need_tagged, need_symref are placed globally,
    need_color_reset_at_eol is placed inside ref_format.
 2. parse_sorting_atom() uses a dummy ref_format, which causes us to need a
    lot of extra logic to check in verify_ref_format(), because dummy
    ref_format records some state, while truly ref_format does not.
 3. struct ref_sorting uses a very strange singly linked list
    implementation, and there is a memory leak with it.

Therefore, the following modifications are made to make the logic of
ref-filter cleaner:

 1. Let ref_sorting use list api.
 2. Eliminate dummy ref_format.
 3. Add ref_format to ref_array_item.
 4. Move the check in verify_ref_format() to atom parsing function.
 5. Add deref member to used_atom.
 6. Move need_symref, need_tagged to ref_format.

ZheNing Hu (8):
  [GSOC] ref-filter: use list api to replace ref_sorting linked list
  [GSOC] ref-filter: let parse_sorting_atom() use real ref_format
  [GSOC] ref-filter: add ref_format to ref_array_item
  [GSOC] ref-filter: clean up verify_ref_format()
  [GSOC] ref-filter: introduce symref_atom_parser()
  [GSOC] ref-filter: remove grab_oid() function
  [GSOC] ref-filter: add deref member to struct used_atom
  [GSOC] ref-filter: move need_symref, need_tagged into ref_format

 builtin/branch.c       |  26 +++--
 builtin/for-each-ref.c |  20 ++--
 builtin/ls-remote.c    |  18 ++--
 builtin/tag.c          |  24 +++--
 ref-filter.c           | 236 +++++++++++++++++++++--------------------
 ref-filter.h           |  20 ++--
 6 files changed, 185 insertions(+), 159 deletions(-)


base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1025%2Fadlternative%2Fref-filter-code-clean-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1025/adlternative/ref-filter-code-clean-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1025
-- 
gitgitgadget

[PATCH 1/8] [GSOC] ref-filter: use list api to replace ref_sorting linked list

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

struct ref_sorting is connected by the internal singly linked list,
it is very difficult to use, and each node is added by the head
insertion method but we can only traverse it forward.

Replace it with list api, which is easy to use and introduce a
free_ref_sorting_list() which can free all ref_sorting item and
solve the problem of memory leak.

This will help the logic of ref-filter become clearer.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       | 20 +++++++++++---------
 builtin/for-each-ref.c | 15 ++++++++-------
 builtin/ls-remote.c    | 12 ++++++------
 builtin/tag.c          | 19 +++++++++++--------
 ref-filter.c           | 42 ++++++++++++++++++++++++++++--------------
 ref-filter.h           |  8 +++++---
 6 files changed, 69 insertions(+), 47 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b23b1d1752a..72fafd301c5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -77,7 +77,7 @@ define_list_config_array(color_branch_slots);
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	const char *slot_name;
-	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
+	struct ref_sorting *sorting_tail = (struct ref_sorting *)cb;
 
 	if (!strcmp(var, "branch.sort")) {
 		if (!value)
@@ -624,7 +624,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	enum branch_track track;
 	struct ref_filter filter;
 	int icase = 0;
-	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	struct ref_sorting sorting;
 	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option options[] = {
@@ -665,7 +665,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only branches that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
-		OPT_REF_SORT(sorting_tail),
+		OPT_REF_SORT(&sorting),
 		OPT_CALLBACK(0, "points-at", &filter.points_at, N_("object"),
 			N_("print only branches of the object"), parse_opt_object_name),
 		OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
@@ -678,11 +678,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	memset(&filter, 0, sizeof(filter));
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
+	INIT_LIST_HEAD(&sorting.list);
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
-	git_config(git_branch_config, sorting_tail);
+	git_config(git_branch_config, &sorting);
 
 	track = git_branch_track;
 
@@ -748,12 +749,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 * local branches 'refs/heads/...' and finally remote-tracking
 		 * branches 'refs/remotes/...'.
 		 */
-		if (!sorting)
-			sorting = ref_default_sorting();
-		ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
+		if (list_empty(&sorting.list))
+			ref_default_sorting(&sorting);
+		ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 		ref_sorting_set_sort_flags_all(
-			sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
-		print_ref_list(&filter, sorting, &format);
+			&sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
+		print_ref_list(&filter, &sorting, &format);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
@@ -864,5 +865,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
+	free_ref_sorting_list(&sorting);
 	return 0;
 }
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 89cb6307d46..3cd38eacd7c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -17,7 +17,7 @@ static char const * const for_each_ref_usage[] = {
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	struct ref_sorting sorting;
 	int maxcount = 0, icase = 0;
 	struct ref_array array;
 	struct ref_filter filter;
@@ -39,7 +39,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT__COLOR(&format.use_color, N_("respect format colors")),
-		OPT_REF_SORT(sorting_tail),
+		OPT_REF_SORT(&sorting),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,
 			     N_("object"), N_("print only refs which points at the given object"),
 			     parse_opt_object_name),
@@ -51,6 +51,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	INIT_LIST_HEAD(&sorting.list);
 	memset(&array, 0, sizeof(array));
 	memset(&filter, 0, sizeof(filter));
 
@@ -70,15 +71,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	if (verify_ref_format(&format))
 		usage_with_options(for_each_ref_usage, opts);
 
-	if (!sorting)
-		sorting = ref_default_sorting();
-	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
+	if (list_empty(&sorting.list))
+		ref_default_sorting(&sorting);
+	ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 
 	filter.name_patterns = argv;
 	filter.match_as_path = 1;
 	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
-	ref_array_sort(sorting, &array);
+	ref_array_sort(&sorting, &array);
 
 	if (!maxcount || array.nr < maxcount)
 		maxcount = array.nr;
@@ -96,6 +97,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	ref_array_clear(&array);
 	free_commit_list(filter.with_commit);
 	free_commit_list(filter.no_commit);
-	UNLEAK(sorting);
+	free_ref_sorting_list(&sorting);
 	return 0;
 }
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1794548c711..717ecde03d1 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -54,7 +54,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct transport *transport;
 	const struct ref *ref;
 	struct ref_array ref_array;
-	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	static struct ref_sorting sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -68,7 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
-		OPT_REF_SORT(sorting_tail),
+		OPT_REF_SORT(&sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -79,13 +79,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	};
 
 	memset(&ref_array, 0, sizeof(ref_array));
+	INIT_LIST_HEAD(&sorting.list);
 
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
 
-	UNLEAK(sorting);
-
 	if (argc > 1) {
 		int i;
 		CALLOC_ARRAY(pattern, argc);
@@ -137,8 +136,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		item->symref = xstrdup_or_null(ref->symref);
 	}
 
-	if (sorting)
-		ref_array_sort(sorting, &ref_array);
+	if (!list_empty(&sorting.list))
+		ref_array_sort(&sorting, &ref_array);
 
 	for (i = 0; i < ref_array.nr; i++) {
 		const struct ref_array_item *ref = ref_array.items[i];
@@ -149,6 +148,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	ref_array_clear(&ref_array);
+	free_ref_sorting_list(&sorting);
 	if (transport_disconnect(transport))
 		return 1;
 	return status;
diff --git a/builtin/tag.c b/builtin/tag.c
index 452558ec957..9d057b214e2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -178,7 +178,7 @@ static const char tag_template_nocleanup[] =
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	int status;
-	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
+	struct ref_sorting *sorting_tail = (struct ref_sorting *)cb;
 
 	if (!strcmp(var, "tag.gpgsign")) {
 		config_sign_tag = git_config_bool(var, value);
@@ -438,7 +438,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
-	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	struct ref_sorting sorting;
 	struct ref_format format = REF_FORMAT_INIT;
 	int icase = 0;
 	int edit_flag = 0;
@@ -472,7 +472,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
 		OPT_MERGED(&filter, N_("print only tags that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
-		OPT_REF_SORT(sorting_tail),
+		OPT_REF_SORT(&sorting),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
@@ -487,7 +487,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	setup_ref_filter_porcelain_msg();
 
-	git_config(git_tag_config, sorting_tail);
+	INIT_LIST_HEAD(&sorting.list);
+	git_config(git_tag_config, &sorting);
 
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
@@ -526,9 +527,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
 	}
-	if (!sorting)
-		sorting = ref_default_sorting();
-	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
+	if (list_empty(&sorting.list))
+		ref_default_sorting(&sorting);
+	ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 	if (cmdmode == 'l') {
 		int ret;
@@ -539,11 +540,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting, &format);
+		ret = list_tags(&filter, &sorting, &format);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
 	}
+	free_ref_sorting_list(&sorting);
+
 	if (filter.lines != -1)
 		die(_("-n option is only allowed in list mode"));
 	if (filter.with_commit)
diff --git a/ref-filter.c b/ref-filter.c
index 93ce2a6ef2e..e9e3841c326 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2534,14 +2534,16 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
 {
 	struct ref_array_item *a = *((struct ref_array_item **)a_);
 	struct ref_array_item *b = *((struct ref_array_item **)b_);
-	struct ref_sorting *s;
+	struct ref_sorting *s = ref_sorting;
+	struct list_head *pos;
 
-	for (s = ref_sorting; s; s = s->next) {
-		int cmp = cmp_ref_sorting(s, a, b);
+	list_for_each_prev(pos, &s->list) {
+		int cmp = cmp_ref_sorting(list_entry(pos, struct ref_sorting, list),
+						     a, b);
 		if (cmp)
 			return cmp;
 	}
-	s = ref_sorting;
+
 	return s && s->sort_flags & REF_SORTING_ICASE ?
 		strcasecmp(a->refname, b->refname) :
 		strcmp(a->refname, b->refname);
@@ -2550,11 +2552,15 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
 void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
 				    unsigned int mask, int on)
 {
-	for (; sorting; sorting = sorting->next) {
+	struct list_head *pos;
+	struct ref_sorting *entry;
+
+	list_for_each(pos, &sorting->list) {
+		entry = list_entry(pos, struct ref_sorting, list);
 		if (on)
-			sorting->sort_flags |= mask;
+			entry->sort_flags |= mask;
 		else
-			sorting->sort_flags &= ~mask;
+			entry->sort_flags &= ~mask;
 	}
 }
 
@@ -2667,24 +2673,32 @@ static int parse_sorting_atom(const char *atom)
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-struct ref_sorting *ref_default_sorting(void)
+void ref_default_sorting(struct ref_sorting *sorting_list)
 {
 	static const char cstr_name[] = "refname";
 
 	struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
-
-	sorting->next = NULL;
+	list_add_tail(&sorting->list, &sorting_list->list);
 	sorting->atom = parse_sorting_atom(cstr_name);
-	return sorting;
 }
 
-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
+void free_ref_sorting_list(struct ref_sorting *sorting_list) {
+	struct list_head *pos, *tmp;
+	struct ref_sorting *item;
+
+	list_for_each_safe(pos, tmp, &sorting_list->list) {
+		item = list_entry(pos, struct ref_sorting, list);
+		list_del(pos);
+		free(item);
+	}
+}
+
+void parse_ref_sorting(struct ref_sorting *sorting_list, const char *arg)
 {
 	struct ref_sorting *s;
 
 	CALLOC_ARRAY(s, 1);
-	s->next = *sorting_tail;
-	*sorting_tail = s;
+	list_add_tail(&s->list, &sorting_list->list);
 
 	if (*arg == '-') {
 		s->sort_flags |= REF_SORTING_REVERSE;
diff --git a/ref-filter.h b/ref-filter.h
index c15dee8d6b9..a502c2758e9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -26,7 +26,7 @@
 struct atom_value;
 
 struct ref_sorting {
-	struct ref_sorting *next;
+	struct list_head list;
 	int atom; /* index into used_atom array (internal) */
 	enum {
 		REF_SORTING_REVERSE = 1<<0,
@@ -123,11 +123,13 @@ int format_ref_array_item(struct ref_array_item *info,
 			  struct strbuf *final_buf,
 			  struct strbuf *error_buf);
 /*  Parse a single sort specifier and add it to the list */
-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
+void parse_ref_sorting(struct ref_sorting *sorting_list, const char *arg);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
-struct ref_sorting *ref_default_sorting(void);
+void ref_default_sorting(struct ref_sorting *sorting_list);
+/* Free all ref_sorting items in sorting list */
+void free_ref_sorting_list(struct ref_sorting *sorting_list);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
 /*  Get the current HEAD's description */
-- 
gitgitgadget

[PATCH 2/8] [GSOC] ref-filter: let parse_sorting_atom() use real ref_format

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

parse_sorting_atom() currently uses a "dummy" ref_format, because
it doesn't care about the details of real ref_format. But in fact,
we expect to save some state in ref_format, and we need to check
these statuses when executing some functions, e.g. in
verify_ref_format() we check format->quote_style and
format->need_color_reset_at_eol. And we intentionally want some
global options (e.g. need_tagged) to be added to struct ref_format,
so we need to care about ref_format details now, but the "dummy"
ref_format prevents the truly ref_format to record them correctly.

So let parse_sorting_atom() use the real ref_format, this requires us
to separate the work of parsing the sort atom from parse_ref_sorting().
Add an "arg" member to struct ref_sorting to record the name of the
sort atom, and parse_ref_sorting() only records the atom name, and
introduce a parse_ref_sorting_list() to traverse the ref_sorting
linked list and parsing each sort atom.

This will help us add some global options to struct ref_format later.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       |  1 +
 builtin/for-each-ref.c |  1 +
 builtin/ls-remote.c    |  6 ++++--
 builtin/tag.c          |  1 +
 ref-filter.c           | 25 ++++++++++++++-----------
 ref-filter.h           |  3 +++
 6 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 72fafd301c5..3b21c2b54b9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -751,6 +751,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 */
 		if (list_empty(&sorting.list))
 			ref_default_sorting(&sorting);
+		parse_ref_sorting_list(&sorting, &format);
 		ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 		ref_sorting_set_sort_flags_all(
 			&sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3cd38eacd7c..8948b7bcd32 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -73,6 +73,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	if (list_empty(&sorting.list))
 		ref_default_sorting(&sorting);
+	parse_ref_sorting_list(&sorting, &format);
 	ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 717ecde03d1..7fac069a29d 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -49,6 +49,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int i;
 	struct string_list server_options = STRING_LIST_INIT_DUP;
+	struct ref_format dummy = REF_FORMAT_INIT;
 
 	struct remote *remote;
 	struct transport *transport;
@@ -136,9 +137,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		item->symref = xstrdup_or_null(ref->symref);
 	}
 
-	if (!list_empty(&sorting.list))
+	if (!list_empty(&sorting.list)) {
+		parse_ref_sorting_list(&sorting, &dummy);
 		ref_array_sort(&sorting, &ref_array);
-
+	}
 	for (i = 0; i < ref_array.nr; i++) {
 		const struct ref_array_item *ref = ref_array.items[i];
 		if (show_symref_target && ref->symref)
diff --git a/builtin/tag.c b/builtin/tag.c
index 9d057b214e2..a0d4aa775e3 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -529,6 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (list_empty(&sorting.list))
 		ref_default_sorting(&sorting);
+	parse_ref_sorting_list(&sorting, &format);
 	ref_sorting_set_sort_flags_all(&sorting, REF_SORTING_ICASE, icase);
 	filter.ignore_case = icase;
 	if (cmdmode == 'l') {
diff --git a/ref-filter.c b/ref-filter.c
index e9e3841c326..88a3e06eea9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2656,30 +2656,33 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 	free_array_item(ref_item);
 }
 
-static int parse_sorting_atom(const char *atom)
+static int parse_sorting_atom(struct ref_format *format, const char *atom)
 {
-	/*
-	 * This parses an atom using a dummy ref_format, since we don't
-	 * actually care about the formatting details.
-	 */
-	struct ref_format dummy = REF_FORMAT_INIT;
 	const char *end = atom + strlen(atom);
 	struct strbuf err = STRBUF_INIT;
-	int res = parse_ref_filter_atom(&dummy, atom, end, &err);
+	int res = parse_ref_filter_atom(format, atom, end, &err);
 	if (res < 0)
 		die("%s", err.buf);
 	strbuf_release(&err);
 	return res;
 }
 
+void parse_ref_sorting_list(struct ref_sorting *sorting_list, struct ref_format *format) {
+	struct list_head *pos;
+	struct ref_sorting *entry;
+
+	list_for_each(pos, &sorting_list->list) {
+		entry = list_entry(pos, struct ref_sorting, list);
+		entry->atom = parse_sorting_atom(format, entry->arg);
+	}
+}
+
 /*  If no sorting option is given, use refname to sort as default */
 void ref_default_sorting(struct ref_sorting *sorting_list)
 {
-	static const char cstr_name[] = "refname";
-
 	struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
 	list_add_tail(&sorting->list, &sorting_list->list);
-	sorting->atom = parse_sorting_atom(cstr_name);
+	sorting->arg = "refname";
 }
 
 void free_ref_sorting_list(struct ref_sorting *sorting_list) {
@@ -2707,7 +2710,7 @@ void parse_ref_sorting(struct ref_sorting *sorting_list, const char *arg)
 	if (skip_prefix(arg, "version:", &arg) ||
 	    skip_prefix(arg, "v:", &arg))
 		s->sort_flags |= REF_SORTING_VERSION;
-	s->atom = parse_sorting_atom(arg);
+	s->arg = arg;
 }
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
diff --git a/ref-filter.h b/ref-filter.h
index a502c2758e9..8f602583788 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,6 +28,7 @@ struct atom_value;
 struct ref_sorting {
 	struct list_head list;
 	int atom; /* index into used_atom array (internal) */
+	const char *arg;
 	enum {
 		REF_SORTING_REVERSE = 1<<0,
 		REF_SORTING_ICASE = 1<<1,
@@ -128,6 +129,8 @@ void parse_ref_sorting(struct ref_sorting *sorting_list, const char *arg);
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
 void ref_default_sorting(struct ref_sorting *sorting_list);
+/* Parse every sort atom of ref_sorting list */
+void parse_ref_sorting_list(struct ref_sorting *sorting_list, struct ref_format *format);
 /* Free all ref_sorting items in sorting list */
 void free_ref_sorting_list(struct ref_sorting *sorting_list);
 /*  Function to parse --merged and --no-merged options */
-- 
gitgitgadget

[PATCH 3/8] [GSOC] ref-filter: add ref_format to ref_array_item

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Since we expect to move some global options to ref_format,
we may want check these global options in some functions
like populate_value(), but the problem is that these
functions cannot use ref_format, which means we may need
a deep call chain to pass ref_format to them, this will
be a bit cumbersome.

We can use another method: Add the ref_format member to
ref_array_item. Since we have successfully eliminated dummy
ref_format, this can ensure that the same ref_array can use
the same ref_format, so we pass ref_format through the call
chain when ref_array_item is initialized, we also add
ref_format member to ref_cbdata.

This will help us move the global options to ref_format later.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       |  5 +++--
 builtin/for-each-ref.c |  4 ++--
 builtin/ls-remote.c    |  2 +-
 builtin/tag.c          |  4 ++--
 ref-filter.c           | 24 ++++++++++++++----------
 ref-filter.h           |  7 ++++---
 6 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3b21c2b54b9..4c6e7ef5f10 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -427,7 +427,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 
 	memset(&array, 0, sizeof(array));
 
-	filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
+	filter_refs(&array, filter, format, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
 
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
@@ -444,7 +444,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	for (i = 0; i < array.nr; i++) {
 		strbuf_reset(&err);
 		strbuf_reset(&out);
-		if (format_ref_array_item(array.items[i], format, &out, &err))
+		array.items[i]->format = format;
+		if (format_ref_array_item(array.items[i], &out, &err))
 			die("%s", err.buf);
 		if (column_active(colopts)) {
 			assert(!filter->verbose && "--column and --verbose are incompatible");
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8948b7bcd32..4a71caca929 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -79,7 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 
 	filter.name_patterns = argv;
 	filter.match_as_path = 1;
-	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
+	filter_refs(&array, &filter, &format, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
 	ref_array_sort(&sorting, &array);
 
 	if (!maxcount || array.nr < maxcount)
@@ -87,7 +87,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < maxcount; i++) {
 		strbuf_reset(&err);
 		strbuf_reset(&output);
-		if (format_ref_array_item(array.items[i], &format, &output, &err))
+		if (format_ref_array_item(array.items[i], &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 7fac069a29d..2dd19a8583d 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -133,7 +133,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
-		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
+		item = ref_array_push(&ref_array, ref->name, &ref->old_oid, &dummy);
 		item->symref = xstrdup_or_null(ref->symref);
 	}
 
diff --git a/builtin/tag.c b/builtin/tag.c
index a0d4aa775e3..9a66c4d37a5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -62,13 +62,13 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	if (verify_ref_format(format))
 		die(_("unable to parse format string"));
 	filter->with_commit_tag_algo = 1;
-	filter_refs(&array, filter, FILTER_REFS_TAGS);
+	filter_refs(&array, filter, format, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
 		strbuf_reset(&output);
 		strbuf_reset(&err);
-		if (format_ref_array_item(array.items[i], format, &output, &err))
+		if (format_ref_array_item(array.items[i], &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
diff --git a/ref-filter.c b/ref-filter.c
index 88a3e06eea9..f4fa3102da6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2167,22 +2167,24 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
  * Callers can then fill in other struct members at their leisure.
  */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid)
+						 const struct object_id *oid,
+						 struct ref_format *format)
 {
 	struct ref_array_item *ref;
 
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
 	ref->rest = NULL;
-
+	ref->format = format;
 	return ref;
 }
 
 struct ref_array_item *ref_array_push(struct ref_array *array,
 				      const char *refname,
-				      const struct object_id *oid)
+				      const struct object_id *oid,
+				      struct ref_format *format)
 {
-	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+	struct ref_array_item *ref = new_ref_array_item(refname, oid, format);
 
 	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
 	array->items[array->nr++] = ref;
@@ -2226,6 +2228,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 struct ref_filter_cbdata {
 	struct ref_array *array;
 	struct ref_filter *filter;
+	struct ref_format *format;
 	struct contains_cache contains_cache;
 	struct contains_cache no_contains_cache;
 };
@@ -2238,6 +2241,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 {
 	struct ref_filter_cbdata *ref_cbdata = cb_data;
 	struct ref_filter *filter = ref_cbdata->filter;
+	struct ref_format *format = ref_cbdata->format;
 	struct ref_array_item *ref;
 	struct commit *commit = NULL;
 	unsigned int kind;
@@ -2288,7 +2292,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = ref_array_push(ref_cbdata->array, refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid, format);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
@@ -2401,7 +2405,7 @@ static void reach_filter(struct ref_array *array,
  * as per the given ref_filter structure and finally store the
  * filtered refs in the ref_array structure.
  */
-int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type)
+int filter_refs(struct ref_array *array, struct ref_filter *filter, struct ref_format *format, unsigned int type)
 {
 	struct ref_filter_cbdata ref_cbdata;
 	int ret = 0;
@@ -2409,7 +2413,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 
 	ref_cbdata.array = array;
 	ref_cbdata.filter = filter;
-
+	ref_cbdata.format = format;
 	if (type & FILTER_REFS_INCLUDE_BROKEN)
 		broken = 1;
 	filter->kind = type & FILTER_REFS_KIND_MASK;
@@ -2592,12 +2596,12 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 }
 
 int format_ref_array_item(struct ref_array_item *info,
-			  struct ref_format *format,
 			  struct strbuf *final_buf,
 			  struct strbuf *error_buf)
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+	struct ref_format *format = info->format;
 
 	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
@@ -2644,9 +2648,9 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 	struct strbuf output = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 
-	ref_item = new_ref_array_item(name, oid);
+	ref_item = new_ref_array_item(name, oid, format);
 	ref_item->kind = ref_kind_from_refname(name);
-	if (format_ref_array_item(ref_item, format, &output, &err))
+	if (format_ref_array_item(ref_item, &output, &err))
 		die("%s", err.buf);
 	fwrite(output.buf, 1, output.len, stdout);
 	putchar('\n');
diff --git a/ref-filter.h b/ref-filter.h
index 8f602583788..e95c055fb86 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,7 @@ struct ref_array_item {
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
+	struct ref_format *format;
 	char refname[FLEX_ARRAY];
 };
 
@@ -109,7 +110,7 @@ struct ref_format {
  * as per the given ref_filter structure and finally store the
  * filtered refs in the ref_array structure.
  */
-int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type);
+int filter_refs(struct ref_array *array, struct ref_filter *filter, struct ref_format *format, unsigned int type);
 /*  Clear all memory allocated to ref_array */
 void ref_array_clear(struct ref_array *array);
 /*  Used to verify if the given format is correct and to parse out the used atoms */
@@ -120,7 +121,6 @@ void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting, unsigned int mask, int on);
 /*  Based on the given format and quote_style, fill the strbuf */
 int format_ref_array_item(struct ref_array_item *info,
-			  struct ref_format *format,
 			  struct strbuf *final_buf,
 			  struct strbuf *error_buf);
 /*  Parse a single sort specifier and add it to the list */
@@ -153,6 +153,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
  */
 struct ref_array_item *ref_array_push(struct ref_array *array,
 				      const char *refname,
-				      const struct object_id *oid);
+				      const struct object_id *oid,
+				      struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
gitgitgadget

[PATCH 4/8] [GSOC] ref-filter: clean up verify_ref_format()

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Because we may not use the real ref_format when using
parse_ref_filter_atom() before, we have to perform some
atom checks in verify_ref_format() (instead of the parsing
function of each atom), which is very inelegant.

Since we have eliminated dummy ref_format, we can freely put
some of the check logic from verify_ref_format() to their
corresponding atom parsing functions.

Introduce raw_atom_reject_quote_style() to check if there is
any incompatibility quote style with the %(raw) atom.

This can make the logic of the program more concise.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f4fa3102da6..e42cd738c32 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -242,6 +242,7 @@ static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
 	 */
 	if (!want_color(format->use_color))
 		color_parse("", atom->u.color);
+	format->need_color_reset_at_eol = !!strcmp(color_value, "reset");
 	return 0;
 }
 
@@ -432,12 +433,29 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
 	return 0;
 }
 
+static int raw_atom_reject_quote_style(int quote_style) {
+	switch (quote_style) {
+	case QUOTE_PYTHON:
+	case QUOTE_SHELL:
+	case QUOTE_TCL:
+		return 1;
+	case QUOTE_PERL:
+	case 0:
+		return 0;
+	default:
+		BUG("unknown quote style %d", quote_style);
+	}
+}
+
 static int raw_atom_parser(struct ref_format *format, struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
-	if (!arg)
+	if (!arg) {
 		atom->u.raw_data.option = RAW_BARE;
-	else if (!strcmp(arg, "size"))
+		if (raw_atom_reject_quote_style(format->quote_style))
+			return strbuf_addf_ret(err, -1, _("--format=%%(raw) cannot be used with"
+						   "--python, --shell, --tcl"));
+	} else if (!strcmp(arg, "size"))
 		atom->u.raw_data.option = RAW_LENGTH;
 	else
 		return strbuf_addf_ret(err, -1, _("unrecognized %%(raw) argument: %s"), arg);
@@ -567,7 +585,7 @@ static int rest_atom_parser(struct ref_format *format, struct used_atom *atom,
 	if (arg)
 		return strbuf_addf_ret(err, -1, _("%%(rest) does not take arguments"));
 	format->use_rest = 1;
-	return 0;
+	return strbuf_addf_ret(err, -1, _("this command reject atom %%(rest)"));
 }
 
 static int head_atom_parser(struct ref_format *format, struct used_atom *atom,
@@ -1004,11 +1022,6 @@ static const char *find_next(const char *cp)
 	return NULL;
 }
 
-static int reject_atom(enum atom_type atom_type)
-{
-	return atom_type == ATOM_REST;
-}
-
 /*
  * Make sure the format string is well formed, and parse out
  * the used atoms.
@@ -1020,7 +1033,7 @@ int verify_ref_format(struct ref_format *format)
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		struct strbuf err = STRBUF_INIT;
-		const char *color, *ep = strchr(sp, ')');
+		const char *ep = strchr(sp, ')');
 		int at;
 
 		if (!ep)
@@ -1029,20 +1042,8 @@ int verify_ref_format(struct ref_format *format)
 		at = parse_ref_filter_atom(format, sp + 2, ep, &err);
 		if (at < 0)
 			die("%s", err.buf);
-		if (reject_atom(used_atom[at].atom_type))
-			die(_("this command reject atom %%(%.*s)"), (int)(ep - sp - 2), sp + 2);
-
-		if ((format->quote_style == QUOTE_PYTHON ||
-		     format->quote_style == QUOTE_SHELL ||
-		     format->quote_style == QUOTE_TCL) &&
-		     used_atom[at].atom_type == ATOM_RAW &&
-		     used_atom[at].u.raw_data.option == RAW_BARE)
-			die(_("--format=%.*s cannot be used with"
-			      "--python, --shell, --tcl"), (int)(ep - sp - 2), sp + 2);
 		cp = ep + 1;
 
-		if (skip_prefix(used_atom[at].name, "color:", &color))
-			format->need_color_reset_at_eol = !!strcmp(color, "reset");
 		strbuf_release(&err);
 	}
 	if (format->need_color_reset_at_eol && !want_color(format->use_color))
-- 
gitgitgadget

[PATCH 5/8] [GSOC] ref-filter: introduce symref_atom_parser()

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

parse_ref_filter_atom() sets need_symref by checking
whether the atom type is ATOM_SYMREF. When we are operating
other atoms here, this step of checking is not necessary.

So add the symref_atom_parser() specifically to parse the
%(symref) atom, and set need_symref in it.

This can make the program logic more concise.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e42cd738c32..8dca273f720 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -495,6 +495,13 @@ static int person_email_atom_parser(struct ref_format *format, struct used_atom
 	return 0;
 }
 
+static int symref_atom_parser(struct ref_format *format, struct used_atom *atom,
+			       const char *arg, struct strbuf *err)
+{
+	need_symref = 1;
+	return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
+}
+
 static int refname_atom_parser(struct ref_format *format, struct used_atom *atom,
 			       const char *arg, struct strbuf *err)
 {
@@ -634,7 +641,7 @@ static struct {
 	[ATOM_RAW] = { "raw", SOURCE_OBJ, FIELD_STR, raw_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_SYMREF] = { "symref", SOURCE_NONE, FIELD_STR, symref_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 },
@@ -758,8 +765,6 @@ static int parse_ref_filter_atom(struct ref_format *format,
 		return -1;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (i == ATOM_SYMREF)
-		need_symref = 1;
 	return at;
 }
 
-- 
gitgitgadget

[PATCH 6/8] [GSOC] ref-filter: remove grab_oid() function

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

Because "atom_type == ATOM_OBJECTNAME" implies the condition
of `starts_with(name, "objectname")`, "atom_type == ATOM_TREE"
implies the condition of `starts_with(name, "tree")`, so the
check for `starts_with(name, field)` in grab_oid() is redundant.

Remove the grab_oid() from ref-filter, to reduce repeated check.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8dca273f720..67f62a3b591 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1071,16 +1071,6 @@ static const char *do_grab_oid(const char *field, const struct object_id *oid,
 	}
 }
 
-static int grab_oid(const char *name, const char *field, const struct object_id *oid,
-		    struct atom_value *v, struct used_atom *atom)
-{
-	if (starts_with(name, field)) {
-		v->s = xstrdup(do_grab_oid(field, oid, atom));
-		return 1;
-	}
-	return 0;
-}
-
 /* See grab_values */
 static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)
 {
@@ -1106,8 +1096,9 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			}
 		} else if (atom_type == ATOM_DELTABASE)
 			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
-		else if (atom_type == ATOM_OBJECTNAME && deref)
-			grab_oid(name, "objectname", &oi->oid, v, &used_atom[i]);
+		else if (atom_type == ATOM_OBJECTNAME && deref) {
+			v->s = xstrdup(do_grab_oid("objectname", &oi->oid, &used_atom[i]));
+		}
 	}
 }
 
@@ -1148,9 +1139,10 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 			continue;
 		if (deref)
 			name++;
-		if (atom_type == ATOM_TREE &&
-		    grab_oid(name, "tree", get_commit_tree_oid(commit), v, &used_atom[i]))
+		if (atom_type == ATOM_TREE) {
+			v->s = xstrdup(do_grab_oid("tree", get_commit_tree_oid(commit), &used_atom[i]));
 			continue;
+		}
 		if (atom_type == ATOM_NUMPARENT) {
 			v->value = commit_list_count(commit->parents);
 			v->s = xstrfmt("%lu", (unsigned long)v->value);
@@ -1923,9 +1915,9 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && atom_type == ATOM_OBJECTNAME &&
-			   grab_oid(name, "objectname", &ref->objectname, v, atom)) {
-				continue;
+		} else if (!deref && atom_type == ATOM_OBJECTNAME) {
+			   v->s = xstrdup(do_grab_oid("objectname", &ref->objectname, atom));
+			   continue;
 		} else if (atom_type == ATOM_HEAD) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
 				v->s = xstrdup("*");
-- 
gitgitgadget

[PATCH 7/8] [GSOC] ref-filter: add deref member to struct used_atom

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

In the ref-filter logic, the dereference flag '*' is stored
at the beginning of used_atom[i].name (e.g. '*objectname:short'),
this leads us to make some check on used_atom[i].name[0] to
know if the atom need to be dereferenced. This is not only
cumbersome to use, but also poor in readability.

So add `deref` flag to struct used_atom used to indicate
whether the atom needs to be dereferenced and let used_atom
record only the rest part (i.e. 'objectname:short').

This can make the logic of the program clearer and easy to use.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 55 ++++++++++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 34 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 67f62a3b591..e4f96dad504 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -171,6 +171,7 @@ enum atom_type {
  * array.
  */
 static struct used_atom {
+	unsigned int deref : 1;
 	enum atom_type atom_type;
 	const char *name;
 	cmp_type type;
@@ -319,7 +320,7 @@ static int objecttype_atom_parser(struct ref_format *format, struct used_atom *a
 {
 	if (arg)
 		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
-	if (*atom->name == '*')
+	if (atom->deref)
 		oi_deref.info.typep = &oi_deref.type;
 	else
 		oi.info.typep = &oi.type;
@@ -331,13 +332,13 @@ static int objectsize_atom_parser(struct ref_format *format, struct used_atom *a
 {
 	if (!arg) {
 		atom->u.objectsize.option = O_SIZE;
-		if (*atom->name == '*')
+		if (atom->deref)
 			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 == '*')
+		if (atom->deref)
 			oi_deref.info.disk_sizep = &oi_deref.disk_size;
 		else
 			oi.info.disk_sizep = &oi.disk_size;
@@ -351,7 +352,7 @@ static int deltabase_atom_parser(struct ref_format *format, struct used_atom *at
 {
 	if (arg)
 		return strbuf_addf_ret(err, -1, _("%%(deltabase) does not take arguments"));
-	if (*atom->name == '*')
+	if (atom->deref)
 		oi_deref.info.delta_base_oid = &oi_deref.delta_base_oid;
 	else
 		oi.info.delta_base_oid = &oi.delta_base_oid;
@@ -697,10 +698,13 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	const char *sp;
 	const char *arg;
 	int i, at, atom_len;
+	int deref = 0;
 
 	sp = atom;
-	if (*sp == '*' && sp < ep)
+	if (*sp == '*' && sp < ep) {
 		sp++; /* deref */
+		deref = 1;
+	}
 	if (ep <= sp)
 		return strbuf_addf_ret(err, -1, _("malformed field name: %.*s"),
 				       (int)(ep-atom), atom);
@@ -717,7 +721,7 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	/* Do we have the atom already used elsewhere? */
 	for (i = 0; i < used_atom_cnt; i++) {
 		int len = strlen(used_atom[i].name);
-		if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
+		if (len == ep - sp && !memcmp(used_atom[i].name, sp, len))
 			return i;
 	}
 
@@ -741,17 +745,18 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	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].deref = deref;
+	used_atom[at].name = xmemdupz(sp, ep - sp);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;
 	if (used_atom[at].source == SOURCE_OBJ) {
-		if (*atom == '*')
+		if (deref)
 			oi_deref.info.contentp = &oi_deref.content;
 		else
 			oi.info.contentp = &oi.content;
 	}
 	if (arg) {
-		arg = used_atom[at].name + (arg - atom) + 1;
+		arg = used_atom[at].name + (arg - sp) + 1;
 		if (!*arg) {
 			/*
 			 * Treat empty sub-arguments list as NULL (i.e.,
@@ -763,7 +768,7 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
 		return -1;
-	if (*atom == '*')
+	if (deref)
 		need_tagged = 1;
 	return at;
 }
@@ -1077,13 +1082,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 	int i;
 
 	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 == '*'))
+		if (!!deref != used_atom[i].deref)
 			continue;
-		if (deref)
-			name++;
 		if (atom_type == ATOM_OBJECTTYPE)
 			v->s = xstrdup(type_name(oi->type));
 		else if (atom_type == ATOM_OBJECTSIZE) {
@@ -1109,13 +1111,10 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 	struct tag *tag = (struct tag *) obj;
 
 	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 == '*'))
+		if (!!deref != used_atom[i].deref)
 			continue;
-		if (deref)
-			name++;
 		if (atom_type == ATOM_TAG)
 			v->s = xstrdup(tag->tag);
 		else if (atom_type == ATOM_TYPE && tag->tagged)
@@ -1132,13 +1131,10 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 	struct commit *commit = (struct commit *) obj;
 
 	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 == '*'))
+		if (!!deref != used_atom[i].deref)
 			continue;
-		if (deref)
-			name++;
 		if (atom_type == ATOM_TREE) {
 			v->s = xstrdup(do_grab_oid("tree", get_commit_tree_oid(commit), &used_atom[i]));
 			continue;
@@ -1290,10 +1286,8 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
+		if (!!deref != used_atom[i].deref)
 			continue;
-		if (deref)
-			name++;
 		if (strncmp(who, name, wholen))
 			continue;
 		if (name[wholen] != 0 &&
@@ -1432,10 +1426,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
 		struct atom_value *v = &val[i];
 		enum atom_type atom_type = atom->atom_type;
 
-		if (!!deref != (*name == '*'))
+		if (!!deref != used_atom[i].deref)
 			continue;
-		if (deref)
-			name++;
 
 		if (atom_type == ATOM_RAW) {
 			unsigned long buf_size = data->size;
@@ -1840,7 +1832,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		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;
+		int deref = atom->deref;
 		const char *refname;
 		struct branch *branch = NULL;
 
@@ -1848,11 +1840,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		v->handler = append_atom;
 		v->atom = atom;
 
-		if (*name == '*') {
-			deref = 1;
-			name++;
-		}
-
 		if (atom_type == ATOM_REFNAME)
 			refname = get_refname(atom, ref);
 		else if (atom_type == ATOM_WORKTREEPATH) {
-- 
gitgitgadget

[PATCH 8/8] [GSOC] ref-filter: move need_symref, need_tagged into ref_format

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

There are many global options variables in ref-filter, e.g.
need_tagged, need_symref, which may affect the reentrancy of
the functions. Moving these variables into ref_format which
can make the code cleaner.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 14 +++++++-------
 ref-filter.h |  2 ++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e4f96dad504..29be1e865d2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -213,7 +213,7 @@ static struct used_atom {
 		char *head;
 	} u;
 } *used_atom;
-static int used_atom_cnt, need_tagged, need_symref;
+static int used_atom_cnt;
 
 /*
  * Expand string, append it to strbuf *sb, then return error code ret.
@@ -499,7 +499,7 @@ static int person_email_atom_parser(struct ref_format *format, struct used_atom
 static int symref_atom_parser(struct ref_format *format, struct used_atom *atom,
 			       const char *arg, struct strbuf *err)
 {
-	need_symref = 1;
+	format->need_symref = 1;
 	return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
 }
 
@@ -769,7 +769,7 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
 		return -1;
 	if (deref)
-		need_tagged = 1;
+		format->need_tagged = 1;
 	return at;
 }
 
@@ -1816,10 +1816,10 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	struct object *obj;
 	int i;
 	struct object_info empty = OBJECT_INFO_INIT;
-
+	struct ref_format *format = ref->format;
 	CALLOC_ARRAY(ref->value, used_atom_cnt);
 
-	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
+	if (format->need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
 		ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
 					     NULL, NULL);
 		if (!ref->symref)
@@ -1958,7 +1958,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 					       oid_to_hex(&ref->objectname), ref->refname);
 	}
 
-	if (need_tagged)
+	if (format->need_tagged)
 		oi.info.contentp = &oi.content;
 	if (!memcmp(&oi.info, &empty, sizeof(empty)) &&
 	    !memcmp(&oi_deref.info, &empty, sizeof(empty)))
@@ -1973,7 +1973,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * If there is no atom that wants to know about tagged
 	 * object, we are done.
 	 */
-	if (!need_tagged || (obj->type != OBJ_TAG))
+	if (!format->need_tagged || (obj->type != OBJ_TAG))
 		return 0;
 
 	/*
diff --git a/ref-filter.h b/ref-filter.h
index e95c055fb86..ec4cc1e5f9c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -86,6 +86,8 @@ struct ref_format {
 
 	/* Internal state to ref-filter */
 	int need_color_reset_at_eol;
+	int need_tagged;
+	int need_symref;
 };
 
 #define REF_FORMAT_INIT { .use_color = -1 }
-- 
gitgitgadget