Re: [PATCH v2 5/7] fetch: refactor fetch refs to be more extendable

On Wed, Aug 25, 2021 at 10:19:27AM -0400, Derrick Stolee wrote:

On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:

quoted

Refactor `fetch_refs()` code to make it more extendable by explicitly
handling error cases. The refactored code should behave the same.

[snip]

quoted

-	if (!ret)
-		/*
-		 * Keep the new pack's ".keep" file around to allow the caller
-		 * time to update refs to reference the new objects.
-		 */
-		return 0;
-	transport_unlock_pack(transport);
-	return ret;
+
+	/*
+	 * Keep the new pack's ".keep" file around to allow the caller
+	 * time to update refs to reference the new objects.
+	 */
+	return 0;

And it happens that 'ret' is zero here. Should we keep returning 'ret'
or perhaps add an "assert(!ret);" before the return? The assert()
doesn't do much, but at minimum would serve as an extra indicator to
anyone working in this method in the future.

The assert isn't really needed: in the subsequent patch, we always
unlock the packfile on exit.

Patrick