Re: [PATCH v2 1/2] transport: remove unnecessary indenting in transport_push()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 20, 2022, 11:24 UTC
Message-ID: <220520.86y1yw1lxj.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220520103507.1717236-2-frantisek@hrbata.com>

On Fri, May 20 2022, Frantisek Hrbata wrote:

> Remove the big indented block for push_refs() check in transport vtable
> and let's just return error immediately. Hopefully this makes the code
> more readable.

s/push_refs/transport_push/, push_refs is the name of the field in the callback (and there's more than just this function).

This looks good to me....

> Is there a reason to return 1 instead of -1 when push_refs() is not
> set in transport vtable? Looking at the code I think it might return
> -1 also and make it more consistent.

No, looking at it (I tried renaming the function) the only user is builtin/push.c, which we can easily see doesn't care about the 1 v.s. -1 here.

Perhaps it's worthwhile to add this in-between the two patches you have:
	
	diff --git a/transport.c b/transport.c
	index 0b9c5a427d7..5348fac36ef 100644
	--- a/transport.c
	+++ b/transport.c
	@@ -1283,22 +1283,23 @@ int transport_push(struct repository *r,
	 	int quiet = (transport->verbose < 0);
	 	int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
	 	int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
	-	int push_ret, ret, err;
	+	int push_ret, err;
	+	int ret = -1;
	 	struct transport_ls_refs_options transport_options =
	 		TRANSPORT_LS_REFS_OPTIONS_INIT;
	 
	 	*reject_reasons = 0;
	 
	 	if (transport_color_config() < 0)
	-		return -1;
	+		goto done;
	 
	 	if (!transport->vtable->push_refs)
	-		return 1;
	+		goto done;
	 
	 	local_refs = get_local_heads();
	 
	 	if (check_push_refs(local_refs, rs) < 0)
	-		return -1;
	+		goto done;
	 
	 	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
	 
	@@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,
	 		match_flags |= MATCH_REFS_FOLLOW_TAGS;
	 
	 	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
	-		return -1;
	+		goto done;
	 
	 	if (transport->smart_options &&
	 	    transport->smart_options->cas &&
	@@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,
	 
	 	if (!(flags & TRANSPORT_PUSH_NO_HOOK))
	 		if (run_pre_push_hook(transport, remote_refs))
	-			return -1;
	+			goto done;
	 
	 	if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
	 		      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
	@@ -1417,6 +1418,7 @@ int transport_push(struct repository *r,
	 	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
	 		fprintf(stderr, "Everything up-to-date\n");
	 
	+done:
	 	return ret;
	 }

Which would make your new 3/3 truly trivial, i.e. just adding the free_refs() for the two.

*Maybe* (but probably not) it would then make sense to do this as that 3/3:
	
	diff --git a/transport.c b/transport.c
	index 5348fac36ef..d4952bf5f6a 100644
	--- a/transport.c
	+++ b/transport.c
	@@ -1291,15 +1291,17 @@ int transport_push(struct repository *r,
	 	*reject_reasons = 0;
	 
	 	if (transport_color_config() < 0)
	-		goto done;
	+		return ret;
	 
	 	if (!transport->vtable->push_refs)
	-		goto done;
	+		return ret;
	 
	 	local_refs = get_local_heads();
	 
	-	if (check_push_refs(local_refs, rs) < 0)
	+	if (check_push_refs(local_refs, rs) < 0) {
	+		remote_refs = NULL;
	 		goto done;
	+	}
	 
	 	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
	 
	@@ -1419,6 +1421,8 @@ int transport_push(struct repository *r,
	 		fprintf(stderr, "Everything up-to-date\n");
	 
	 done:
	+	free_refs(local_refs);
	+	free_refs(remote_refs);
	 	return ret;
	 }

I.e. entirely skip the NULL assignment for the two, which helps the compiler catch if we don't init it before the later goto's, but because of that we'd need to "return" instead of "goto done" for the early ones, and set it for the check_push_refs() failure case.

So nah, probably best just to keep it as you have it, i.e. always "goto done".