Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic
Michael Haggerty 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. -- 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 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic
On 01/10/2014 12:01 AM, Junio C Hamano wrote:
> Michael Haggerty 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
>> ---
>> 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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic
Michael Haggerty 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
> ---
> 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);
--
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 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic
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
---
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
--
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
