Jeff King [off-list ref] writes:

On Thu, May 15, 2025 at 04:07:25PM +0200, Karthik Nayak wrote:

quoted

+char *ref_transaction_error_msg(enum ref_transaction_error err)
+{
+	const char *reason = "";
+
+	switch (err) {
+	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
+		reason = "refname conflict";
+		break;
+	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
+		reason = "reference already exists";
+		break;
+	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
+		reason = "reference does not exist";
+		break;
+	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
+		reason = "incorrect old value provided";
+		break;
+	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
+		reason = "invalid new value provided";
+		break;
+	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
+		reason = "expected symref but found regular ref";
+		break;
+	default:
+		reason = "unkown failure";
+	}
+
+	return xstrdup(reason);
+}

The assignment of "" is dead code, I think? We will always assign
"unknown failure" as a last resort. Not a big deal, but just something I
noticed while reading this related to what's going on in patch 4.

Yeah, I mostly moved the code without much thought into it. This can be
cleaned up. Specifically because we can avoid all the memory allocation
and directly return the string as a 'const char *' as Junio mentioned.

Also, s/unkown/unknown/, but that is present in the pre-image. I hope we
don't need to retain it for bug-for-bug plumbing compatibility. :)

Thanks, will change.

(I guess the dead store of "" was present in the original, too, for that
matter).

-Peff

PS Sorry for all the nit-picky comments. I was just going down the
   Coverity rabbit hole and didn't really review the rest of the series.
   But I wanted to say that the numbers you are seeing are very cool!

I think all these points have good value. I'm happy to address them :)