{"thread":{"id":"57037","subject":"[PATCH v1] common-main.c: call exit(), don't return","startedAt":"2021-12-06T16:15:58Z","lastAt":"2021-12-07T10:13:58Z","messageCount":3,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"443164","messageId":"patch-v1-1.1-6fedf9969b6-20211206T161001Z-avarab@gmail.com","threadId":"57037","inReplyTo":null,"subject":"[PATCH v1] common-main.c: call exit(), don't return","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-06T16:11:03Z","receivedAt":"2021-12-06T16:15:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the main() function to call \"exit()\" instead of ending with a\n\"return\" statement. The \"exit()\" function is our own wrapper that\ncalls trace2_cmd_exit_fl() for us, from git-compat-util.h:\n\n\t#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n\nThat \"exit()\" wrapper has been in use ever since ee4512ed481 (trace2:\ncreate new combined trace facility, 2019-02-22).\n\nThis changes nothing about how we \"exit()\", as we'd invoke\n\"trace2_cmd_exit_fl()\" in both cases due to the wrapper, this change\nmakes it easier to reason about this code, as we're now always\nobviously relying on our \"exit()\" wrapper.\n\nThere is already code immediately downstream of our \"main()\" which has\na hard reliance on that, e.g. the various \"exit()\" calls downstream of\n\"cmd_main()\" in \"git.c\".\n\nWe even had a comment in \"t/helper/test-trace2.c\" that seemed to be\nconfused about how the \"exit()\" wrapper interacted with uses of\n\"return\", even though it was introduced in the same trace2 series in\na15860dca3f (trace2: t/helper/test-trace2, t0210.sh, t0211.sh,\nt0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps\nit pre-dated the \"exit()\" wrapper?\n\nLet's also update both the documentation and comments accordingly: The\ndocumentation added in e544221d97a (trace2:\nDocumentation/technical/api-trace2.txt, 2019-02-22) already said of\nthe \"exit\" event that \"[it] is emitted when git calls `exit()\". But\nthe \"main()\" example then called trace2_cmd_exit(). Let's have it\ninvoke \"exit()\" instead, as the code in \"common-main.c\" now does.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThe diffstat here is neutral, and as noted this changes no behavior,\nso this isn't really needed for anything.\n\nBut as argued above I think this makes things a lot easier for readers\nof the code. I've had at least a couple of traces through git's\nexecution from command-main.c downwards and thought that the \"exit()\"\ncalls in git.c might be a bug, until I (re-)discovered that we're\ndefining an exit() wrapper via a macro.\n\nIt might be a good follow-up at some point to see if we could hoist\nsome of the cleanups we do in run_builtin() to to this level. I.e. the\ncode referenced in my 338abb0f045 (builtins + test helpers: use return\ninstead of exit() in cmd_*, 2021-06-08) (and similar).\n\nEven that code could probably do with some tweaks, e.g. we should\nprobably try to fflush() stdout/stderr even if we're about to return\nnon-zero (now we'll only do it on success).\n\nBut for now this is one small readability improvement to some code\ncentral to git's execution.\n\nA version of this was originally submitted as\nhttps://lore.kernel.org/git/RFC-patch-07.21-3f897bf6b0e-20211115T220831Z-avarab@gmail.com/;\nrange-diff against that initial version below.\n\nRange-diff against v0:\n1:  3f897bf6b0e ! 1:  6fedf9969b6 common-main.c: call exit(), don't return\n    @@ Metadata\n      ## Commit message ##\n         common-main.c: call exit(), don't return\n     \n    -    Refactor the main() function so that we always take the same path\n    -    towards trace2_cmd_exit() whether exit() is invoked, or we end up in\n    -    the \"return\" in the pre-image. This contains no functional change, and\n    -    is only intended for the benefit of readers of the code, who'll now be\n    -    pointed to our exit() wrapper.\n    -\n    -    Since ee4512ed481 (trace2: create new combined trace facility,\n    -    2019-02-22) we've defined \"exit\" with a macro to call\n    -    trace2_cmd_exit() for us in \"git-compat-util.h\". So in cases where an\n    -    exit() is invoked (such as in several places in \"git.c\") we don't\n    -    reach the trace2_cmd_exit() in the pre-image. This makes it so that\n    -    we'll always take that same exit() path.\n    +    Change the main() function to call \"exit()\" instead of ending with a\n    +    \"return\" statement. The \"exit()\" function is our own wrapper that\n    +    calls trace2_cmd_exit_fl() for us, from git-compat-util.h:\n    +\n    +            #define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n    +\n    +    That \"exit()\" wrapper has been in use ever since ee4512ed481 (trace2:\n    +    create new combined trace facility, 2019-02-22).\n    +\n    +    This changes nothing about how we \"exit()\", as we'd invoke\n    +    \"trace2_cmd_exit_fl()\" in both cases due to the wrapper, this change\n    +    makes it easier to reason about this code, as we're now always\n    +    obviously relying on our \"exit()\" wrapper.\n    +\n    +    There is already code immediately downstream of our \"main()\" which has\n    +    a hard reliance on that, e.g. the various \"exit()\" calls downstream of\n    +    \"cmd_main()\" in \"git.c\".\n    +\n    +    We even had a comment in \"t/helper/test-trace2.c\" that seemed to be\n    +    confused about how the \"exit()\" wrapper interacted with uses of\n    +    \"return\", even though it was introduced in the same trace2 series in\n    +    a15860dca3f (trace2: t/helper/test-trace2, t0210.sh, t0211.sh,\n    +    t0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps\n    +    it pre-dated the \"exit()\" wrapper?\n    +\n    +    Let's also update both the documentation and comments accordingly: The\n    +    documentation added in e544221d97a (trace2:\n    +    Documentation/technical/api-trace2.txt, 2019-02-22) already said of\n    +    the \"exit\" event that \"[it] is emitted when git calls `exit()\". But\n    +    the \"main()\" example then called trace2_cmd_exit(). Let's have it\n    +    invoke \"exit()\" instead, as the code in \"common-main.c\" now does.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    + ## Documentation/technical/api-trace2.txt ##\n    +@@ Documentation/technical/api-trace2.txt: Initialization::\n    + ----------------\n    + int main(int argc, const char **argv)\n    + {\n    +-\tint exit_code;\n    +-\n    + \ttrace2_initialize();\n    + \ttrace2_cmd_start(argv);\n    + \n    +-\texit_code = cmd_main(argc, argv);\n    +-\n    +-\ttrace2_cmd_exit(exit_code);\n    +-\n    +-\treturn exit_code;\n    ++\t/* Our exit() will call trace2_cmd_exit_fl() */\n    ++\texit(cmd_main(argc, argv));\n    + }\n    + ----------------\n    + \n    +\n      ## common-main.c ##\n     @@ common-main.c: int main(int argc, const char **argv)\n      \n    @@ common-main.c: int main(int argc, const char **argv)\n     +\t */\n     +\texit(result);\n      }\n    +\n    + ## t/helper/test-trace2.c ##\n    +@@ t/helper/test-trace2.c: static int print_usage(void)\n    +  *    [] the \"cmd_name\" event has been generated.\n    +  *    [] this writes various \"def_param\" events for interesting config values.\n    +  *\n    +- * We further assume that if we return (rather than exit()), trace2_cmd_exit()\n    +- * will be called by test-tool.c:cmd_main().\n    ++ * It doesn't matter if we \"return\" here or call \"exit()\", since our\n    ++ * \"exit()\" is a wrapper that will call trace2_cmd_exit_fl. It would\n    ++ * matter if we bypassed it and called \"_exit()\". Even if it doesn't\n    ++ * matter for the narrow case of trace2 testing, let's be nice to\n    ++ * test-tool.c's \"cmd_main()\" and common-main.c's \"main()\" and\n    ++ * \"return\" here.\n    +  */\n    + int cmd__trace2(int argc, const char **argv)\n    + {\n    +\n    + ## trace2.h ##\n    +@@ trace2.h: void trace2_cmd_start_fl(const char *file, int line, const char **argv);\n    +  */\n    + int trace2_cmd_exit_fl(const char *file, int line, int code);\n    + \n    +-#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n    +-\n    + /*\n    +  * Emit an 'error' event.\n    +  *\n\n Documentation/technical/api-trace2.txt | 9 ++-------\n common-main.c                          | 9 ++++++---\n t/helper/test-trace2.c                 | 8 ++++++--\n trace2.h                               | 2 --\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\nindex bb13ca3db8b..568a909222a 100644\n--- a/Documentation/technical/api-trace2.txt\n+++ b/Documentation/technical/api-trace2.txt\n@@ -828,16 +828,11 @@ Initialization::\n ----------------\n int main(int argc, const char **argv)\n {\n-\tint exit_code;\n-\n \ttrace2_initialize();\n \ttrace2_cmd_start(argv);\n \n-\texit_code = cmd_main(argc, argv);\n-\n-\ttrace2_cmd_exit(exit_code);\n-\n-\treturn exit_code;\n+\t/* Our exit() will call trace2_cmd_exit_fl() */\n+\texit(cmd_main(argc, argv));\n }\n ----------------\n \ndiff --git a/common-main.c b/common-main.c\nindex 71e21dd20a3..eafc70718a5 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -51,7 +51,10 @@ int main(int argc, const char **argv)\n \n \tresult = cmd_main(argc, argv);\n \n-\ttrace2_cmd_exit(result);\n-\n-\treturn result;\n+\t/*\n+\t * We define exit() to call trace2_cmd_exit_fl() in\n+\t * git-compat-util.h. Whether we reach this or exit()\n+\t * elsewhere we'll always run our trace2 exit handler.\n+\t */\n+\texit(result);\n }\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex f93633f895a..9954010bc89 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -262,8 +262,12 @@ static int print_usage(void)\n  *    [] the \"cmd_name\" event has been generated.\n  *    [] this writes various \"def_param\" events for interesting config values.\n  *\n- * We further assume that if we return (rather than exit()), trace2_cmd_exit()\n- * will be called by test-tool.c:cmd_main().\n+ * It doesn't matter if we \"return\" here or call \"exit()\", since our\n+ * \"exit()\" is a wrapper that will call trace2_cmd_exit_fl. It would\n+ * matter if we bypassed it and called \"_exit()\". Even if it doesn't\n+ * matter for the narrow case of trace2 testing, let's be nice to\n+ * test-tool.c's \"cmd_main()\" and common-main.c's \"main()\" and\n+ * \"return\" here.\n  */\n int cmd__trace2(int argc, const char **argv)\n {\ndiff --git a/trace2.h b/trace2.h\nindex 0cc7b5f5312..73876781294 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -110,8 +110,6 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv);\n  */\n int trace2_cmd_exit_fl(const char *file, int line, int code);\n \n-#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n-\n /*\n  * Emit an 'error' event.\n  *\n-- \n2.34.1.898.g5a552c2e5f0\n\n"},{"id":"443186","messageId":"xmqqzgpdfub1.fsf@gitster.g","threadId":"57037","inReplyTo":"patch-v1-1.1-6fedf9969b6-20211206T161001Z-avarab@gmail.com","subject":"Re: [PATCH v1] common-main.c: call exit(), don't return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-06T18:43:30Z","receivedAt":"2021-12-06T18:43:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the main() function to call \"exit()\" instead of ending with a\n> \"return\" statement. The \"exit()\" function is our own wrapper that\n> calls trace2_cmd_exit_fl() for us, from git-compat-util.h:\n>\n> \t#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n>\n> That \"exit()\" wrapper has been in use ever since ee4512ed481 (trace2:\n> create new combined trace facility, 2019-02-22).\n>\n> This changes nothing about how we \"exit()\", as we'd invoke\n> \"trace2_cmd_exit_fl()\" in both cases due to the wrapper, this change\n> makes it easier to reason about this code, as we're now always\n> obviously relying on our \"exit()\" wrapper.\n\nUp to this point, this makes readers expect that the result would be\na simple patch\n\n    diff --git i/common-main.c w/common-main.c\n    index 71e21dd20a..f6b3a18c7f 100644\n    --- i/common-main.c\n    +++ w/common-main.c\n    @@ -49,9 +49,5 @@ int main(int argc, const char **argv)\n            trace2_cmd_start(argv);\n            trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);\n\n    -\tresult = cmd_main(argc, argv);\n    -\n    -\ttrace2_cmd_exit(result);\n    -\n    -\treturn result;\n    +\texit(cmd_main(argc, argv));\n     }\n\nand that would have been justified quite well.  trace2_cmd_exit() is \na thin wrapper around trace2_cmd_exit_fl and the whole thing amounts\nto the same thing given how exit() is defined in compat-util as the\nearlier part of the log message showed us.\n\n>  Documentation/technical/api-trace2.txt | 9 ++-------\n>  common-main.c                          | 9 ++++++---\n>  t/helper/test-trace2.c                 | 8 ++++++--\n>  trace2.h                               | 2 --\n>  4 files changed, 14 insertions(+), 14 deletions(-)\n\nSo readers would naturally wonder why do we have all other changes.\nLet's find out.\n\n> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\n> index bb13ca3db8b..568a909222a 100644\n> --- a/Documentation/technical/api-trace2.txt\n> +++ b/Documentation/technical/api-trace2.txt\n> @@ -828,16 +828,11 @@ Initialization::\n>  ----------------\n>  int main(int argc, const char **argv)\n>  {\n> -\tint exit_code;\n> -\n>  \ttrace2_initialize();\n>  \ttrace2_cmd_start(argv);\n>  \n> -\texit_code = cmd_main(argc, argv);\n> -\n> -\ttrace2_cmd_exit(exit_code);\n> -\n> -\treturn exit_code;\n> +\t/* Our exit() will call trace2_cmd_exit_fl() */\n> +\texit(cmd_main(argc, argv));\n>  }\n>  ----------------\n\nThe comment above this hunk says\n\n    == Example Trace2 API Usage\n\n    Here is a hypothetical usage of the Trace2 API showing the intended\n    usage (without worrying about the actual Git details).\n\n    Initialization::\n\n            Initialization happens in `main()`.  Behind the scenes, an\n            `atexit` and `signal` handler are registered.\n\nI doubt that \"Our exit() does this, so trace2_cmd_exit() is not\nnecessary\" sits well in that context.\n\n> diff --git a/common-main.c b/common-main.c\n> index 71e21dd20a3..eafc70718a5 100644\n> --- a/common-main.c\n> +++ b/common-main.c\n> @@ -51,7 +51,10 @@ int main(int argc, const char **argv)\n>  \n>  \tresult = cmd_main(argc, argv);\n>  \n> -\ttrace2_cmd_exit(result);\n> -\n> -\treturn result;\n> +\t/*\n> +\t * We define exit() to call trace2_cmd_exit_fl() in\n> +\t * git-compat-util.h. Whether we reach this or exit()\n> +\t * elsewhere we'll always run our trace2 exit handler.\n> +\t */\n> +\texit(result);\n>  }\n\nThis is good; the new comment is also good.\n\n> diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\n> index f93633f895a..9954010bc89 100644\n> --- a/t/helper/test-trace2.c\n> +++ b/t/helper/test-trace2.c\n> @@ -262,8 +262,12 @@ static int print_usage(void)\n>   *    [] the \"cmd_name\" event has been generated.\n>   *    [] this writes various \"def_param\" events for interesting config values.\n>   *\n> - * We further assume that if we return (rather than exit()), trace2_cmd_exit()\n> - * will be called by test-tool.c:cmd_main().\n> + * It doesn't matter if we \"return\" here or call \"exit()\", since our\n> + * \"exit()\" is a wrapper that will call trace2_cmd_exit_fl. It would\n> + * matter if we bypassed it and called \"_exit()\". Even if it doesn't\n> + * matter for the narrow case of trace2 testing, let's be nice to\n> + * test-tool.c's \"cmd_main()\" and common-main.c's \"main()\" and\n> + * \"return\" here.\n>   */\n\nHmph.  While nothing is technically incorrect in the new text, I\nthink small fixup to the original should suffice.\n\n    We return from here and let test-tool.c::cmd_main() pass the\n    exit code to common-main.c::main(), which will use it to call\n    trace2_cmd_exit().\n\nThere is nothing gained by saying \"we assume it is OK to do this\"\nwhen \"we do this\" is enough.  \"it does not matter if we did this\nor did this some other way, but we chose this because\" is something\nyou'd want in the log message but not here.\n\n>  int cmd__trace2(int argc, const char **argv)\n>  {\n> diff --git a/trace2.h b/trace2.h\n> index 0cc7b5f5312..73876781294 100644\n> --- a/trace2.h\n> +++ b/trace2.h\n> @@ -110,8 +110,6 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv);\n>   */\n>  int trace2_cmd_exit_fl(const char *file, int line, int code);\n>  \n> -#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n> -\n\nThis is documented in Documentation/technical/api-trace2.txt and I\ndo not think the proposed update in this patch is a good idea, which\nmakes it better to keep this #define here.\n"},{"id":"443251","messageId":"patch-v2-1.1-4f52ecc94ba-20211207T101207Z-avarab@gmail.com","threadId":"57037","inReplyTo":"patch-v1-1.1-6fedf9969b6-20211206T161001Z-avarab@gmail.com","subject":"[PATCH v2] common-main.c: call exit(), don't return","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-07T10:13:51Z","receivedAt":"2021-12-07T10:13:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the main() function to call \"exit()\" instead of ending with a\n\"return\" statement. The \"exit()\" function is our own wrapper that\ncalls trace2_cmd_exit_fl() for us, from git-compat-util.h:\n\n\t#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n\nThat \"exit()\" wrapper has been in use ever since ee4512ed481 (trace2:\ncreate new combined trace facility, 2019-02-22).\n\nThis changes nothing about how we \"exit()\", as we'd invoke\n\"trace2_cmd_exit_fl()\" in both cases due to the wrapper, this change\nmakes it easier to reason about this code, as we're now always\nobviously relying on our \"exit()\" wrapper.\n\nThere is already code immediately downstream of our \"main()\" which has\na hard reliance on that, e.g. the various \"exit()\" calls downstream of\n\"cmd_main()\" in \"git.c\".\n\nWe even had a comment in \"t/helper/test-trace2.c\" that seemed to be\nconfused about how the \"exit()\" wrapper interacted with uses of\n\"return\", even though it was introduced in the same trace2 series in\na15860dca3f (trace2: t/helper/test-trace2, t0210.sh, t0211.sh,\nt0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps\nit pre-dated the \"exit()\" wrapper?\n\nThis change makes the \"trace2_cmd_exit()\" macro orphaned, we now\nalways use \"trace2_cmd_exit_fl()\" directly, but let's keep that\nsimpler example in place. Even if we're unlikely to get another\n\"main()\" other than the one in our \"common-main.c\", there's some value\nin having the API documentation and example discuss a simpler version\nthat doesn't require an \"exit()\" wrapper macro.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nJunio: I think this addresses all the feedback you had on the v1 in\n[1]. Thanks for the review.\n\nI wasn't sure what trade-off to strike with leaving that small amount\nof dead in-tree code, but per the updated commit message I think it\nmakes sense to have the API docs discuss the simpler example & keep\nthe macro, as you suggest.\n\n1. https://lore.kernel.org/git/xmqqzgpdfub1.fsf@gitster.g/\n\nRange-diff against v1:\n1:  6fedf9969b6 ! 1:  4f52ecc94ba common-main.c: call exit(), don't return\n    @@ Commit message\n         t0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps\n         it pre-dated the \"exit()\" wrapper?\n     \n    -    Let's also update both the documentation and comments accordingly: The\n    -    documentation added in e544221d97a (trace2:\n    -    Documentation/technical/api-trace2.txt, 2019-02-22) already said of\n    -    the \"exit\" event that \"[it] is emitted when git calls `exit()\". But\n    -    the \"main()\" example then called trace2_cmd_exit(). Let's have it\n    -    invoke \"exit()\" instead, as the code in \"common-main.c\" now does.\n    +    This change makes the \"trace2_cmd_exit()\" macro orphaned, we now\n    +    always use \"trace2_cmd_exit_fl()\" directly, but let's keep that\n    +    simpler example in place. Even if we're unlikely to get another\n    +    \"main()\" other than the one in our \"common-main.c\", there's some value\n    +    in having the API documentation and example discuss a simpler version\n    +    that doesn't require an \"exit()\" wrapper macro.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    - ## Documentation/technical/api-trace2.txt ##\n    -@@ Documentation/technical/api-trace2.txt: Initialization::\n    - ----------------\n    - int main(int argc, const char **argv)\n    - {\n    --\tint exit_code;\n    --\n    - \ttrace2_initialize();\n    - \ttrace2_cmd_start(argv);\n    - \n    --\texit_code = cmd_main(argc, argv);\n    --\n    --\ttrace2_cmd_exit(exit_code);\n    --\n    --\treturn exit_code;\n    -+\t/* Our exit() will call trace2_cmd_exit_fl() */\n    -+\texit(cmd_main(argc, argv));\n    - }\n    - ----------------\n    - \n    -\n      ## common-main.c ##\n     @@ common-main.c: int main(int argc, const char **argv)\n      \n    @@ t/helper/test-trace2.c: static int print_usage(void)\n       *\n     - * We further assume that if we return (rather than exit()), trace2_cmd_exit()\n     - * will be called by test-tool.c:cmd_main().\n    -+ * It doesn't matter if we \"return\" here or call \"exit()\", since our\n    -+ * \"exit()\" is a wrapper that will call trace2_cmd_exit_fl. It would\n    -+ * matter if we bypassed it and called \"_exit()\". Even if it doesn't\n    -+ * matter for the narrow case of trace2 testing, let's be nice to\n    -+ * test-tool.c's \"cmd_main()\" and common-main.c's \"main()\" and\n    -+ * \"return\" here.\n    ++ * We return from here and let test-tool.c::cmd_main() pass the exit\n    ++ * code to common-main.c::main(), which will use it to call\n    ++ * trace2_cmd_exit().\n       */\n      int cmd__trace2(int argc, const char **argv)\n      {\n    -\n    - ## trace2.h ##\n    -@@ trace2.h: void trace2_cmd_start_fl(const char *file, int line, const char **argv);\n    -  */\n    - int trace2_cmd_exit_fl(const char *file, int line, int code);\n    - \n    --#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n    --\n    - /*\n    -  * Emit an 'error' event.\n    -  *\n\n common-main.c          | 9 ++++++---\n t/helper/test-trace2.c | 5 +++--\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/common-main.c b/common-main.c\nindex 71e21dd20a3..eafc70718a5 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -51,7 +51,10 @@ int main(int argc, const char **argv)\n \n \tresult = cmd_main(argc, argv);\n \n-\ttrace2_cmd_exit(result);\n-\n-\treturn result;\n+\t/*\n+\t * We define exit() to call trace2_cmd_exit_fl() in\n+\t * git-compat-util.h. Whether we reach this or exit()\n+\t * elsewhere we'll always run our trace2 exit handler.\n+\t */\n+\texit(result);\n }\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex f93633f895a..59b124bb5f1 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -262,8 +262,9 @@ static int print_usage(void)\n  *    [] the \"cmd_name\" event has been generated.\n  *    [] this writes various \"def_param\" events for interesting config values.\n  *\n- * We further assume that if we return (rather than exit()), trace2_cmd_exit()\n- * will be called by test-tool.c:cmd_main().\n+ * We return from here and let test-tool.c::cmd_main() pass the exit\n+ * code to common-main.c::main(), which will use it to call\n+ * trace2_cmd_exit().\n  */\n int cmd__trace2(int argc, const char **argv)\n {\n-- \n2.34.1.898.g5a552c2e5f0\n\n"}]}