[5/6] fetch: refactor fetch refs to be more extendable

Commit Message

Patrick Steinhardt Aug. 20, 2021, 10:08 a.m. UTC

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

Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 builtin/fetch.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

Comments

Derrick Stolee Aug. 20, 2021, 2:41 p.m. UTC | #1

On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:
> Refactor `fetch_refs()` code to make it more extendable by explicitly
> handling error cases. The refactored code should behave the same.

It took unrolling this diff to understand that this code behaves the
same, and it's because of the previous code using "if (!ret) return 0;"
to handle two possible ways that 'ret' could become zero.

I agree that the new code makes it clear that we can leave early after
a successful call to check_exist_and_connected() and again after a
successful call to transport_fetch_refs().

Thanks
-Stolee

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 76ce73ccf5..20fcfe0f45 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1284,20 +1284,29 @@  static int check_exist_and_connected(struct ref *ref_map)
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
 {
-	int ret = check_exist_and_connected(ref_map);
-	if (ret) {
-		trace2_region_enter("fetch", "fetch_refs", the_repository);
-		ret = transport_fetch_refs(transport, ref_map);
-		trace2_region_leave("fetch", "fetch_refs", the_repository);
-	}
+	int ret;
+
+	/*
+	 * We don't need to perform a fetch in case we can already satisfy all
+	 * refs.
+	 */
+	ret = check_exist_and_connected(ref_map);
 	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;
+
+	trace2_region_enter("fetch", "fetch_refs", the_repository);
+	ret = transport_fetch_refs(transport, ref_map);
+	trace2_region_leave("fetch", "fetch_refs", the_repository);
+	if (ret) {
+		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;
 }
 
 /* Update local refs based on the ref values fetched from a remote */