[v2] common-main.c: call exit(), don't return

Commit Message

Ævar Arnfjörð Bjarmason Dec. 7, 2021, 10:13 a.m. UTC

Change the main() function to call "exit()" instead of ending with a
"return" statement. The "exit()" function is our own wrapper that
calls trace2_cmd_exit_fl() for us, from git-compat-util.h:

	#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))

That "exit()" wrapper has been in use ever since ee4512ed481 (trace2:
create new combined trace facility, 2019-02-22).

This changes nothing about how we "exit()", as we'd invoke
"trace2_cmd_exit_fl()" in both cases due to the wrapper, this change
makes it easier to reason about this code, as we're now always
obviously relying on our "exit()" wrapper.

There is already code immediately downstream of our "main()" which has
a hard reliance on that, e.g. the various "exit()" calls downstream of
"cmd_main()" in "git.c".

We even had a comment in "t/helper/test-trace2.c" that seemed to be
confused about how the "exit()" wrapper interacted with uses of
"return", even though it was introduced in the same trace2 series in
a15860dca3f (trace2: t/helper/test-trace2, t0210.sh, t0211.sh,
t0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps
it pre-dated the "exit()" wrapper?

This change makes the "trace2_cmd_exit()" macro orphaned, we now
always use "trace2_cmd_exit_fl()" directly, but let's keep that
simpler example in place. Even if we're unlikely to get another
"main()" other than the one in our "common-main.c", there's some value
in having the API documentation and example discuss a simpler version
that doesn't require an "exit()" wrapper macro.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---

Junio: I think this addresses all the feedback you had on the v1 in
[1]. Thanks for the review.

I wasn't sure what trade-off to strike with leaving that small amount
of dead in-tree code, but per the updated commit message I think it
makes sense to have the API docs discuss the simpler example & keep
the macro, as you suggest.

1. https://lore.kernel.org/git/xmqqzgpdfub1.fsf@gitster.g/

Range-diff against v1:
1:  6fedf9969b6 ! 1:  4f52ecc94ba common-main.c: call exit(), don't return
    @@ Commit message
         t0212.sh, 2019-02-22), after the aforementioned ee4512ed481. Perhaps
         it pre-dated the "exit()" wrapper?
     
    -    Let's also update both the documentation and comments accordingly: The
    -    documentation added in e544221d97a (trace2:
    -    Documentation/technical/api-trace2.txt, 2019-02-22) already said of
    -    the "exit" event that "[it] is emitted when git calls `exit()". But
    -    the "main()" example then called trace2_cmd_exit(). Let's have it
    -    invoke "exit()" instead, as the code in "common-main.c" now does.
    +    This change makes the "trace2_cmd_exit()" macro orphaned, we now
    +    always use "trace2_cmd_exit_fl()" directly, but let's keep that
    +    simpler example in place. Even if we're unlikely to get another
    +    "main()" other than the one in our "common-main.c", there's some value
    +    in having the API documentation and example discuss a simpler version
    +    that doesn't require an "exit()" wrapper macro.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    - ## Documentation/technical/api-trace2.txt ##
    -@@ Documentation/technical/api-trace2.txt: Initialization::
    - ----------------
    - int main(int argc, const char **argv)
    - {
    --	int exit_code;
    --
    - 	trace2_initialize();
    - 	trace2_cmd_start(argv);
    - 
    --	exit_code = cmd_main(argc, argv);
    --
    --	trace2_cmd_exit(exit_code);
    --
    --	return exit_code;
    -+	/* Our exit() will call trace2_cmd_exit_fl() */
    -+	exit(cmd_main(argc, argv));
    - }
    - ----------------
    - 
    -
      ## common-main.c ##
     @@ common-main.c: int main(int argc, const char **argv)
      
    @@ t/helper/test-trace2.c: static int print_usage(void)
       *
     - * We further assume that if we return (rather than exit()), trace2_cmd_exit()
     - * will be called by test-tool.c:cmd_main().
    -+ * It doesn't matter if we "return" here or call "exit()", since our
    -+ * "exit()" is a wrapper that will call trace2_cmd_exit_fl. It would
    -+ * matter if we bypassed it and called "_exit()". Even if it doesn't
    -+ * matter for the narrow case of trace2 testing, let's be nice to
    -+ * test-tool.c's "cmd_main()" and common-main.c's "main()" and
    -+ * "return" here.
    ++ * We return from here and let test-tool.c::cmd_main() pass the exit
    ++ * code to common-main.c::main(), which will use it to call
    ++ * trace2_cmd_exit().
       */
      int cmd__trace2(int argc, const char **argv)
      {
    -
    - ## trace2.h ##
    -@@ trace2.h: void trace2_cmd_start_fl(const char *file, int line, const char **argv);
    -  */
    - int trace2_cmd_exit_fl(const char *file, int line, int code);
    - 
    --#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
    --
    - /*
    -  * Emit an 'error' event.
    -  *

 common-main.c          | 9 ++++++---
 t/helper/test-trace2.c | 5 +++--
 2 files changed, 9 insertions(+), 5 deletions(-)

Patch

diff --git a/common-main.c b/common-main.c
index 71e21dd20a3..eafc70718a5 100644
--- a/common-main.c
+++ b/common-main.c
@@ -51,7 +51,10 @@  int main(int argc, const char **argv)
 
 	result = cmd_main(argc, argv);
 
-	trace2_cmd_exit(result);
-
-	return result;
+	/*
+	 * We define exit() to call trace2_cmd_exit_fl() in
+	 * git-compat-util.h. Whether we reach this or exit()
+	 * elsewhere we'll always run our trace2 exit handler.
+	 */
+	exit(result);
 }
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index f93633f895a..59b124bb5f1 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -262,8 +262,9 @@  static int print_usage(void)
  *    [] the "cmd_name" event has been generated.
  *    [] this writes various "def_param" events for interesting config values.
  *
- * We further assume that if we return (rather than exit()), trace2_cmd_exit()
- * will be called by test-tool.c:cmd_main().
+ * We return from here and let test-tool.c::cmd_main() pass the exit
+ * code to common-main.c::main(), which will use it to call
+ * trace2_cmd_exit().
  */
 int cmd__trace2(int argc, const char **argv)
 {