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

2015-05-28 Thread Matthieu Moy
Karthik Nayak karthik@gmail.com writes:

 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.

 - 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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-05-28 Thread Karthik Nayak

On 05/29/2015 01:43 AM, Matthieu Moy wrote:

Karthik Nayak karthik@gmail.com writes:


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.




The patch looks much nicer now.


-   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.



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

--
Regards,
Karthik
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-05-28 Thread Karthik Nayak
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 gits...@pobox.com
Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
 builtin/for-each-ref.c | 76 --
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 83f9cf9..919d45e 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -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;
 }
 
-- 
2.4.1

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html