From 6448182a831a5704601b7ad29afa1a26c40a4d53 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Fri, 20 May 2022 14:49:50 +0200 Subject: [PATCH 1/3] transport: remove unnecessary indenting in transport_push() 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 Reviewed-by: Josh Steadmon Signed-off-by: Junio C Hamano --- transport.c | 242 ++++++++++++++++++++++++++-------------------------- 1 file changed, 122 insertions(+), 120 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 (!transport->vtable->push_refs) + return 1; - if (check_push_refs(local_refs, rs) < 0) + local_refs = get_local_heads(); + + 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; - refspec_ref_prefixes(rs, &transport_options.ref_prefixes); + 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", "get_refs_list", r); - remote_refs = transport->vtable->get_refs_list(transport, 1, - &transport_options); - trace2_region_leave("transport_push", "get_refs_list", r); + 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); - 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 ((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")); - } + 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; + 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); + 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 (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); } - - 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"); - - return ret; + string_list_clear(&needs_pushing, 0); + oid_array_clear(&commits); + trace2_region_leave("transport_push", "check_submodules", r); } - return 1; + + 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"); + + return ret; } const struct ref *transport_get_remote_refs(struct transport *transport, From 35919bf1ab9d1451547d229f3a11a63c70f385df Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Fri, 20 May 2022 14:49:51 +0200 Subject: [PATCH 2/3] transport: unify return values and exit point from transport_push() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Frantisek Hrbata Reviewed-by: Josh Steadmon Signed-off-by: Junio C Hamano --- 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; } From 8c49d704ef8487b689acaf8a3b5e9ca098d48f30 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Fri, 20 May 2022 14:49:52 +0200 Subject: [PATCH 3/3] transport: free local and remote refs in 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) Signed-off-by: Frantisek Hrbata Reviewed-by: Josh Steadmon Signed-off-by: Junio C Hamano --- 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; }