[PATCH] transport: free local and remote refs in transport_push()

[PATCH] transport: free local and remote refs in transport_push()

[Frantisek Hrbata]

Fix memory leaks in transport_push(), where remote_refs and local_refs
are never freed. While at it, remove the unnecessary indenting and make
the code hopefully more readable.

116 bytes in 1 blocks are definitely lost in loss record 56 of 103
   at 0x484486F: malloc (vg_replace_malloc.c:381)
   by 0x4938D7E: strdup (strdup.c:42)
   by 0x628418: xstrdup (wrapper.c:39)
   by 0x4FD454: process_capabilities (connect.c:232)
   by 0x4FD454: get_remote_heads (connect.c:354)
   by 0x610A38: handshake (transport.c:333)
   by 0x612B02: transport_push (transport.c:1302)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)
   by 0x404F17: main (common-main.c:56)

5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103
   at 0x4849464: calloc (vg_replace_malloc.c:1328)
   by 0x628705: xcalloc (wrapper.c:150)
   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)
   by 0x5C232A: alloc_ref (remote.c:983)
   by 0x5C232A: one_local_ref (remote.c:2299)
   by 0x5C232A: one_local_ref (remote.c:2289)
   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)
   by 0x5B4C4F: do_for_each_ref (refs.c:1486)
   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)
   by 0x5B4C4F: for_each_ref (refs.c:1497)
   by 0x5C6ADF: get_local_heads (remote.c:2310)
   by 0x612A85: transport_push (transport.c:1286)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)

Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
For remote_refs it might be worth to leave the freeing to
transport_disconnect() and introduce helper similar to
transport_get_remote_refs().

 transport.c | 254 +++++++++++++++++++++++++++-------------------------
 1 file changed, 130 insertions(+), 124 deletions(-)

diff --git a/transport.c b/transport.c
index 3d64a43ab3..4c5d9db7f2 100644
--- a/transport.c
+++ b/transport.c
@@ -1276,146 +1276,152 @@ int transport_push(struct repository *r,
 		   struct refspec *rs, int flags,
 		   unsigned int *reject_reasons)
 {
+	struct ref *remote_refs = NULL;
+	struct ref *local_refs = NULL;
+	int match_flags = MATCH_REFS_NONE;
+	int verbose = (transport->verbose > 0);
+	int quiet = (transport->verbose < 0);
+	int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
+	int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
+	int push_ret, ret, err;
+	struct transport_ls_refs_options transport_options =
+		TRANSPORT_LS_REFS_OPTIONS_INIT;
+
 	*reject_reasons = 0;
 
 	if (transport_color_config() < 0)
 		return -1;
 
-	if (transport->vtable->push_refs) {
-		struct ref *remote_refs;
-		struct ref *local_refs = get_local_heads();
-		int match_flags = MATCH_REFS_NONE;
-		int verbose = (transport->verbose > 0);
-		int quiet = (transport->verbose < 0);
-		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
-		int push_ret, ret, err;
-		struct transport_ls_refs_options transport_options =
-			TRANSPORT_LS_REFS_OPTIONS_INIT;
-
-		if (check_push_refs(local_refs, rs) < 0)
-			return -1;
-
-		refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
-
-		trace2_region_enter("transport_push", "get_refs_list", r);
-		remote_refs = transport->vtable->get_refs_list(transport, 1,
-							       &transport_options);
-		trace2_region_leave("transport_push", "get_refs_list", r);
-
-		transport_ls_refs_options_release(&transport_options);
-
-		if (flags & TRANSPORT_PUSH_ALL)
-			match_flags |= MATCH_REFS_ALL;
-		if (flags & TRANSPORT_PUSH_MIRROR)
-			match_flags |= MATCH_REFS_MIRROR;
-		if (flags & TRANSPORT_PUSH_PRUNE)
-			match_flags |= MATCH_REFS_PRUNE;
-		if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
-			match_flags |= MATCH_REFS_FOLLOW_TAGS;
-
-		if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
-			return -1;
-
-		if (transport->smart_options &&
-		    transport->smart_options->cas &&
-		    !is_empty_cas(transport->smart_options->cas))
-			apply_push_cas(transport->smart_options->cas,
-				       transport->remote, remote_refs);
-
-		set_ref_status_for_push(remote_refs,
-			flags & TRANSPORT_PUSH_MIRROR,
-			flags & TRANSPORT_PUSH_FORCE);
-
-		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
-			if (run_pre_push_hook(transport, remote_refs))
-				return -1;
+	if (!transport->vtable->push_refs)
+		return 1;
 
-		if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-			      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		    !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "push_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (!push_unpushed_submodules(r,
-						      &commits,
-						      transport->remote,
-						      rs,
-						      transport->push_options,
-						      pretend)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "push_submodules", r);
-				die(_("failed to push all needed submodules"));
-			}
+	ret = -1;
+	local_refs = get_local_heads();
+
+	if (check_push_refs(local_refs, rs) < 0)
+		goto done;
+
+	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
+
+	trace2_region_enter("transport_push", "get_refs_list", r);
+	remote_refs = transport->vtable->get_refs_list(transport, 1,
+						       &transport_options);
+	trace2_region_leave("transport_push", "get_refs_list", r);
+
+	transport_ls_refs_options_release(&transport_options);
+
+	if (flags & TRANSPORT_PUSH_ALL)
+		match_flags |= MATCH_REFS_ALL;
+	if (flags & TRANSPORT_PUSH_MIRROR)
+		match_flags |= MATCH_REFS_MIRROR;
+	if (flags & TRANSPORT_PUSH_PRUNE)
+		match_flags |= MATCH_REFS_PRUNE;
+	if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
+		match_flags |= MATCH_REFS_FOLLOW_TAGS;
+
+	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
+		goto done;
+
+	if (transport->smart_options &&
+	    transport->smart_options->cas &&
+	    !is_empty_cas(transport->smart_options->cas))
+		apply_push_cas(transport->smart_options->cas,
+			       transport->remote, remote_refs);
+
+	set_ref_status_for_push(remote_refs,
+		flags & TRANSPORT_PUSH_MIRROR,
+		flags & TRANSPORT_PUSH_FORCE);
+
+	if (!(flags & TRANSPORT_PUSH_NO_HOOK))
+		if (run_pre_push_hook(transport, remote_refs))
+			goto done;
+
+	if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+		      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	    !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "push_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (!push_unpushed_submodules(r,
+					      &commits,
+					      transport->remote,
+					      rs,
+					      transport->push_options,
+					      pretend)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "push_submodules", r);
+			die(_("failed to push all needed submodules"));
 		}
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "push_submodules", r);
+	}
 
-		if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
-		     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-				TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		      !pretend)) && !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct string_list needs_pushing = STRING_LIST_INIT_DUP;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "check_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (find_unpushed_submodules(r,
-						     &commits,
-						     transport->remote->name,
-						     &needs_pushing)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "check_submodules", r);
-				die_with_unpushed_submodules(&needs_pushing);
-			}
-			string_list_clear(&needs_pushing, 0);
+	if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
+	     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+			TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	      !pretend)) && !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "check_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (find_unpushed_submodules(r,
+					     &commits,
+					     transport->remote->name,
+					     &needs_pushing)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "check_submodules", r);
+			die_with_unpushed_submodules(&needs_pushing);
 		}
+		string_list_clear(&needs_pushing, 0);
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "check_submodules", r);
+	}
 
-		if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
-			trace2_region_enter("transport_push", "push_refs", r);
-			push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
-			trace2_region_leave("transport_push", "push_refs", r);
-		} else
-			push_ret = 0;
-		err = push_had_errors(remote_refs);
-		ret = push_ret | err;
-
-		if (!quiet || err)
-			transport_print_push_status(transport->url, remote_refs,
-					verbose | porcelain, porcelain,
-					reject_reasons);
-
-		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
-			set_upstreams(transport, remote_refs, pretend);
-
-		if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
-			       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
-			struct ref *ref;
-			for (ref = remote_refs; ref; ref = ref->next)
-				transport_update_tracking_ref(transport->remote, ref, verbose);
-		}
+	if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
+		trace2_region_enter("transport_push", "push_refs", r);
+		push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
+		trace2_region_leave("transport_push", "push_refs", r);
+	} else
+		push_ret = 0;
+	err = push_had_errors(remote_refs);
+	ret = push_ret | err;
+
+	if (!quiet || err)
+		transport_print_push_status(transport->url, remote_refs,
+				verbose | porcelain, porcelain,
+				reject_reasons);
+
+	if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
+		set_upstreams(transport, remote_refs, pretend);
+
+	if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
+		       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
+		struct ref *ref;
+		for (ref = remote_refs; ref; ref = ref->next)
+			transport_update_tracking_ref(transport->remote, ref, verbose);
+	}
 
-		if (porcelain && !push_ret)
-			puts("Done");
-		else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+	if (porcelain && !push_ret)
+		puts("Done");
+	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
+		fprintf(stderr, "Everything up-to-date\n");
 
-		return ret;
-	}
-	return 1;
+done:
+	free_refs(local_refs);
+	free_refs(remote_refs);
+	return ret;
 }
 
 const struct ref *transport_get_remote_refs(struct transport *transport,

base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
-- 
2.35.1

Re: [PATCH] transport: free local and remote refs in transport_push()

[Ævar Arnfjörð Bjarmason]

On Fri, May 20 2022, Frantisek Hrbata wrote:

> Fix memory leaks in transport_push(), where remote_refs and local_refs
> are never freed. While at it, remove the unnecessary indenting and make
> the code hopefully more readable.

If at all possible it would be very nice to split up such re-indenting /
formatting into another commit, so we can see what the "real" code
changes are.

Re: [PATCH] transport: free local and remote refs in transport_push()

[Frantisek Hrbata]

Hello,

On Fri, May 20, 2022 at 10:40:33AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, May 20 2022, Frantisek Hrbata wrote:
> 
> > Fix memory leaks in transport_push(), where remote_refs and local_refs
> > are never freed. While at it, remove the unnecessary indenting and make
> > the code hopefully more readable.
> 
> If at all possible it would be very nice to split up such re-indenting /
> formatting into another commit, so we can see what the "real" code
> changes are.

of course, I will split it into two patch series and post v2. First with the
indenting adjustment only and second with mem leak fixes.

Thank you


-- 
Frantisek Hrbata

[PATCH v2 0/2] fix memory leaks in transport_push()

[Frantisek Hrbata]

Fix memory leaks in transport_push(), where remote_refs and local_refs
are never freed. While at it, remove the unnecessary indenting and make
the code hopefully more readable.

Changes since v1:
 * Slit into series of two patches. The first one just changes
   indenting in transport_push(). Second one adds the fix for
   the local_refs and remote_refs memory leaks.

 * The resulting trees are the same, there is no code change.

Frantisek Hrbata (2):
  transport: remove unnecessary indenting in transport_push()
  transport: free local and remote refs in transport_push()

 transport.c | 254 +++++++++++++++++++++++++++-------------------------
 1 file changed, 130 insertions(+), 124 deletions(-)

Range-diff against v1:
1:  fb8b90d247 ! 1:  daf042cd2e transport: free local and remote refs in transport_push()
    @@ Metadata
     Author: Frantisek Hrbata <frantisek@hrbata.com>
     
      ## Commit message ##
    -    transport: free local and remote refs in transport_push()
    +    transport: remove unnecessary indenting in transport_push()
     
    -    Fix memory leaks in transport_push(), where remote_refs and local_refs
    -    are never freed. While at it, remove the unnecessary indenting and make
    -    the code hopefully more readable.
    -
    -    116 bytes in 1 blocks are definitely lost in loss record 56 of 103
    -       at 0x484486F: malloc (vg_replace_malloc.c:381)
    -       by 0x4938D7E: strdup (strdup.c:42)
    -       by 0x628418: xstrdup (wrapper.c:39)
    -       by 0x4FD454: process_capabilities (connect.c:232)
    -       by 0x4FD454: get_remote_heads (connect.c:354)
    -       by 0x610A38: handshake (transport.c:333)
    -       by 0x612B02: transport_push (transport.c:1302)
    -       by 0x4803D6: push_with_options (push.c:357)
    -       by 0x4811D6: do_push (push.c:414)
    -       by 0x4811D6: cmd_push (push.c:650)
    -       by 0x405210: run_builtin (git.c:465)
    -       by 0x405210: handle_builtin (git.c:719)
    -       by 0x406363: run_argv (git.c:786)
    -       by 0x406363: cmd_main (git.c:917)
    -       by 0x404F17: main (common-main.c:56)
    -
    -    5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103
    -       at 0x4849464: calloc (vg_replace_malloc.c:1328)
    -       by 0x628705: xcalloc (wrapper.c:150)
    -       by 0x5C216D: alloc_ref_with_prefix (remote.c:975)
    -       by 0x5C232A: alloc_ref (remote.c:983)
    -       by 0x5C232A: one_local_ref (remote.c:2299)
    -       by 0x5C232A: one_local_ref (remote.c:2289)
    -       by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)
    -       by 0x5B4C4F: do_for_each_ref (refs.c:1486)
    -       by 0x5B4C4F: refs_for_each_ref (refs.c:1492)
    -       by 0x5B4C4F: for_each_ref (refs.c:1497)
    -       by 0x5C6ADF: get_local_heads (remote.c:2310)
    -       by 0x612A85: transport_push (transport.c:1286)
    -       by 0x4803D6: push_with_options (push.c:357)
    -       by 0x4811D6: do_push (push.c:414)
    -       by 0x4811D6: cmd_push (push.c:650)
    -       by 0x405210: run_builtin (git.c:465)
    -       by 0x405210: handle_builtin (git.c:719)
    -       by 0x406363: run_argv (git.c:786)
    -       by 0x406363: cmd_main (git.c:917)
    +    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.
     
         Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
     
    @@ transport.c: int transport_push(struct repository *r,
      		   struct refspec *rs, int flags,
      		   unsigned int *reject_reasons)
      {
    -+	struct ref *remote_refs = NULL;
    -+	struct ref *local_refs = NULL;
    ++	struct ref *remote_refs;
    ++	struct ref *local_refs;
     +	int match_flags = MATCH_REFS_NONE;
     +	int verbose = (transport->verbose > 0);
     +	int quiet = (transport->verbose < 0);
    @@ transport.c: int transport_push(struct repository *r,
     -
     -		if (check_push_refs(local_refs, rs) < 0)
     -			return -1;
    --
    ++	if (!transport->vtable->push_refs)
    ++		return 1;
    + 
     -		refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
    --
    ++	local_refs = get_local_heads();
    + 
     -		trace2_region_enter("transport_push", "get_refs_list", r);
     -		remote_refs = transport->vtable->get_refs_list(transport, 1,
     -							       &transport_options);
     -		trace2_region_leave("transport_push", "get_refs_list", r);
    --
    ++	if (check_push_refs(local_refs, rs) < 0)
    ++		return -1;
    + 
     -		transport_ls_refs_options_release(&transport_options);
    --
    ++	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
    + 
     -		if (flags & TRANSPORT_PUSH_ALL)
     -			match_flags |= MATCH_REFS_ALL;
     -		if (flags & TRANSPORT_PUSH_MIRROR)
    @@ transport.c: int transport_push(struct repository *r,
     -			match_flags |= MATCH_REFS_PRUNE;
     -		if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
     -			match_flags |= MATCH_REFS_FOLLOW_TAGS;
    --
    ++	trace2_region_enter("transport_push", "get_refs_list", r);
    ++	remote_refs = transport->vtable->get_refs_list(transport, 1,
    ++						       &transport_options);
    ++	trace2_region_leave("transport_push", "get_refs_list", r);
    + 
     -		if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
     -			return -1;
    --
    ++	transport_ls_refs_options_release(&transport_options);
    + 
     -		if (transport->smart_options &&
     -		    transport->smart_options->cas &&
     -		    !is_empty_cas(transport->smart_options->cas))
     -			apply_push_cas(transport->smart_options->cas,
     -				       transport->remote, remote_refs);
    --
    ++	if (flags & TRANSPORT_PUSH_ALL)
    ++		match_flags |= MATCH_REFS_ALL;
    ++	if (flags & TRANSPORT_PUSH_MIRROR)
    ++		match_flags |= MATCH_REFS_MIRROR;
    ++	if (flags & TRANSPORT_PUSH_PRUNE)
    ++		match_flags |= MATCH_REFS_PRUNE;
    ++	if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
    ++		match_flags |= MATCH_REFS_FOLLOW_TAGS;
    + 
     -		set_ref_status_for_push(remote_refs,
     -			flags & TRANSPORT_PUSH_MIRROR,
     -			flags & TRANSPORT_PUSH_FORCE);
    --
    ++	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
    ++		return -1;
    + 
     -		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
     -			if (run_pre_push_hook(transport, remote_refs))
     -				return -1;
    -+	if (!transport->vtable->push_refs)
    -+		return 1;
    ++	if (transport->smart_options &&
    ++	    transport->smart_options->cas &&
    ++	    !is_empty_cas(transport->smart_options->cas))
    ++		apply_push_cas(transport->smart_options->cas,
    ++			       transport->remote, remote_refs);
      
     -		if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
     -			      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
    @@ transport.c: int transport_push(struct repository *r,
     -				trace2_region_leave("transport_push", "push_submodules", r);
     -				die(_("failed to push all needed submodules"));
     -			}
    -+	ret = -1;
    -+	local_refs = get_local_heads();
    -+
    -+	if (check_push_refs(local_refs, rs) < 0)
    -+		goto done;
    -+
    -+	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
    -+
    -+	trace2_region_enter("transport_push", "get_refs_list", r);
    -+	remote_refs = transport->vtable->get_refs_list(transport, 1,
    -+						       &transport_options);
    -+	trace2_region_leave("transport_push", "get_refs_list", r);
    -+
    -+	transport_ls_refs_options_release(&transport_options);
    -+
    -+	if (flags & TRANSPORT_PUSH_ALL)
    -+		match_flags |= MATCH_REFS_ALL;
    -+	if (flags & TRANSPORT_PUSH_MIRROR)
    -+		match_flags |= MATCH_REFS_MIRROR;
    -+	if (flags & TRANSPORT_PUSH_PRUNE)
    -+		match_flags |= MATCH_REFS_PRUNE;
    -+	if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
    -+		match_flags |= MATCH_REFS_FOLLOW_TAGS;
    -+
    -+	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
    -+		goto done;
    -+
    -+	if (transport->smart_options &&
    -+	    transport->smart_options->cas &&
    -+	    !is_empty_cas(transport->smart_options->cas))
    -+		apply_push_cas(transport->smart_options->cas,
    -+			       transport->remote, remote_refs);
    -+
     +	set_ref_status_for_push(remote_refs,
     +		flags & TRANSPORT_PUSH_MIRROR,
     +		flags & TRANSPORT_PUSH_FORCE);
     +
     +	if (!(flags & TRANSPORT_PUSH_NO_HOOK))
     +		if (run_pre_push_hook(transport, remote_refs))
    -+			goto done;
    ++			return -1;
     +
     +	if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
     +		      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
    @@ transport.c: int transport_push(struct repository *r,
     -		return ret;
     -	}
     -	return 1;
    -+done:
    -+	free_refs(local_refs);
    -+	free_refs(remote_refs);
     +	return ret;
      }
      
-:  ---------- > 2:  9d25176b92 transport: free local and remote refs in transport_push()

base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
-- 
2.35.1

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

[Frantisek Hrbata]

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.

Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
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.

 transport.c | 234 ++++++++++++++++++++++++++--------------------------
 1 file changed, 118 insertions(+), 116 deletions(-)

diff --git a/transport.c b/transport.c
index 3d64a43ab3..0b9c5a427d 100644
--- a/transport.c
+++ b/transport.c
@@ -1276,146 +1276,148 @@ int transport_push(struct repository *r,
 		   struct refspec *rs, int flags,
 		   unsigned int *reject_reasons)
 {
+	struct ref *remote_refs;
+	struct ref *local_refs;
+	int match_flags = MATCH_REFS_NONE;
+	int verbose = (transport->verbose > 0);
+	int quiet = (transport->verbose < 0);
+	int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
+	int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
+	int push_ret, ret, err;
+	struct transport_ls_refs_options transport_options =
+		TRANSPORT_LS_REFS_OPTIONS_INIT;
+
 	*reject_reasons = 0;
 
 	if (transport_color_config() < 0)
 		return -1;
 
-	if (transport->vtable->push_refs) {
-		struct ref *remote_refs;
-		struct ref *local_refs = get_local_heads();
-		int match_flags = MATCH_REFS_NONE;
-		int verbose = (transport->verbose > 0);
-		int quiet = (transport->verbose < 0);
-		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
-		int push_ret, ret, err;
-		struct transport_ls_refs_options transport_options =
-			TRANSPORT_LS_REFS_OPTIONS_INIT;
-
-		if (check_push_refs(local_refs, rs) < 0)
-			return -1;
+	if (!transport->vtable->push_refs)
+		return 1;
 
-		refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
+	local_refs = get_local_heads();
 
-		trace2_region_enter("transport_push", "get_refs_list", r);
-		remote_refs = transport->vtable->get_refs_list(transport, 1,
-							       &transport_options);
-		trace2_region_leave("transport_push", "get_refs_list", r);
+	if (check_push_refs(local_refs, rs) < 0)
+		return -1;
 
-		transport_ls_refs_options_release(&transport_options);
+	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
 
-		if (flags & TRANSPORT_PUSH_ALL)
-			match_flags |= MATCH_REFS_ALL;
-		if (flags & TRANSPORT_PUSH_MIRROR)
-			match_flags |= MATCH_REFS_MIRROR;
-		if (flags & TRANSPORT_PUSH_PRUNE)
-			match_flags |= MATCH_REFS_PRUNE;
-		if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
-			match_flags |= MATCH_REFS_FOLLOW_TAGS;
+	trace2_region_enter("transport_push", "get_refs_list", r);
+	remote_refs = transport->vtable->get_refs_list(transport, 1,
+						       &transport_options);
+	trace2_region_leave("transport_push", "get_refs_list", r);
 
-		if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
-			return -1;
+	transport_ls_refs_options_release(&transport_options);
 
-		if (transport->smart_options &&
-		    transport->smart_options->cas &&
-		    !is_empty_cas(transport->smart_options->cas))
-			apply_push_cas(transport->smart_options->cas,
-				       transport->remote, remote_refs);
+	if (flags & TRANSPORT_PUSH_ALL)
+		match_flags |= MATCH_REFS_ALL;
+	if (flags & TRANSPORT_PUSH_MIRROR)
+		match_flags |= MATCH_REFS_MIRROR;
+	if (flags & TRANSPORT_PUSH_PRUNE)
+		match_flags |= MATCH_REFS_PRUNE;
+	if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
+		match_flags |= MATCH_REFS_FOLLOW_TAGS;
 
-		set_ref_status_for_push(remote_refs,
-			flags & TRANSPORT_PUSH_MIRROR,
-			flags & TRANSPORT_PUSH_FORCE);
+	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
+		return -1;
 
-		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
-			if (run_pre_push_hook(transport, remote_refs))
-				return -1;
+	if (transport->smart_options &&
+	    transport->smart_options->cas &&
+	    !is_empty_cas(transport->smart_options->cas))
+		apply_push_cas(transport->smart_options->cas,
+			       transport->remote, remote_refs);
 
-		if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-			      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		    !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "push_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (!push_unpushed_submodules(r,
-						      &commits,
-						      transport->remote,
-						      rs,
-						      transport->push_options,
-						      pretend)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "push_submodules", r);
-				die(_("failed to push all needed submodules"));
-			}
+	set_ref_status_for_push(remote_refs,
+		flags & TRANSPORT_PUSH_MIRROR,
+		flags & TRANSPORT_PUSH_FORCE);
+
+	if (!(flags & TRANSPORT_PUSH_NO_HOOK))
+		if (run_pre_push_hook(transport, remote_refs))
+			return -1;
+
+	if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+		      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	    !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "push_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (!push_unpushed_submodules(r,
+					      &commits,
+					      transport->remote,
+					      rs,
+					      transport->push_options,
+					      pretend)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "push_submodules", r);
+			die(_("failed to push all needed submodules"));
 		}
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "push_submodules", r);
+	}
 
-		if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
-		     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-				TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		      !pretend)) && !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct string_list needs_pushing = STRING_LIST_INIT_DUP;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "check_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (find_unpushed_submodules(r,
-						     &commits,
-						     transport->remote->name,
-						     &needs_pushing)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "check_submodules", r);
-				die_with_unpushed_submodules(&needs_pushing);
-			}
-			string_list_clear(&needs_pushing, 0);
+	if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
+	     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+			TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	      !pretend)) && !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "check_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (find_unpushed_submodules(r,
+					     &commits,
+					     transport->remote->name,
+					     &needs_pushing)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "check_submodules", r);
+			die_with_unpushed_submodules(&needs_pushing);
 		}
+		string_list_clear(&needs_pushing, 0);
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "check_submodules", r);
+	}
 
-		if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
-			trace2_region_enter("transport_push", "push_refs", r);
-			push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
-			trace2_region_leave("transport_push", "push_refs", r);
-		} else
-			push_ret = 0;
-		err = push_had_errors(remote_refs);
-		ret = push_ret | err;
-
-		if (!quiet || err)
-			transport_print_push_status(transport->url, remote_refs,
-					verbose | porcelain, porcelain,
-					reject_reasons);
-
-		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
-			set_upstreams(transport, remote_refs, pretend);
-
-		if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
-			       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
-			struct ref *ref;
-			for (ref = remote_refs; ref; ref = ref->next)
-				transport_update_tracking_ref(transport->remote, ref, verbose);
-		}
+	if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
+		trace2_region_enter("transport_push", "push_refs", r);
+		push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
+		trace2_region_leave("transport_push", "push_refs", r);
+	} else
+		push_ret = 0;
+	err = push_had_errors(remote_refs);
+	ret = push_ret | err;
+
+	if (!quiet || err)
+		transport_print_push_status(transport->url, remote_refs,
+				verbose | porcelain, porcelain,
+				reject_reasons);
+
+	if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
+		set_upstreams(transport, remote_refs, pretend);
+
+	if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
+		       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
+		struct ref *ref;
+		for (ref = remote_refs; ref; ref = ref->next)
+			transport_update_tracking_ref(transport->remote, ref, verbose);
+	}
 
-		if (porcelain && !push_ret)
-			puts("Done");
-		else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+	if (porcelain && !push_ret)
+		puts("Done");
+	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
+		fprintf(stderr, "Everything up-to-date\n");
 
-		return ret;
-	}
-	return 1;
+	return ret;
 }
 
 const struct ref *transport_get_remote_refs(struct transport *transport,
-- 
2.35.1

[PATCH v2 2/2] transport: free local and remote refs in transport_push()

[Frantisek Hrbata]

Fix memory leaks in transport_push(), where remote_refs and local_refs
are never freed.

116 bytes in 1 blocks are definitely lost in loss record 56 of 103
   at 0x484486F: malloc (vg_replace_malloc.c:381)
   by 0x4938D7E: strdup (strdup.c:42)
   by 0x628418: xstrdup (wrapper.c:39)
   by 0x4FD454: process_capabilities (connect.c:232)
   by 0x4FD454: get_remote_heads (connect.c:354)
   by 0x610A38: handshake (transport.c:333)
   by 0x612B02: transport_push (transport.c:1302)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)
   by 0x404F17: main (common-main.c:56)

5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103
   at 0x4849464: calloc (vg_replace_malloc.c:1328)
   by 0x628705: xcalloc (wrapper.c:150)
   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)
   by 0x5C232A: alloc_ref (remote.c:983)
   by 0x5C232A: one_local_ref (remote.c:2299)
   by 0x5C232A: one_local_ref (remote.c:2289)
   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)
   by 0x5B4C4F: do_for_each_ref (refs.c:1486)
   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)
   by 0x5B4C4F: for_each_ref (refs.c:1497)
   by 0x5C6ADF: get_local_heads (remote.c:2310)
   by 0x612A85: transport_push (transport.c:1286)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)

Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
For remote_refs it might be worth to leave the freeing to
transport_disconnect() and introduce helper similar to
transport_get_remote_refs().

 transport.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/transport.c b/transport.c
index 0b9c5a427d..4c5d9db7f2 100644
--- a/transport.c
+++ b/transport.c
@@ -1276,8 +1276,8 @@ int transport_push(struct repository *r,
 		   struct refspec *rs, int flags,
 		   unsigned int *reject_reasons)
 {
-	struct ref *remote_refs;
-	struct ref *local_refs;
+	struct ref *remote_refs = NULL;
+	struct ref *local_refs = NULL;
 	int match_flags = MATCH_REFS_NONE;
 	int verbose = (transport->verbose > 0);
 	int quiet = (transport->verbose < 0);
@@ -1295,10 +1295,11 @@ int transport_push(struct repository *r,
 	if (!transport->vtable->push_refs)
 		return 1;
 
+	ret = -1;
 	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,9 @@ int transport_push(struct repository *r,
 	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
 		fprintf(stderr, "Everything up-to-date\n");
 
+done:
+	free_refs(local_refs);
+	free_refs(remote_refs);
 	return ret;
 }
 
-- 
2.35.1

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

[Ævar Arnfjörð Bjarmason]

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".

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

[Frantisek Hrbata]

On Fri, May 20, 2022 at 01:24:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 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).

uf, I will fix that, thank you for noticing.

> 
> 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.

I was thinking about this too. Just use the done label as a single exit point.
Let me incorporate your suggestions in v3. I will add a 2/3 patch e.g.
"unify return values and exit point for transport_push()"

> 
> *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".

I agree. Let me prepare v3 and let's see what happens :)

Thank you for your input!

-- 
Frantisek Hrbata

[PATCH v3 0/3] fix memory leaks in transport_push()

[Frantisek Hrbata]

Fix memory leaks in transport_push(), where remote_refs and local_refs
are never freed. While at it, remove the unnecessary indenting and make
the code hopefully more readable.

Changes since v2:

 * "transport: remove unnecessary indenting in transport_push()"
   s/push_refs/transport_push/ in commit message as noticed
   by Ævar Arnfjörð Bjarmason

 * "transport: unify return values and exit point from transport_push()"
   Added as suggested by Ævar Arnfjörð Bjarmason. It allows the following
   memory leak fix to be a very simple patch.

 * "transport: free local and remote refs in transport_push()"
   Just free remote_refs and local_refs. The other changes were
   included in the previous patch.

Changes since v1:

 * Slit into series of two patches. The first one just changes
   indenting in transport_push(). Second one adds the fix for
   the local_refs and remote_refs memory leaks.

 * The resulting trees are the same, there is no code change.

Frantisek Hrbata (3):
  transport: remove unnecessary indenting in transport_push()
  transport: unify return values and exit point from transport_push()
  transport: free local and remote refs in transport_push()

 transport.c | 260 +++++++++++++++++++++++++++-------------------------
 1 file changed, 133 insertions(+), 127 deletions(-)

Range-diff against v2:
1:  daf042cd2e ! 1:  d986d8e7a9 transport: remove unnecessary indenting in transport_push()
    @@ Metadata
      ## Commit message ##
         transport: remove unnecessary indenting in transport_push()
     
    -    Remove the big indented block for push_refs() check in transport vtable
    +    Remove the big indented block for transport_push() check in transport vtable
         and let's just return error immediately. Hopefully this makes the code
         more readable.
     
2:  9d25176b92 ! 2:  4a69662b28 transport: free local and remote refs in transport_push()
    @@ Metadata
     Author: Frantisek Hrbata <frantisek@hrbata.com>
     
      ## Commit message ##
    -    transport: free local and remote refs in transport_push()
    +    transport: unify return values and exit point from transport_push()
     
    -    Fix memory leaks in transport_push(), where remote_refs and local_refs
    -    are never freed.
    -
    -    116 bytes in 1 blocks are definitely lost in loss record 56 of 103
    -       at 0x484486F: malloc (vg_replace_malloc.c:381)
    -       by 0x4938D7E: strdup (strdup.c:42)
    -       by 0x628418: xstrdup (wrapper.c:39)
    -       by 0x4FD454: process_capabilities (connect.c:232)
    -       by 0x4FD454: get_remote_heads (connect.c:354)
    -       by 0x610A38: handshake (transport.c:333)
    -       by 0x612B02: transport_push (transport.c:1302)
    -       by 0x4803D6: push_with_options (push.c:357)
    -       by 0x4811D6: do_push (push.c:414)
    -       by 0x4811D6: cmd_push (push.c:650)
    -       by 0x405210: run_builtin (git.c:465)
    -       by 0x405210: handle_builtin (git.c:719)
    -       by 0x406363: run_argv (git.c:786)
    -       by 0x406363: cmd_main (git.c:917)
    -       by 0x404F17: main (common-main.c:56)
    -
    -    5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103
    -       at 0x4849464: calloc (vg_replace_malloc.c:1328)
    -       by 0x628705: xcalloc (wrapper.c:150)
    -       by 0x5C216D: alloc_ref_with_prefix (remote.c:975)
    -       by 0x5C232A: alloc_ref (remote.c:983)
    -       by 0x5C232A: one_local_ref (remote.c:2299)
    -       by 0x5C232A: one_local_ref (remote.c:2289)
    -       by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)
    -       by 0x5B4C4F: do_for_each_ref (refs.c:1486)
    -       by 0x5B4C4F: refs_for_each_ref (refs.c:1492)
    -       by 0x5B4C4F: for_each_ref (refs.c:1497)
    -       by 0x5C6ADF: get_local_heads (remote.c:2310)
    -       by 0x612A85: transport_push (transport.c:1286)
    -       by 0x4803D6: push_with_options (push.c:357)
    -       by 0x4811D6: do_push (push.c:414)
    -       by 0x4811D6: cmd_push (push.c:650)
    -       by 0x405210: run_builtin (git.c:465)
    -       by 0x405210: handle_builtin (git.c:719)
    -       by 0x406363: run_argv (git.c:786)
    -       by 0x406363: cmd_main (git.c:917)
    +    It seems there is no reason to return 1 instead of -1 when push_refs()
    +    is not set in transport vtable. Let's unify the error return values and
    +    use the done label as a single exit point from transport_push().
     
    +    Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
     
      ## transport.c ##
    @@ transport.c: int transport_push(struct repository *r,
      	int match_flags = MATCH_REFS_NONE;
      	int verbose = (transport->verbose > 0);
      	int quiet = (transport->verbose < 0);
    -@@ transport.c: int transport_push(struct repository *r,
    + 	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;
    +-		return 1;
    ++		goto done;
      
    -+	ret = -1;
      	local_refs = get_local_heads();
      
      	if (check_push_refs(local_refs, rs) < 0)
    @@ transport.c: int transport_push(struct repository *r,
      		fprintf(stderr, "Everything up-to-date\n");
      
     +done:
    -+	free_refs(local_refs);
    -+	free_refs(remote_refs);
      	return ret;
      }
      
-:  ---------- > 3:  a4984a4835 transport: free local and remote refs in transport_push()

base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
-- 
2.35.1

[PATCH v3 1/3] transport: remove unnecessary indenting in transport_push()

[Frantisek Hrbata]

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

Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
 transport.c | 234 ++++++++++++++++++++++++++--------------------------
 1 file changed, 118 insertions(+), 116 deletions(-)

diff --git a/transport.c b/transport.c
index 3d64a43ab3..0b9c5a427d 100644
--- a/transport.c
+++ b/transport.c
@@ -1276,146 +1276,148 @@ int transport_push(struct repository *r,
 		   struct refspec *rs, int flags,
 		   unsigned int *reject_reasons)
 {
+	struct ref *remote_refs;
+	struct ref *local_refs;
+	int match_flags = MATCH_REFS_NONE;
+	int verbose = (transport->verbose > 0);
+	int quiet = (transport->verbose < 0);
+	int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
+	int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
+	int push_ret, ret, err;
+	struct transport_ls_refs_options transport_options =
+		TRANSPORT_LS_REFS_OPTIONS_INIT;
+
 	*reject_reasons = 0;
 
 	if (transport_color_config() < 0)
 		return -1;
 
-	if (transport->vtable->push_refs) {
-		struct ref *remote_refs;
-		struct ref *local_refs = get_local_heads();
-		int match_flags = MATCH_REFS_NONE;
-		int verbose = (transport->verbose > 0);
-		int quiet = (transport->verbose < 0);
-		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
-		int push_ret, ret, err;
-		struct transport_ls_refs_options transport_options =
-			TRANSPORT_LS_REFS_OPTIONS_INIT;
-
-		if (check_push_refs(local_refs, rs) < 0)
-			return -1;
+	if (!transport->vtable->push_refs)
+		return 1;
 
-		refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
+	local_refs = get_local_heads();
 
-		trace2_region_enter("transport_push", "get_refs_list", r);
-		remote_refs = transport->vtable->get_refs_list(transport, 1,
-							       &transport_options);
-		trace2_region_leave("transport_push", "get_refs_list", r);
+	if (check_push_refs(local_refs, rs) < 0)
+		return -1;
 
-		transport_ls_refs_options_release(&transport_options);
+	refspec_ref_prefixes(rs, &transport_options.ref_prefixes);
 
-		if (flags & TRANSPORT_PUSH_ALL)
-			match_flags |= MATCH_REFS_ALL;
-		if (flags & TRANSPORT_PUSH_MIRROR)
-			match_flags |= MATCH_REFS_MIRROR;
-		if (flags & TRANSPORT_PUSH_PRUNE)
-			match_flags |= MATCH_REFS_PRUNE;
-		if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
-			match_flags |= MATCH_REFS_FOLLOW_TAGS;
+	trace2_region_enter("transport_push", "get_refs_list", r);
+	remote_refs = transport->vtable->get_refs_list(transport, 1,
+						       &transport_options);
+	trace2_region_leave("transport_push", "get_refs_list", r);
 
-		if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
-			return -1;
+	transport_ls_refs_options_release(&transport_options);
 
-		if (transport->smart_options &&
-		    transport->smart_options->cas &&
-		    !is_empty_cas(transport->smart_options->cas))
-			apply_push_cas(transport->smart_options->cas,
-				       transport->remote, remote_refs);
+	if (flags & TRANSPORT_PUSH_ALL)
+		match_flags |= MATCH_REFS_ALL;
+	if (flags & TRANSPORT_PUSH_MIRROR)
+		match_flags |= MATCH_REFS_MIRROR;
+	if (flags & TRANSPORT_PUSH_PRUNE)
+		match_flags |= MATCH_REFS_PRUNE;
+	if (flags & TRANSPORT_PUSH_FOLLOW_TAGS)
+		match_flags |= MATCH_REFS_FOLLOW_TAGS;
 
-		set_ref_status_for_push(remote_refs,
-			flags & TRANSPORT_PUSH_MIRROR,
-			flags & TRANSPORT_PUSH_FORCE);
+	if (match_push_refs(local_refs, &remote_refs, rs, match_flags))
+		return -1;
 
-		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
-			if (run_pre_push_hook(transport, remote_refs))
-				return -1;
+	if (transport->smart_options &&
+	    transport->smart_options->cas &&
+	    !is_empty_cas(transport->smart_options->cas))
+		apply_push_cas(transport->smart_options->cas,
+			       transport->remote, remote_refs);
 
-		if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-			      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		    !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "push_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (!push_unpushed_submodules(r,
-						      &commits,
-						      transport->remote,
-						      rs,
-						      transport->push_options,
-						      pretend)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "push_submodules", r);
-				die(_("failed to push all needed submodules"));
-			}
+	set_ref_status_for_push(remote_refs,
+		flags & TRANSPORT_PUSH_MIRROR,
+		flags & TRANSPORT_PUSH_FORCE);
+
+	if (!(flags & TRANSPORT_PUSH_NO_HOOK))
+		if (run_pre_push_hook(transport, remote_refs))
+			return -1;
+
+	if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+		      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	    !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "push_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (!push_unpushed_submodules(r,
+					      &commits,
+					      transport->remote,
+					      rs,
+					      transport->push_options,
+					      pretend)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "push_submodules", r);
+			die(_("failed to push all needed submodules"));
 		}
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "push_submodules", r);
+	}
 
-		if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
-		     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
-				TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
-		      !pretend)) && !is_bare_repository()) {
-			struct ref *ref = remote_refs;
-			struct string_list needs_pushing = STRING_LIST_INIT_DUP;
-			struct oid_array commits = OID_ARRAY_INIT;
-
-			trace2_region_enter("transport_push", "check_submodules", r);
-			for (; ref; ref = ref->next)
-				if (!is_null_oid(&ref->new_oid))
-					oid_array_append(&commits,
-							  &ref->new_oid);
-
-			if (find_unpushed_submodules(r,
-						     &commits,
-						     transport->remote->name,
-						     &needs_pushing)) {
-				oid_array_clear(&commits);
-				trace2_region_leave("transport_push", "check_submodules", r);
-				die_with_unpushed_submodules(&needs_pushing);
-			}
-			string_list_clear(&needs_pushing, 0);
+	if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
+	     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+			TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&
+	      !pretend)) && !is_bare_repository()) {
+		struct ref *ref = remote_refs;
+		struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+		struct oid_array commits = OID_ARRAY_INIT;
+
+		trace2_region_enter("transport_push", "check_submodules", r);
+		for (; ref; ref = ref->next)
+			if (!is_null_oid(&ref->new_oid))
+				oid_array_append(&commits,
+						  &ref->new_oid);
+
+		if (find_unpushed_submodules(r,
+					     &commits,
+					     transport->remote->name,
+					     &needs_pushing)) {
 			oid_array_clear(&commits);
 			trace2_region_leave("transport_push", "check_submodules", r);
+			die_with_unpushed_submodules(&needs_pushing);
 		}
+		string_list_clear(&needs_pushing, 0);
+		oid_array_clear(&commits);
+		trace2_region_leave("transport_push", "check_submodules", r);
+	}
 
-		if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
-			trace2_region_enter("transport_push", "push_refs", r);
-			push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
-			trace2_region_leave("transport_push", "push_refs", r);
-		} else
-			push_ret = 0;
-		err = push_had_errors(remote_refs);
-		ret = push_ret | err;
-
-		if (!quiet || err)
-			transport_print_push_status(transport->url, remote_refs,
-					verbose | porcelain, porcelain,
-					reject_reasons);
-
-		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
-			set_upstreams(transport, remote_refs, pretend);
-
-		if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
-			       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
-			struct ref *ref;
-			for (ref = remote_refs; ref; ref = ref->next)
-				transport_update_tracking_ref(transport->remote, ref, verbose);
-		}
+	if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
+		trace2_region_enter("transport_push", "push_refs", r);
+		push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
+		trace2_region_leave("transport_push", "push_refs", r);
+	} else
+		push_ret = 0;
+	err = push_had_errors(remote_refs);
+	ret = push_ret | err;
+
+	if (!quiet || err)
+		transport_print_push_status(transport->url, remote_refs,
+				verbose | porcelain, porcelain,
+				reject_reasons);
+
+	if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
+		set_upstreams(transport, remote_refs, pretend);
+
+	if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
+		       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
+		struct ref *ref;
+		for (ref = remote_refs; ref; ref = ref->next)
+			transport_update_tracking_ref(transport->remote, ref, verbose);
+	}
 
-		if (porcelain && !push_ret)
-			puts("Done");
-		else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+	if (porcelain && !push_ret)
+		puts("Done");
+	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
+		fprintf(stderr, "Everything up-to-date\n");
 
-		return ret;
-	}
-	return 1;
+	return ret;
 }
 
 const struct ref *transport_get_remote_refs(struct transport *transport,
-- 
2.35.1

[PATCH v3 2/3] transport: unify return values and exit point from transport_push()

[Frantisek Hrbata]

It seems there is no reason to return 1 instead of -1 when push_refs()
is not set in transport vtable. Let's unify the error return values and
use the done label as a single exit point from transport_push().

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
 transport.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/transport.c b/transport.c
index 0b9c5a427d..dd7fca51bf 100644
--- a/transport.c
+++ b/transport.c
@@ -1276,29 +1276,30 @@ int transport_push(struct repository *r,
 		   struct refspec *rs, int flags,
 		   unsigned int *reject_reasons)
 {
-	struct ref *remote_refs;
-	struct ref *local_refs;
+	struct ref *remote_refs = NULL;
+	struct ref *local_refs = NULL;
 	int match_flags = MATCH_REFS_NONE;
 	int verbose = (transport->verbose > 0);
 	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;
 }
 
-- 
2.35.1

[PATCH v3 3/3] transport: free local and remote refs in transport_push()

[Frantisek Hrbata]

Fix memory leaks in transport_push(), where remote_refs and local_refs
are never freed.

116 bytes in 1 blocks are definitely lost in loss record 56 of 103
   at 0x484486F: malloc (vg_replace_malloc.c:381)
   by 0x4938D7E: strdup (strdup.c:42)
   by 0x628418: xstrdup (wrapper.c:39)
   by 0x4FD454: process_capabilities (connect.c:232)
   by 0x4FD454: get_remote_heads (connect.c:354)
   by 0x610A38: handshake (transport.c:333)
   by 0x612B02: transport_push (transport.c:1302)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)
   by 0x404F17: main (common-main.c:56)

5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103
   at 0x4849464: calloc (vg_replace_malloc.c:1328)
   by 0x628705: xcalloc (wrapper.c:150)
   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)
   by 0x5C232A: alloc_ref (remote.c:983)
   by 0x5C232A: one_local_ref (remote.c:2299)
   by 0x5C232A: one_local_ref (remote.c:2289)
   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)
   by 0x5B4C4F: do_for_each_ref (refs.c:1486)
   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)
   by 0x5B4C4F: for_each_ref (refs.c:1497)
   by 0x5C6ADF: get_local_heads (remote.c:2310)
   by 0x612A85: transport_push (transport.c:1286)
   by 0x4803D6: push_with_options (push.c:357)
   by 0x4811D6: do_push (push.c:414)
   by 0x4811D6: cmd_push (push.c:650)
   by 0x405210: run_builtin (git.c:465)
   by 0x405210: handle_builtin (git.c:719)
   by 0x406363: run_argv (git.c:786)
   by 0x406363: cmd_main (git.c:917)

Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>
---
For remote_refs it might be worth to leave the freeing to
transport_disconnect() and introduce helper similar to
transport_get_remote_refs().

 transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/transport.c b/transport.c
index dd7fca51bf..110a0d5cc5 100644
--- a/transport.c
+++ b/transport.c
@@ -1419,6 +1419,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;
 }
 
-- 
2.35.1

Re: [PATCH v3 0/3] fix memory leaks in transport_push()

[Josh Steadmon]

On 2022.05.20 14:49, Frantisek Hrbata wrote:
> Fix memory leaks in transport_push(), where remote_refs and local_refs
> are never freed. While at it, remove the unnecessary indenting and make
> the code hopefully more readable.
> 
> Changes since v2:
> 
>  * "transport: remove unnecessary indenting in transport_push()"
>    s/push_refs/transport_push/ in commit message as noticed
>    by Ævar Arnfjörð Bjarmason
> 
>  * "transport: unify return values and exit point from transport_push()"
>    Added as suggested by Ævar Arnfjörð Bjarmason. It allows the following
>    memory leak fix to be a very simple patch.
> 
>  * "transport: free local and remote refs in transport_push()"
>    Just free remote_refs and local_refs. The other changes were
>    included in the previous patch.
> 
> Changes since v1:
> 
>  * Slit into series of two patches. The first one just changes
>    indenting in transport_push(). Second one adds the fix for
>    the local_refs and remote_refs memory leaks.
> 
>  * The resulting trees are the same, there is no code change.
> 
> Frantisek Hrbata (3):
>   transport: remove unnecessary indenting in transport_push()
>   transport: unify return values and exit point from transport_push()
>   transport: free local and remote refs in transport_push()
> 
>  transport.c | 260 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 133 insertions(+), 127 deletions(-)

Sorry for the late review. This all looks good to me, thanks for the
fix!

Reviewed-by: Josh Steadmon <steadmon@google.com>