{"thread":{"id":"51945","subject":"[PATCH v5] commit-graph: remove a duplicate assignment","startedAt":"2019-10-01T02:30:05Z","lastAt":"2019-10-02T15:32:09Z","messageCount":5,"participants":["Alex Henrie","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"383187","messageId":"20191001022936.1757141-1-alexhenrie24@gmail.com","threadId":"51945","inReplyTo":null,"subject":"[PATCH v5] commit-graph: remove a duplicate assignment","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-10-01T02:29:34Z","receivedAt":"2019-10-01T02:30:05Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Leave the variable 'g' uninitialized before it is set just before its\nfirst use in front of a loop, which is a much more appropriate place to\nindicate what it is used for.\n\nAlso initialize the variable 'num_commits' just before the loop instead\nof at the beginning of the function for the same reason.\n\nReviewed-by: Derrick Stolee \nSigned-off-by: Alex Henrie \n---\n commit-graph.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b02d2c426..4028508e9c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1522,8 +1522,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n {\n-\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n-\tuint32_t num_commits = ctx->commits.nr;\n+\tstruct commit_graph *g;\n+\tuint32_t num_commits;\n \tuint32_t i;\n \n \tint max_commits = 0;\n@@ -1535,6 +1535,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t}\n \n \tg = ctx->r->objects->commit_graph;\n+\tnum_commits = ctx->commits.nr;\n \tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n-- \n2.23.0\n\n"},{"id":"383189","messageId":"20191001022936.1757141-2-alexhenrie24@gmail.com","threadId":"51945","inReplyTo":"20191001022936.1757141-1-alexhenrie24@gmail.com","subject":"[PATCH v5] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-10-01T02:29:35Z","receivedAt":"2019-10-01T02:30:08Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The condition \"if (q->nr <= j)\" checks whether the loop exited normally\nor via a break statement. Avoid this check by replacing the jump out of\nthe inner loop with a jump to the end of the outer loop, which makes it\nobvious that diff_q is not executed when the peer survives.\n\nSigned-off-by: Alex Henrie \n---\n diffcore-break.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 875aefd3fe..9d20a6a6fc 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -286,17 +286,17 @@ void diffcore_merge_broken(void)\n \t\t\t\t\t/* Peer survived. Merge them */\n \t\t\t\t\tmerge_broken(p, pp, &outq);\n \t\t\t\t\tq->queue[j] = NULL;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto next;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (q->nr <= j)\n-\t\t\t\t/* The peer did not survive, so we keep\n-\t\t\t\t * it in the output.\n-\t\t\t\t */\n-\t\t\t\tdiff_q(&outq, p);\n+\t\t\t/* The peer did not survive, so we keep\n+\t\t\t * it in the output.\n+\t\t\t */\n+\t\t\tdiff_q(&outq, p);\n \t\t}\n \t\telse\n \t\t\tdiff_q(&outq, p);\n+next:;\n \t}\n \tfree(q->queue);\n \t*q = outq;\n-- \n2.23.0\n\n"},{"id":"383188","messageId":"20191001022936.1757141-3-alexhenrie24@gmail.com","threadId":"51945","inReplyTo":"20191001022936.1757141-1-alexhenrie24@gmail.com","subject":"[PATCH v5] wrapper: use a loop instead of repetitive statements","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-10-01T02:29:36Z","receivedAt":"2019-10-01T02:30:09Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"A check into the history of this code revealed no particular reason for\nthe code to be written in this way. All popular compilers are capable of\nunrolling loops if it benefits performance, and once this code is\nreplaced with a loop, the magic number 6 used in multiple places in this\nfunction can be replaced with a named constant.\n\nReviewed-by: Derrick Stolee \nReviewed-by: Johannes Schindelin \nReviewed-by: Jeff King \nSigned-off-by: Alex Henrie \n---\n wrapper.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex c55d7722d7..c23ac6adcd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \tfilename_template = &pattern[len - 6 - suffix_len];\n \tfor (count = 0; count < TMP_MAX; ++count) {\n \t\tuint64_t v = value;\n+\t\tint i;\n \t\t/* Fill in the random bits. */\n-\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n+\t\tfor (i = 0; i < 6; i++) {\n+\t\t\tfilename_template[i] = letters[v % num_letters];\n+\t\t\tv /= num_letters;\n+\t\t}\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n \t\tif (fd >= 0)\n-- \n2.23.0\n\n"},{"id":"383248","messageId":"xmqqbluzfzql.fsf@gitster-ct.c.googlers.com","threadId":"51945","inReplyTo":"20191001022936.1757141-3-alexhenrie24@gmail.com","subject":"Re: [PATCH v5] wrapper: use a loop instead of repetitive statements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T06:06:10Z","receivedAt":"2019-10-02T06:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All three patches looked sensible.\n\nEven though there is no dependency or relationships among them\n(beyond that they got written at the same time by the same person),\nI'll just queue them on a single ah/cleanups topic and get them\nadvance together, as I do not expect any one of them to be blocking\nthe other two.\n\nTHanks.\n"},{"id":"383277","messageId":"20191002153207.GA6116@sigill.intra.peff.net","threadId":"51945","inReplyTo":"xmqqbluzfzql.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5] wrapper: use a loop instead of repetitive statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-02T15:32:07Z","receivedAt":"2019-10-02T15:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 02, 2019 at 03:06:10PM +0900, Junio C Hamano wrote:\n\n> All three patches looked sensible.\n> \n> Even though there is no dependency or relationships among them\n> (beyond that they got written at the same time by the same person),\n> I'll just queue them on a single ah/cleanups topic and get them\n> advance together, as I do not expect any one of them to be blocking\n> the other two.\n\nJust re-posting this follow-on from the earlier thread (it goes on top\nof the third one):\n\n-- >8 --\nSubject: git_mkstemps_mode(): replace magic numbers with computed value\n\nThe magic number \"6\" appears several times in the function, and is\nrelated to the size of the \"XXXXXX\" string we expect to find in the\ntemplate. Let's pull that \"XXXXXX\" into a constant array, whose size we\ncan get at compile time with ARRAY_SIZE().\n\nNote that we probably can't just change this value, since callers will\nbe feeding us a certain number of X's, but it hopefully makes the\nfunction itself easier to follow.\n\nWhile we're here, let's do the same with the \"letters\" array (which we\n_could_ modify if we wanted to include more characters).\n\nSigned-off-by: Jeff King \n---\n wrapper.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 54541386c1..75992cff02 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -478,7 +478,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\t\"abcdefghijklmnopqrstuvwxyz\"\n \t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n \t\t\"0123456789\";\n-\tstatic const int num_letters = 62;\n+\tstatic const int num_letters = ARRAY_SIZE(letters) - 1;\n+\tstatic const char x_pattern[] = \"XXXXXX\";\n+\tstatic const int num_x = ARRAY_SIZE(x_pattern) - 1;\n \tuint64_t value;\n \tstruct timeval tv;\n \tchar *filename_template;\n@@ -487,12 +489,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \n \tlen = strlen(pattern);\n \n-\tif (len < 6 + suffix_len) {\n+\tif (len < num_x + suffix_len) {\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \n-\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n+\tif (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n@@ -503,12 +505,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t */\n \tgettimeofday(&tv, NULL);\n \tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n-\tfilename_template = &pattern[len - 6 - suffix_len];\n+\tfilename_template = &pattern[len - num_x - suffix_len];\n \tfor (count = 0; count < TMP_MAX; ++count) {\n \t\tuint64_t v = value;\n \t\tint i;\n \t\t/* Fill in the random bits. */\n-\t\tfor (i = 0; i < 6; i++) {\n+\t\tfor (i = 0; i < num_x; i++) {\n \t\t\tfilename_template[i] = letters[v % num_letters];\n \t\t\tv /= num_letters;\n \t\t}\n-- \n2.23.0.926.g3dc922fbbc\n\n"}]}