Re: [PATCH 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames

2013-05-06 Thread Junio C Hamano
Johan Herland  writes:

> diff --git a/refs.c b/refs.c
> index 7231f54..8b02140 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)
>   0);
>  }
>  
> -const char *ref_rev_parse_rules[] = {
> +static void ref_expand_txtly(const struct ref_expand_rule *rule,
> +  char *dst, size_t dst_len,
> +  const char *shortname, size_t shortname_len)
> +{
> + mksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);
> +}
> +
> +const struct ref_expand_rule ref_expand_rules[] = {
> + { ref_expand_txtly, "%.*s" },
> + { ref_expand_txtly, "refs/%.*s" },
> + { ref_expand_txtly, "refs/tags/%.*s" },
> + { ref_expand_txtly, "refs/heads/%.*s" },
> + { ref_expand_txtly, "refs/remotes/%.*s" },
> + { ref_expand_txtly, "refs/remotes/%.*s/HEAD" },
> + { NULL, NULL }
> +};
> +
> +static const char *ref_rev_parse_rules[] = {
>   "%.*s",
>   "refs/%.*s",
>   "refs/tags/%.*s",
> @@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {
>   NULL
>  };
>  
> -int refname_match(const char *abbrev_name, const char *full_name, const char 
> **rules)
> +int refname_match(const char *abbrev_name, const char *full_name,
> +   const struct ref_expand_rule *rules)
>  {
> - const char **p;
> + const struct ref_expand_rule *p;
>   const int abbrev_name_len = strlen(abbrev_name);
> + char n[PATH_MAX];

Hmmm, is it too much to ask to do this without a fixed length
buffer?  I think we have long learned the value of using strbuf to
avoid having to worry about buffer overruns.

I am OK with the idea to make ref_expand_rules[] customizable, and
the overall strategy taken by this step to refactor the current code
looks reasonably sensible.

--
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 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames

2013-05-04 Thread Johan Herland
In preparation for allowing alternative ways of expanding shorthand refs
(like "master") into full refnames (like "refs/heads/master"): Expand
the current ref_rev_parse_rules list into ref_expand_rules, a list of
struct ref_expand_rule objects that encode both an expansion pattern
(e.g. "refs/heads/%.*s") and an associated expansion function
(e.g. the code that applies "master" to "refs/heads/%.*s" to produce
"refs/heads/master"). This allows us to later add expansion rules that
do something other than the current purely textual expansion.

The current expansion behavior is encoded in the new ref_expand_txtly()
helper function, which does the mksnpath() call that were previously
performed by all users of ref_rev_parse_rules.

The end result is identical in behavior to the existing code, but makes
it easier to adjust the way ref expansion happens for remote-tracking
branches in future patches

Most of the existing code that uses ref_rev_parse_rules to expand
shorthand refs are converted to use ref_expand_rules instead.

Signed-off-by: Johan Herland 
---
 cache.h  |  4 
 refs.c   | 46 +-
 refs.h   | 11 +++
 remote.c |  6 +++---
 4 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/cache.h b/cache.h
index 7ce9061..6adab04 100644
--- a/cache.h
+++ b/cache.h
@@ -875,10 +875,6 @@ extern int dwim_log(const char *str, int len, unsigned 
char *sha1, char **ref);
 extern int interpret_branch_name(const char *str, struct strbuf *);
 extern int get_sha1_mb(const char *str, unsigned char *sha1);
 
-extern int refname_match(const char *abbrev_name, const char *full_name, const 
char **rules);
-extern const char *ref_rev_parse_rules[];
-#define ref_fetch_rules ref_rev_parse_rules
-
 extern int create_symref(const char *ref, const char *refs_heads_master, const 
char *logmsg);
 extern int validate_headref(const char *ref);
 
diff --git a/refs.c b/refs.c
index 7231f54..8b02140 100644
--- a/refs.c
+++ b/refs.c
@@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)
0);
 }
 
-const char *ref_rev_parse_rules[] = {
+static void ref_expand_txtly(const struct ref_expand_rule *rule,
+char *dst, size_t dst_len,
+const char *shortname, size_t shortname_len)
+{
+   mksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);
+}
+
+const struct ref_expand_rule ref_expand_rules[] = {
+   { ref_expand_txtly, "%.*s" },
+   { ref_expand_txtly, "refs/%.*s" },
+   { ref_expand_txtly, "refs/tags/%.*s" },
+   { ref_expand_txtly, "refs/heads/%.*s" },
+   { ref_expand_txtly, "refs/remotes/%.*s" },
+   { ref_expand_txtly, "refs/remotes/%.*s/HEAD" },
+   { NULL, NULL }
+};
+
+static const char *ref_rev_parse_rules[] = {
"%.*s",
"refs/%.*s",
"refs/tags/%.*s",
@@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {
NULL
 };
 
-int refname_match(const char *abbrev_name, const char *full_name, const char 
**rules)
+int refname_match(const char *abbrev_name, const char *full_name,
+ const struct ref_expand_rule *rules)
 {
-   const char **p;
+   const struct ref_expand_rule *p;
const int abbrev_name_len = strlen(abbrev_name);
+   char n[PATH_MAX];
 
-   for (p = rules; *p; p++) {
-   if (!strcmp(full_name, mkpath(*p, abbrev_name_len, 
abbrev_name))) {
+   for (p = rules; p->expand; p++) {
+   p->expand(p, n, sizeof(n), abbrev_name, abbrev_name_len);
+   if (!strcmp(full_name, n))
return 1;
-   }
}
 
return 0;
@@ -1807,21 +1826,22 @@ static char *substitute_branch_name(const char 
**string, int *len)
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
char *last_branch = substitute_branch_name(&str, &len);
-   const char **p, *r;
+   const struct ref_expand_rule *p;
+   const char *r;
int refs_found = 0;
 
*ref = NULL;
-   for (p = ref_rev_parse_rules; *p; p++) {
+   for (p = ref_expand_rules; p->expand; p++) {
char fullref[PATH_MAX];
unsigned char sha1_from_ref[20];
unsigned char *this_result;
int flag;
 
this_result = refs_found ? sha1_from_ref : sha1;
-   mksnpath(fullref, sizeof(fullref), *p, len, str);
+   p->expand(p, fullref, sizeof(fullref), str, len);
r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
if (r) {
-   if (!refs_found++)
+   if ((!*ref || strcmp(*ref, r)) && !refs_found++)
*ref = xstrdup(r);
if (!warn_ambiguous_refs)
break;
@@ -1838,17 +1858,17 @@ int dwim_ref(const char *str, int len, unsigned char 
*sha1, char **ref)
 int dwim_log(const char *st