{"thread":{"id":"35626","subject":"[PATCH 0/3] Generate scanf_fmts more simply","startedAt":"2014-01-08T14:43:37Z","lastAt":"2014-01-15T16:54:58Z","messageCount":11,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"232913","messageId":"1389192220-13913-1-git-send-email-mhagger@alum.mit.edu","threadId":"35626","inReplyTo":null,"subject":"[PATCH 0/3] Generate scanf_fmts more simply","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T14:43:37Z","receivedAt":"2014-01-08T14:43:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is just a fun little thing that I noticed while poking around the\ncode: the function gen_scanf_fmt() can be replaced with a simple call\nto snprintf().\n\nMichael Haggerty (3):\n shorten_unambiguous_ref(): introduce a new local variable\n gen_scanf_fmt(): delete function and use snprintf() instead\n shorten_unambiguous_ref(): tighten up pointer arithmetic\n\n refs.c | 47 +++++++++++++++--------------------------------\n 1 file changed, 15 insertions(+), 32 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"232914","messageId":"1389192220-13913-2-git-send-email-mhagger@alum.mit.edu","threadId":"35626","inReplyTo":"1389192220-13913-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/3] shorten_unambiguous_ref(): introduce a new local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T14:43:38Z","receivedAt":"2014-01-08T14:43:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When filling the scanf_fmts array, use a separate variable to keep\ntrack of the offset to avoid clobbering total_len (which we will need\nin the next commit).\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3926136..9530480 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3367,6 +3367,7 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t/* pre generate scanf formats from ref_rev_parse_rules[] */\n \tif (!nr_rules) {\n \t\tsize_t total_len = 0;\n+\t\tsize_t offset = 0;\n \n \t\t/* the rule list is NULL terminated, count them first */\n \t\tfor (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)\n@@ -3375,12 +3376,11 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \n \t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n \n-\t\ttotal_len = 0;\n+\t\toffset = 0;\n \t\tfor (i = 0; i < nr_rules; i++) {\n-\t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules]\n-\t\t\t\t\t+ total_len;\n+\t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;\n \t\t\tgen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[i]);\n+\t\t\toffset += strlen(ref_rev_parse_rules[i]);\n \t\t}\n \t}\n \n-- \n1.8.5.2\n"},{"id":"232916","messageId":"1389192220-13913-3-git-send-email-mhagger@alum.mit.edu","threadId":"35626","inReplyTo":"1389192220-13913-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/3] gen_scanf_fmt(): delete function and use snprintf() instead","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T14:43:39Z","receivedAt":"2014-01-08T14:43:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"To replace \"%.*s\" with \"%s\", all we have to do is use snprintf()\nto interpolate \"%s\" into the pattern.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 35 +++++++++--------------------------\n 1 file changed, 9 insertions(+), 26 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9530480..ef9cdea 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3334,29 +3334,6 @@ cleanup:\n \treturn ret;\n }\n \n-/*\n- * generate a format suitable for scanf from a ref_rev_parse_rules\n- * rule, that is replace the \"%.*s\" spec with a \"%s\" spec\n- */\n-static void gen_scanf_fmt(char *scanf_fmt, const char *rule)\n-{\n-\tchar *spec;\n-\n-\tspec = strstr(rule, \"%.*s\");\n-\tif (!spec || strstr(spec + 4, \"%.*s\"))\n-\t\tdie(\"invalid rule in ref_rev_parse_rules: %s\", rule);\n-\n-\t/* copy all until spec */\n-\tstrncpy(scanf_fmt, rule, spec - rule);\n-\tscanf_fmt[spec - rule] = '\\0';\n-\t/* copy new spec */\n-\tstrcat(scanf_fmt, \"%s\");\n-\t/* copy remaining rule */\n-\tstrcat(scanf_fmt, spec + 4);\n-\n-\treturn;\n-}\n-\n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\n@@ -3364,8 +3341,13 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \tstatic int nr_rules;\n \tchar *short_name;\n \n-\t/* pre generate scanf formats from ref_rev_parse_rules[] */\n \tif (!nr_rules) {\n+\t\t/*\n+\t\t * Pre-generate scanf formats from ref_rev_parse_rules[].\n+\t\t * Generate a format suitable for scanf from a\n+\t\t * ref_rev_parse_rules rule by interpolating \"%s\" at the\n+\t\t * location of the \"%.*s\".\n+\t\t */\n \t\tsize_t total_len = 0;\n \t\tsize_t offset = 0;\n \n@@ -3378,9 +3360,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \n \t\toffset = 0;\n \t\tfor (i = 0; i < nr_rules; i++) {\n+\t\t\tassert(offset < total_len);\n \t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;\n-\t\t\tgen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);\n-\t\t\toffset += strlen(ref_rev_parse_rules[i]);\n+\t\t\toffset += snprintf(scanf_fmts[i], total_len - offset,\n+\t\t\t\t\t ref_rev_parse_rules[i], 2, \"%s\") + 1;\n \t\t}\n \t}\n \n-- \n1.8.5.2\n"},{"id":"232915","messageId":"1389192220-13913-4-git-send-email-mhagger@alum.mit.edu","threadId":"35626","inReplyTo":"1389192220-13913-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-08T14:43:40Z","receivedAt":"2014-01-08T14:43:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"As long as we're being pathologically stingy with mallocs, we might as\nwell do the math right and save 6 (!) bytes.\n\nSigned-off-by: Michael Haggerty \n---\nIt is left to the reader to show how another 7 bytes could be saved\n(11 bytes on a 64-bit architecture!)\n\nIt probably wouldn't kill performance to use a string_list here\ninstead.\n\n refs.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ef9cdea..63b3a71 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\tsize_t total_len = 0;\n \t\tsize_t offset = 0;\n \n-\t\t/* the rule list is NULL terminated, count them first */\n+\t\t/* the rule list is NUL terminated, count them first */\n \t\tfor (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)\n-\t\t\t/* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n-\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]);\n+\t\t\t/* -2 for strlen(\"%.*s\") - strlen(\"%s\"); +1 for NUL */\n+\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;\n \n \t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n \n-- \n1.8.5.2\n"},{"id":"232978","messageId":"xmqqy52o933e.fsf@gitster.dls.corp.google.com","threadId":"35626","inReplyTo":"1389192220-13913-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/3] gen_scanf_fmt(): delete function and use snprintf() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T22:56:21Z","receivedAt":"2014-01-09T22:56:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> To replace \"%.*s\" with \"%s\", all we have to do is use snprintf()\n> to interpolate \"%s\" into the pattern.\n\nCute ;-). Thanks.\n"},{"id":"232979","messageId":"xmqqtxdc92ub.fsf@gitster.dls.corp.google.com","threadId":"35626","inReplyTo":"1389192220-13913-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T23:01:48Z","receivedAt":"2014-01-09T23:01:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> As long as we're being pathologically stingy with mallocs, we might as\n> well do the math right and save 6 (!) bytes.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> It is left to the reader to show how another 7 bytes could be saved\n> (11 bytes on a 64-bit architecture!)\n>\n> It probably wouldn't kill performance to use a string_list here\n> instead.\n>\n> refs.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index ef9cdea..63b3a71 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n> \t\tsize_t total_len = 0;\n> \t\tsize_t offset = 0;\n> \n> -\t\t/* the rule list is NULL terminated, count them first */\n> +\t\t/* the rule list is NUL terminated, count them first */\n\nI think this _is_ wrong; it talks about the NULL termination of the\nref_rev_parse_rules[] array, not each string that is an element of\nthe array being NUL terminated.\n\nOutput from \"git grep -e refname_match -e ref_rev_parse_rules\"\nsuggests me that we actually could make ref_rev_parse_rules[] a\nfile-scope static to refs.c, remove its NULL termination and convert\nall the iterators of the array to use ARRAY_SIZE() on it, after\ndropping the third parameter to refname_match(). That way, we do\nnot have to count them first here.\n\nBut that is obviously a separate topic.\n\n> \t\tfor (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)\n> -\t\t\t/* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n> -\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]);\n> +\t\t\t/* -2 for strlen(\"%.*s\") - strlen(\"%s\"); +1 for NUL */\n> +\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;\n> \n> \t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n"},{"id":"232997","messageId":"52D000CC.1060402@alum.mit.edu","threadId":"35626","inReplyTo":"xmqqtxdc92ub.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-10T14:16:44Z","receivedAt":"2014-01-10T14:16:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/10/2014 12:01 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> As long as we're being pathologically stingy with mallocs, we might as\n>> well do the math right and save 6 (!) bytes.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> It is left to the reader to show how another 7 bytes could be saved\n>> (11 bytes on a 64-bit architecture!)\n>>\n>> It probably wouldn't kill performance to use a string_list here\n>> instead.\n>>\n>> refs.c | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index ef9cdea..63b3a71 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n>> \t\tsize_t total_len = 0;\n>> \t\tsize_t offset = 0;\n>> \n>> -\t\t/* the rule list is NULL terminated, count them first */\n>> +\t\t/* the rule list is NUL terminated, count them first */\n> \n> I think this _is_ wrong; it talks about the NULL termination of the\n> ref_rev_parse_rules[] array, not each string that is an element of\n> the array being NUL terminated.\n\nYes, you're right. Thanks for catching my sloppiness. Would you mind\nsquashing the fix onto my patch?\n\n> Output from \"git grep -e refname_match -e ref_rev_parse_rules\"\n> suggests me that we actually could make ref_rev_parse_rules[] a\n> file-scope static to refs.c, remove its NULL termination and convert\n> all the iterators of the array to use ARRAY_SIZE() on it, after\n> dropping the third parameter to refname_match(). That way, we do\n> not have to count them first here.\n> \n> But that is obviously a separate topic.\n> \n>> \t\tfor (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)\n>> -\t\t\t/* no +1 because strlen(\"%s\") < strlen(\"%.*s\") */\n>> -\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]);\n>> +\t\t\t/* -2 for strlen(\"%.*s\") - strlen(\"%s\"); +1 for NUL */\n>> +\t\t\ttotal_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;\n>> \n>> \t\tscanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);\n\nThe way the code is written now (e.g., as long as it is not converted to\nuse a string_list or something) needs this loop not only to count the\nnumber of rules but also to compute the total_len of the string into\nwhich will be written all of the scanf format strings.\n\nAs for removing the third argument of refname_match(): although all\ncallers pass it ref_ref_parse_rules, that array is sometimes passed to\nthe function via the alias \"ref_fetch_rules\". So I suppose somebody\nwanted to leave the way open to make these two rule sets diverge (though\nI don't know how likely that is to occur). If we discard the third\nargument to refname_match(), then we loose the distinction.\n\nThanks for your feedback,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232999","messageId":"xmqqlhyn90ka.fsf@gitster.dls.corp.google.com","threadId":"35626","inReplyTo":"52D000CC.1060402@alum.mit.edu","subject":"Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-10T18:03:17Z","receivedAt":"2014-01-10T18:03:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> As for removing the third argument of refname_match(): although all\n> callers pass it ref_ref_parse_rules, that array is sometimes passed to\n> the function via the alias \"ref_fetch_rules\". So I suppose somebody\n> wanted to leave the way open to make these two rule sets diverge (though\n> I don't know how likely that is to occur).\n\nIt is the other way around. We used to use two separate rules for\nthe normal ref resolution dwimming and dwimming done to decide which\nremote ref to grab. For example, 'x' in 'git log x' can mean\n'refs/remotes/x/HEAD', but 'git fetch origin x' would not grab\n'refs/remotes/x/HEAD' at 'origin'. Also, 'git fetch origin v1.0'\ndid not look into 'refs/tags/v1.0' in the 'origin' repository back\nwhen these two rules were different.\n\nThis was originally done very much on purpose, in order to avoid\nsurprises and to discourage meaningless usage---you would not expect\nus to be interested in the state of a third repository that was\nobserved by our 'origin' the last time (which we do not even know\nwhen).\n\nWhen we harmonized these two rules later and we #defined out\nref_fetch_rules away to avoid potential breakages for in-flight\ntopics. The old synonym can now go safely.\n"},{"id":"233080","messageId":"1389669367-27343-1-git-send-email-mhagger@alum.mit.edu","threadId":"35626","inReplyTo":"xmqqlhyn90ka.fsf@gitster.dls.corp.google.com","subject":"[PATCH] refname_match(): always use the rules in ref_rev_parse_rules","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-14T03:16:07Z","receivedAt":"2014-01-14T03:16:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We used to use two separate rules for the normal ref resolution\ndwimming and dwimming done to decide which remote ref to grab. The\nthird parameter to refname_match() selected which rules to use.\n\nWhen these two rules were harmonized in\n\n 2011-11-04 dd621df9cd refs DWIMmery: use the same rule for both \"git fetch\" and others\n\n, ref_fetch_rules was #defined to avoid potential breakages for\nin-flight topics.\n\nIt is now safe to remove the backwards-compatibility code, so remove\nrefname_match()'s third parameter, make ref_rev_parse_rules private to\nrefs.c, and remove ref_fetch_rules entirely.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\nSee\n\n http://article.gmane.org/gmane.comp.version-control.git/240305\n\nin which Junio made the suggestion and wrote most of the commit\nmessage :-)\n\n cache.h | 9 ++++++---\n refs.c | 6 +++---\n remote.c | 8 ++++----\n 3 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 83a2726..508c49a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -893,9 +893,12 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\n extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n extern int get_sha1_mb(const char *str, unsigned char *sha1);\n \n-extern int refname_match(const char *abbrev_name, const char *full_name, const char **rules);\n-extern const char *ref_rev_parse_rules[];\n-#define ref_fetch_rules ref_rev_parse_rules\n+/*\n+ * Return true iff abbrev_name is a possible abbreviation for\n+ * full_name according to the rules defined by ref_rev_parse_rules in\n+ * refs.c.\n+ */\n+extern int refname_match(const char *abbrev_name, const char *full_name);\n \n extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n extern int validate_headref(const char *ref);\ndiff --git a/refs.c b/refs.c\nindex 490b57b..fc33ee8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1880,7 +1880,7 @@ const char *prettify_refname(const char *name)\n \t\t0);\n }\n \n-const char *ref_rev_parse_rules[] = {\n+static const char *ref_rev_parse_rules[] = {\n \t\"%.*s\",\n \t\"refs/%.*s\",\n \t\"refs/tags/%.*s\",\n@@ -1890,12 +1890,12 @@ const char *ref_rev_parse_rules[] = {\n \tNULL\n };\n \n-int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n+int refname_match(const char *abbrev_name, const char *full_name)\n {\n \tconst char **p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\n \n-\tfor (p = rules; *p; p++) {\n+\tfor (p = ref_rev_parse_rules; *p; p++) {\n \t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n \t\t\treturn 1;\n \t\t}\ndiff --git a/remote.c b/remote.c\nindex a89efab..e41251e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1000,7 +1000,7 @@ int count_refspec_match(const char *pattern,\n \t\tchar *name = refs->name;\n \t\tint namelen = strlen(name);\n \n-\t\tif (!refname_match(pattern, name, ref_rev_parse_rules))\n+\t\tif (!refname_match(pattern, name))\n \t\t\tcontinue;\n \n \t\t/* A match is \"weak\" if it is with refs outside\n@@ -1571,7 +1571,7 @@ int branch_merge_matches(struct branch *branch,\n {\n \tif (!branch || i < 0 || i >= branch->merge_nr)\n \t\treturn 0;\n-\treturn refname_match(branch->merge[i]->src, refname, ref_fetch_rules);\n+\treturn refname_match(branch->merge[i]->src, refname);\n }\n \n static int ignore_symref_update(const char *refname)\n@@ -1624,7 +1624,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c\n {\n \tconst struct ref *ref;\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name, ref_fetch_rules))\n+\t\tif (refname_match(name, ref->name))\n \t\t\treturn ref;\n \t}\n \treturn NULL;\n@@ -2121,7 +2121,7 @@ static void apply_cas(struct push_cas_option *cas,\n \t/* Find an explicit --