for-each-ref: re-structure code for moving to 'ref-filter'

PATCH Superseded Karthik Nayak 3 participants 15 messages Jun 15, 2016 Kbuild

lore

Share & tools

lore

Karthik Nayak

author

2016-06-15 23:05 UTC #0 lore raw

PATCH v3 1/4 for-each-ref: re-structure code for moving to 'ref-filter'

Hello,

After the second version of the WIP/PATCH ($gmane/269836), This is a follow up.

Changes:
* Broke down the first commit into 3 separate commits, cause they were logically different and needed to built up incrementally.
* renamed some functions which were made public so that they don't seem vague.
* other small changes.

Signature · 2 lines

Regards,
Karthik

Karthik Nayak

author

2016-06-15 23:05 UTC #1 lore raw

PATCH v3 1/4 for-each-ref: extract helper functions out of grab_single_ref()

Extract two helper functions out of grab_single_ref(). Firstly, new_refinfo() which is used to allocate memory for a new refinfo structure and copy the objectname, refname and flag to it. Secondly, match_name_as_path() which when given an array of patterns and the refname checks if the refname matches any of the patterns given while the pattern is a pathname, also while supporting wild characters.

This is a preperatory patch for restructuring 'for-each-ref' and evntually moving most of it to 'ref-filter' to provide the functionality to similar commands via public API's.

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

Mentored-by Christian Couder <[email protected]>

Mentored-by Matthieu Moy <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---
 builtin/for-each-ref.c | 76 ++++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 33 deletions(-)

builtin/for-each-ref.c +43 ~~-33~~

index 83f9cf9..919d45e 100644
@@ -837,6 +837,43 @@ struct grab_ref_cbdata {
 };
 
 /*
+ * Given a refname, return 1 if the refname matches with one of the patterns
+ * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
+ * and so on, else return 0. Supports use of wild characters.
+ */
+static int match_name_as_path(const char **pattern, const char *refname)
+{
+	int namelen = strlen(refname);
+	for (; *pattern; pattern++) {
+		const char *p = *pattern;
+		int plen = strlen(p);
+
+		if ((plen <= namelen) &&
+		    !strncmp(refname, p, plen) &&
+		    (refname[plen] == '\0' ||
+		     refname[plen] == '/' ||
+		     p[plen-1] == '/'))
+			return 1;
+		if (!wildmatch(p, refname, WM_PATHNAME, NULL))
+			return 1;
+	}
+	return 0;
+}
+
+/* Allocate space for a new refinfo and copy the objectname and flag to it */
+static struct refinfo *new_refinfo(const char *refname,
+				   const unsigned char *objectname,
+				   int flag)
+{
+	struct refinfo *ref = xcalloc(1, sizeof(struct refinfo));
+	ref->refname = xstrdup(refname);
+	hashcpy(ref->objectname, objectname);
+	ref->flag = flag;
+
+	return ref;
+}
+
+/*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
  */

@@ -844,47 +881,20 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 {
 	struct grab_ref_cbdata *cb = cb_data;
 	struct refinfo *ref;
-	int cnt;
 
 	if (flag & REF_BAD_NAME) {
 		  warning("ignoring ref with broken name %s", refname);
 		  return 0;
 	}
 
-	if (*cb->grab_pattern) {
-		const char **pattern;
-		int namelen = strlen(refname);
-		for (pattern = cb->grab_pattern; *pattern; pattern++) {
-			const char *p = *pattern;
-			int plen = strlen(p);
-
-			if ((plen <= namelen) &&
-			    !strncmp(refname, p, plen) &&
-			    (refname[plen] == '\0' ||
-			     refname[plen] == '/' ||
-			     p[plen-1] == '/'))
-				break;
-			if (!wildmatch(p, refname, WM_PATHNAME, NULL))
-				break;
-		}
-		if (!*pattern)
-			return 0;
-	}
+	if (*cb->grab_pattern && !match_name_as_path(cb->grab_pattern, refname))
+		return 0;
 
-	/*
-	 * We do not open the object yet; sort may only need refname
-	 * to do its job and the resulting list may yet to be pruned
-	 * by maxcount logic.
-	 */
-	ref = xcalloc(1, sizeof(*ref));
-	ref->refname = xstrdup(refname);
-	hashcpy(ref->objectname, sha1);
-	ref->flag = flag;
+	ref = new_refinfo(refname, sha1, flag);
+
+	REALLOC_ARRAY(cb->grab_array, cb->grab_cnt + 1);
+	cb->grab_array[cb->grab_cnt++] = ref;
 
-	cnt = cb->grab_cnt;
-	REALLOC_ARRAY(cb->grab_array, cnt + 1);
-	cb->grab_array[cnt++] = ref;
-	cb->grab_cnt = cnt;
 	return 0;
 }

Signature · 1 line

2.4.1

Matthieu Moy

2016-06-15 23:05 UTC #2 lore raw

Re: [PATCH/WIP v3 1/4] for-each-ref: extract helper functions out of grab_single_ref()

Quoted 2 lines · Karthik Nayak

This is a preperatory patch for restructuring 'for-each-ref' and evntually moving most of it to 'ref-filter' to provide the

s/evntually/eventually/

The patch looks much nicer now.

builtin/for-each-ref.c static int grab_single_ref(const char *refname, const unsigned char *sha1, int f · line 887

Quoted 9 lines

builtin/for-each-ref.c +2 -5

-	int cnt;
[...]
+	REALLOC_ARRAY(cb->grab_array, cb->grab_cnt + 1);
+	cb->grab_array[cb->grab_cnt++] = ref;
 
-	cnt = cb->grab_cnt;
-	REALLOC_ARRAY(cb->grab_array, cnt + 1);
-	cb->grab_array[cnt++] = ref;
-	cb->grab_cnt = cnt;

This hunk is not advertized in the commit message, and I had to fight a bit to understand what it does with "we're extracting helper functions" in mind. It would have been much easier to review in a separate patch entitled "for-each-ref: simplify code" for example.

But I agree that your version is simpler indeed.

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Karthik Nayak

author

2016-06-15 23:05 UTC #3 lore raw

Re: [PATCH/WIP v3 1/4] for-each-ref: extract helper functions out of grab_single_ref()

Quoted 6 lines · Matthieu Moy

Quoted 2 lines · Karthik Nayak
This is a preperatory patch for restructuring 'for-each-ref' and evntually moving most of it to 'ref-filter' to provide the
s/evntually/eventually/

Noted.

Quoted 18 lines

The patch looks much nicer now.
builtin/for-each-ref.c static int grab_single_ref(const char *refname, const unsigned char *sha1, int f · line 887
Quoted 9 lines
builtin/for-each-ref.c +2 -5
-	int cnt;
[...]
+	REALLOC_ARRAY(cb->grab_array, cb->grab_cnt + 1);
+	cb->grab_array[cb->grab_cnt++] = ref;
 
-	cnt = cb->grab_cnt;
-	REALLOC_ARRAY(cb->grab_array, cnt + 1);
-	cb->grab_array[cnt++] = ref;
-	cb->grab_cnt = cnt;
This hunk is not advertized in the commit message, and I had to fight a bit to understand what it does with "we're extracting helper functions" in mind. It would have been much easier to review in a separate patch entitled "for-each-ref: simplify code" for example.

Quoted 2 lines

But I agree that your version is simpler indeed.

Will put it in a separate patch. Thanks for the review.

Signature · 2 lines

Regards,
Karthik

Karthik Nayak

author

2016-06-15 23:05 UTC #4 lore raw

PATCH v3 2/4 for-each-ref: introduce new structures for better organisation

In reply to Karthik Nayak

Rename 'refinfo' to 'ref_array_item' and intoduce 'ref_filter_cbdata' which will hold 'ref_filter' (Conditions to filter the refs on) and 'ref_array' (The array of ref_array_items). Modify the code to use these new structures.

Re-order the fields in 'ref_array_item' so that refname can be eventually converted to a FLEX_ARRAY.

Introduce 'ref_filter_clear_data' to clear all allocated memory.

This is a preparatory patch to eventually move code from 'for-each-ref' to 'ref-filter' and making it publically available.

Mentored-by Christian Couder <[email protected]> Mentored-by: Matthieu Moy <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---
 builtin/for-each-ref.c | 103 +++++++++++++++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

builtin/for-each-ref.c +61 ~~-42~~

index 919d45e..d9fd512 100644
@@ -31,12 +31,26 @@ struct ref_sort {
 	unsigned reverse : 1;
 };
 
-struct refinfo {
-	char *refname;
+struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
 	const char *symref;
 	struct atom_value *value;
+	char *refname;
+};
+
+struct ref_array {
+	int nr, alloc;
+	struct ref_array_item **items;
+};
+
+struct ref_filter {
+	const char **name_patterns;
+};
+
+struct ref_filter_cbdata {
+	struct ref_array array;
+	struct ref_filter filter;
 };
 
 static struct {

@@ -85,7 +99,7 @@ static struct {
  * a "*" to denote deref_tag().
  *
  * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  refinfo
+ * of properties that we need to extract out of objects. ref_array_item
  * structure will hold an array of values extracted that can be
  * indexed with the "atom number", which is an index into this
  * array.

@@ -622,7 +636,7 @@ static inline char *copy_advance(char *dst, const char *src)
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct refinfo *ref)
+static void populate_value(struct ref_array_item *ref)
 {
 	void *buf;
 	struct object *obj;

@@ -821,7 +835,7 @@ static void populate_value(struct refinfo *ref)
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_value(struct refinfo *ref, int atom, struct atom_value **v)
+static void get_value(struct ref_array_item *ref, int atom, struct atom_value **v)
 {
 	if (!ref->value) {
 		populate_value(ref);

@@ -830,12 +844,6 @@ static void get_value(struct refinfo *ref, int atom, struct atom_value **v)
 	*v = &ref->value[atom];
 }
 
-struct grab_ref_cbdata {
-	struct refinfo **grab_array;
-	const char **grab_pattern;
-	int grab_cnt;
-};
-
 /*
  * Given a refname, return 1 if the refname matches with one of the patterns
  * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'

@@ -860,12 +868,12 @@ static int match_name_as_path(const char **pattern, const char *refname)
 	return 0;
 }
 
-/* Allocate space for a new refinfo and copy the objectname and flag to it */
-static struct refinfo *new_refinfo(const char *refname,
-				   const unsigned char *objectname,
-				   int flag)
+/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+static struct ref_array_item *new_ref_array_item(const char *refname,
+						 const unsigned char *objectname,
+						 int flag)
 {
-	struct refinfo *ref = xcalloc(1, sizeof(struct refinfo));
+	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item));
 	ref->refname = xstrdup(refname);
 	hashcpy(ref->objectname, objectname);
 	ref->flag = flag;

@@ -879,26 +887,39 @@ static struct refinfo *new_refinfo(const char *refname,
  */
 static int grab_single_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	struct grab_ref_cbdata *cb = cb_data;
-	struct refinfo *ref;
+	struct ref_filter_cbdata *ref_cbdata = cb_data;
+	struct ref_filter *filter = &ref_cbdata->filter;
+	struct ref_array_item *ref;
 
 	if (flag & REF_BAD_NAME) {
-		  warning("ignoring ref with broken name %s", refname);
-		  return 0;
+		warning("ignoring ref with broken name %s", refname);
+		return 0;
 	}
 
-	if (*cb->grab_pattern && !match_name_as_path(cb->grab_pattern, refname))
+	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
 		return 0;
 
-	ref = new_refinfo(refname, sha1, flag);
+	ref = new_ref_array_item(refname, sha1, flag);
 
-	REALLOC_ARRAY(cb->grab_array, cb->grab_cnt + 1);
-	cb->grab_array[cb->grab_cnt++] = ref;
+	REALLOC_ARRAY(ref_cbdata->array.items, ref_cbdata->array.nr + 1);
+	ref_cbdata->array.items[ref_cbdata->array.nr++] = ref;
 
 	return 0;
 }
 
-static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b)
+/* Free all memory allocated for ref_filter_cbdata */
+void ref_filter_clear_data(struct ref_filter_cbdata *ref_cbdata)
+{
+	int i;
+
+	for (i = 0; i < ref_cbdata->array.nr; i++)
+		free(ref_cbdata->array.items[i]);
+	free(ref_cbdata->array.items);
+	ref_cbdata->array.items = NULL;
+	ref_cbdata->array.nr = ref_cbdata->array.alloc = 0;
+}
+
+static int cmp_ref_sort(struct ref_sort *s, struct ref_array_item *a, struct ref_array_item *b)
 {
 	struct atom_value *va, *vb;
 	int cmp;

@@ -925,8 +946,8 @@ static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b
 static struct ref_sort *ref_sort;
 static int compare_refs(const void *a_, const void *b_)
 {
-	struct refinfo *a = *((struct refinfo **)a_);
-	struct refinfo *b = *((struct refinfo **)b_);
+	struct ref_array_item *a = *((struct ref_array_item **)a_);
+	struct ref_array_item *b = *((struct ref_array_item **)b_);
 	struct ref_sort *s;
 
 	for (s = ref_sort; s; s = s->next) {

@@ -937,10 +958,10 @@ static int compare_refs(const void *a_, const void *b_)
 	return 0;
 }
 
-static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs)
+static void sort_refs(struct ref_sort *sort, struct ref_array *array)
 {
 	ref_sort = sort;
-	qsort(refs, num_refs, sizeof(struct refinfo *), compare_refs);
+	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
 static void print_value(struct atom_value *v, int quote_style)

@@ -1007,7 +1028,7 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
-static void show_ref(struct refinfo *info, const char *format, int quote_style)
+static void show_ref(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;

@@ -1076,12 +1097,12 @@ static char const * const for_each_ref_usage[] = {
 
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
-	int i, num_refs;
+	int i;
 	const char *format = "%(objectname) %(objecttype)\t%(refname)";
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
 	int maxcount = 0, quote_style = 0;
-	struct refinfo **refs;
-	struct grab_ref_cbdata cbdata;
+	struct ref_filter_cbdata ref_cbdata;
+	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &quote_style,

@@ -1119,17 +1140,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	/* for warn_ambiguous_refs */
 	git_config(git_default_config, NULL);
 
-	memset(&cbdata, 0, sizeof(cbdata));
-	cbdata.grab_pattern = argv;
-	for_each_rawref(grab_single_ref, &cbdata);
-	refs = cbdata.grab_array;
-	num_refs = cbdata.grab_cnt;
+	ref_cbdata.filter.name_patterns = argv;
+	for_each_rawref(grab_single_ref, &ref_cbdata);
 
-	sort_refs(sort, refs, num_refs);
+	sort_refs(sort, &ref_cbdata.array);
 
-	if (!maxcount || num_refs < maxcount)
-		maxcount = num_refs;
+	if (!maxcount || ref_cbdata.array.nr < maxcount)
+		maxcount = ref_cbdata.array.nr;
 	for (i = 0; i < maxcount; i++)
-		show_ref(refs[i], format, quote_style);
+		show_ref(ref_cbdata.array.items[i], format, quote_style);
+	ref_filter_clear_data(&ref_cbdata);
 	return 0;
 }

Signature · 1 line

2.4.1

Matthieu Moy

2016-06-15 23:05 UTC #5 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

Quoted 1 line · Karthik Nayak

Rename 'refinfo' to 'ref_array_item' and intoduce 'ref_filter_cbdata'

The fact that you need to use "and" to describe your changes is a hint that you can split better.

The patch looks much better, but I think you still need to split more to make it easier to review.

Quoted 2 lines

- * of properties that we need to extract out of objects. refinfo
+ * of properties that we need to extract out of objects. ref_array_item

Not very important, but two spaces after a period is what one is supposed to do in English. Not everybody follow the rule, but it seems backward to change the code to break it.

builtin/for-each-ref.c static struct refinfo *new_refinfo(const char *refname, · line 896

Quoted 5 lines

builtin/for-each-ref.c +2 -2

 	if (flag & REF_BAD_NAME) {
-		  warning("ignoring ref with broken name %s", refname);
-		  return 0;
+		warning("ignoring ref with broken name %s", refname);
+		return 0;

Whitespace fix mixed with actual code.

builtin/for-each-ref.c static struct refinfo *new_refinfo(const char *refname, · line 920

Quoted 12 lines

builtin/for-each-ref.c +11 -1

-static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b)
+/* Free all memory allocated for ref_filter_cbdata */
+void ref_filter_clear_data(struct ref_filter_cbdata *ref_cbdata)
+{
+	int i;
+
+	for (i = 0; i < ref_cbdata->array.nr; i++)
+		free(ref_cbdata->array.items[i]);
+	free(ref_cbdata->array.items);
+	ref_cbdata->array.items = NULL;
+	ref_cbdata->array.nr = ref_cbdata->array.alloc = 0;
+}

And this one is a real behavior change, which would be much better documented in its own patch with a proper commit message (we had a memory leak before, we didn't care because it happened right before exiting, but we can't accept that in a clean library).

Reviewing is not just about having a look and seeing if there's something stupid. Reviewers are actually taking a lot of time to see if the patch does not introduce new bugs, or looking for better ways to do the same thing. Be nice with them!

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Karthik Nayak

author

2016-06-15 23:05 UTC #6 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

Quoted 7 lines · Matthieu Moy

Quoted 1 line · Karthik Nayak
Rename 'refinfo' to 'ref_array_item' and intoduce 'ref_filter_cbdata'
The fact that you need to use "and" to describe your changes is a hint that you can split better.

But the patch alone wouldn't make much sense here, as the whole idea is the introduction of the new structures and renaming 'refinfo' to 'ref_array_item' was more of a byproduct to go along with the new structures introduced.

Quoted 11 lines

The patch looks much better, but I think you still need to split more to make it easier to review.
Quoted 2 lines
- * of properties that we need to extract out of objects. refinfo
+ * of properties that we need to extract out of objects. ref_array_item
Not very important, but two spaces after a period is what one is supposed to do in English. Not everybody follow the rule, but it seems backward to change the code to break it.

I'm just so used to single spacing, Will change it.

Quoted 8 lines

builtin/for-each-ref.c static struct refinfo *new_refinfo(const char *refname, · line 896
Quoted 5 lines
  	if (flag & REF_BAD_NAME) {
builtin/for-each-ref.c +2 -2
-		  warning("ignoring ref with broken name %s", refname);
-		  return 0;
+		warning("ignoring ref with broken name %s", refname);
+		return 0;
Whitespace fix mixed with actual code.

Noted.

Quoted 9 lines

builtin/for-each-ref.c +8 -1

-static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b)
+/* Free all memory allocated for ref_filter_cbdata */
+void ref_filter_clear_data(struct ref_filter_cbdata *ref_cbdata)
+{
+	int i;
+
+	for (i = 0; i < ref_cbdata->array.nr; i++)
+		free(ref_cbdata->array.items[i]);
+	free(ref_cbdata->array.items);

Quoted 9 lines

builtin/for-each-ref.c static struct refinfo *new_refinfo(const char *refname, · line 920
Quoted 3 lines
builtin/for-each-ref.c +3 -0
+	ref_cbdata->array.items = NULL;
+	ref_cbdata->array.nr = ref_cbdata->array.alloc = 0;
+}
And this one is a real behavior change, which would be much better documented in its own patch with a proper commit message (we had a memory leak before, we didn't care because it happened right before exiting, but we can't accept that in a clean library).

Ok will put that into a separate commit.

Quoted 5 lines

Reviewing is not just about having a look and seeing if there's something stupid. Reviewers are actually taking a lot of time to see if the patch does not introduce new bugs, or looking for better ways to do the same thing. Be nice with them!

Thanks for the effort from your side, will try to split things as much as possible and make it easier for Reviewers.

Signature · 2 lines

Regards,
Karthik

Matthieu Moy

2016-06-15 23:05 UTC #7 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

Quoted 5 lines · Karthik Nayak

Quoted 4 lines · Matthieu Moy
Quoted 1 line · Karthik Nayak
Rename 'refinfo' to 'ref_array_item' and intoduce 'ref_filter_cbdata'

Quoted 8 lines

Quoted 3 lines
The fact that you need to use "and" to describe your changes is a hint that you can split better.
But the patch alone wouldn't make much sense here, as the whole idea is the introduction of the new structures and renaming 'refinfo' to ref_array_item' was more of a byproduct to go along with the new structures introduced.

The point of separating them is that the rename implies a relatively long patch (you have 17 occurences of 'refinfo' in the deletion part of your patch), but it is straightforward to review (apply, run "git diff --color-words" and press space a few times). But it is no longer so simple once you mix it with anything else.

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Karthik Nayak

author

2016-06-15 23:05 UTC #8 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

Quoted 6 lines · Matthieu Moy

The point of separating them is that the rename implies a relatively long patch (you have 17 occurences of 'refinfo' in the deletion part of your patch), but it is straightforward to review (apply, run "git diff --color-words" and press space a few times). But it is no longer so simple once you mix it with anything else.

Makes sense, thanks for putting it out.

Signature · 2 lines

Regards,
Karthik

Junio C Hamano

2016-06-15 23:05 UTC #9 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

In reply to Matthieu Moy

Quoted 3 lines · Matthieu Moy

Not very important, but two spaces after a period is what one is supposed to do in English. Not everybody follow the rule, but it seems backward to change the code to break it.

I am old fashioned enough to personally agree, but this practice that originates in the typewriter era is frowned upon in many circles, including those who work with professional typesetters, I think.

Sent off-list to reduce noise level; this is an issue that attracts a lot of useless comments.

Junio C Hamano

2016-06-15 23:05 UTC #10 lore raw

Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

Quoted 2 lines · Junio C Hamano

Sent off-list to reduce noise level; this is an issue that attracts a lot of useless comments.

Heh, somehow I screwed up and sent on-list X-<.

Karthik Nayak

author

2016-06-15 23:05 UTC #11 lore raw

PATCH v3 3/4 for-each-ref: rename some functions and make them public

In reply to Karthik Nayak

Rename some of the functions and make them publically available. This is a preparatory step for moving code from 'for-each-ref' to 'ref-filter' to make meaningful, targeted services available to other commands via public APIs.

Based-on-patch-by Jeff King <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Matthieu Moy <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---
 builtin/for-each-ref.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

builtin/for-each-ref.c +23 ~~-22~~

index d9fd512..f045def 100644
@@ -112,7 +112,7 @@ static int need_color_reset_at_eol;
 /*
  * Used to parse format string and sort specifiers
  */
-static int parse_atom(const char *atom, const char *ep)
+int parse_ref_filter_atom(const char *atom, const char *ep)
 {
 	const char *sp;
 	int i, at;

@@ -189,7 +189,7 @@ static const char *find_next(const char *cp)
  * Make sure the format string is well formed, and parse out
  * the used atoms.
  */
-static int verify_format(const char *format)
+int verify_ref_format(const char *format)
 {
 	const char *cp, *sp;

@@ -201,7 +201,7 @@ static int verify_format(const char *format)
 		if (!ep)
 			return error("malformed format string %s", sp);
 		/* sp points at "%(" and ep points at the closing ")" */
-		at = parse_atom(sp + 2, ep);
+		at = parse_ref_filter_atom(sp + 2, ep);
 		cp = ep + 1;
 
 		if (skip_prefix(used_atom[at], "color:", &color))

@@ -408,7 +408,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	/*
 	 * We got here because atomname ends in "date" or "date<something>";
 	 * it's not possible that <something> is not ":<format>" because
-	 * parse_atom() wouldn't have allowed it, so we can assume that no
+	 * parse_ref_filter_atom() wouldn't have allowed it, so we can assume that no
 	 * ":" means no format is specified, and use the default.
 	 */
 	formatp = strchr(atomname, ':');

@@ -835,7 +835,7 @@ static void populate_value(struct ref_array_item *ref)
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
 {
 	if (!ref->value) {
 		populate_value(ref);

@@ -882,10 +882,10 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 }
 
 /*
- * A call-back given to for_each_ref().  Filter refs and keep them for
+ * A call-back given to for_each_ref(). Filter refs and keep them for
  * later object processing.
  */
-static int grab_single_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+int ref_filter_handler(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	struct ref_filter_cbdata *ref_cbdata = cb_data;
 	struct ref_filter *filter = &ref_cbdata->filter;

@@ -925,8 +925,8 @@ static int cmp_ref_sort(struct ref_sort *s, struct ref_array_item *a, struct ref
 	int cmp;
 	cmp_type cmp_type = used_atom_type[s->atom];
 
-	get_value(a, s->atom, &va);
-	get_value(b, s->atom, &vb);
+	get_ref_atom_value(a, s->atom, &va);
+	get_ref_atom_value(b, s->atom, &vb);
 	switch (cmp_type) {
 	case FIELD_STR:
 		cmp = strcmp(va->s, vb->s);

@@ -958,7 +958,7 @@ static int compare_refs(const void *a_, const void *b_)
 	return 0;
 }
 
-static void sort_refs(struct ref_sort *sort, struct ref_array *array)
+void sort_ref_array(struct ref_sort *sort, struct ref_array *array)
 {
 	ref_sort = sort;
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);

@@ -1028,7 +1028,7 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
-static void show_ref(struct ref_array_item *info, const char *format, int quote_style)
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;

@@ -1038,7 +1038,7 @@ static void show_ref(struct ref_array_item *info, const char *format, int quote_
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			emit(cp, sp);
-		get_value(info, parse_atom(sp + 2, ep), &atomv);
+		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
 		print_value(atomv, quote_style);
 	}
 	if (*cp) {

@@ -1057,18 +1057,19 @@ static void show_ref(struct ref_array_item *info, const char *format, int quote_
 	putchar('\n');
 }
 
-static struct ref_sort *default_sort(void)
+/*  If no sorting option is given, use refname to sort as default */
+struct ref_sort *ref_default_sort(void)
 {
 	static const char cstr_name[] = "refname";
 
 	struct ref_sort *sort = xcalloc(1, sizeof(*sort));
 
 	sort->next = NULL;
-	sort->atom = parse_atom(cstr_name, cstr_name + strlen(cstr_name));
+	sort->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name));
 	return sort;
 }
 
-static int opt_parse_sort(const struct option *opt, const char *arg, int unset)
+int opt_parse_ref_sort(const struct option *opt, const char *arg, int unset)
 {
 	struct ref_sort **sort_tail = opt->value;
 	struct ref_sort *s;

@@ -1086,7 +1087,7 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)
 		arg++;
 	}
 	len = strlen(arg);
-	s->atom = parse_atom(arg, arg+len);
+	s->atom = parse_ref_filter_atom(arg, arg+len);
 	return 0;
 }

@@ -1118,7 +1119,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, N_("format"), N_("format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
-			    N_("field name to sort on"), &opt_parse_sort),
+			    N_("field name to sort on"), &opt_parse_ref_sort),
 		OPT_END(),
 	};

@@ -1131,24 +1132,24 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		error("more than one quoting style?");
 		usage_with_options(for_each_ref_usage, opts);
 	}
-	if (verify_format(format))
+	if (verify_ref_format(format))
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sort)
-		sort = default_sort();
+		sort = ref_default_sort();
 
 	/* for warn_ambiguous_refs */
 	git_config(git_default_config, NULL);
 
 	ref_cbdata.filter.name_patterns = argv;
-	for_each_rawref(grab_single_ref, &ref_cbdata);
+	for_each_rawref(ref_filter_handler, &ref_cbdata);
 
-	sort_refs(sort, &ref_cbdata.array);
+	sort_ref_array(sort, &ref_cbdata.array);
 
 	if (!maxcount || ref_cbdata.array.nr < maxcount)
 		maxcount = ref_cbdata.array.nr;
 	for (i = 0; i < maxcount; i++)
-		show_ref(ref_cbdata.array.items[i], format, quote_style);
+		show_ref_array_item(ref_cbdata.array.items[i], format, quote_style);
 	ref_filter_clear_data(&ref_cbdata);
 	return 0;
 }

Signature · 1 line

2.4.1

Karthik Nayak

author

2016-06-15 23:05 UTC #12 lore raw

PATCH v3 4/4 ref-filter: move code from 'for-each-ref'

In reply to Karthik Nayak

Move most of the code from 'for-each-ref' to 'ref-filter' to make it publically available to other commands, this is to unify the code of 'tag -l', 'branch -l' and 'for-each-ref' so that they can share their implementations with each other.

Mentored-by Christian Couder <[email protected]>

Mentored-by Matthieu Moy <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---
 Makefile               |    1 +
 builtin/for-each-ref.c | 1089 +-----------------------------------------------
 ref-filter.c           | 1051 ++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h           |   66 +++
 4 files changed, 1119 insertions(+), 1088 deletions(-)
 create mode 100644 ref-filter.c
 create mode 100644 ref-filter.h

Makefile +1 -0

index 36655d5..068d313 100644
@@ -761,6 +761,7 @@ LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
 LIB_OBJS += replace_object.o
 LIB_OBJS += rerere.o

builtin/for-each-ref.c +1 ~~-1088~~

1,096 lines not on this page · raw message

ref-filter.c new file +1051 -0

1,051 lines not on this page · raw message

ref-filter.h new file +66 -0

66 lines not on this page · raw message

Signature · 1 line

2.4.1

Matthieu Moy

2016-06-15 23:05 UTC #13 lore raw

Re: [PATCH/WIP v3 4/4] ref-filter: move code from 'for-each-ref'

Quoted 2 lines · Karthik Nayak

Move most of the code from 'for-each-ref' to 'ref-filter' to make it publically available to other commands, this is to unify the code

s/publically/publicly/

ref-filter.h

Quoted 2 lines

ref-filter.h new file +0 -0

Moving file to the .h file should be done in a separate patch. I don't want to review the 1050 lines of cut-and-paste other than by doing the cut-and-paste myself and see if I get the same result, but this part is not as straightforward and needs proper thinking and review.

In short: the big code movement should be *only* a cut-and-paste, alone in its own patch.

On overall, we're getting close to an acceptable version for these 4 patches. My advice: prioritize polishing these few patches, so that we can do a detailed review and then consider this part "done" (either merged in git.git or not, but I'd like to avoid having to review the code movement & refactoring again when we move to the next patches).

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Karthik Nayak

author

2016-06-15 23:05 UTC #14 lore raw

Re: [PATCH/WIP v3 4/4] ref-filter: move code from 'for-each-ref'

Quoted 15 lines · Matthieu Moy

Quoted 2 lines · Karthik Nayak
Move most of the code from 'for-each-ref' to 'ref-filter' to make it publically available to other commands, this is to unify the code
s/publically/publicly/
ref-filter.h
Quoted 2 lines
ref-filter.h new file +0 -0
Moving file to the .h file should be done in a separate patch. I don't want to review the 1050 lines of cut-and-paste other than by doing the cut-and-paste myself and see if I get the same result, but this part is not as straightforward and needs proper thinking and review.

Quoted 3 lines

In short: the big code movement should be *only* a cut-and-paste, alone in its own patch.

Ok, will separate it into two patches.

Quoted 7 lines

On overall, we're getting close to an acceptable version for these 4 patches. My advice: prioritize polishing these few patches, so that we can do a detailed review and then consider this part "done" (either merged in git.git or not, but I'd like to avoid having to review the code movement & refactoring again when we move to the next patches).

Sure I'll fix changes suggested by you and push to Github probably by tonight (IST), will wait for a day or two to see if there are more suggestions by others on the mailing list before sending a new patch series here.

Signature · 2 lines

Regards,
Karthik