patch, 2 partsAvoid the comma operator

The comma operator [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is rarely used in C anymore, and typically indicates a typo. Just like in these instances, where a semicolon was meant to be used, as there is no need to discard the first statement's result here.

Johannes Schindelin (2):
  remote-curl: avoid using the comma operator unnecessarily
  rebase: avoid using the comma operator unnecessarily

 builtin/rebase.c | 2 +-
 remote-curl.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1889

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Johannes

On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:

clang's -Wcomma finds another instance in this file

On Tue, Mar 25, 2025 at 04:28:32PM +0000, Phillip Wood wrote:

It looks like there are a few tricky cases. Inside a loop condition, a comma can be used to smuggle in an extra line. E.g., in wildmatch:

  do {
    ...
  } while (prev_ch = p_ch, (p_ch = *++p) != ']');

or there are a few spots in clar like:

  while ((d = (errno = 0, readdir(source_dir))) != NULL) {
    ...
  }
  cl_assert_(errno == 0, "Failed to iterate source dir");

These could probably be re-written, but it's not a totally trivial change.

The rest of them seem pretty straight-forward (though you do have to watch out for conditionals using it to stuff multiple lines into a single "statement"):

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:

 > > diff --git a/builtin/rebase.c b/builtin/rebase.c

This is obviously a typo - thanks for fixing it

Best Wishes

Phillip

On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:

Minor aside: How were these 'discovered'?

Philip

Hi Philip,

On Tue, 25 Mar 2025, Philip Oakley wrote:

I am working on a GitHub workflow that uses CodeQL to find such issues, that's how I found them. (I also worked with the CodeQL team to get this query added, way back when I was still working at GitHub.)

Ciao, Johannes

On Tue, Mar 25, 2025 at 03:12:21PM +0100, Johannes Schindelin wrote:

Neat. I will be curious to see how the results compare/contrast to what those of us who run Coverity get.

Thanks, Taylor

On Tue, Mar 25, 2025 at 08:01:48AM +0000, Johannes Schindelin via GitGitGadget wrote:

The changes look obviously good to me, thanks. The reftable library and backend also had several instances where the operator was used by accident, and I've gotten rid of those over time. They typically don't do any harm as the result is essentially the same, but sometimes they may cause issues. And at the very least they cause confusion.

It would be great if there was a compiler warning we could enable for cases where the operator likely isn't intentional. But I couldn't find any, unfortunately.

Thanks!

Patrick

Hi Patrick,

On Tue, 25 Mar 2025, Patrick Steinhardt wrote:

Thank you for addressing these in the reftable library!

I was not actually planning on adding the CodeQL workflow to git/git, seeing as its CI is already taking way too much CPU time for my liking. But in `microsoft/git`, I am kind of required to, so we'll catch those issues there.

Ciao, Johannes

On Tue, Mar 25, 2025 at 03:13:47PM +0100, Johannes Schindelin wrote:

Heh. I should have read this email from you before replying to the last one ;-).

I would actually be interested in having CodeQL support "ship" for copies of git.git that are hosted on GitHub. I wonder if we could do a similar trick as in a56b6230d0 (ci: add a GitHub workflow to submit Coverity scans, 2023-09-25), where the ENABLE_COVERITY_SCAN_FOR_BRANCHES variable controls whether or not the job actually executes.

Thanks, Taylor

Patrick Steinhardt <ps@pks.im> writes:

Clang does have '-Wcomma' [1], which seems to be exactly what we want.

[1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wcomma

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Well spotted.

These two looked somehow surprisingly bad.

If I hadn't known better, I may have spent quite some time wondering if these are some ways to hide an unexpected behaviour behind the differences between a comma and a semicolon for nefarious purposes.

Will queue.  Thanks.

The comma operator [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is rarely used in C anymore, and typically indicates a typo. Just like in these instances, where a semicolon was meant to be used, as there is no need to discard the first statement's result here.

Changes since v1:

 * Use -Wcomma when compiling with clang and with DEVELOPER=1.
 * Address the remaining instances pointed out by clang (and by Phillip).

Johannes Schindelin (10):
  remote-curl: avoid using the comma operator unnecessarily
  rebase: avoid using the comma operator unnecessarily
  kwset: avoid using the comma operator unnecessarily
  clar: avoid using the comma operator unnecessarily
  xdiff: avoid using the comma operator unnecessarily
  diff-delta: explicitly mark intentional use of the comma operator
  wildmatch: explicitly mark intentional use of the comma operator
  compat/regex: explicitly mark intentional use of the comma operator
  clang: warn when the comma operator is used
  detect-compiler: detect clang even if it found CUDA

 builtin/rebase.c              |  2 +-
 compat/regex/regex_internal.c |  7 +++--
 compat/regex/regexec.c        |  2 +-
 config.mak.dev                |  4 +++
 detect-compiler               |  2 +-
 diff-delta.c                  | 12 ++++----
 kwset.c                       | 54 +++++++++++++++++++----------------
 remote-curl.c                 |  4 +--
 t/unit-tests/clar/clar/fs.h   | 10 +++++--
 wildmatch.c                   |  2 +-
 xdiff/xdiffi.c                | 12 +++++---
 11 files changed, 65 insertions(+), 46 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1889

Range-diff vs v1:

  1:  e3069fd4564 !  1:  913c7a0d296 remote-curl: avoid using the comma operator unnecessarily
     @@ Commit message
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## remote-curl.c ##
     +@@ remote-curl.c: 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);
      @@ remote-curl.c: static int push_git(struct discovery *heads, int nr_spec, const char **specs)
       	packet_buf_flush(&preamble);
       
  2:  7dfbdc48954 =  2:  37ff88b8275 rebase: avoid using the comma operator unnecessarily
  -:  ----------- >  3:  f601f4e74a5 kwset: avoid using the comma operator unnecessarily
  -:  ----------- >  4:  f60ebe376e1 clar: avoid using the comma operator unnecessarily
  -:  ----------- >  5:  7239078413f xdiff: avoid using the comma operator unnecessarily
  -:  ----------- >  6:  5e0e8325620 diff-delta: explicitly mark intentional use of the comma operator
  -:  ----------- >  7:  9a6de12b807 wildmatch: explicitly mark intentional use of the comma operator
  -:  ----------- >  8:  dc626f36df3 compat/regex: explicitly mark intentional use of the comma operator
  -:  ----------- >  9:  91f86c3aba9 clang: warn when the comma operator is used
  -:  ----------- > 10:  2f6f31240fe detect-compiler: detect clang even if it found CUDA

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/unit-tests/clar/clar/fs.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:

This code has changed upstream already, but let's roll with your change anyway. I plan to update the clar to the upstream version soonish once I have landed integer comparisons, and will take care that there aren't any other comment operators left.

Patrick

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

Thank you for putting so much energy into improving the tests, I really appreciate it!

Ciao, Johannes

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 xdiff/xdiffi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

To mark this usage as intentional, the return value of the statement before the comma needs to be cast to `void`, which we do here.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-delta.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:

Hm. I think the end result is even more confusing than before. Why don't we introduce curly braces here, same as in preceding commits?

Patrick

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

The interleaved -/+ lines make it admittedly hard to see what I meant. I'll unwind it a bit (presenting only the `moff` part, the same consideration applies to `msize`):

		if (moff & 0x000000ff)
			(void)(out[outpos++] = moff >> 0),  i |= 0x01;
		if (moff & 0x0000ff00)
			(void)(out[outpos++] = moff >> 8),  i |= 0x02;
		if (moff & 0x00ff0000)
			(void)(out[outpos++] = moff >> 16), i |= 0x04;
		if (moff & 0xff000000)
			(void)(out[outpos++] = moff >> 24), i |= 0x08;

In this form, it is very obvious (from comparing the right-side half of the lines) that a shifted version of `moff` is appended to `out` and `i` gets a bit set, and the correlation between shift width and the set bit is relatively easy to see and validate (at least my brain has an easy time here, thanks to the alignment and thanks to visual similarity between the non-blank parts).

It is admittedly quite a bit harder not to be distracted by the repetitive `(void)(out[...` parts to understand and validate the `if` conditions on the left-hand side, but thanks to those repetitive parts being identical, and being only one line between those `if` lines, I can bring my brain to focus only on the differences of the bitmask and understand and verify them with relatively little effort.

When I compared this form to the following, the cognitive load to wrap my head around the code is quite a bit higher there:

		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;
		}

The reason is the visual distance between the near-identical code.

Having said that, I do realize that my brain quite possibly works in special ways here and that the general preference is to go with the latter form.

Do you have a strong opinion which form to use?

Ciao, Johannes

Hi Johannes

On 26/03/2025 07:20, Johannes Schindelin wrote:

That makes it pretty unreadable for me. If you're worried about the vertical space we could perhaps keep both statements on a single line so that we're only adding a single newline for the closing brace rather than two.

Best Wishes

Phillip

  but thanks to those repetitive parts being identical,

On Wed, Mar 26, 2025 at 10:17:50AM +0000, Phillip Wood wrote:

My $.02 here would be that the form:

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0;
        i |= 0x01;
    }

is the most readable, and fits within the conventions of our CodingGuidelines. I don't really love the idea of writing:

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0); i |= 0x01;
    }

, since it looks like a single-line if-statement and is thus tempting to drop the braces, which would make the code incorrect, as 'i |= 0x01' would be executed unconditionally. I suppose you could write

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0); i |= 0x01; }

, but that feels even uglier to me ;-).

Thanks, Taylor

(Just picking the latest message to reply to, not really quite right)

Might be overkill but:

        #define XXX(index) do { \
                if (moff & (0xffUL << ((index) * 8))) {
                        out[outpos++] = moff >> ((index) * 8);
                        i |= 1 << (index);
                }
        } while (0)

        XXX(0);
        XXX(1);
        XXX(2);
        XXX(3);

        #undef XXX

would do the trick.  Pick a proper name for XXX of course.

Chris

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

To mark such a usage as intentional, the value needs to be cast to `void`, which we do here.

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.

The alternative to using the comma operator would be to move those assignments from the condition into the loop body; In this particular case that would require the assignments to either be duplicated or to introduce and use a `goto` target before the assignments, though, 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).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wildmatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:

In this case I agree that it makes sense to not introduce curly braces for brevity.

Patrick

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

I should probably have mentioned that this patch took the longest to write of the entire patch series, by far. Not because of the changed code, that was easy. No, when I wrote the commit message and spotted the `continue`, I did not want to leave it at that because the code around that `continue` looks... well, let's just say that it could be rewritten for clarity.

In fact, when I looked at the following part, I was immediately _convinced_ that it is incorrect, and had to work very hard to understand why it works correctly, even going so far as to single-step through a couple of examples, e.g. `test-tool wildmatch wildmatch 'b' '[[:a-z]'`:

				} else if (p_ch == '[' && p[1] == ':') {
					const uchar *s;
					int i;
					for (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/
					if (!p_ch)
						return WM_ABORT_ALL;
					i = p - s - 1;
					if (i < 0 || p[-1] != ':') {
						/* Didn't find ":]", so treat like a normal set. */
						p = s - 2;
						p_ch = '[';
						if (t_ch == p_ch)
							matched = 1;
						continue;
					}

For context, here is a link: https://gitlab.com/gitlab-org/git/-/blob/v2.49.0/wildmatch.c?ref_type=tags#L213-227. At this point, `t_ch` is the current character in the text to match; `p_ch` (and `*p`) is the current character in the _pattern_, and it is _inside_ a `[...]` character range, and it wants to match a character class of the form `[:alnum:]` but also allow for a regular character range that starts by including the colon. And that latter scenario, where it is _not_ a special character class, is what this `continue` is all about.

What threw me was that `t_ch == p_ch` condition _directly_ after assigning `p_ch = '['`. It is still a pattern I would always immediately suspect to be a bug: Why not compare `t_ch == '['` instead, which would be much more obvious?

You will also note that the value of `i` is only used in the condition, and it is basically used to determine whether the the colon was immediately followed by the closing bracket or not, which could be rewritten to be a lot more obvious.

So what does the `continue` do here? It skips back to the outer loop, continuing with the `:` as next pattern character in the character range, and for that it is crucial that the `p_ch` be set to the opening bracket and `p` is rewound _just_ so that the assignments in the loop condition can set things up for the next loop iteration (still within that `case '['`) not to be thrown by that `[:`.

I probably did a terrible job explaining why the code works as intended, but I'll just chalk it up to the contortions my brain had to exercise to understand that code.

But all that's safely outside the scope of the question whether to use a comma operator or not ;-)

Ciao, Johannes

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Hmph, I personally do not find the (void) convention any easier to read and understand than the original, and more importantly, it does not look like a good signal to say that the author knows what they are doing.

For this particular loop, it probably makes a lot more sense to do a stupid and more obvious rewrite. And the same thing can be said for the previous step.

Perhaps like so?

 diff-delta.c | 37 ++++++++++++++++++++++++-------------
 wildmatch.c  |  4 +++-
 2 files changed, 27 insertions(+), 14 deletions(-)

Hi Johannes

On 25/03/2025 23:32, Johannes Schindelin via GitGitGadget wrote:

I think that would be clearer, something like the diff below

Best Wishes

Phillip

---- >8 ----

Phillip Wood <phillip.wood123@gmail.com> writes:

Ah, I missed that "continue"; with the "next:" label, it looks quite clear what is going on.

I like it.  Thanks.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 compat/regex/regex_internal.c | 7 ++++---
 compat/regex/regexec.c        | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:

Should the other arms of this conditional have matching curly-braces?

Thanks, Taylor

Hi Taylor,

On Wed, 26 Mar 2025, Taylor Blau wrote:

No. Have a look around in that file, that's not the coding convention.

However, a valid concern is that I used Git's convention for the curly brackets (appending the open bracket to the `if` line), which is _not_ the convention used in this file, which uses GNU conventions instead. I will address that concern in the next iteration.

Ciao, Johannes

On Thu, Mar 27, 2025 at 11:29:16AM +0100, Johannes Schindelin wrote:

I was just about to respond that even though it breaks the convention, that we should encourage good hygeine by ensuring new code follows the CodingGuidelines, even if it looks wonky in the context of the rest of the file.

But this is compat/regex code, which clearly does not need to follow the convention. Sorry about that!

Thanks, Taylor

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.dev | 4 ++++
 1 file changed, 4 insertions(+)

On Tue, Mar 25, 2025 at 11:32:13PM +0000, Johannes Schindelin via GitGitGadget wrote:

Let's squash the below diff into this commit. The loop already makes sure that the compiler supports the flag, so there is no need to special case Clang.

Patrick

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

Okay, will do.

For my curiosity...

This `get_argument_syntax() == 'gcc'` condition catches `clang`? What other compilers that aren't GCC does it catch?

Ciao, Johannes

On Wed, Mar 26, 2025 at 08:50:24AM +0100, Johannes Schindelin wrote:

Yes, it does catch Clang as well as the Intel compiler. The list of compilers and their respective syntax can be found at [1]

Patrick

[1]: https://mesonbuild.com/Reference-tables.html#compiler-ids

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

Thank you for this valuable reference!

Ciao, Johannes

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 detect-compiler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:

Makes sense. I wondered if this:

might be more readable with "grep -m1", but it looks like "-m" is not in POSIX. So what you wrote is probably safer.

-Peff

On Wed, Mar 26, 2025 at 1:44 PM Jeff King <peff@peff.net> wrote:

It's probably an indication that I've done too much `sed` programming, but I find Dscho's version more obvious. That aside, your response made me take a closer look at what Dscho wrote and I noticed that it is syntactically flawed, at least for BSD-lineage `sed`. Testing on macOS reveals that this is indeed so:

    % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
    sed: 1: "/ version /{p;q}": extra characters at the end of q command

The problem is that the `q` function takes no arguments, but BSD-lineage `sed` thinks that the closing `}` is an argument rather than a terminator. Fixing this requires inserting a terminator after `q`, which will be either a newline character or a semicolon. So, the correct form is:

    sed -n '/ version /{p;q;}

On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:

Heh, I think it was the braces and semicolons that made my spider-sense tingle, probably because I've been bitten by those subtleties in the past.

I think just "/foo/p;q" works on GNU sed, but no idea if it does elsewhere. What you wrote seems the safest.

-Peff

On Thu, Mar 27, 2025 at 1:14 AM Jeff King <peff@peff.net> wrote:

That's not quite the same, though. The patternless `q` will cause `sed` to terminate upon reading the first line of input, not upon the first line which contains " version ". This matters, for instance, if the first line output by `$CC -v` is not the version string (i.e. it might be a copyright notice).

On Thu, Mar 27, 2025 at 01:21:03AM -0400, Eric Sunshine wrote:

Oh, duh, you're right. I even tested to make sure were still quitting after matching, but of course that is because we were quitting even when _not_ matching.

For some reason I also thought /foo/pq would work, but it doesn't. I wonder if it used to under some implementations, or if I'm simply going senile.

(The point still remains that what you wrote is the correct thing).

-Peff

On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:

Thanks for all of these fixes!

Patrick

On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:

Thanks for fixing these. I checked the diff against the quick-and-dirty patch I posted earlier in the thread, and your resolutions for the "easy" cases look good (though like others, I'd prefer switching to semicolons for the one in diff-delta.c).

For the harder cases inside while() conditionals, the rewrites all look correct to me. I do think that getting rid of the commas often makes the result easier to read, but the discussion on wildmatch shows that it's easy to get the transformation wrong. So I'd be happy enough slapping a "(void)" on that one and moving on with our lives. The goal is not to prettify that code (which was not even written for Git in the first place) but to silence -Wcomma false positives so that we can find the actual problems.

-Peff

The comma operator [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is rarely used in C anymore, and typically indicates a typo. Just like in these instances, where a semicolon was meant to be used, as there is no need to discard the first statement's result here.

Changes since v2:

 * Made the sed construct in detect-compiler portable (thanks, Eric
   Sunshine!)
 * The majority of the feedback disagreed with the more compact format in
   diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)
 * The more succinct and safer, but less readable, cast in the loop
   condition of the dowild() function was replaced with the goto-based
   alternative I had mentioned as a possibility in the commit message
   (thanks, Phillip Wood!)
 * I adjusted the style of my compat/regex/ patch to the surrounding code's.
 * The -Wcomma option is now used in Meson-based clang builds, too (thanks,
   Patrick Steinhardt!)

Changes since v1:

 * Use -Wcomma when compiling with clang and with DEVELOPER=1.
 * Address the remaining instances pointed out by clang (and by Phillip).

Johannes Schindelin (10):
  remote-curl: avoid using the comma operator unnecessarily
  rebase: avoid using the comma operator unnecessarily
  kwset: avoid using the comma operator unnecessarily
  clar: avoid using the comma operator unnecessarily
  xdiff: avoid using the comma operator unnecessarily
  diff-delta: avoid using the comma operator
  wildmatch: avoid using of the comma operator
  compat/regex: explicitly mark intentional use of the comma operator
  clang: warn when the comma operator is used
  detect-compiler: detect clang even if it found CUDA

 builtin/rebase.c              |  2 +-
 compat/regex/regex_internal.c |  5 +++-
 compat/regex/regexec.c        |  2 +-
 config.mak.dev                |  4 +++
 detect-compiler               |  2 +-
 diff-delta.c                  | 38 +++++++++++++++---------
 kwset.c                       | 54 +++++++++++++++++++----------------
 meson.build                   |  1 +
 remote-curl.c                 |  4 +--
 t/unit-tests/clar/clar/fs.h   | 10 +++++--
 wildmatch.c                   |  7 +++--
 xdiff/xdiffi.c                | 12 +++++---
 12 files changed, 89 insertions(+), 52 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1889

Range-diff vs v2:

  1:  913c7a0d296 =  1:  913c7a0d296 remote-curl: avoid using the comma operator unnecessarily
  2:  37ff88b8275 =  2:  37ff88b8275 rebase: avoid using the comma operator unnecessarily
  3:  f601f4e74a5 =  3:  f601f4e74a5 kwset: avoid using the comma operator unnecessarily
  4:  f60ebe376e1 =  4:  f60ebe376e1 clar: avoid using the comma operator unnecessarily
  5:  7239078413f =  5:  7239078413f xdiff: avoid using the comma operator unnecessarily
  6:  5e0e8325620 !  6:  045d695d73e diff-delta: explicitly mark intentional use of the comma operator
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    diff-delta: explicitly mark intentional use of the comma operator
     +    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
     @@ Commit message
          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.
     +    statements is easier to see for a human reader this way. At least in my
     +    opinion.
      
     -    To mark this usage as intentional, the return value of the statement
     -    before the comma needs to be cast to `void`, which we do here.
     +    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 <phillip.wood123@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## diff-delta.c ##
      @@ diff-delta.c: create_delta(const struct delta_index *index,
     + 			op = out + outpos++;
       			i = 0x80;
       
     - 			if (moff & 0x000000ff)
     +-			if (moff & 0x000000ff)
      -				out[outpos++] = moff >> 0,  i |= 0x01;
     -+				(void)(out[outpos++] = moff >> 0),  i |= 0x01;
     - 			if (moff & 0x0000ff00)
     +-			if (moff & 0x0000ff00)
      -				out[outpos++] = moff >> 8,  i |= 0x02;
     -+				(void)(out[outpos++] = moff >> 8),  i |= 0x02;
     - 			if (moff & 0x00ff0000)
     +-			if (moff & 0x00ff0000)
      -				out[outpos++] = moff >> 16, i |= 0x04;
     -+				(void)(out[outpos++] = moff >> 16), i |= 0x04;
     - 			if (moff & 0xff000000)
     +-			if (moff & 0xff000000)
      -				out[outpos++] = moff >> 24, i |= 0x08;
     -+				(void)(out[outpos++] = moff >> 24), i |= 0x08;
     - 
     - 			if (msize & 0x00ff)
     +-
     +-			if (msize & 0x00ff)
      -				out[outpos++] = msize >> 0, i |= 0x10;
     -+				(void)(out[outpos++] = msize >> 0), i |= 0x10;
     - 			if (msize & 0xff00)
     +-			if (msize & 0xff00)
      -				out[outpos++] = msize >> 8, i |= 0x20;
     -+				(void)(out[outpos++] = msize >> 8), i |= 0x20;
     ++			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;
     ++			}
       
       			*op = i;
       
  7:  9a6de12b807 !  7:  1d0ce59cb68 wildmatch: explicitly mark intentional use of the comma operator
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    wildmatch: explicitly mark intentional use of the comma operator
     +    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.
      
     -    To mark such a usage as intentional, the value needs to be cast to
     -    `void`, which we do here.
     -
          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.
      
     -    The alternative to using the comma operator would be to move those
     +    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 would require the assignments to either be duplicated or to
     -    introduce and use a `goto` target before the assignments, though,
     -    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).
     +    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 <phillip.wood@dunelm.org.uk>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## wildmatch.c ##
     +@@ wildmatch.c: 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))
      @@ wildmatch.c: 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) != ']');
     -+			} while ((void)(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;
  8:  dc626f36df3 !  8:  b8405f3d237 compat/regex: explicitly mark intentional use of the comma operator
     @@ Commit message
      
       ## compat/regex/regex_internal.c ##
      @@ compat/regex/regex_internal.c: re_node_set_merge (re_node_set *dest, const re_node_set *src)
     -   for (sbase = dest->nelem + 2 * src->nelem,
              is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
           {
     --      if (dest->elems[id] == src->elems[is])
     +       if (dest->elems[id] == src->elems[is])
      -	is--, id--;
     --      else if (dest->elems[id] < src->elems[is])
     -+      if (dest->elems[id] == src->elems[is]) {
     -+	is--;
     -+	id--;
     -+      } else if (dest->elems[id] < src->elems[is])
     ++	{
     ++	  is--;
     ++	  id--;
     ++	}
     +       else if (dest->elems[id] < src->elems[is])
       	dest->elems[--sbase] = src->elems[is--];
             else /* if (dest->elems[id] > src->elems[is]) */
     - 	--id;
      
       ## compat/regex/regexec.c ##
      @@ compat/regex/regexec.c: sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
  9:  91f86c3aba9 !  9:  6b6cd556465 clang: warn when the comma operator is used
     @@ Commit message
          warn about code using the comma operator (because it is typically
          unintentional and wants to use the semicolon instead).
      
     +    Helped-by: Patrick Steinhardt <ps@pks.im>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## config.mak.dev ##
     @@ config.mak.dev: DEVELOPER_CFLAGS += -Wvla
       ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
       DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
       endif
     +
     + ## meson.build ##
     +@@ meson.build: 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',
 10:  2f6f31240fe ! 10:  77f1dcaca1c detect-compiler: detect clang even if it found CUDA
     @@ Commit message
          Let's unconfuse the script by letting it parse the first matching line
          and ignore the rest.
      
     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## detect-compiler ##
     @@ detect-compiler: 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}'
     ++	LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'
       }
       
       get_family() {

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/unit-tests/clar/clar/fs.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 xdiff/xdiffi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <phillip.wood123@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff-delta.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wildmatch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 compat/regex/regex_internal.c | 5 ++++-
 compat/regex/regexec.c        | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.dev | 4 ++++
 meson.build    | 1 +
 2 files changed, 5 insertions(+)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <sunshine@sunshineco.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 detect-compiler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Johannes

On 27/03/2025 11:52, Johannes Schindelin via GitGitGadget wrote:

The range-diff below looks good to me, thanks for making our code base clearer.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Yup, thanks Dscho, and all who gave valuable input to polish the series.

Will queue.