[PATCH 0/3] Generate scanf_fmts more simply

[PATCH 0/3] Generate scanf_fmts more simply

[Michael Haggerty]

This is just a fun little thing that I noticed while poking around the
code: the function gen_scanf_fmt() can be replaced with a simple call
to snprintf().

Michael Haggerty (3):
  shorten_unambiguous_ref(): introduce a new local variable
  gen_scanf_fmt(): delete function and use snprintf() instead
  shorten_unambiguous_ref(): tighten up pointer arithmetic

 refs.c | 47 +++++++++++++++--------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

-- 
1.8.5.2

[PATCH 1/3] shorten_unambiguous_ref(): introduce a new local variable

[Michael Haggerty]

When filling the scanf_fmts array, use a separate variable to keep
track of the offset to avoid clobbering total_len (which we will need
in the next commit).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 3926136..9530480 100644
--- a/refs.c
+++ b/refs.c
@@ -3367,6 +3367,7 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	/* pre generate scanf formats from ref_rev_parse_rules[] */
 	if (!nr_rules) {
 		size_t total_len = 0;
+		size_t offset = 0;
 
 		/* the rule list is NULL terminated, count them first */
 		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
@@ -3375,12 +3376,11 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 
 		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
 
-		total_len = 0;
+		offset = 0;
 		for (i = 0; i < nr_rules; i++) {
-			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
-					+ total_len;
+			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;
 			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
-			total_len += strlen(ref_rev_parse_rules[i]);
+			offset += strlen(ref_rev_parse_rules[i]);
 		}
 	}
 
-- 
1.8.5.2

[PATCH 2/3] gen_scanf_fmt(): delete function and use snprintf() instead

[Michael Haggerty]

To replace "%.*s" with "%s", all we have to do is use snprintf()
to interpolate "%s" into the pattern.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/refs.c b/refs.c
index 9530480..ef9cdea 100644
--- a/refs.c
+++ b/refs.c
@@ -3334,29 +3334,6 @@ cleanup:
 	return ret;
 }
 
-/*
- * generate a format suitable for scanf from a ref_rev_parse_rules
- * rule, that is replace the "%.*s" spec with a "%s" spec
- */
-static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
-{
-	char *spec;
-
-	spec = strstr(rule, "%.*s");
-	if (!spec || strstr(spec + 4, "%.*s"))
-		die("invalid rule in ref_rev_parse_rules: %s", rule);
-
-	/* copy all until spec */
-	strncpy(scanf_fmt, rule, spec - rule);
-	scanf_fmt[spec - rule] = '\0';
-	/* copy new spec */
-	strcat(scanf_fmt, "%s");
-	/* copy remaining rule */
-	strcat(scanf_fmt, spec + 4);
-
-	return;
-}
-
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
 	int i;
@@ -3364,8 +3341,13 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	static int nr_rules;
 	char *short_name;
 
-	/* pre generate scanf formats from ref_rev_parse_rules[] */
 	if (!nr_rules) {
+		/*
+		 * Pre-generate scanf formats from ref_rev_parse_rules[].
+		 * Generate a format suitable for scanf from a
+		 * ref_rev_parse_rules rule by interpolating "%s" at the
+		 * location of the "%.*s".
+		 */
 		size_t total_len = 0;
 		size_t offset = 0;
 
@@ -3378,9 +3360,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 
 		offset = 0;
 		for (i = 0; i < nr_rules; i++) {
+			assert(offset < total_len);
 			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;
-			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
-			offset += strlen(ref_rev_parse_rules[i]);
+			offset += snprintf(scanf_fmts[i], total_len - offset,
+					   ref_rev_parse_rules[i], 2, "%s") + 1;
 		}
 	}
 
-- 
1.8.5.2

Re: [PATCH 2/3] gen_scanf_fmt(): delete function and use snprintf() instead

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> To replace "%.*s" with "%s", all we have to do is use snprintf()
> to interpolate "%s" into the pattern.

Cute ;-).  Thanks.

[PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic

[Michael Haggerty]

As long as we're being pathologically stingy with mallocs, we might as
well do the math right and save 6 (!) bytes.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
It is left to the reader to show how another 7 bytes could be saved
(11 bytes on a 64-bit architecture!)

It probably wouldn't kill performance to use a string_list here
instead.

 refs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index ef9cdea..63b3a71 100644
--- a/refs.c
+++ b/refs.c
@@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		size_t total_len = 0;
 		size_t offset = 0;
 
-		/* the rule list is NULL terminated, count them first */
+		/* the rule list is NUL terminated, count them first */
 		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
-			/* no +1 because strlen("%s") < strlen("%.*s") */
-			total_len += strlen(ref_rev_parse_rules[nr_rules]);
+			/* -2 for strlen("%.*s") - strlen("%s"); +1 for NUL */
+			total_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;
 
 		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
 
-- 
1.8.5.2

Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> As long as we're being pathologically stingy with mallocs, we might as
> well do the math right and save 6 (!) bytes.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> It is left to the reader to show how another 7 bytes could be saved
> (11 bytes on a 64-bit architecture!)
>
> It probably wouldn't kill performance to use a string_list here
> instead.
>
>  refs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ef9cdea..63b3a71 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
>  		size_t total_len = 0;
>  		size_t offset = 0;
>  
> -		/* the rule list is NULL terminated, count them first */
> +		/* the rule list is NUL terminated, count them first */

I think this _is_ wrong; it talks about the NULL termination of the
ref_rev_parse_rules[] array, not each string that is an element of
the array being NUL terminated.

Output from "git grep -e refname_match -e ref_rev_parse_rules"
suggests me that we actually could make ref_rev_parse_rules[] a
file-scope static to refs.c, remove its NULL termination and convert
all the iterators of the array to use ARRAY_SIZE() on it, after
dropping the third parameter to refname_match().  That way, we do
not have to count them first here.

But that is obviously a separate topic.

>  		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
> -			/* no +1 because strlen("%s") < strlen("%.*s") */
> -			total_len += strlen(ref_rev_parse_rules[nr_rules]);
> +			/* -2 for strlen("%.*s") - strlen("%s"); +1 for NUL */
> +			total_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;
>  
>  		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);

Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic

[Michael Haggerty]

On 01/10/2014 12:01 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> As long as we're being pathologically stingy with mallocs, we might as
>> well do the math right and save 6 (!) bytes.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> It is left to the reader to show how another 7 bytes could be saved
>> (11 bytes on a 64-bit architecture!)
>>
>> It probably wouldn't kill performance to use a string_list here
>> instead.
>>
>>  refs.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index ef9cdea..63b3a71 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
>>  		size_t total_len = 0;
>>  		size_t offset = 0;
>>  
>> -		/* the rule list is NULL terminated, count them first */
>> +		/* the rule list is NUL terminated, count them first */
> 
> I think this _is_ wrong; it talks about the NULL termination of the
> ref_rev_parse_rules[] array, not each string that is an element of
> the array being NUL terminated.

Yes, you're right.  Thanks for catching my sloppiness.  Would you mind
squashing the fix onto my patch?

> Output from "git grep -e refname_match -e ref_rev_parse_rules"
> suggests me that we actually could make ref_rev_parse_rules[] a
> file-scope static to refs.c, remove its NULL termination and convert
> all the iterators of the array to use ARRAY_SIZE() on it, after
> dropping the third parameter to refname_match().  That way, we do
> not have to count them first here.
> 
> But that is obviously a separate topic.
> 
>>  		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
>> -			/* no +1 because strlen("%s") < strlen("%.*s") */
>> -			total_len += strlen(ref_rev_parse_rules[nr_rules]);
>> +			/* -2 for strlen("%.*s") - strlen("%s"); +1 for NUL */
>> +			total_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;
>>  
>>  		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);

The way the code is written now (e.g., as long as it is not converted to
use a string_list or something) needs this loop not only to count the
number of rules but also to compute the total_len of the string into
which will be written all of the scanf format strings.

As for removing the third argument of refname_match(): although all
callers pass it ref_ref_parse_rules, that array is sometimes passed to
the function via the alias "ref_fetch_rules".  So I suppose somebody
wanted to leave the way open to make these two rule sets diverge (though
I don't know how likely that is to occur).  If we discard the third
argument to refname_match(), then we loose the distinction.

Thanks for your feedback,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> As for removing the third argument of refname_match(): although all
> callers pass it ref_ref_parse_rules, that array is sometimes passed to
> the function via the alias "ref_fetch_rules".  So I suppose somebody
> wanted to leave the way open to make these two rule sets diverge (though
> I don't know how likely that is to occur).

It is the other way around.  We used to use two separate rules for
the normal ref resolution dwimming and dwimming done to decide which
remote ref to grab.  For example, 'x' in 'git log x' can mean
'refs/remotes/x/HEAD', but 'git fetch origin x' would not grab
'refs/remotes/x/HEAD' at 'origin'.  Also, 'git fetch origin v1.0'
did not look into 'refs/tags/v1.0' in the 'origin' repository back
when these two rules were different.

This was originally done very much on purpose, in order to avoid
surprises and to discourage meaningless usage---you would not expect
us to be interested in the state of a third repository that was
observed by our 'origin' the last time (which we do not even know
when).

When we harmonized these two rules later and we #defined out
ref_fetch_rules away to avoid potential breakages for in-flight
topics.  The old synonym can now go safely.

[PATCH] refname_match(): always use the rules in ref_rev_parse_rules

[Michael Haggerty]

We used to use two separate rules for the normal ref resolution
dwimming and dwimming done to decide which remote ref to grab.  The
third parameter to refname_match() selected which rules to use.

When these two rules were harmonized in

    2011-11-04 dd621df9cd refs DWIMmery: use the same rule for both "git fetch" and others

, ref_fetch_rules was #defined to avoid potential breakages for
in-flight topics.

It is now safe to remove the backwards-compatibility code, so remove
refname_match()'s third parameter, make ref_rev_parse_rules private to
refs.c, and remove ref_fetch_rules entirely.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
See

    http://article.gmane.org/gmane.comp.version-control.git/240305

in which Junio made the suggestion and wrote most of the commit
message :-)

 cache.h  | 9 ++++++---
 refs.c   | 6 +++---
 remote.c | 8 ++++----
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 83a2726..508c49a 100644
--- a/cache.h
+++ b/cache.h
@@ -893,9 +893,12 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 extern int interpret_branch_name(const char *str, int len, 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
+/*
+ * Return true iff abbrev_name is a possible abbreviation for
+ * full_name according to the rules defined by ref_rev_parse_rules in
+ * refs.c.
+ */
+extern int refname_match(const char *abbrev_name, const char *full_name);
 
 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 490b57b..fc33ee8 100644
--- a/refs.c
+++ b/refs.c
@@ -1880,7 +1880,7 @@ const char *prettify_refname(const char *name)
 		0);
 }
 
-const char *ref_rev_parse_rules[] = {
+static const char *ref_rev_parse_rules[] = {
 	"%.*s",
 	"refs/%.*s",
 	"refs/tags/%.*s",
@@ -1890,12 +1890,12 @@ 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 char **p;
 	const int abbrev_name_len = strlen(abbrev_name);
 
-	for (p = rules; *p; p++) {
+	for (p = ref_rev_parse_rules; *p; p++) {
 		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
 			return 1;
 		}
diff --git a/remote.c b/remote.c
index a89efab..e41251e 100644
--- a/remote.c
+++ b/remote.c
@@ -1000,7 +1000,7 @@ int count_refspec_match(const char *pattern,
 		char *name = refs->name;
 		int namelen = strlen(name);
 
-		if (!refname_match(pattern, name, ref_rev_parse_rules))
+		if (!refname_match(pattern, name))
 			continue;
 
 		/* A match is "weak" if it is with refs outside
@@ -1571,7 +1571,7 @@ int branch_merge_matches(struct branch *branch,
 {
 	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
+	return refname_match(branch->merge[i]->src, refname);
 }
 
 static int ignore_symref_update(const char *refname)
@@ -1624,7 +1624,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c
 {
 	const struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		if (refname_match(name, ref->name, ref_fetch_rules))
+		if (refname_match(name, ref->name))
 			return ref;
 	}
 	return NULL;
@@ -2121,7 +2121,7 @@ static void apply_cas(struct push_cas_option *cas,
 	/* Find an explicit --<option>=<name>[:<value>] entry */
 	for (i = 0; i < cas->nr; i++) {
 		struct push_cas *entry = &cas->entry[i];
-		if (!refname_match(entry->refname, ref->name, ref_rev_parse_rules))
+		if (!refname_match(entry->refname, ref->name))
 			continue;
 		ref->expect_old_sha1 = 1;
 		if (!entry->use_tracking)
-- 
1.8.5.2

Re: [PATCH] refname_match(): always use the rules in ref_rev_parse_rules

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> We used to use two separate rules for the normal ref resolution
> dwimming and dwimming done to decide which remote ref to grab.  The
> third parameter to refname_match() selected which rules to use.
>
> When these two rules were harmonized in
>
>     2011-11-04 dd621df9cd refs DWIMmery: use the same rule for both "git fetch" and others
>
> , ref_fetch_rules was #defined to avoid potential breakages for
> in-flight topics.
>
> It is now safe to remove the backwards-compatibility code, so remove
> refname_match()'s third parameter, make ref_rev_parse_rules private to
> refs.c, and remove ref_fetch_rules entirely.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> See
>
>     http://article.gmane.org/gmane.comp.version-control.git/240305
>
> in which Junio made the suggestion and wrote most of the commit
> message :-)

;-) ...and on top of it this may be an obvious endgame follow-up.

was done mindlessly and mechanically, so there may be some slip-ups,
though.


 refs.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index 5a10c25..b1c9cf5 100644
--- a/refs.c
+++ b/refs.c
@@ -1886,16 +1886,16 @@ static const char *ref_rev_parse_rules[] = {
 	"refs/tags/%.*s",
 	"refs/heads/%.*s",
 	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
-	NULL
+	"refs/remotes/%.*s/HEAD"
 };
 
 int refname_match(const char *abbrev_name, const char *full_name)
 {
-	const char **p;
+	int i;
 	const int abbrev_name_len = strlen(abbrev_name);
 
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (i = 0; i < ARRAY_SIZE(ref_rev_parse_rules); i++) {
+		const char **p = &ref_rev_parse_rules[i];
 		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
 			return 1;
 		}
@@ -1963,11 +1963,13 @@ 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;
+	int i;
+	const char *r;
 	int refs_found = 0;
 
 	*ref = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (i = 0; i < ARRAY_SIZE(ref_rev_parse_rules); i++) {
+		const char **p = &ref_rev_parse_rules[i];
 		char fullref[PATH_MAX];
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
@@ -1994,11 +1996,11 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
-	const char **p;
-	int logs_found = 0;
+	int logs_found = 0, i;
 
 	*log = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (i = 0; i < ARRAY_SIZE(ref_rev_parse_rules); i++) {
+		const char **p = &ref_rev_parse_rules[i];
 		struct stat st;
 		unsigned char hash[20];
 		char path[PATH_MAX];
@@ -3368,8 +3370,8 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	if (!nr_rules) {
 		size_t total_len = 0;
 
-		/* the rule list is NULL terminated, count them first */
-		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
+		/* Count the bytesize needed to hold rule strings */
+		for (nr_rules = 0; ARRAY_SIZE(ref_rev_parse_rules); nr_rules++)
 			/* no +1 because strlen("%s") < strlen("%.*s") */
 			total_len += strlen(ref_rev_parse_rules[nr_rules]);
 

Re: [PATCH] refname_match(): always use the rules in ref_rev_parse_rules

[Michael Haggerty]

On 01/14/2014 11:16 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> We used to use two separate rules for the normal ref resolution
>> dwimming and dwimming done to decide which remote ref to grab.  The
>> third parameter to refname_match() selected which rules to use.
>>
>> When these two rules were harmonized in
>>
>>     2011-11-04 dd621df9cd refs DWIMmery: use the same rule for both "git fetch" and others
>>
>> , ref_fetch_rules was #defined to avoid potential breakages for
>> in-flight topics.
>>
>> It is now safe to remove the backwards-compatibility code, so remove
>> refname_match()'s third parameter, make ref_rev_parse_rules private to
>> refs.c, and remove ref_fetch_rules entirely.
>>
>> Suggested-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> See
>>
>>     http://article.gmane.org/gmane.comp.version-control.git/240305
>>
>> in which Junio made the suggestion and wrote most of the commit
>> message :-)
> 
> ;-) ...and on top of it this may be an obvious endgame follow-up.
> 
> was done mindlessly and mechanically, so there may be some slip-ups,
> though.
> 
> 
>  refs.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5a10c25..b1c9cf5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1886,16 +1886,16 @@ static const char *ref_rev_parse_rules[] = {
>  	"refs/tags/%.*s",
>  	"refs/heads/%.*s",
>  	"refs/remotes/%.*s",
> -	"refs/remotes/%.*s/HEAD",
> -	NULL
> +	"refs/remotes/%.*s/HEAD"
>  };
> [...rewrite loops to use ARRAY_SIZE()...]

It's doable, but I don't see the point.  It's several lines longer (tho
that's not significant either).  The performance difference should be
negligible.  There are no users who benefit from knowing the length of
the list ahead of time.  If we ever decide to make the list vary at
runtime your version won't help.  Why bother?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/