{"thread":{"id":"57902","subject":"[PATCH] transport: free local and remote refs in transport_push()","startedAt":"2022-05-20T08:27:07Z","lastAt":"2022-05-27T20:23:05Z","messageCount":13,"participants":["Frantisek Hrbata","Ævar Arnfjörð Bjarmason","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455555","messageId":"20220520081723.1031830-1-frantisek@hrbata.com","threadId":"57902","inReplyTo":null,"subject":"[PATCH] transport: free local and remote refs in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T08:17:22Z","receivedAt":"2022-05-20T08:27:07Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Fix memory leaks in transport_push(), where remote_refs and local_refs\nare never freed. While at it, remove the unnecessary indenting and make\nthe code hopefully more readable.\n\n116 bytes in 1 blocks are definitely lost in loss record 56 of 103\n   at 0x484486F: malloc (vg_replace_malloc.c:381)\n   by 0x4938D7E: strdup (strdup.c:42)\n   by 0x628418: xstrdup (wrapper.c:39)\n   by 0x4FD454: process_capabilities (connect.c:232)\n   by 0x4FD454: get_remote_heads (connect.c:354)\n   by 0x610A38: handshake (transport.c:333)\n   by 0x612B02: transport_push (transport.c:1302)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n   by 0x404F17: main (common-main.c:56)\n\n5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103\n   at 0x4849464: calloc (vg_replace_malloc.c:1328)\n   by 0x628705: xcalloc (wrapper.c:150)\n   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)\n   by 0x5C232A: alloc_ref (remote.c:983)\n   by 0x5C232A: one_local_ref (remote.c:2299)\n   by 0x5C232A: one_local_ref (remote.c:2289)\n   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)\n   by 0x5B4C4F: do_for_each_ref (refs.c:1486)\n   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)\n   by 0x5B4C4F: for_each_ref (refs.c:1497)\n   by 0x5C6ADF: get_local_heads (remote.c:2310)\n   by 0x612A85: transport_push (transport.c:1286)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\nFor remote_refs it might be worth to leave the freeing to\ntransport_disconnect() and introduce helper similar to\ntransport_get_remote_refs().\n\n transport.c | 254 +++++++++++++++++++++++++++-------------------------\n 1 file changed, 130 insertions(+), 124 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3d64a43ab3..4c5d9db7f2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1276,146 +1276,152 @@ int transport_push(struct repository *r,\n \t\t   struct refspec *rs, int flags,\n \t\t   unsigned int *reject_reasons)\n {\n+\tstruct ref *remote_refs = NULL;\n+\tstruct ref *local_refs = NULL;\n+\tint match_flags = MATCH_REFS_NONE;\n+\tint verbose = (transport->verbose > 0);\n+\tint quiet = (transport->verbose < 0);\n+\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n+\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n+\tint push_ret, ret, err;\n+\tstruct transport_ls_refs_options transport_options =\n+\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n+\n \t*reject_reasons = 0;\n \n \tif (transport_color_config() < 0)\n \t\treturn -1;\n \n-\tif (transport->vtable->push_refs) {\n-\t\tstruct ref *remote_refs;\n-\t\tstruct ref *local_refs = get_local_heads();\n-\t\tint match_flags = MATCH_REFS_NONE;\n-\t\tint verbose = (transport->verbose > 0);\n-\t\tint quiet = (transport->verbose < 0);\n-\t\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n-\t\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n-\t\tint push_ret, ret, err;\n-\t\tstruct transport_ls_refs_options transport_options =\n-\t\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n-\n-\t\tif (check_push_refs(local_refs, rs) < 0)\n-\t\t\treturn -1;\n-\n-\t\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n-\n-\t\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n-\t\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n-\t\t\t\t\t\t\t       &transport_options);\n-\t\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n-\n-\t\ttransport_ls_refs_options_release(&transport_options);\n-\n-\t\tif (flags & TRANSPORT_PUSH_ALL)\n-\t\t\tmatch_flags |= MATCH_REFS_ALL;\n-\t\tif (flags & TRANSPORT_PUSH_MIRROR)\n-\t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n-\t\tif (flags & TRANSPORT_PUSH_PRUNE)\n-\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n-\t\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n-\t\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n-\n-\t\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n-\t\t\treturn -1;\n-\n-\t\tif (transport->smart_options &&\n-\t\t    transport->smart_options->cas &&\n-\t\t    !is_empty_cas(transport->smart_options->cas))\n-\t\t\tapply_push_cas(transport->smart_options->cas,\n-\t\t\t\t       transport->remote, remote_refs);\n-\n-\t\tset_ref_status_for_push(remote_refs,\n-\t\t\tflags & TRANSPORT_PUSH_MIRROR,\n-\t\t\tflags & TRANSPORT_PUSH_FORCE);\n-\n-\t\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n-\t\t\tif (run_pre_push_hook(transport, remote_refs))\n-\t\t\t\treturn -1;\n+\tif (!transport->vtable->push_refs)\n+\t\treturn 1;\n \n-\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t    !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (!push_unpushed_submodules(r,\n-\t\t\t\t\t\t      &commits,\n-\t\t\t\t\t\t      transport->remote,\n-\t\t\t\t\t\t      rs,\n-\t\t\t\t\t\t      transport->push_options,\n-\t\t\t\t\t\t      pretend)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n-\t\t\t\tdie(_(\"failed to push all needed submodules\"));\n-\t\t\t}\n+\tret = -1;\n+\tlocal_refs = get_local_heads();\n+\n+\tif (check_push_refs(local_refs, rs) < 0)\n+\t\tgoto done;\n+\n+\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n+\n+\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n+\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n+\t\t\t\t\t\t       &transport_options);\n+\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n+\n+\ttransport_ls_refs_options_release(&transport_options);\n+\n+\tif (flags & TRANSPORT_PUSH_ALL)\n+\t\tmatch_flags |= MATCH_REFS_ALL;\n+\tif (flags & TRANSPORT_PUSH_MIRROR)\n+\t\tmatch_flags |= MATCH_REFS_MIRROR;\n+\tif (flags & TRANSPORT_PUSH_PRUNE)\n+\t\tmatch_flags |= MATCH_REFS_PRUNE;\n+\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n+\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n+\n+\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n+\t\tgoto done;\n+\n+\tif (transport->smart_options &&\n+\t    transport->smart_options->cas &&\n+\t    !is_empty_cas(transport->smart_options->cas))\n+\t\tapply_push_cas(transport->smart_options->cas,\n+\t\t\t       transport->remote, remote_refs);\n+\n+\tset_ref_status_for_push(remote_refs,\n+\t\tflags & TRANSPORT_PUSH_MIRROR,\n+\t\tflags & TRANSPORT_PUSH_FORCE);\n+\n+\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n+\t\tif (run_pre_push_hook(transport, remote_refs))\n+\t\t\tgoto done;\n+\n+\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t    !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (!push_unpushed_submodules(r,\n+\t\t\t\t\t      &commits,\n+\t\t\t\t\t      transport->remote,\n+\t\t\t\t\t      rs,\n+\t\t\t\t\t      transport->push_options,\n+\t\t\t\t\t      pretend)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t\t\tdie(_(\"failed to push all needed submodules\"));\n \t\t}\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t}\n \n-\t\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n-\t\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t      !pretend)) && !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (find_unpushed_submodules(r,\n-\t\t\t\t\t\t     &commits,\n-\t\t\t\t\t\t     transport->remote->name,\n-\t\t\t\t\t\t     &needs_pushing)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n-\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n-\t\t\t}\n-\t\t\tstring_list_clear(&needs_pushing, 0);\n+\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n+\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t      !pretend)) && !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (find_unpushed_submodules(r,\n+\t\t\t\t\t     &commits,\n+\t\t\t\t\t     transport->remote->name,\n+\t\t\t\t\t     &needs_pushing)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n+\t\tstring_list_clear(&needs_pushing, 0);\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t}\n \n-\t\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n-\t\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n-\t\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n-\t\t} else\n-\t\t\tpush_ret = 0;\n-\t\terr = push_had_errors(remote_refs);\n-\t\tret = push_ret | err;\n-\n-\t\tif (!quiet || err)\n-\t\t\ttransport_print_push_status(transport->url, remote_refs,\n-\t\t\t\t\tverbose | porcelain, porcelain,\n-\t\t\t\t\treject_reasons);\n-\n-\t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n-\t\t\tset_upstreams(transport, remote_refs, pretend);\n-\n-\t\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n-\t\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n-\t\t\tstruct ref *ref;\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n-\t\t}\n+\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n+\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n+\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n+\t} else\n+\t\tpush_ret = 0;\n+\terr = push_had_errors(remote_refs);\n+\tret = push_ret | err;\n+\n+\tif (!quiet || err)\n+\t\ttransport_print_push_status(transport->url, remote_refs,\n+\t\t\t\tverbose | porcelain, porcelain,\n+\t\t\t\treject_reasons);\n+\n+\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n+\t\tset_upstreams(transport, remote_refs, pretend);\n+\n+\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n+\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n+\t\tstruct ref *ref;\n+\t\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n+\t}\n \n-\t\tif (porcelain && !push_ret)\n-\t\t\tputs(\"Done\");\n-\t\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n-\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\tif (porcelain && !push_ret)\n+\t\tputs(\"Done\");\n+\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n+\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n-\t\treturn ret;\n-\t}\n-\treturn 1;\n+done:\n+\tfree_refs(local_refs);\n+\tfree_refs(remote_refs);\n+\treturn ret;\n }\n \n const struct ref *transport_get_remote_refs(struct transport *transport,\n\nbase-commit: e54793a95afeea1e10de1e5ad7eab914e7416250\n-- \n2.35.1\n\n"},{"id":"455557","messageId":"220520.8635h438aa.gmgdl@evledraar.gmail.com","threadId":"57902","inReplyTo":"20220520081723.1031830-1-frantisek@hrbata.com","subject":"Re: [PATCH] transport: free local and remote refs in transport_push()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-20T08:40:33Z","receivedAt":"2022-05-20T08:42:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 20 2022, Frantisek Hrbata wrote:\n\n> Fix memory leaks in transport_push(), where remote_refs and local_refs\n> are never freed. While at it, remove the unnecessary indenting and make\n> the code hopefully more readable.\n\nIf at all possible it would be very nice to split up such re-indenting /\nformatting into another commit, so we can see what the \"real\" code\nchanges are.\n"},{"id":"455558","messageId":"YodXrProOdVzvgQA@fedora","threadId":"57902","inReplyTo":"220520.8635h438aa.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] transport: free local and remote refs in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T08:56:12Z","receivedAt":"2022-05-20T08:56:22Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Hello,\n\nOn Fri, May 20, 2022 at 10:40:33AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, May 20 2022, Frantisek Hrbata wrote:\n> \n> > Fix memory leaks in transport_push(), where remote_refs and local_refs\n> > are never freed. While at it, remove the unnecessary indenting and make\n> > the code hopefully more readable.\n> \n> If at all possible it would be very nice to split up such re-indenting /\n> formatting into another commit, so we can see what the \"real\" code\n> changes are.\n\nof course, I will split it into two patch series and post v2. First with the\nindenting adjustment only and second with mem leak fixes.\n\nThank you\n\n\n-- \nFrantisek Hrbata\n"},{"id":"455559","messageId":"20220520103507.1717236-1-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520081723.1031830-1-frantisek@hrbata.com","subject":"[PATCH v2 0/2] fix memory leaks in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T10:35:05Z","receivedAt":"2022-05-20T10:35:16Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Fix memory leaks in transport_push(), where remote_refs and local_refs\nare never freed. While at it, remove the unnecessary indenting and make\nthe code hopefully more readable.\n\nChanges since v1:\n * Slit into series of two patches. The first one just changes\n   indenting in transport_push(). Second one adds the fix for\n   the local_refs and remote_refs memory leaks.\n\n * The resulting trees are the same, there is no code change.\n\nFrantisek Hrbata (2):\n  transport: remove unnecessary indenting in transport_push()\n  transport: free local and remote refs in transport_push()\n\n transport.c | 254 +++++++++++++++++++++++++++-------------------------\n 1 file changed, 130 insertions(+), 124 deletions(-)\n\nRange-diff against v1:\n1:  fb8b90d247 ! 1:  daf042cd2e transport: free local and remote refs in transport_push()\n    @@ Metadata\n     Author: Frantisek Hrbata <frantisek@hrbata.com>\n     \n      ## Commit message ##\n    -    transport: free local and remote refs in transport_push()\n    +    transport: remove unnecessary indenting in transport_push()\n     \n    -    Fix memory leaks in transport_push(), where remote_refs and local_refs\n    -    are never freed. While at it, remove the unnecessary indenting and make\n    -    the code hopefully more readable.\n    -\n    -    116 bytes in 1 blocks are definitely lost in loss record 56 of 103\n    -       at 0x484486F: malloc (vg_replace_malloc.c:381)\n    -       by 0x4938D7E: strdup (strdup.c:42)\n    -       by 0x628418: xstrdup (wrapper.c:39)\n    -       by 0x4FD454: process_capabilities (connect.c:232)\n    -       by 0x4FD454: get_remote_heads (connect.c:354)\n    -       by 0x610A38: handshake (transport.c:333)\n    -       by 0x612B02: transport_push (transport.c:1302)\n    -       by 0x4803D6: push_with_options (push.c:357)\n    -       by 0x4811D6: do_push (push.c:414)\n    -       by 0x4811D6: cmd_push (push.c:650)\n    -       by 0x405210: run_builtin (git.c:465)\n    -       by 0x405210: handle_builtin (git.c:719)\n    -       by 0x406363: run_argv (git.c:786)\n    -       by 0x406363: cmd_main (git.c:917)\n    -       by 0x404F17: main (common-main.c:56)\n    -\n    -    5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103\n    -       at 0x4849464: calloc (vg_replace_malloc.c:1328)\n    -       by 0x628705: xcalloc (wrapper.c:150)\n    -       by 0x5C216D: alloc_ref_with_prefix (remote.c:975)\n    -       by 0x5C232A: alloc_ref (remote.c:983)\n    -       by 0x5C232A: one_local_ref (remote.c:2299)\n    -       by 0x5C232A: one_local_ref (remote.c:2289)\n    -       by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)\n    -       by 0x5B4C4F: do_for_each_ref (refs.c:1486)\n    -       by 0x5B4C4F: refs_for_each_ref (refs.c:1492)\n    -       by 0x5B4C4F: for_each_ref (refs.c:1497)\n    -       by 0x5C6ADF: get_local_heads (remote.c:2310)\n    -       by 0x612A85: transport_push (transport.c:1286)\n    -       by 0x4803D6: push_with_options (push.c:357)\n    -       by 0x4811D6: do_push (push.c:414)\n    -       by 0x4811D6: cmd_push (push.c:650)\n    -       by 0x405210: run_builtin (git.c:465)\n    -       by 0x405210: handle_builtin (git.c:719)\n    -       by 0x406363: run_argv (git.c:786)\n    -       by 0x406363: cmd_main (git.c:917)\n    +    Remove the big indented block for push_refs() check in transport vtable\n    +    and let's just return error immediately. Hopefully this makes the code\n    +    more readable.\n     \n         Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n     \n    @@ transport.c: int transport_push(struct repository *r,\n      \t\t   struct refspec *rs, int flags,\n      \t\t   unsigned int *reject_reasons)\n      {\n    -+\tstruct ref *remote_refs = NULL;\n    -+\tstruct ref *local_refs = NULL;\n    ++\tstruct ref *remote_refs;\n    ++\tstruct ref *local_refs;\n     +\tint match_flags = MATCH_REFS_NONE;\n     +\tint verbose = (transport->verbose > 0);\n     +\tint quiet = (transport->verbose < 0);\n    @@ transport.c: int transport_push(struct repository *r,\n     -\n     -\t\tif (check_push_refs(local_refs, rs) < 0)\n     -\t\t\treturn -1;\n    --\n    ++\tif (!transport->vtable->push_refs)\n    ++\t\treturn 1;\n    + \n     -\t\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n    --\n    ++\tlocal_refs = get_local_heads();\n    + \n     -\t\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n     -\t\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n     -\t\t\t\t\t\t\t       &transport_options);\n     -\t\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n    --\n    ++\tif (check_push_refs(local_refs, rs) < 0)\n    ++\t\treturn -1;\n    + \n     -\t\ttransport_ls_refs_options_release(&transport_options);\n    --\n    ++\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n    + \n     -\t\tif (flags & TRANSPORT_PUSH_ALL)\n     -\t\t\tmatch_flags |= MATCH_REFS_ALL;\n     -\t\tif (flags & TRANSPORT_PUSH_MIRROR)\n    @@ transport.c: int transport_push(struct repository *r,\n     -\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n     -\t\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n     -\t\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n    --\n    ++\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n    ++\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n    ++\t\t\t\t\t\t       &transport_options);\n    ++\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n    + \n     -\t\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n     -\t\t\treturn -1;\n    --\n    ++\ttransport_ls_refs_options_release(&transport_options);\n    + \n     -\t\tif (transport->smart_options &&\n     -\t\t    transport->smart_options->cas &&\n     -\t\t    !is_empty_cas(transport->smart_options->cas))\n     -\t\t\tapply_push_cas(transport->smart_options->cas,\n     -\t\t\t\t       transport->remote, remote_refs);\n    --\n    ++\tif (flags & TRANSPORT_PUSH_ALL)\n    ++\t\tmatch_flags |= MATCH_REFS_ALL;\n    ++\tif (flags & TRANSPORT_PUSH_MIRROR)\n    ++\t\tmatch_flags |= MATCH_REFS_MIRROR;\n    ++\tif (flags & TRANSPORT_PUSH_PRUNE)\n    ++\t\tmatch_flags |= MATCH_REFS_PRUNE;\n    ++\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n    ++\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n    + \n     -\t\tset_ref_status_for_push(remote_refs,\n     -\t\t\tflags & TRANSPORT_PUSH_MIRROR,\n     -\t\t\tflags & TRANSPORT_PUSH_FORCE);\n    --\n    ++\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n    ++\t\treturn -1;\n    + \n     -\t\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n     -\t\t\tif (run_pre_push_hook(transport, remote_refs))\n     -\t\t\t\treturn -1;\n    -+\tif (!transport->vtable->push_refs)\n    -+\t\treturn 1;\n    ++\tif (transport->smart_options &&\n    ++\t    transport->smart_options->cas &&\n    ++\t    !is_empty_cas(transport->smart_options->cas))\n    ++\t\tapply_push_cas(transport->smart_options->cas,\n    ++\t\t\t       transport->remote, remote_refs);\n      \n     -\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n     -\t\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n    @@ transport.c: int transport_push(struct repository *r,\n     -\t\t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n     -\t\t\t\tdie(_(\"failed to push all needed submodules\"));\n     -\t\t\t}\n    -+\tret = -1;\n    -+\tlocal_refs = get_local_heads();\n    -+\n    -+\tif (check_push_refs(local_refs, rs) < 0)\n    -+\t\tgoto done;\n    -+\n    -+\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n    -+\n    -+\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n    -+\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n    -+\t\t\t\t\t\t       &transport_options);\n    -+\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n    -+\n    -+\ttransport_ls_refs_options_release(&transport_options);\n    -+\n    -+\tif (flags & TRANSPORT_PUSH_ALL)\n    -+\t\tmatch_flags |= MATCH_REFS_ALL;\n    -+\tif (flags & TRANSPORT_PUSH_MIRROR)\n    -+\t\tmatch_flags |= MATCH_REFS_MIRROR;\n    -+\tif (flags & TRANSPORT_PUSH_PRUNE)\n    -+\t\tmatch_flags |= MATCH_REFS_PRUNE;\n    -+\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n    -+\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n    -+\n    -+\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n    -+\t\tgoto done;\n    -+\n    -+\tif (transport->smart_options &&\n    -+\t    transport->smart_options->cas &&\n    -+\t    !is_empty_cas(transport->smart_options->cas))\n    -+\t\tapply_push_cas(transport->smart_options->cas,\n    -+\t\t\t       transport->remote, remote_refs);\n    -+\n     +\tset_ref_status_for_push(remote_refs,\n     +\t\tflags & TRANSPORT_PUSH_MIRROR,\n     +\t\tflags & TRANSPORT_PUSH_FORCE);\n     +\n     +\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n     +\t\tif (run_pre_push_hook(transport, remote_refs))\n    -+\t\t\tgoto done;\n    ++\t\t\treturn -1;\n     +\n     +\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n     +\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n    @@ transport.c: int transport_push(struct repository *r,\n     -\t\treturn ret;\n     -\t}\n     -\treturn 1;\n    -+done:\n    -+\tfree_refs(local_refs);\n    -+\tfree_refs(remote_refs);\n     +\treturn ret;\n      }\n      \n-:  ---------- > 2:  9d25176b92 transport: free local and remote refs in transport_push()\n\nbase-commit: e54793a95afeea1e10de1e5ad7eab914e7416250\n-- \n2.35.1\n\n"},{"id":"455560","messageId":"20220520103507.1717236-2-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520103507.1717236-1-frantisek@hrbata.com","subject":"[PATCH v2 1/2] transport: remove unnecessary indenting in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T10:35:06Z","receivedAt":"2022-05-20T10:35:18Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Remove the big indented block for push_refs() check in transport vtable\nand let's just return error immediately. Hopefully this makes the code\nmore readable.\n\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\nIs there a reason to return 1 instead of -1 when push_refs() is not\nset in transport vtable? Looking at the code I think it might return\n-1 also and make it more consistent.\n\n transport.c | 234 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 118 insertions(+), 116 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3d64a43ab3..0b9c5a427d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1276,146 +1276,148 @@ int transport_push(struct repository *r,\n \t\t   struct refspec *rs, int flags,\n \t\t   unsigned int *reject_reasons)\n {\n+\tstruct ref *remote_refs;\n+\tstruct ref *local_refs;\n+\tint match_flags = MATCH_REFS_NONE;\n+\tint verbose = (transport->verbose > 0);\n+\tint quiet = (transport->verbose < 0);\n+\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n+\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n+\tint push_ret, ret, err;\n+\tstruct transport_ls_refs_options transport_options =\n+\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n+\n \t*reject_reasons = 0;\n \n \tif (transport_color_config() < 0)\n \t\treturn -1;\n \n-\tif (transport->vtable->push_refs) {\n-\t\tstruct ref *remote_refs;\n-\t\tstruct ref *local_refs = get_local_heads();\n-\t\tint match_flags = MATCH_REFS_NONE;\n-\t\tint verbose = (transport->verbose > 0);\n-\t\tint quiet = (transport->verbose < 0);\n-\t\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n-\t\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n-\t\tint push_ret, ret, err;\n-\t\tstruct transport_ls_refs_options transport_options =\n-\t\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n-\n-\t\tif (check_push_refs(local_refs, rs) < 0)\n-\t\t\treturn -1;\n+\tif (!transport->vtable->push_refs)\n+\t\treturn 1;\n \n-\t\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n+\tlocal_refs = get_local_heads();\n \n-\t\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n-\t\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n-\t\t\t\t\t\t\t       &transport_options);\n-\t\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n+\tif (check_push_refs(local_refs, rs) < 0)\n+\t\treturn -1;\n \n-\t\ttransport_ls_refs_options_release(&transport_options);\n+\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n \n-\t\tif (flags & TRANSPORT_PUSH_ALL)\n-\t\t\tmatch_flags |= MATCH_REFS_ALL;\n-\t\tif (flags & TRANSPORT_PUSH_MIRROR)\n-\t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n-\t\tif (flags & TRANSPORT_PUSH_PRUNE)\n-\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n-\t\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n-\t\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n+\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n+\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n+\t\t\t\t\t\t       &transport_options);\n+\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n \n-\t\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n-\t\t\treturn -1;\n+\ttransport_ls_refs_options_release(&transport_options);\n \n-\t\tif (transport->smart_options &&\n-\t\t    transport->smart_options->cas &&\n-\t\t    !is_empty_cas(transport->smart_options->cas))\n-\t\t\tapply_push_cas(transport->smart_options->cas,\n-\t\t\t\t       transport->remote, remote_refs);\n+\tif (flags & TRANSPORT_PUSH_ALL)\n+\t\tmatch_flags |= MATCH_REFS_ALL;\n+\tif (flags & TRANSPORT_PUSH_MIRROR)\n+\t\tmatch_flags |= MATCH_REFS_MIRROR;\n+\tif (flags & TRANSPORT_PUSH_PRUNE)\n+\t\tmatch_flags |= MATCH_REFS_PRUNE;\n+\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n+\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n \n-\t\tset_ref_status_for_push(remote_refs,\n-\t\t\tflags & TRANSPORT_PUSH_MIRROR,\n-\t\t\tflags & TRANSPORT_PUSH_FORCE);\n+\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n+\t\treturn -1;\n \n-\t\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n-\t\t\tif (run_pre_push_hook(transport, remote_refs))\n-\t\t\t\treturn -1;\n+\tif (transport->smart_options &&\n+\t    transport->smart_options->cas &&\n+\t    !is_empty_cas(transport->smart_options->cas))\n+\t\tapply_push_cas(transport->smart_options->cas,\n+\t\t\t       transport->remote, remote_refs);\n \n-\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t    !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (!push_unpushed_submodules(r,\n-\t\t\t\t\t\t      &commits,\n-\t\t\t\t\t\t      transport->remote,\n-\t\t\t\t\t\t      rs,\n-\t\t\t\t\t\t      transport->push_options,\n-\t\t\t\t\t\t      pretend)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n-\t\t\t\tdie(_(\"failed to push all needed submodules\"));\n-\t\t\t}\n+\tset_ref_status_for_push(remote_refs,\n+\t\tflags & TRANSPORT_PUSH_MIRROR,\n+\t\tflags & TRANSPORT_PUSH_FORCE);\n+\n+\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n+\t\tif (run_pre_push_hook(transport, remote_refs))\n+\t\t\treturn -1;\n+\n+\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t    !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (!push_unpushed_submodules(r,\n+\t\t\t\t\t      &commits,\n+\t\t\t\t\t      transport->remote,\n+\t\t\t\t\t      rs,\n+\t\t\t\t\t      transport->push_options,\n+\t\t\t\t\t      pretend)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t\t\tdie(_(\"failed to push all needed submodules\"));\n \t\t}\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t}\n \n-\t\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n-\t\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t      !pretend)) && !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (find_unpushed_submodules(r,\n-\t\t\t\t\t\t     &commits,\n-\t\t\t\t\t\t     transport->remote->name,\n-\t\t\t\t\t\t     &needs_pushing)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n-\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n-\t\t\t}\n-\t\t\tstring_list_clear(&needs_pushing, 0);\n+\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n+\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t      !pretend)) && !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (find_unpushed_submodules(r,\n+\t\t\t\t\t     &commits,\n+\t\t\t\t\t     transport->remote->name,\n+\t\t\t\t\t     &needs_pushing)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n+\t\tstring_list_clear(&needs_pushing, 0);\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t}\n \n-\t\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n-\t\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n-\t\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n-\t\t} else\n-\t\t\tpush_ret = 0;\n-\t\terr = push_had_errors(remote_refs);\n-\t\tret = push_ret | err;\n-\n-\t\tif (!quiet || err)\n-\t\t\ttransport_print_push_status(transport->url, remote_refs,\n-\t\t\t\t\tverbose | porcelain, porcelain,\n-\t\t\t\t\treject_reasons);\n-\n-\t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n-\t\t\tset_upstreams(transport, remote_refs, pretend);\n-\n-\t\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n-\t\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n-\t\t\tstruct ref *ref;\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n-\t\t}\n+\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n+\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n+\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n+\t} else\n+\t\tpush_ret = 0;\n+\terr = push_had_errors(remote_refs);\n+\tret = push_ret | err;\n+\n+\tif (!quiet || err)\n+\t\ttransport_print_push_status(transport->url, remote_refs,\n+\t\t\t\tverbose | porcelain, porcelain,\n+\t\t\t\treject_reasons);\n+\n+\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n+\t\tset_upstreams(transport, remote_refs, pretend);\n+\n+\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n+\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n+\t\tstruct ref *ref;\n+\t\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n+\t}\n \n-\t\tif (porcelain && !push_ret)\n-\t\t\tputs(\"Done\");\n-\t\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n-\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\tif (porcelain && !push_ret)\n+\t\tputs(\"Done\");\n+\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n+\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n-\t\treturn ret;\n-\t}\n-\treturn 1;\n+\treturn ret;\n }\n \n const struct ref *transport_get_remote_refs(struct transport *transport,\n-- \n2.35.1\n\n"},{"id":"455561","messageId":"20220520103507.1717236-3-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520103507.1717236-1-frantisek@hrbata.com","subject":"[PATCH v2 2/2] transport: free local and remote refs in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T10:35:07Z","receivedAt":"2022-05-20T10:35:21Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Fix memory leaks in transport_push(), where remote_refs and local_refs\nare never freed.\n\n116 bytes in 1 blocks are definitely lost in loss record 56 of 103\n   at 0x484486F: malloc (vg_replace_malloc.c:381)\n   by 0x4938D7E: strdup (strdup.c:42)\n   by 0x628418: xstrdup (wrapper.c:39)\n   by 0x4FD454: process_capabilities (connect.c:232)\n   by 0x4FD454: get_remote_heads (connect.c:354)\n   by 0x610A38: handshake (transport.c:333)\n   by 0x612B02: transport_push (transport.c:1302)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n   by 0x404F17: main (common-main.c:56)\n\n5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103\n   at 0x4849464: calloc (vg_replace_malloc.c:1328)\n   by 0x628705: xcalloc (wrapper.c:150)\n   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)\n   by 0x5C232A: alloc_ref (remote.c:983)\n   by 0x5C232A: one_local_ref (remote.c:2299)\n   by 0x5C232A: one_local_ref (remote.c:2289)\n   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)\n   by 0x5B4C4F: do_for_each_ref (refs.c:1486)\n   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)\n   by 0x5B4C4F: for_each_ref (refs.c:1497)\n   by 0x5C6ADF: get_local_heads (remote.c:2310)\n   by 0x612A85: transport_push (transport.c:1286)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\nFor remote_refs it might be worth to leave the freeing to\ntransport_disconnect() and introduce helper similar to\ntransport_get_remote_refs().\n\n transport.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 0b9c5a427d..4c5d9db7f2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1276,8 +1276,8 @@ int transport_push(struct repository *r,\n \t\t   struct refspec *rs, int flags,\n \t\t   unsigned int *reject_reasons)\n {\n-\tstruct ref *remote_refs;\n-\tstruct ref *local_refs;\n+\tstruct ref *remote_refs = NULL;\n+\tstruct ref *local_refs = NULL;\n \tint match_flags = MATCH_REFS_NONE;\n \tint verbose = (transport->verbose > 0);\n \tint quiet = (transport->verbose < 0);\n@@ -1295,10 +1295,11 @@ int transport_push(struct repository *r,\n \tif (!transport->vtable->push_refs)\n \t\treturn 1;\n \n+\tret = -1;\n \tlocal_refs = get_local_heads();\n \n \tif (check_push_refs(local_refs, rs) < 0)\n-\t\treturn -1;\n+\t\tgoto done;\n \n \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n \n@@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,\n \t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n \n \tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n-\t\treturn -1;\n+\t\tgoto done;\n \n \tif (transport->smart_options &&\n \t    transport->smart_options->cas &&\n@@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,\n \n \tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n \t\tif (run_pre_push_hook(transport, remote_refs))\n-\t\t\treturn -1;\n+\t\t\tgoto done;\n \n \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n@@ -1417,6 +1418,9 @@ int transport_push(struct repository *r,\n \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n+done:\n+\tfree_refs(local_refs);\n+\tfree_refs(remote_refs);\n \treturn ret;\n }\n \n-- \n2.35.1\n\n"},{"id":"455562","messageId":"220520.86y1yw1lxj.gmgdl@evledraar.gmail.com","threadId":"57902","inReplyTo":"20220520103507.1717236-2-frantisek@hrbata.com","subject":"Re: [PATCH v2 1/2] transport: remove unnecessary indenting in transport_push()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-20T11:24:28Z","receivedAt":"2022-05-20T11:30:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 20 2022, Frantisek Hrbata wrote:\n\n> Remove the big indented block for push_refs() check in transport vtable\n> and let's just return error immediately. Hopefully this makes the code\n> more readable.\n\ns/push_refs/transport_push/, push_refs is the name of the field in the\ncallback (and there's more than just this function).\n\nThis looks good to me....\n\n> Is there a reason to return 1 instead of -1 when push_refs() is not\n> set in transport vtable? Looking at the code I think it might return\n> -1 also and make it more consistent.\n\nNo, looking at it (I tried renaming the function) the only user is\nbuiltin/push.c, which we can easily see doesn't care about the 1 v.s. -1 here.\n\nPerhaps it's worthwhile to add this in-between the two patches you have:\n\t\n\tdiff --git a/transport.c b/transport.c\n\tindex 0b9c5a427d7..5348fac36ef 100644\n\t--- a/transport.c\n\t+++ b/transport.c\n\t@@ -1283,22 +1283,23 @@ int transport_push(struct repository *r,\n\t \tint quiet = (transport->verbose < 0);\n\t \tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n\t \tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n\t-\tint push_ret, ret, err;\n\t+\tint push_ret, err;\n\t+\tint ret = -1;\n\t \tstruct transport_ls_refs_options transport_options =\n\t \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n\t \n\t \t*reject_reasons = 0;\n\t \n\t \tif (transport_color_config() < 0)\n\t-\t\treturn -1;\n\t+\t\tgoto done;\n\t \n\t \tif (!transport->vtable->push_refs)\n\t-\t\treturn 1;\n\t+\t\tgoto done;\n\t \n\t \tlocal_refs = get_local_heads();\n\t \n\t \tif (check_push_refs(local_refs, rs) < 0)\n\t-\t\treturn -1;\n\t+\t\tgoto done;\n\t \n\t \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n\t \n\t@@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,\n\t \t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n\t \n\t \tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n\t-\t\treturn -1;\n\t+\t\tgoto done;\n\t \n\t \tif (transport->smart_options &&\n\t \t    transport->smart_options->cas &&\n\t@@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,\n\t \n\t \tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n\t \t\tif (run_pre_push_hook(transport, remote_refs))\n\t-\t\t\treturn -1;\n\t+\t\t\tgoto done;\n\t \n\t \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n\t \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n\t@@ -1417,6 +1418,7 @@ int transport_push(struct repository *r,\n\t \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n\t \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n\t \n\t+done:\n\t \treturn ret;\n\t }\n\nWhich would make your new 3/3 truly trivial, i.e. just adding the\nfree_refs() for the two.\n\n*Maybe* (but probably not) it would then make sense to do this as that 3/3:\n\t\n\tdiff --git a/transport.c b/transport.c\n\tindex 5348fac36ef..d4952bf5f6a 100644\n\t--- a/transport.c\n\t+++ b/transport.c\n\t@@ -1291,15 +1291,17 @@ int transport_push(struct repository *r,\n\t \t*reject_reasons = 0;\n\t \n\t \tif (transport_color_config() < 0)\n\t-\t\tgoto done;\n\t+\t\treturn ret;\n\t \n\t \tif (!transport->vtable->push_refs)\n\t-\t\tgoto done;\n\t+\t\treturn ret;\n\t \n\t \tlocal_refs = get_local_heads();\n\t \n\t-\tif (check_push_refs(local_refs, rs) < 0)\n\t+\tif (check_push_refs(local_refs, rs) < 0) {\n\t+\t\tremote_refs = NULL;\n\t \t\tgoto done;\n\t+\t}\n\t \n\t \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n\t \n\t@@ -1419,6 +1421,8 @@ int transport_push(struct repository *r,\n\t \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n\t \n\t done:\n\t+\tfree_refs(local_refs);\n\t+\tfree_refs(remote_refs);\n\t \treturn ret;\n\t }\n\nI.e. entirely skip the NULL assignment for the two, which helps the\ncompiler catch if we don't init it before the later goto's, but because\nof that we'd need to \"return\" instead of \"goto done\" for the early ones,\nand set it for the check_push_refs() failure case.\n\nSo nah, probably best just to keep it as you have it, i.e. always \"goto\ndone\".\n"},{"id":"455563","messageId":"YoeBTeJOCbP449BW@fedora","threadId":"57902","inReplyTo":"220520.86y1yw1lxj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] transport: remove unnecessary indenting in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T11:53:49Z","receivedAt":"2022-05-20T11:53:56Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"On Fri, May 20, 2022 at 01:24:28PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, May 20 2022, Frantisek Hrbata wrote:\n> \n> > Remove the big indented block for push_refs() check in transport vtable\n> > and let's just return error immediately. Hopefully this makes the code\n> > more readable.\n> \n> s/push_refs/transport_push/, push_refs is the name of the field in the\n> callback (and there's more than just this function).\n\nuf, I will fix that, thank you for noticing.\n\n> \n> This looks good to me....\n> \n> > Is there a reason to return 1 instead of -1 when push_refs() is not\n> > set in transport vtable? Looking at the code I think it might return\n> > -1 also and make it more consistent.\n> \n> No, looking at it (I tried renaming the function) the only user is\n> builtin/push.c, which we can easily see doesn't care about the 1 v.s. -1 here.\n> \n> Perhaps it's worthwhile to add this in-between the two patches you have:\n> \t\n> \tdiff --git a/transport.c b/transport.c\n> \tindex 0b9c5a427d7..5348fac36ef 100644\n> \t--- a/transport.c\n> \t+++ b/transport.c\n> \t@@ -1283,22 +1283,23 @@ int transport_push(struct repository *r,\n> \t \tint quiet = (transport->verbose < 0);\n> \t \tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n> \t \tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n> \t-\tint push_ret, ret, err;\n> \t+\tint push_ret, err;\n> \t+\tint ret = -1;\n> \t \tstruct transport_ls_refs_options transport_options =\n> \t \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n> \t \n> \t \t*reject_reasons = 0;\n> \t \n> \t \tif (transport_color_config() < 0)\n> \t-\t\treturn -1;\n> \t+\t\tgoto done;\n> \t \n> \t \tif (!transport->vtable->push_refs)\n> \t-\t\treturn 1;\n> \t+\t\tgoto done;\n> \t \n> \t \tlocal_refs = get_local_heads();\n> \t \n> \t \tif (check_push_refs(local_refs, rs) < 0)\n> \t-\t\treturn -1;\n> \t+\t\tgoto done;\n> \t \n> \t \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n> \t \n> \t@@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,\n> \t \t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n> \t \n> \t \tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n> \t-\t\treturn -1;\n> \t+\t\tgoto done;\n> \t \n> \t \tif (transport->smart_options &&\n> \t \t    transport->smart_options->cas &&\n> \t@@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,\n> \t \n> \t \tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n> \t \t\tif (run_pre_push_hook(transport, remote_refs))\n> \t-\t\t\treturn -1;\n> \t+\t\t\tgoto done;\n> \t \n> \t \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n> \t \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n> \t@@ -1417,6 +1418,7 @@ int transport_push(struct repository *r,\n> \t \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n> \t \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> \t \n> \t+done:\n> \t \treturn ret;\n> \t }\n> \n> Which would make your new 3/3 truly trivial, i.e. just adding the\n> free_refs() for the two.\n\nI was thinking about this too. Just use the done label as a single exit point.\nLet me incorporate your suggestions in v3. I will add a 2/3 patch e.g.\n\"unify return values and exit point for transport_push()\"\n\n> \n> *Maybe* (but probably not) it would then make sense to do this as that 3/3:\n> \t\n> \tdiff --git a/transport.c b/transport.c\n> \tindex 5348fac36ef..d4952bf5f6a 100644\n> \t--- a/transport.c\n> \t+++ b/transport.c\n> \t@@ -1291,15 +1291,17 @@ int transport_push(struct repository *r,\n> \t \t*reject_reasons = 0;\n> \t \n> \t \tif (transport_color_config() < 0)\n> \t-\t\tgoto done;\n> \t+\t\treturn ret;\n> \t \n> \t \tif (!transport->vtable->push_refs)\n> \t-\t\tgoto done;\n> \t+\t\treturn ret;\n> \t \n> \t \tlocal_refs = get_local_heads();\n> \t \n> \t-\tif (check_push_refs(local_refs, rs) < 0)\n> \t+\tif (check_push_refs(local_refs, rs) < 0) {\n> \t+\t\tremote_refs = NULL;\n> \t \t\tgoto done;\n> \t+\t}\n> \t \n> \t \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n> \t \n> \t@@ -1419,6 +1421,8 @@ int transport_push(struct repository *r,\n> \t \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> \t \n> \t done:\n> \t+\tfree_refs(local_refs);\n> \t+\tfree_refs(remote_refs);\n> \t \treturn ret;\n> \t }\n> \n> I.e. entirely skip the NULL assignment for the two, which helps the\n> compiler catch if we don't init it before the later goto's, but because\n> of that we'd need to \"return\" instead of \"goto done\" for the early ones,\n> and set it for the check_push_refs() failure case.\n> \n> So nah, probably best just to keep it as you have it, i.e. always \"goto\n> done\".\n\nI agree. Let me prepare v3 and let's see what happens :)\n\nThank you for your input!\n\n-- \nFrantisek Hrbata\n"},{"id":"455565","messageId":"20220520124952.2393299-2-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520124952.2393299-1-frantisek@hrbata.com","subject":"[PATCH v3 1/3] transport: remove unnecessary indenting in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T12:49:50Z","receivedAt":"2022-05-20T12:50:04Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Remove the big indented block for transport_push() check in transport vtable\nand let's just return error immediately. Hopefully this makes the code\nmore readable.\n\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\n transport.c | 234 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 118 insertions(+), 116 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3d64a43ab3..0b9c5a427d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1276,146 +1276,148 @@ int transport_push(struct repository *r,\n \t\t   struct refspec *rs, int flags,\n \t\t   unsigned int *reject_reasons)\n {\n+\tstruct ref *remote_refs;\n+\tstruct ref *local_refs;\n+\tint match_flags = MATCH_REFS_NONE;\n+\tint verbose = (transport->verbose > 0);\n+\tint quiet = (transport->verbose < 0);\n+\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n+\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n+\tint push_ret, ret, err;\n+\tstruct transport_ls_refs_options transport_options =\n+\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n+\n \t*reject_reasons = 0;\n \n \tif (transport_color_config() < 0)\n \t\treturn -1;\n \n-\tif (transport->vtable->push_refs) {\n-\t\tstruct ref *remote_refs;\n-\t\tstruct ref *local_refs = get_local_heads();\n-\t\tint match_flags = MATCH_REFS_NONE;\n-\t\tint verbose = (transport->verbose > 0);\n-\t\tint quiet = (transport->verbose < 0);\n-\t\tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n-\t\tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n-\t\tint push_ret, ret, err;\n-\t\tstruct transport_ls_refs_options transport_options =\n-\t\t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n-\n-\t\tif (check_push_refs(local_refs, rs) < 0)\n-\t\t\treturn -1;\n+\tif (!transport->vtable->push_refs)\n+\t\treturn 1;\n \n-\t\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n+\tlocal_refs = get_local_heads();\n \n-\t\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n-\t\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n-\t\t\t\t\t\t\t       &transport_options);\n-\t\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n+\tif (check_push_refs(local_refs, rs) < 0)\n+\t\treturn -1;\n \n-\t\ttransport_ls_refs_options_release(&transport_options);\n+\trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n \n-\t\tif (flags & TRANSPORT_PUSH_ALL)\n-\t\t\tmatch_flags |= MATCH_REFS_ALL;\n-\t\tif (flags & TRANSPORT_PUSH_MIRROR)\n-\t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n-\t\tif (flags & TRANSPORT_PUSH_PRUNE)\n-\t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n-\t\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n-\t\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n+\ttrace2_region_enter(\"transport_push\", \"get_refs_list\", r);\n+\tremote_refs = transport->vtable->get_refs_list(transport, 1,\n+\t\t\t\t\t\t       &transport_options);\n+\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n \n-\t\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n-\t\t\treturn -1;\n+\ttransport_ls_refs_options_release(&transport_options);\n \n-\t\tif (transport->smart_options &&\n-\t\t    transport->smart_options->cas &&\n-\t\t    !is_empty_cas(transport->smart_options->cas))\n-\t\t\tapply_push_cas(transport->smart_options->cas,\n-\t\t\t\t       transport->remote, remote_refs);\n+\tif (flags & TRANSPORT_PUSH_ALL)\n+\t\tmatch_flags |= MATCH_REFS_ALL;\n+\tif (flags & TRANSPORT_PUSH_MIRROR)\n+\t\tmatch_flags |= MATCH_REFS_MIRROR;\n+\tif (flags & TRANSPORT_PUSH_PRUNE)\n+\t\tmatch_flags |= MATCH_REFS_PRUNE;\n+\tif (flags & TRANSPORT_PUSH_FOLLOW_TAGS)\n+\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n \n-\t\tset_ref_status_for_push(remote_refs,\n-\t\t\tflags & TRANSPORT_PUSH_MIRROR,\n-\t\t\tflags & TRANSPORT_PUSH_FORCE);\n+\tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n+\t\treturn -1;\n \n-\t\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n-\t\t\tif (run_pre_push_hook(transport, remote_refs))\n-\t\t\t\treturn -1;\n+\tif (transport->smart_options &&\n+\t    transport->smart_options->cas &&\n+\t    !is_empty_cas(transport->smart_options->cas))\n+\t\tapply_push_cas(transport->smart_options->cas,\n+\t\t\t       transport->remote, remote_refs);\n \n-\t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t    !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (!push_unpushed_submodules(r,\n-\t\t\t\t\t\t      &commits,\n-\t\t\t\t\t\t      transport->remote,\n-\t\t\t\t\t\t      rs,\n-\t\t\t\t\t\t      transport->push_options,\n-\t\t\t\t\t\t      pretend)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n-\t\t\t\tdie(_(\"failed to push all needed submodules\"));\n-\t\t\t}\n+\tset_ref_status_for_push(remote_refs,\n+\t\tflags & TRANSPORT_PUSH_MIRROR,\n+\t\tflags & TRANSPORT_PUSH_FORCE);\n+\n+\tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n+\t\tif (run_pre_push_hook(transport, remote_refs))\n+\t\t\treturn -1;\n+\n+\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t    !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"push_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (!push_unpushed_submodules(r,\n+\t\t\t\t\t      &commits,\n+\t\t\t\t\t      transport->remote,\n+\t\t\t\t\t      rs,\n+\t\t\t\t\t      transport->push_options,\n+\t\t\t\t\t      pretend)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t\t\tdie(_(\"failed to push all needed submodules\"));\n \t\t}\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_submodules\", r);\n+\t}\n \n-\t\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n-\t\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n-\t\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t\t      !pretend)) && !is_bare_repository()) {\n-\t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n-\t\t\tstruct oid_array commits = OID_ARRAY_INIT;\n-\n-\t\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\toid_array_append(&commits,\n-\t\t\t\t\t\t\t  &ref->new_oid);\n-\n-\t\t\tif (find_unpushed_submodules(r,\n-\t\t\t\t\t\t     &commits,\n-\t\t\t\t\t\t     transport->remote->name,\n-\t\t\t\t\t\t     &needs_pushing)) {\n-\t\t\t\toid_array_clear(&commits);\n-\t\t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n-\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n-\t\t\t}\n-\t\t\tstring_list_clear(&needs_pushing, 0);\n+\tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n+\t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n+\t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n+\t      !pretend)) && !is_bare_repository()) {\n+\t\tstruct ref *ref = remote_refs;\n+\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n+\t\tstruct oid_array commits = OID_ARRAY_INIT;\n+\n+\t\ttrace2_region_enter(\"transport_push\", \"check_submodules\", r);\n+\t\tfor (; ref; ref = ref->next)\n+\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\toid_array_append(&commits,\n+\t\t\t\t\t\t  &ref->new_oid);\n+\n+\t\tif (find_unpushed_submodules(r,\n+\t\t\t\t\t     &commits,\n+\t\t\t\t\t     transport->remote->name,\n+\t\t\t\t\t     &needs_pushing)) {\n \t\t\toid_array_clear(&commits);\n \t\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n+\t\tstring_list_clear(&needs_pushing, 0);\n+\t\toid_array_clear(&commits);\n+\t\ttrace2_region_leave(\"transport_push\", \"check_submodules\", r);\n+\t}\n \n-\t\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n-\t\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n-\t\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n-\t\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n-\t\t} else\n-\t\t\tpush_ret = 0;\n-\t\terr = push_had_errors(remote_refs);\n-\t\tret = push_ret | err;\n-\n-\t\tif (!quiet || err)\n-\t\t\ttransport_print_push_status(transport->url, remote_refs,\n-\t\t\t\t\tverbose | porcelain, porcelain,\n-\t\t\t\t\treject_reasons);\n-\n-\t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n-\t\t\tset_upstreams(transport, remote_refs, pretend);\n-\n-\t\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n-\t\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n-\t\t\tstruct ref *ref;\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n-\t\t}\n+\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {\n+\t\ttrace2_region_enter(\"transport_push\", \"push_refs\", r);\n+\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n+\t\ttrace2_region_leave(\"transport_push\", \"push_refs\", r);\n+\t} else\n+\t\tpush_ret = 0;\n+\terr = push_had_errors(remote_refs);\n+\tret = push_ret | err;\n+\n+\tif (!quiet || err)\n+\t\ttransport_print_push_status(transport->url, remote_refs,\n+\t\t\t\tverbose | porcelain, porcelain,\n+\t\t\t\treject_reasons);\n+\n+\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n+\t\tset_upstreams(transport, remote_refs, pretend);\n+\n+\tif (!(flags & (TRANSPORT_PUSH_DRY_RUN |\n+\t\t       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {\n+\t\tstruct ref *ref;\n+\t\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\t\ttransport_update_tracking_ref(transport->remote, ref, verbose);\n+\t}\n \n-\t\tif (porcelain && !push_ret)\n-\t\t\tputs(\"Done\");\n-\t\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n-\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\tif (porcelain && !push_ret)\n+\t\tputs(\"Done\");\n+\telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n+\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n-\t\treturn ret;\n-\t}\n-\treturn 1;\n+\treturn ret;\n }\n \n const struct ref *transport_get_remote_refs(struct transport *transport,\n-- \n2.35.1\n\n"},{"id":"455566","messageId":"20220520124952.2393299-1-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520081723.1031830-1-frantisek@hrbata.com","subject":"[PATCH v3 0/3] fix memory leaks in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T12:49:49Z","receivedAt":"2022-05-20T12:50:05Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Fix memory leaks in transport_push(), where remote_refs and local_refs\nare never freed. While at it, remove the unnecessary indenting and make\nthe code hopefully more readable.\n\nChanges since v2:\n\n * \"transport: remove unnecessary indenting in transport_push()\"\n   s/push_refs/transport_push/ in commit message as noticed\n   by Ævar Arnfjörð Bjarmason\n\n * \"transport: unify return values and exit point from transport_push()\"\n   Added as suggested by Ævar Arnfjörð Bjarmason. It allows the following\n   memory leak fix to be a very simple patch.\n\n * \"transport: free local and remote refs in transport_push()\"\n   Just free remote_refs and local_refs. The other changes were\n   included in the previous patch.\n\nChanges since v1:\n\n * Slit into series of two patches. The first one just changes\n   indenting in transport_push(). Second one adds the fix for\n   the local_refs and remote_refs memory leaks.\n\n * The resulting trees are the same, there is no code change.\n\nFrantisek Hrbata (3):\n  transport: remove unnecessary indenting in transport_push()\n  transport: unify return values and exit point from transport_push()\n  transport: free local and remote refs in transport_push()\n\n transport.c | 260 +++++++++++++++++++++++++++-------------------------\n 1 file changed, 133 insertions(+), 127 deletions(-)\n\nRange-diff against v2:\n1:  daf042cd2e ! 1:  d986d8e7a9 transport: remove unnecessary indenting in transport_push()\n    @@ Metadata\n      ## Commit message ##\n         transport: remove unnecessary indenting in transport_push()\n     \n    -    Remove the big indented block for push_refs() check in transport vtable\n    +    Remove the big indented block for transport_push() check in transport vtable\n         and let's just return error immediately. Hopefully this makes the code\n         more readable.\n     \n2:  9d25176b92 ! 2:  4a69662b28 transport: free local and remote refs in transport_push()\n    @@ Metadata\n     Author: Frantisek Hrbata <frantisek@hrbata.com>\n     \n      ## Commit message ##\n    -    transport: free local and remote refs in transport_push()\n    +    transport: unify return values and exit point from transport_push()\n     \n    -    Fix memory leaks in transport_push(), where remote_refs and local_refs\n    -    are never freed.\n    -\n    -    116 bytes in 1 blocks are definitely lost in loss record 56 of 103\n    -       at 0x484486F: malloc (vg_replace_malloc.c:381)\n    -       by 0x4938D7E: strdup (strdup.c:42)\n    -       by 0x628418: xstrdup (wrapper.c:39)\n    -       by 0x4FD454: process_capabilities (connect.c:232)\n    -       by 0x4FD454: get_remote_heads (connect.c:354)\n    -       by 0x610A38: handshake (transport.c:333)\n    -       by 0x612B02: transport_push (transport.c:1302)\n    -       by 0x4803D6: push_with_options (push.c:357)\n    -       by 0x4811D6: do_push (push.c:414)\n    -       by 0x4811D6: cmd_push (push.c:650)\n    -       by 0x405210: run_builtin (git.c:465)\n    -       by 0x405210: handle_builtin (git.c:719)\n    -       by 0x406363: run_argv (git.c:786)\n    -       by 0x406363: cmd_main (git.c:917)\n    -       by 0x404F17: main (common-main.c:56)\n    -\n    -    5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103\n    -       at 0x4849464: calloc (vg_replace_malloc.c:1328)\n    -       by 0x628705: xcalloc (wrapper.c:150)\n    -       by 0x5C216D: alloc_ref_with_prefix (remote.c:975)\n    -       by 0x5C232A: alloc_ref (remote.c:983)\n    -       by 0x5C232A: one_local_ref (remote.c:2299)\n    -       by 0x5C232A: one_local_ref (remote.c:2289)\n    -       by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)\n    -       by 0x5B4C4F: do_for_each_ref (refs.c:1486)\n    -       by 0x5B4C4F: refs_for_each_ref (refs.c:1492)\n    -       by 0x5B4C4F: for_each_ref (refs.c:1497)\n    -       by 0x5C6ADF: get_local_heads (remote.c:2310)\n    -       by 0x612A85: transport_push (transport.c:1286)\n    -       by 0x4803D6: push_with_options (push.c:357)\n    -       by 0x4811D6: do_push (push.c:414)\n    -       by 0x4811D6: cmd_push (push.c:650)\n    -       by 0x405210: run_builtin (git.c:465)\n    -       by 0x405210: handle_builtin (git.c:719)\n    -       by 0x406363: run_argv (git.c:786)\n    -       by 0x406363: cmd_main (git.c:917)\n    +    It seems there is no reason to return 1 instead of -1 when push_refs()\n    +    is not set in transport vtable. Let's unify the error return values and\n    +    use the done label as a single exit point from transport_push().\n     \n    +    Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n         Signed-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n     \n      ## transport.c ##\n    @@ transport.c: int transport_push(struct repository *r,\n      \tint match_flags = MATCH_REFS_NONE;\n      \tint verbose = (transport->verbose > 0);\n      \tint quiet = (transport->verbose < 0);\n    -@@ transport.c: int transport_push(struct repository *r,\n    + \tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n    + \tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n    +-\tint push_ret, ret, err;\n    ++\tint push_ret, err;\n    ++\tint ret = -1;\n    + \tstruct transport_ls_refs_options transport_options =\n    + \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n    + \n    + \t*reject_reasons = 0;\n    + \n    + \tif (transport_color_config() < 0)\n    +-\t\treturn -1;\n    ++\t\tgoto done;\n    + \n      \tif (!transport->vtable->push_refs)\n    - \t\treturn 1;\n    +-\t\treturn 1;\n    ++\t\tgoto done;\n      \n    -+\tret = -1;\n      \tlocal_refs = get_local_heads();\n      \n      \tif (check_push_refs(local_refs, rs) < 0)\n    @@ transport.c: int transport_push(struct repository *r,\n      \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n      \n     +done:\n    -+\tfree_refs(local_refs);\n    -+\tfree_refs(remote_refs);\n      \treturn ret;\n      }\n      \n-:  ---------- > 3:  a4984a4835 transport: free local and remote refs in transport_push()\n\nbase-commit: e54793a95afeea1e10de1e5ad7eab914e7416250\n-- \n2.35.1\n\n"},{"id":"455567","messageId":"20220520124952.2393299-4-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520124952.2393299-1-frantisek@hrbata.com","subject":"[PATCH v3 3/3] transport: free local and remote refs in transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T12:49:52Z","receivedAt":"2022-05-20T12:50:07Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"Fix memory leaks in transport_push(), where remote_refs and local_refs\nare never freed.\n\n116 bytes in 1 blocks are definitely lost in loss record 56 of 103\n   at 0x484486F: malloc (vg_replace_malloc.c:381)\n   by 0x4938D7E: strdup (strdup.c:42)\n   by 0x628418: xstrdup (wrapper.c:39)\n   by 0x4FD454: process_capabilities (connect.c:232)\n   by 0x4FD454: get_remote_heads (connect.c:354)\n   by 0x610A38: handshake (transport.c:333)\n   by 0x612B02: transport_push (transport.c:1302)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n   by 0x404F17: main (common-main.c:56)\n\n5,912 (388 direct, 5,524 indirect) bytes in 2 blocks are definitely lost in loss record 98 of 103\n   at 0x4849464: calloc (vg_replace_malloc.c:1328)\n   by 0x628705: xcalloc (wrapper.c:150)\n   by 0x5C216D: alloc_ref_with_prefix (remote.c:975)\n   by 0x5C232A: alloc_ref (remote.c:983)\n   by 0x5C232A: one_local_ref (remote.c:2299)\n   by 0x5C232A: one_local_ref (remote.c:2289)\n   by 0x5BDB03: do_for_each_repo_ref_iterator (iterator.c:418)\n   by 0x5B4C4F: do_for_each_ref (refs.c:1486)\n   by 0x5B4C4F: refs_for_each_ref (refs.c:1492)\n   by 0x5B4C4F: for_each_ref (refs.c:1497)\n   by 0x5C6ADF: get_local_heads (remote.c:2310)\n   by 0x612A85: transport_push (transport.c:1286)\n   by 0x4803D6: push_with_options (push.c:357)\n   by 0x4811D6: do_push (push.c:414)\n   by 0x4811D6: cmd_push (push.c:650)\n   by 0x405210: run_builtin (git.c:465)\n   by 0x405210: handle_builtin (git.c:719)\n   by 0x406363: run_argv (git.c:786)\n   by 0x406363: cmd_main (git.c:917)\n\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\nFor remote_refs it might be worth to leave the freeing to\ntransport_disconnect() and introduce helper similar to\ntransport_get_remote_refs().\n\n transport.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport.c b/transport.c\nindex dd7fca51bf..110a0d5cc5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1419,6 +1419,8 @@ int transport_push(struct repository *r,\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n done:\n+\tfree_refs(local_refs);\n+\tfree_refs(remote_refs);\n \treturn ret;\n }\n \n-- \n2.35.1\n\n"},{"id":"455568","messageId":"20220520124952.2393299-3-frantisek@hrbata.com","threadId":"57902","inReplyTo":"20220520124952.2393299-1-frantisek@hrbata.com","subject":"[PATCH v3 2/3] transport: unify return values and exit point from transport_push()","fromName":"Frantisek Hrbata","fromEmail":"frantisek@hrbata.com","sentAt":"2022-05-20T12:49:51Z","receivedAt":"2022-05-20T12:50:09Z","isPatch":true,"sender":{"key":"frantisek@hrbata.com","avatar":"https://avatars.githubusercontent.com/u/3449323?v=4"},"body":"It seems there is no reason to return 1 instead of -1 when push_refs()\nis not set in transport vtable. Let's unify the error return values and\nuse the done label as a single exit point from transport_push().\n\nSuggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Frantisek Hrbata <frantisek@hrbata.com>\n---\n transport.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 0b9c5a427d..dd7fca51bf 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1276,29 +1276,30 @@ int transport_push(struct repository *r,\n \t\t   struct refspec *rs, int flags,\n \t\t   unsigned int *reject_reasons)\n {\n-\tstruct ref *remote_refs;\n-\tstruct ref *local_refs;\n+\tstruct ref *remote_refs = NULL;\n+\tstruct ref *local_refs = NULL;\n \tint match_flags = MATCH_REFS_NONE;\n \tint verbose = (transport->verbose > 0);\n \tint quiet = (transport->verbose < 0);\n \tint porcelain = flags & TRANSPORT_PUSH_PORCELAIN;\n \tint pretend = flags & TRANSPORT_PUSH_DRY_RUN;\n-\tint push_ret, ret, err;\n+\tint push_ret, err;\n+\tint ret = -1;\n \tstruct transport_ls_refs_options transport_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n \n \t*reject_reasons = 0;\n \n \tif (transport_color_config() < 0)\n-\t\treturn -1;\n+\t\tgoto done;\n \n \tif (!transport->vtable->push_refs)\n-\t\treturn 1;\n+\t\tgoto done;\n \n \tlocal_refs = get_local_heads();\n \n \tif (check_push_refs(local_refs, rs) < 0)\n-\t\treturn -1;\n+\t\tgoto done;\n \n \trefspec_ref_prefixes(rs, &transport_options.ref_prefixes);\n \n@@ -1319,7 +1320,7 @@ int transport_push(struct repository *r,\n \t\tmatch_flags |= MATCH_REFS_FOLLOW_TAGS;\n \n \tif (match_push_refs(local_refs, &remote_refs, rs, match_flags))\n-\t\treturn -1;\n+\t\tgoto done;\n \n \tif (transport->smart_options &&\n \t    transport->smart_options->cas &&\n@@ -1333,7 +1334,7 @@ int transport_push(struct repository *r,\n \n \tif (!(flags & TRANSPORT_PUSH_NO_HOOK))\n \t\tif (run_pre_push_hook(transport, remote_refs))\n-\t\t\treturn -1;\n+\t\t\tgoto done;\n \n \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n@@ -1417,6 +1418,7 @@ int transport_push(struct repository *r,\n \telse if (!quiet && !ret && !transport_refs_pushed(remote_refs))\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \n+done:\n \treturn ret;\n }\n \n-- \n2.35.1\n\n"},{"id":"456320","messageId":"YpEzHfrD4e0S2Sjq@google.com","threadId":"57902","inReplyTo":"20220520124952.2393299-1-frantisek@hrbata.com","subject":"Re: [PATCH v3 0/3] fix memory leaks in transport_push()","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-05-27T20:22:53Z","receivedAt":"2022-05-27T20:23:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.05.20 14:49, Frantisek Hrbata wrote:\n> Fix memory leaks in transport_push(), where remote_refs and local_refs\n> are never freed. While at it, remove the unnecessary indenting and make\n> the code hopefully more readable.\n> \n> Changes since v2:\n> \n>  * \"transport: remove unnecessary indenting in transport_push()\"\n>    s/push_refs/transport_push/ in commit message as noticed\n>    by Ævar Arnfjörð Bjarmason\n> \n>  * \"transport: unify return values and exit point from transport_push()\"\n>    Added as suggested by Ævar Arnfjörð Bjarmason. It allows the following\n>    memory leak fix to be a very simple patch.\n> \n>  * \"transport: free local and remote refs in transport_push()\"\n>    Just free remote_refs and local_refs. The other changes were\n>    included in the previous patch.\n> \n> Changes since v1:\n> \n>  * Slit into series of two patches. The first one just changes\n>    indenting in transport_push(). Second one adds the fix for\n>    the local_refs and remote_refs memory leaks.\n> \n>  * The resulting trees are the same, there is no code change.\n> \n> Frantisek Hrbata (3):\n>   transport: remove unnecessary indenting in transport_push()\n>   transport: unify return values and exit point from transport_push()\n>   transport: free local and remote refs in transport_push()\n> \n>  transport.c | 260 +++++++++++++++++++++++++++-------------------------\n>  1 file changed, 133 insertions(+), 127 deletions(-)\n\nSorry for the late review. This all looks good to me, thanks for the\nfix!\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"}]}