{"thread":{"id":"26484","subject":"[PATCH 2/4] Extract function trim_url and optimize calls of it.","startedAt":"2011-02-12T22:38:27Z","lastAt":"2011-02-17T02:36:41Z","messageCount":12,"participants":["Vasyl' Vavrychuk","Sverre Rabbelier","Vasyl'","Junio C Hamano","João P. Sampaio"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"160964","messageId":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","threadId":"26484","inReplyTo":null,"subject":"[PATCH 0/4] clean-up store_updated_refs","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-12T22:38:27Z","receivedAt":"2011-02-12T22:38:27Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Vasyl' Vavrychuk (4):\n Reduce url_len variable scope\n Extract function trim_url and optimize calls of it.\n Dont use the same variable for different things\n Replace pointer arithmetic with strbuf\n\n builtin/fetch.c | 63 ++++++++++++++++++++++++++++++++----------------------\n 1 files changed, 37 insertions(+), 26 deletions(-)\n"},{"id":"160965","messageId":"1297550311-17723-2-git-send-email-vvavrychuk@gmail.com","threadId":"26484","inReplyTo":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","subject":"[PATCH 1/4] Reduce url_len variable scope","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-12T22:38:28Z","receivedAt":"2011-02-12T22:38:28Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Signed-off-by: Vasyl' Vavrychuk \n---\n builtin/fetch.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 357f3cd..2b0b11e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -385,6 +385,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\turl_len = i + 1;\n \t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n \t\t\turl_len = i - 3;\n+\t\turl[url_len] = '\\0';\n \n \t\tnote_len = 0;\n \t\tif (*what) {\n@@ -399,7 +400,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t rm->old_sha1),\n \t\t\trm->merge ? \"\" : \"not-for-merge\",\n \t\t\tnote);\n-\t\tfor (i = 0; i < url_len; ++i)\n+\t\tfor (i = 0; url[i]; ++i)\n \t\t\tif ('\\n' == url[i])\n \t\t\t\tfputs(\"\\\\n\", fp);\n \t\t\telse\n@@ -415,8 +416,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t REFCOL_WIDTH, *what ? what : \"HEAD\");\n \t\tif (*note) {\n \t\t\tif (verbosity >= 0 && !shown_url) {\n-\t\t\t\tfprintf(stderr, \"From %.*s\\n\",\n-\t\t\t\t\t\turl_len, url);\n+\t\t\t\tfprintf(stderr, \"From %s\\n\", url);\n \t\t\t\tshown_url = 1;\n \t\t\t}\n \t\t\tif (verbosity >= 0)\n-- \n1.7.1\n"},{"id":"160961","messageId":"1297550311-17723-3-git-send-email-vvavrychuk@gmail.com","threadId":"26484","inReplyTo":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","subject":"[PATCH 2/4] Extract function trim_url and optimize calls of it.","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-12T22:38:29Z","receivedAt":"2011-02-12T22:38:29Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Extract compact code into trim_url. Dont call it every iteration in the loop since no reason.\n\nSigned-off-by: Vasyl' Vavrychuk \n---\n builtin/fetch.c | 29 ++++++++++++++++++-----------\n 1 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 2b0b11e..46d8fd6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -89,6 +89,19 @@ static void unlock_pack_on_signal(int signo)\n \traise(signo);\n }\n \n+static void trim_url(char *url)\n+{\n+\tint i, url_len;\n+\n+\turl_len = strlen(url);\n+\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n+\t\t;\n+\turl_len = i + 1;\n+\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n+\t\turl_len = i - 3;\n+\turl[url_len] = '\\0';\n+}\n+\n static void add_merge_config(struct ref **head,\n \t\t\t const struct ref *remote_refs,\n \t\t struct branch *branch,\n@@ -329,7 +342,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n {\n \tFILE *fp;\n \tstruct commit *commit;\n-\tint url_len, i, note_len, shown_url = 0, rc = 0;\n+\tint i, note_len, shown_url = 0, rc = 0;\n \tchar note[1024];\n \tconst char *what, *kind;\n \tstruct ref *rm;\n@@ -339,10 +352,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tif (!fp)\n \t\treturn error(\"cannot open %s: %s\\n\", filename, strerror(errno));\n \n-\tif (raw_url)\n+\tif (raw_url) {\n \t\turl = transport_anonymize_url(raw_url);\n-\telse\n+\t\ttrim_url(url);\n+\t} else {\n \t\turl = xstrdup(\"foreign\");\n+\t}\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tstruct ref *ref = NULL;\n \n@@ -379,14 +394,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\twhat = rm->name;\n \t\t}\n \n-\t\turl_len = strlen(url);\n-\t\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t\t;\n-\t\turl_len = i + 1;\n-\t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\t\turl_len = i - 3;\n-\t\turl[url_len] = '\\0';\n-\n \t\tnote_len = 0;\n \t\tif (*what) {\n \t\t\tif (*kind)\n-- \n1.7.1\n"},{"id":"160962","messageId":"1297550311-17723-4-git-send-email-vvavrychuk@gmail.com","threadId":"26484","inReplyTo":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","subject":"[PATCH 3/4] Dont use the same variable for different things","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-12T22:38:30Z","receivedAt":"2011-02-12T22:38:30Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Signed-off-by: Vasyl' Vavrychuk \n---\n builtin/fetch.c | 13 +++++++------\n 1 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 46d8fd6..ddb40c6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -343,7 +343,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tFILE *fp;\n \tstruct commit *commit;\n \tint i, note_len, shown_url = 0, rc = 0;\n-\tchar note[1024];\n+\tchar note[1024], display[1024];\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url, *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n@@ -415,19 +415,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\tfputc('\\n', fp);\n \n \t\tif (ref) {\n-\t\t\trc |= update_local_ref(ref, what, note);\n+\t\t\trc |= update_local_ref(ref, what, display);\n \t\t\tfree(ref);\n-\t\t} else\n-\t\t\tsprintf(note, \"* %-*s %-*s -> FETCH_HEAD\",\n+\t\t} else {\n+\t\t\tsprintf(display, \"* %-*s %-*s -> FETCH_HEAD\",\n \t\t\t\tTRANSPORT_SUMMARY_WIDTH, *kind ? kind : \"branch\",\n \t\t\t\t REFCOL_WIDTH, *what ? what : \"HEAD\");\n-\t\tif (*note) {\n+\t\t}\n+\t\tif (*display) {\n \t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\tfprintf(stderr, \"From %s\\n\", url);\n \t\t\t\tshown_url = 1;\n \t\t\t}\n \t\t\tif (verbosity >= 0)\n-\t\t\t\tfprintf(stderr, \" %s\\n\", note);\n+\t\t\t\tfprintf(stderr, \" %s\\n\", display);\n \t\t}\n \t}\n \tfree(url);\n-- \n1.7.1\n"},{"id":"160963","messageId":"1297550311-17723-5-git-send-email-vvavrychuk@gmail.com","threadId":"26484","inReplyTo":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","subject":"[PATCH 4/4] Replace pointer arithmetic with strbuf","fromName":"Vasyl' Vavrychuk","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-12T22:38:31Z","receivedAt":"2011-02-12T22:38:31Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Signed-off-by: Vasyl' Vavrychuk \n---\n builtin/fetch.c | 21 ++++++++++++---------\n 1 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ddb40c6..eaea475 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -342,8 +342,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n {\n \tFILE *fp;\n \tstruct commit *commit;\n-\tint i, note_len, shown_url = 0, rc = 0;\n-\tchar note[1024], display[1024];\n+\tint i, shown_url = 0, rc = 0;\n+\tstruct strbuf note = STRBUF_INIT;\n+\tchar display[1024];\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url, *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n@@ -394,19 +395,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\twhat = rm->name;\n \t\t}\n \n-\t\tnote_len = 0;\n \t\tif (*what) {\n-\t\t\tif (*kind)\n-\t\t\t\tnote_len += sprintf(note + note_len, \"%s \",\n-\t\t\t\t\t\t kind);\n-\t\t\tnote_len += sprintf(note + note_len, \"'%s' of \", what);\n+\t\t\tif (*kind) {\n+\t\t\t\tstrbuf_addstr(¬e, kind);\n+\t\t\t\tstrbuf_addch(¬e, ' ');\n+\t\t\t}\n+\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n \t\t}\n-\t\tnote[note_len] = '\\0';\n \t\tfprintf(fp, \"%s\\t%s\\t%s\",\n \t\t\tsha1_to_hex(commit ? commit->object.sha1 :\n \t\t\t\t rm->old_sha1),\n \t\t\trm->merge ? \"\" : \"not-for-merge\",\n-\t\t\tnote);\n+\t\t\tnote.buf);\n+\t\tstrbuf_reset(¬e);\n+\n \t\tfor (i = 0; url[i]; ++i)\n \t\t\tif ('\\n' == url[i])\n \t\t\t\tfputs(\"\\\\n\", fp);\n@@ -431,6 +433,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tfprintf(stderr, \" %s\\n\", display);\n \t\t}\n \t}\n+\tstrbuf_release(¬e);\n \tfree(url);\n \tfclose(fp);\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n-- \n1.7.1\n"},{"id":"160970","messageId":"AANLkTi=k9z67zmG6qBCh2uQWiofNceqs2erZ74XQARYq@mail.gmail.com","threadId":"26484","inReplyTo":"1297550311-17723-1-git-send-email-vvavrychuk@gmail.com","subject":"Re: [PATCH 0/4] clean-up store_updated_refs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-13T00:09:12Z","receivedAt":"2011-02-13T00:09:12Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Feb 12, 2011 at 23:38, Vasyl' Vavrychuk wrote:\n> Vasyl' Vavrychuk (4):\n\nNeither the patches, nor this cover letter explain _why_ this is being\ndone. Motivation needed :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"161008","messageId":"AANLkTi=fN+cDjgDqLKas5n0iCAetRTAZfnPqcgyeYoX0@mail.gmail.com","threadId":"26484","inReplyTo":"AANLkTi=k9z67zmG6qBCh2uQWiofNceqs2erZ74XQARYq@mail.gmail.com","subject":"Re: [PATCH 0/4] clean-up store_updated_refs","fromName":"Vasyl'","fromEmail":"vvavrychuk@gmail.com","sentAt":"2011-02-13T18:37:21Z","receivedAt":"2011-02-13T18:37:21Z","isPatch":true,"sender":{"key":"vvavrychuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152563?v=4"},"body":"Hi,\n\nI've needed this as part of bigger change which is collecting all\n.git/FETCH_HEAD related code across different files to\nfetch-head.[ch]. Which in its case I do as part of git-pull.sh to C\nporting. So thats motivation chain :)\n\nOn Sun, Feb 13, 2011 at 2:09 AM, Sverre Rabbelier wrote:\n> Heya,\n>\n> On Sat, Feb 12, 2011 at 23:38, Vasyl' Vavrychuk wrote:\n>> Vasyl' Vavrychuk (4):\n>\n> Neither the patches, nor this cover letter explain _why_ this is being\n> done. Motivation needed :).\n>\n> --\n> Cheers,\n>\n> Sverre Rabbelier\n>\n"},{"id":"161009","messageId":"AANLkTimq+keyCO_EfLvswjwVYwLD=YDUagqcUTB1iVYd@mail.gmail.com","threadId":"26484","inReplyTo":"AANLkTi=fN+cDjgDqLKas5n0iCAetRTAZfnPqcgyeYoX0@mail.gmail.com","subject":"Re: [PATCH 0/4] clean-up store_updated_refs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-13T18:46:07Z","receivedAt":"2011-02-13T18:46:07Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Feb 13, 2011 at 19:37, Vasyl' wrote:\n> I've needed this as part of bigger change which is collecting all\n> .git/FETCH_HEAD related code across different files to\n> fetch-head.[ch]. Which in its case I do as part of git-pull.sh to C\n> porting. So thats motivation chain :)\n\nThanks, that's the kind of stuff that should go into the cover letter\n/ the patch description.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"161362","messageId":"7vhbc3fnad.fsf@alter.siamese.dyndns.org","threadId":"26484","inReplyTo":"1297550311-17723-2-git-send-email-vvavrychuk@gmail.com","subject":"Re: [PATCH 1/4] Reduce url_len variable scope","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-17T01:43:54Z","receivedAt":"2011-02-17T01:43:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vasyl' Vavrychuk writes:\n\n> Subject: Re: [PATCH 1/4] Reduce url_len variable scope\n\nPlease mark the patch in such a way that the readers can tell what\nfile/function this patch is about by reading \"git shortlog\" output\nwithout anything else, i.e.\n\n\tSubject: builtin/fetch.c: shorten lifetime of the url_len variable\n\nHmph. While this does not bring in any new bug to the code, what benefit\ndo we gain from it?\n\nThe scope is not reduced (even though the variable's lifespan is shortened\nin the scope). Are we so short of registers that this change matters?\n\n> builtin/fetch.c | 6 +++---\n> 1 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 357f3cd..2b0b11e 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -385,6 +385,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\turl_len = i + 1;\n> \t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n> \t\t\turl_len = i - 3;\n> +\t\turl[url_len] = '\\0';\n> \n> \t\tnote_len = 0;\n> \t\tif (*what) {\n> @@ -399,7 +400,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\t rm->old_sha1),\n> \t\t\trm->merge ? \"\" : \"not-for-merge\",\n> \t\t\tnote);\n> -\t\tfor (i = 0; i < url_len; ++i)\n> +\t\tfor (i = 0; url[i]; ++i)\n> \t\t\tif ('\\n' == url[i])\n> \t\t\t\tfputs(\"\\\\n\", fp);\n> \t\t\telse\n> @@ -415,8 +416,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\t REFCOL_WIDTH, *what ? what : \"HEAD\");\n> \t\tif (*note) {\n> \t\t\tif (verbosity >= 0 && !shown_url) {\n> -\t\t\t\tfprintf(stderr, \"From %.*s\\n\",\n> -\t\t\t\t\t\turl_len, url);\n> +\t\t\t\tfprintf(stderr, \"From %s\\n\", url);\n> \t\t\t\tshown_url = 1;\n> \t\t\t}\n> \t\t\tif (verbosity >= 0)\n> -- \n> 1.7.1\n"},{"id":"161363","messageId":"7vbp2bfna7.fsf@alter.siamese.dyndns.org","threadId":"26484","inReplyTo":"1297550311-17723-3-git-send-email-vvavrychuk@gmail.com","subject":"Re: [PATCH 2/4] Extract function trim_url and optimize calls of it.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-17T01:44:00Z","receivedAt":"2011-02-17T01:44:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vasyl' Vavrychuk writes:\n\n> Extract compact code into trim_url. Dont call it every iteration in the loop since no reason.\n\nToo long a line; just drop \" since no reason\".\n\n> @@ -379,14 +394,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\twhat = rm->name;\n> \t\t}\n> \n> -\t\turl_len = strlen(url);\n> -\t\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n> -\t\t\t;\n> -\t\turl_len = i + 1;\n> -\t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n> -\t\t\turl_len = i - 3;\n> -\t\turl[url_len] = '\\0';\n> -\n> \t\tnote_len = 0;\n> \t\tif (*what) {\n> \t\t\tif (*kind)\n\nWe repeatedly called strlen(url) for each entry in the ref-map when we\nknow we do not have any more thing to do; silly. This is probably a good\nthing to do.\n"},{"id":"161364","messageId":"7v62sjfna2.fsf@alter.siamese.dyndns.org","threadId":"26484","inReplyTo":"1297550311-17723-4-git-send-email-vvavrychuk@gmail.com","subject":"Re: [PATCH 3/4] Dont use the same variable for different things","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-17T01:44:05Z","receivedAt":"2011-02-17T01:44:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vasyl' Vavrychuk writes:\n\n> Subject: Re: [PATCH 3/4] Dont use the same variable for different things\n\n\tSubject: builtin/fetch.c: do not use ... different things\n\nBut more important question is why?\n\nWhat benefit are we getting by almost doubling the stack footprint of this\nfunction? What problem are you fixing?\n"},{"id":"161365","messageId":"AANLkTim3t3JTFh668YK+GtS+iRDZLjnhS7bMUGW1fim+@mail.gmail.com","threadId":"26484","inReplyTo":"7v62sjfna2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] Dont use the same variable for different things","fromName":"João P. Sampaio","fromEmail":"jpmelos@jpmelos.com","sentAt":"2011-02-17T02:36:41Z","receivedAt":"2011-02-17T02:36:41Z","isPatch":true,"sender":{"key":"jpmelos@jpmelos.com","avatar":"https://gravatar.com/avatar/368a5d2863df47838b2d07638023313133a3495266b986462ababa9c841efcf8?d=mp&s=160"},"body":"On Wed, Feb 16, 2011 at 23:44, Junio C Hamano wrote:\n> But more important question is why?\n>\n> What benefit are we getting by almost doubling the stack footprint of this\n> function?  What problem are you fixing?\n\nWell, in matters of code readability and maintainability, it's best to\nuse different variables for different matters...\n\nNowadays, memory isn't really an issue anymore, is it? I think it's\nvalid to make code more readable, if memory cost isn't absurd.\n\n-- \nJoão Paulo Melo de Sampaio\nComputer Engineering Student @ UFSCar\nWebsite: http://www.jpmelos.com\nTwitter: http://twitter.com/jpmelos (@jpmelos)\n"}]}