{"thread":{"id":"34479","subject":"[PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.","startedAt":"2013-07-18T21:35:25Z","lastAt":"2013-07-19T22:28:57Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"223705","messageId":"1374183327-14939-1-git-send-email-stefanbeller@googlemail.com","threadId":"34479","inReplyTo":null,"subject":"[PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-18T21:35:25Z","receivedAt":"2013-07-18T21:35:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The variable name ret sounds like the variable to be returned, but\nsince e6c111b4 we return error. Hence the variable name is miss leading.\nAs this variable is used only to extract the bits from the callback of\na tree object, I named it cb_bits for callback bits.\n\nAlso the assignment to 0 was removed at the start of the function as well\nafter the if(interesting) block. Those were unneeded as that variable\nis set to the callback return value any time we enter the if(interesting)\nblock, so we'd overwrite old values anyway.\n\nSigned-off-by: Stefan Beller \n---\n tree-walk.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex c366852..3c323d1 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -324,7 +324,6 @@ static inline int prune_traversal(struct name_entry *e,\n \n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n {\n-\tint ret = 0;\n \tint error = 0;\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n \tint i;\n@@ -342,6 +341,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\tstrbuf_setlen(&base, info->pathlen);\n \t}\n \tfor (;;) {\n+\t\tint cb_bits;\n \t\tunsigned long mask, dirmask;\n \t\tconst char *first = NULL;\n \t\tint first_len = 0;\n@@ -405,15 +405,14 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\tif (interesting < 0)\n \t\t\tbreak;\n \t\tif (interesting) {\n-\t\t\tret = info->fn(n, mask, dirmask, entry, info);\n-\t\t\tif (ret < 0) {\n-\t\t\t\terror = ret;\n+\t\t\tcb_bits = info->fn(n, mask, dirmask, entry, info);\n+\t\t\tif (cb_bits < 0) {\n+\t\t\t\terror = cb_bits;\n \t\t\t\tif (!info->show_all_errors)\n \t\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmask &= ret;\n+\t\t\tmask &= cb_bits;\n \t\t}\n-\t\tret = 0;\n \t\tfor (i = 0; i < n; i++)\n \t\t\tif (mask & (1ul << i))\n \t\t\t\tupdate_extended_entry(tx + i, entry + i);\n-- \n1.8.3.3.754.g9c3c367.dirty\n"},{"id":"223706","messageId":"1374183327-14939-2-git-send-email-stefanbeller@googlemail.com","threadId":"34479","inReplyTo":"1374183327-14939-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 2/3] http-push.c, add_send_request: Do not initialize transfer_request.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-18T21:35:26Z","receivedAt":"2013-07-18T21:35:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"That pointer will be assigned to new memory via\nrequest = xmalloc(sizeof(*request));\n20 lines later unconditionally anyway, so it's safe to not assign it to an\narbitrary variable.\n\nAs this patch is a micro-optimization (for readability?) it would be nice\nto have as little work with this patch, so maybe I need a feature to send\na patch without a symmetric number of lines around change-diff. For this\npatch I'd be only interested in showing the next 20 lines, but nothing\nreally before the function declaration.\n\nSigned-off-by: Stefan Beller \n---\n http-push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 395a8cf..6dad188 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -663,7 +663,7 @@ static void add_fetch_request(struct object *obj)\n \n static int add_send_request(struct object *obj, struct remote_lock *lock)\n {\n-\tstruct transfer_request *request = request_queue_head;\n+\tstruct transfer_request *request;\n \tstruct packed_git *target;\n \n \t/* Keep locks active */\n-- \n1.8.3.3.754.g9c3c367.dirty\n"},{"id":"223707","messageId":"1374183327-14939-3-git-send-email-stefanbeller@googlemail.com","threadId":"34479","inReplyTo":"1374183327-14939-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 3/3] apply, find_name_traditional: Do not initialize len to the lines length.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-18T21:35:27Z","receivedAt":"2013-07-18T21:35:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The variable len is set to\nlen = strchrnul(line, '\\n') - line;\nunconditionally 9 lines later, hence we can remove the call to strlen.\n\nSigned-off-by: Stefan Beller \n---\n builtin/apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 1218688..681a40b 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -722,7 +722,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)\n \n static char *find_name_traditional(const char *line, char *def, int p_value)\n {\n-\tsize_t len = strlen(line);\n+\tsize_t len;\n \tsize_t date_len;\n \n \tif (*line == '\"') {\n-- \n1.8.3.3.754.g9c3c367.dirty\n"},{"id":"223789","messageId":"7vli52wh8n.fsf@alter.siamese.dyndns.org","threadId":"34479","inReplyTo":"1374183327-14939-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T18:13:12Z","receivedAt":"2013-07-19T18:13:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"ret\" was meant to mean \"the return value we got from the\ncallback function\", not \"the return value we would give our caller\".\n\nThis rename is a bit misleading in that \"cb_bits == -1\" does not\nmean \"full bits set\", and it does not tell us much what these \"bits\"\nsignify.\n\nThey are used to answer this question: which one of the trees in\nt[0..n] did the callback function consumed (hence needs their\npointers updated).\n\nSo perhaps call it \"trees_used\" or something?\n\nBy the way, our log message usually do not Capitalize the subject\nafter the \":\", i.e. do something like this instead:\n\n Subject: [PATCH 1/3] traverse_trees(): clarify return value of the callback\n\nThanks.\n"},{"id":"223793","messageId":"7va9liwgfy.fsf@alter.siamese.dyndns.org","threadId":"34479","inReplyTo":"1374183327-14939-2-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 2/3] http-push.c, add_send_request: Do not initialize transfer_request.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T18:30:25Z","receivedAt":"2013-07-19T18:30:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patch 2 and 3 look fairly straight-forward. Will queue them\ndirectly on 'maint'.\n\nThanks.\n"},{"id":"223801","messageId":"51E9A0D0.3090701@googlemail.com","threadId":"34479","inReplyTo":"7vli52wh8n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-19T20:25:52Z","receivedAt":"2013-07-19T20:25:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/19/2013 08:13 PM, Junio C Hamano wrote:\n> The \"ret\" was meant to mean \"the return value we got from the\n> callback function\", not \"the return value we would give our caller\".\n\nThanks for clarifying.\nI assumed the \"ret\" was meant as the return value of that function\nas it was the case before e6c111b4c. In other projects I am using \nret as \"the return value we would give our caller\" as it's such a \nconvenient name for that if you cannot come up with a better name.\n\n> \n> This rename is a bit misleading in that \"cb_bits == -1\" does not\n> mean \"full bits set\", and it does not tell us much what these \"bits\"\n> signify.\n> \n> They are used to answer this question: which one of the trees in\n> t[0..n] did the callback function consumed (hence needs their\n> pointers updated).\n> \n> So perhaps call it \"trees_used\" or something?\n\nSounds indeed way better. I'll rename it.\n\n> \n> By the way, our log message usually do not Capitalize the subject\n> after the \":\", i.e. do something like this instead:\n> \n> Subject: [PATCH 1/3] traverse_trees(): clarify return value of the callback\n> \n> Thanks.\n> \n\nThanks for pointing out.\n\nAs a general question: I was mostly doing micro-optimisations or \nthe mailmap file, which are rather small fixups, which I think are\nok for beginners. Is there a tasklist for beginners, other than that?\nSuch as porting shell commands to C or other larger tasks?\nI used git://github.com/gitster/git.git as remote/origin. There the todo\nbranch has the last commit as of 2012/04, so I also found\ngit://git.kernel.org/pub/scm/git/git.git, where the todo branch seems\nmore up-to-date, but the TODO file there also seems a little dated to\nme.\nSo is there any up-to-date task list for beginning contributors?\n\nStefan\n"},{"id":"223802","messageId":"1374265592-2594-1-git-send-email-stefanbeller@googlemail.com","threadId":"34479","inReplyTo":"51E9A0D0.3090701@googlemail.com","subject":"[PATCH] traverse_trees(): clarify return value of the callback.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-19T20:26:32Z","receivedAt":"2013-07-19T20:26:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The variable name ret sounds like the variable to be returned, but\nsince e6c111b4 we return error. Hence the variable name is miss leading.\nAs this variable is used only to extract the bits from the callback of\na tree object, trees_used is a better name.\n\nAlso the assignment to 0 was removed at the start of the function as well\nafter the if(interesting) block. Those were unneeded as that variable\nis set to the callback return value any time we enter the if(interesting)\nblock, so we'd overwrite old values anyway.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n tree-walk.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex c366852..5ece8c3 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -324,7 +324,6 @@ static inline int prune_traversal(struct name_entry *e,\n \n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n {\n-\tint ret = 0;\n \tint error = 0;\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n \tint i;\n@@ -342,6 +341,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\tstrbuf_setlen(&base, info->pathlen);\n \t}\n \tfor (;;) {\n+\t\tint trees_used;\n \t\tunsigned long mask, dirmask;\n \t\tconst char *first = NULL;\n \t\tint first_len = 0;\n@@ -405,15 +405,14 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\tif (interesting < 0)\n \t\t\tbreak;\n \t\tif (interesting) {\n-\t\t\tret = info->fn(n, mask, dirmask, entry, info);\n-\t\t\tif (ret < 0) {\n-\t\t\t\terror = ret;\n+\t\t\ttrees_used = info->fn(n, mask, dirmask, entry, info);\n+\t\t\tif (trees_used < 0) {\n+\t\t\t\terror = trees_used;\n \t\t\t\tif (!info->show_all_errors)\n \t\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmask &= ret;\n+\t\t\tmask &= trees_used;\n \t\t}\n-\t\tret = 0;\n \t\tfor (i = 0; i < n; i++)\n \t\t\tif (mask & (1ul << i))\n \t\t\t\tupdate_extended_entry(tx + i, entry + i);\n-- \n1.8.3.3.754.g9c3c367.dirty\n"},{"id":"223807","messageId":"7vsizatc9i.fsf@alter.siamese.dyndns.org","threadId":"34479","inReplyTo":"51E9A0D0.3090701@googlemail.com","subject":"Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T22:28:57Z","receivedAt":"2013-07-19T22:28:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So is there any up-to-date task list for beginning contributors?\n\nI am fairly bad at keeping track of small things incrementally, as\nit is often quicker to do them myself if/when I were so inclined,\nbut there are too many of them and a day does not have enough number\nof seconds X-<.\n\nThese days, I tend to scan the mailing list backlog in batch every\nonce in a while.\n\n http://git-blame.blogspot.com/search?q=leftover+bits\n\nmay show some of them.\n\nThanks for asking and volunteering.\n"}]}