From f569065fc4ea047e1b2b0d50d2a59d9987a050b3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:54 +0000 Subject: [PATCH 01/10] remote-curl: avoid using the comma operator unnecessarily The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. Better use a semicolon instead. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- remote-curl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 1273507a96..590b228f67 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads, packet_buf_flush(&preamble); memset(&rpc, 0, sizeof(rpc)); - rpc.service_name = "git-upload-pack", + rpc.service_name = "git-upload-pack"; rpc.gzip_request = 1; err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result); @@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs) packet_buf_flush(&preamble); memset(&rpc, 0, sizeof(rpc)); - rpc.service_name = "git-receive-pack", + rpc.service_name = "git-receive-pack"; err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result); if (rpc_result.len) From 38c696d66b9d3a8f4f3363cc4254dd130723f396 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:55 +0000 Subject: [PATCH 02/10] rebase: avoid using the comma operator unnecessarily The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. Better use a semicolon instead. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index d4715ed35d..62bdf7276f 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1843,7 +1843,7 @@ int cmd_rebase(int argc, strbuf_addf(&msg, "%s (start): checkout %s", options.reflog_action, options.onto_name); ropts.oid = &options.onto->object.oid; - ropts.orig_head = &options.orig_head->object.oid, + ropts.orig_head = &options.orig_head->object.oid; ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK; ropts.head_msg = msg.buf; From 22542b6f9ef24d1058b01d7e9f837607728de710 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:56 +0000 Subject: [PATCH 03/10] kwset: avoid using the comma operator unnecessarily The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. Better use a semicolon instead. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- kwset.c | 54 +++++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/kwset.c b/kwset.c index 1714eada60..064329434e 100644 --- a/kwset.c +++ b/kwset.c @@ -197,10 +197,13 @@ kwsincr (kwset_t kws, char const *text, size_t len) while (link && label != link->label) { links[depth] = link; - if (label < link->label) - dirs[depth++] = L, link = link->llink; - else - dirs[depth++] = R, link = link->rlink; + if (label < link->label) { + dirs[depth++] = L; + link = link->llink; + } else { + dirs[depth++] = R; + link = link->rlink; + } } /* The current character doesn't have an outgoing link at @@ -257,14 +260,14 @@ kwsincr (kwset_t kws, char const *text, size_t len) switch (dirs[depth + 1]) { case L: - r = links[depth], t = r->llink, rl = t->rlink; - t->rlink = r, r->llink = rl; + r = links[depth]; t = r->llink; rl = t->rlink; + t->rlink = r; r->llink = rl; t->balance = r->balance = 0; break; case R: - r = links[depth], l = r->llink, t = l->rlink; - rl = t->rlink, lr = t->llink; - t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl; + r = links[depth]; l = r->llink; t = l->rlink; + rl = t->rlink; lr = t->llink; + t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl; l->balance = t->balance != 1 ? 0 : -1; r->balance = t->balance != (char) -1 ? 0 : 1; t->balance = 0; @@ -277,14 +280,14 @@ kwsincr (kwset_t kws, char const *text, size_t len) switch (dirs[depth + 1]) { case R: - l = links[depth], t = l->rlink, lr = t->llink; - t->llink = l, l->rlink = lr; + l = links[depth]; t = l->rlink; lr = t->llink; + t->llink = l; l->rlink = lr; t->balance = l->balance = 0; break; case L: - l = links[depth], r = l->rlink, t = r->llink; - lr = t->llink, rl = t->rlink; - t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl; + l = links[depth]; r = l->rlink; t = r->llink; + lr = t->llink; rl = t->rlink; + t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl; l->balance = t->balance != 1 ? 0 : -1; r->balance = t->balance != (char) -1 ? 0 : 1; t->balance = 0; @@ -567,22 +570,22 @@ bmexec (kwset_t kws, char const *text, size_t size) { while (tp <= ep) { - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; if (d == 0) goto found; - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; if (d == 0) goto found; - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; if (d == 0) goto found; - d = d1[U(tp[-1])], tp += d; - d = d1[U(tp[-1])], tp += d; + d = d1[U(tp[-1])]; tp += d; + d = d1[U(tp[-1])]; tp += d; } break; found: @@ -649,7 +652,8 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch) mch = NULL; else { - mch = text, accept = kwset->trie; + mch = text; + accept = kwset->trie; goto match; } From 0fbbb2c9f595a8460a7fd7c72d4e95081eb96b08 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:57 +0000 Subject: [PATCH 04/10] clar: avoid using the comma operator unnecessarily The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. In this instance, it makes the code harder to read than necessary, too. Better use a semicolon instead. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- t/unit-tests/clar/clar/fs.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/t/unit-tests/clar/clar/fs.h b/t/unit-tests/clar/clar/fs.h index 8b206179fc..2203743fb4 100644 --- a/t/unit-tests/clar/clar/fs.h +++ b/t/unit-tests/clar/clar/fs.h @@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode) mkdir(dest, dest_mode); cl_assert_(source_dir = opendir(source), "Could not open source dir"); - while ((d = (errno = 0, readdir(source_dir))) != NULL) { + for (;;) { char *child; + errno = 0; + if ((d = readdir(source_dir)) == NULL) + break; if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, "..")) continue; @@ -479,9 +482,12 @@ fs_rmdir_helper(const char *path) struct dirent *d; cl_assert_(dir = opendir(path), "Could not open dir"); - while ((d = (errno = 0, readdir(dir))) != NULL) { + for (;;) { char *child; + errno = 0; + if ((d = readdir(dir)) == NULL) + break; if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, "..")) continue; From 324fbaab88126196bd42e7fa383ee94e165d61b5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:58 +0000 Subject: [PATCH 05/10] xdiff: avoid using the comma operator unnecessarily The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. While the code in this patch used the comma operator intentionally (to avoid curly brackets around two statements, each, that want to be guarded by a condition), it is better to surround it with curly brackets and to use a semicolon instead. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- xdiff/xdiffi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 8889b8b62a..5a96e36dfb 100644 --- a/xdiff/xdiffi.c +++ b/xdiff/xdiffi.c @@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1, for (d = fmax; d >= fmin; d -= 2) { i1 = XDL_MIN(kvdf[d], lim1); i2 = i1 - d; - if (lim2 < i2) - i1 = lim2 + d, i2 = lim2; + if (lim2 < i2) { + i1 = lim2 + d; + i2 = lim2; + } if (fbest < i1 + i2) { fbest = i1 + i2; fbest1 = i1; @@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1, for (d = bmax; d >= bmin; d -= 2) { i1 = XDL_MAX(off1, kvdb[d]); i2 = i1 - d; - if (i2 < off2) - i1 = off2 + d, i2 = off2; + if (i2 < off2) { + i1 = off2 + d; + i2 = off2; + } if (i1 + i2 < bbest) { bbest = i1 + i2; bbest1 = i1; From be7a517ce4606b46479fe06fae6c1ab117b0d384 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:52:59 +0000 Subject: [PATCH 06/10] diff-delta: avoid using the comma operator The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. That is why the `-Wcomma` option of clang was introduced: To identify unintentional uses of the comma operator. Intentional uses include situations where one wants to avoid curly brackets around multiple statements that need to be guarded by a condition. This is the case here, as the repetitive nature of the statements is easier to see for a human reader this way. At least in my opinion. However, opinions on this differ wildly, take 10 people and you have 10 different preferences. On the Git mailing list, it seems that the consensus is to use the long form instead, so let's do just that. Suggested-by: Phillip Wood Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- diff-delta.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/diff-delta.c b/diff-delta.c index a4faf73829..71d37368d6 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -438,19 +438,31 @@ create_delta(const struct delta_index *index, op = out + outpos++; i = 0x80; - if (moff & 0x000000ff) - out[outpos++] = moff >> 0, i |= 0x01; - if (moff & 0x0000ff00) - out[outpos++] = moff >> 8, i |= 0x02; - if (moff & 0x00ff0000) - out[outpos++] = moff >> 16, i |= 0x04; - if (moff & 0xff000000) - out[outpos++] = moff >> 24, i |= 0x08; + if (moff & 0x000000ff) { + out[outpos++] = moff >> 0; + i |= 0x01; + } + if (moff & 0x0000ff00) { + out[outpos++] = moff >> 8; + i |= 0x02; + } + if (moff & 0x00ff0000) { + out[outpos++] = moff >> 16; + i |= 0x04; + } + if (moff & 0xff000000) { + out[outpos++] = moff >> 24; + i |= 0x08; + } - if (msize & 0x00ff) - out[outpos++] = msize >> 0, i |= 0x10; - if (msize & 0xff00) - out[outpos++] = msize >> 8, i |= 0x20; + if (msize & 0x00ff) { + out[outpos++] = msize >> 0; + i |= 0x10; + } + if (msize & 0xff00) { + out[outpos++] = msize >> 8; + i |= 0x20; + } *op = i; From 752fe9dc929afe1944e44b852f1248df4fb82986 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:53:00 +0000 Subject: [PATCH 07/10] wildmatch: avoid using of the comma operator The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. That is why the `-Wcomma` option of clang was introduced: To identify unintentional uses of the comma operator. In this instance, the usage is intentional because it allows storing the value of the current character as `prev_ch` before making the next character the current one, all of which happens in the loop condition that lets the loop stop at a closing bracket. However, it is hard to read. The chosen alternative to using the comma operator is to move those assignments from the condition into the loop body; In this particular case that requires special care because the loop body contains a `continue` for the case where a character class is found that starts with `[:` but does not end in `:]` (and the assignments should occur even when that code path is taken), which needs to be turned into a `goto`. Helped-by: Phillip Wood Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- wildmatch.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/wildmatch.c b/wildmatch.c index 8ea29141bd..69a2ae7000 100644 --- a/wildmatch.c +++ b/wildmatch.c @@ -223,7 +223,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags) p_ch = '['; if (t_ch == p_ch) matched = 1; - continue; + goto next; } if (CC_EQ(s,i, "alnum")) { if (ISALNUM(t_ch)) @@ -268,7 +268,10 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags) p_ch = 0; /* This makes "prev_ch" get set to 0. */ } else if (t_ch == p_ch) matched = 1; - } while (prev_ch = p_ch, (p_ch = *++p) != ']'); +next: + prev_ch = p_ch; + p_ch = *++p; + } while (p_ch != ']'); if (matched == negated || ((flags & WM_PATHNAME) && t_ch == '/')) return WM_NOMATCH; From 88c91d7d742b802a8774383641f8d997cfd1cd0c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:53:01 +0000 Subject: [PATCH 08/10] compat/regex: explicitly mark intentional use of the comma operator The comma operator is a somewhat obscure C feature that is often used by mistake and can even cause unintentional code flow. That is why the `-Wcomma` option of clang was introduced: To identify unintentional uses of the comma operator. In the `compat/regex/` code, the comma operator is used twice, once to avoid surrounding two conditional statements with curly brackets, the other one to increment two counters simultaneously in a `do ... while` condition. The first one is replaced with a proper conditional block, surrounded by curly brackets. The second one would be harder to replace because the loop contains two `continue`s. Therefore, the second one is marked as intentional by casting the value-to-discard to `void`. Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- compat/regex/regex_internal.c | 5 ++++- compat/regex/regexec.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c index ec5cc5d2dd..4a4f849629 100644 --- a/compat/regex/regex_internal.c +++ b/compat/regex/regex_internal.c @@ -1232,7 +1232,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src) is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; ) { if (dest->elems[id] == src->elems[is]) - is--, id--; + { + is--; + id--; + } else if (dest->elems[id] < src->elems[is]) dest->elems[--sbase] = src->elems[is--]; else /* if (dest->elems[id] > src->elems[is]) */ diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c index 2eeec82f40..c08f1bbe1f 100644 --- a/compat/regex/regexec.c +++ b/compat/regex/regexec.c @@ -2210,7 +2210,7 @@ sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx, /* mctx->bkref_ents may have changed, reload the pointer. */ entry = mctx->bkref_ents + enabled_idx; } - while (enabled_idx++, entry++->more); + while ((void)enabled_idx++, entry++->more); } err = REG_NOERROR; free_return: From 3db4cb987f114186744025432fc201bbea1ccc7b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:53:02 +0000 Subject: [PATCH 09/10] clang: warn when the comma operator is used When compiling Git using `clang`, the `-Wcomma` option can be used to warn about code using the comma operator (because it is typically unintentional and wants to use the semicolon instead). Helped-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- config.mak.dev | 4 ++++ meson.build | 1 + 2 files changed, 5 insertions(+) diff --git a/config.mak.dev b/config.mak.dev index 0fd8cc4d35..3142363816 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla DEVELOPER_CFLAGS += -Wwrite-strings DEVELOPER_CFLAGS += -fno-common +ifneq ($(filter clang9,$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wcomma +endif + ifneq ($(filter clang4,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare endif diff --git a/meson.build b/meson.build index efe2871c9d..fd8c05dec9 100644 --- a/meson.build +++ b/meson.build @@ -715,6 +715,7 @@ libgit_dependencies = [ ] # Makefile. if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc' foreach cflag : [ + '-Wcomma', '-Wdeclaration-after-statement', '-Wformat-security', '-Wold-style-definition', From abd4192b07c5a7dbb4b13a532d0643982b42526f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Mar 2025 11:53:03 +0000 Subject: [PATCH 10/10] detect-compiler: detect clang even if it found CUDA In my setup, clang finds `/usr/local/cuda` and hence the output of `clang -v` ends with this line: Found CUDA installation: /usr/local/cuda, version This confuses the `detect-compiler` script because it matches _all_ lines that contain the needle "version" surrounded by spaces. As a consequence, the `get_family` function returns two lines: "Ubuntu clang" and above-mentioned line, which the `case` statement does not handle well and hence reports "unknown compiler family" instead of the expected set of "clang14", "clang13", ..., "clang1" output. Let's unconfuse the script by letting it parse the first matching line and ignore the rest. Helped-by: Eric Sunshine Signed-off-by: Johannes Schindelin Acked-by: Phillip Wood Signed-off-by: Junio C Hamano --- detect-compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detect-compiler b/detect-compiler index a87650b71b..124ebdd4c9 100755 --- a/detect-compiler +++ b/detect-compiler @@ -9,7 +9,7 @@ CC="$*" # # FreeBSD clang version 3.4.1 (tags/RELEASE...) get_version_line() { - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version ' + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}' } get_family() {