On Fri, Mar 07, 2025 at 12:46:27PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:

quoted

@@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
 					    update->refname,
 					    oid_to_hex(&update->old_oid));
 				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
+
+				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+					strbuf_setlen(err, 0);
+					ret = 0;
+					continue;
+				}
+
 				goto error;
 			}
 		}

This new code isn't reachable, since we return in the lines shown in the
diff context.

Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
I think the "goto error" was already unreachable, so possibly the error
is in an earlier patch. (I didn't look; Coverity flagged this in the
final state in 'jch').

Sorry about that.  It shows that I lack the bandwidth necessary to
go through fine toothed comb on all the topics I queue.  Perhaps I
should be more selective and queue only the ones I personally had
enough bandwidth to look over (or have seen clear "I looked each and
every line of this series with fine toothed comb, put reviewed-by:
me" messages sent by trusted reviewers) while ignoring others?

Eh, I would not worry about it too much. Things get missed, and that is
why we have many layers of reviews, static analysis, and ultimately
users to help us find bugs. ;)

I was disappointed that the compiler didn't complain, though. Maybe we
should do this:

-- >8 --
Subject: [PATCH] config.mak.dev: enable -Wunreachable-code

Having the compiler point out unreachable code can help avoid bugs, like
the one discussed in:

  https://lore.kernel.org/git/20250307195057.GA3675279@coredump.intra.peff.net/ (local)

In that case it was found by Coverity, but finding it earlier saves
everybody time and effort.

We can use -Wunreachable-code to get some help from the compiler here.
Interestingly, this is a noop in gcc. It was a real warning up until gcc
4.x, when it was removed for being too flaky, but they left the
command-line option to avoid breaking users. See:

  https://stackoverflow.com/questions/17249934/why-does-gcc-not-warn-for-unreachable-code

However, clang does implement this option, and it finds the case
mentioned above (and no other cases within the code base). And since we
run clang in several of our CI jobs, that's enough to get an early
warning of breakage.

We could enable it only for clang, but since gcc is happy to ignore it,
it's simpler to just turn it on for all developer builds.

Signed-off-by: Jeff King <redacted>
---
You can see it in action (merged into 'jch') here:

  https://github.com/peff/git/actions/runs/13729842188

where all of the clang jobs fail.

 config.mak.dev | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.dev b/config.mak.dev
index 0fd8cc4d35..95b7bc46ae 100644
--- a/config.mak.dev
+++ b/config.mak.dev

@@ -39,6 +39,7 @@ DEVELOPER_CFLAGS += -Wunused
 DEVELOPER_CFLAGS += -Wvla
 DEVELOPER_CFLAGS += -Wwrite-strings
 DEVELOPER_CFLAGS += -fno-common
+DEVELOPER_CFLAGS += -Wunreachable-code
 
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare

-- 
2.49.0.rc1.380.g53e738dd21