From 8da02ce62a4e76aea7a02d56b66b10eedc8e6db8 Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Mon, 30 Sep 2019 20:29:34 -0600 Subject: [PATCH 1/4] commit-graph: remove a duplicate assignment Leave the variable 'g' uninitialized before it is set just before its first use in front of a loop, which is a much more appropriate place to indicate what it is used for. Also initialize the variable 'num_commits' just before the loop instead of at the beginning of the function for the same reason. Reviewed-by: Derrick Stolee Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano --- commit-graph.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index fe954ab5f8..9a65ddd684 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1519,8 +1519,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) static void split_graph_merge_strategy(struct write_commit_graph_context *ctx) { - struct commit_graph *g = ctx->r->objects->commit_graph; - uint32_t num_commits = ctx->commits.nr; + struct commit_graph *g; + uint32_t num_commits; uint32_t i; int max_commits = 0; @@ -1532,6 +1532,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx) } g = ctx->r->objects->commit_graph; + num_commits = ctx->commits.nr; ctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1; while (g && (g->num_commits <= size_mult * num_commits || From baed6bbb5b56a501d1137e53caba614f77c3435d Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Mon, 30 Sep 2019 20:29:35 -0600 Subject: [PATCH 2/4] diffcore-break: use a goto instead of a redundant if statement The condition "if (q->nr <= j)" checks whether the loop exited normally or via a break statement. Avoid this check by replacing the jump out of the inner loop with a jump to the end of the outer loop, which makes it obvious that diff_q is not executed when the peer survives. Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano --- diffcore-break.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/diffcore-break.c b/diffcore-break.c index 875aefd3fe..9d20a6a6fc 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -286,17 +286,17 @@ void diffcore_merge_broken(void) /* Peer survived. Merge them */ merge_broken(p, pp, &outq); q->queue[j] = NULL; - break; + goto next; } } - if (q->nr <= j) - /* The peer did not survive, so we keep - * it in the output. - */ - diff_q(&outq, p); + /* The peer did not survive, so we keep + * it in the output. + */ + diff_q(&outq, p); } else diff_q(&outq, p); +next:; } free(q->queue); *q = outq; From 54a80a9ad84af001470ea22fb0a14f6dc844b9c9 Mon Sep 17 00:00:00 2001 From: Alex Henrie Date: Mon, 30 Sep 2019 20:29:36 -0600 Subject: [PATCH 3/4] wrapper: use a loop instead of repetitive statements A check into the history of this code revealed no particular reason for the code to be written in this way. All popular compilers are capable of unrolling loops if it benefits performance, and once this code is replaced with a loop, the magic number 6 used in multiple places in this function can be replaced with a named constant. Reviewed-by: Derrick Stolee Reviewed-by: Johannes Schindelin Reviewed-by: Jeff King Signed-off-by: Alex Henrie Signed-off-by: Junio C Hamano --- wrapper.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/wrapper.c b/wrapper.c index 1e45ab7b92..54541386c1 100644 --- a/wrapper.c +++ b/wrapper.c @@ -506,13 +506,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) filename_template = &pattern[len - 6 - suffix_len]; for (count = 0; count < TMP_MAX; ++count) { uint64_t v = value; + int i; /* Fill in the random bits. */ - filename_template[0] = letters[v % num_letters]; v /= num_letters; - filename_template[1] = letters[v % num_letters]; v /= num_letters; - filename_template[2] = letters[v % num_letters]; v /= num_letters; - filename_template[3] = letters[v % num_letters]; v /= num_letters; - filename_template[4] = letters[v % num_letters]; v /= num_letters; - filename_template[5] = letters[v % num_letters]; v /= num_letters; + for (i = 0; i < 6; i++) { + filename_template[i] = letters[v % num_letters]; + v /= num_letters; + } fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); if (fd >= 0) From 53d687bf5f8008abd52b92120c7e22d4d81bdc71 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 2 Oct 2019 11:32:07 -0400 Subject: [PATCH 4/4] git_mkstemps_mode(): replace magic numbers with computed value The magic number "6" appears several times in the function, and is related to the size of the "XXXXXX" string we expect to find in the template. Let's pull that "XXXXXX" into a constant array, whose size we can get at compile time with ARRAY_SIZE(). Note that we probably can't just change this value, since callers will be feeding us a certain number of X's, but it hopefully makes the function itself easier to follow. While we're here, let's do the same with the "letters" array (which we _could_ modify if we wanted to include more characters). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wrapper.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/wrapper.c b/wrapper.c index 54541386c1..75992cff02 100644 --- a/wrapper.c +++ b/wrapper.c @@ -478,7 +478,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) "abcdefghijklmnopqrstuvwxyz" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "0123456789"; - static const int num_letters = 62; + static const int num_letters = ARRAY_SIZE(letters) - 1; + static const char x_pattern[] = "XXXXXX"; + static const int num_x = ARRAY_SIZE(x_pattern) - 1; uint64_t value; struct timeval tv; char *filename_template; @@ -487,12 +489,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) len = strlen(pattern); - if (len < 6 + suffix_len) { + if (len < num_x + suffix_len) { errno = EINVAL; return -1; } - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + if (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) { errno = EINVAL; return -1; } @@ -503,12 +505,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) */ gettimeofday(&tv, NULL); value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid(); - filename_template = &pattern[len - 6 - suffix_len]; + filename_template = &pattern[len - num_x - suffix_len]; for (count = 0; count < TMP_MAX; ++count) { uint64_t v = value; int i; /* Fill in the random bits. */ - for (i = 0; i < 6; i++) { + for (i = 0; i < num_x; i++) { filename_template[i] = letters[v % num_letters]; v /= num_letters; }