Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine
flat view
From: SZEDER Gábor <hidden>
Date: 2017-10-10 13:43:27
The cmd_foo() function is a moral equivalent of 'main' for a Git subcommand 'git foo', and as such, it is allowed to do many things that make it unsuitable to be called as a subroutine, including - call exit(3) to terminate the process; - allocate resource held and used throughout its lifetime, without releasing it upon return/exit; - rely on global variables being initialized at program startup, and update them as needed, making another clean invocation of the function impossible. The call to cmd_diff_index() "git describe" makes has been working by accident that the function did not call exit(3); it sets a bad precedent for people to cut and paste.
The subject implies to me that this patch eliminates all cmd_*() calls in builtin/describe.c, but a call to cmd_name_rev() still remains. However, that call is special in the sense that cmd_describe() returns immediately thereafter, so none of the above three points are an issue there. Someone might argue that it still sets a bad precedent, but I won't :) To avoid the direct cmd_name_rev() call we would have to use run_command(), because there are no libified helper functions available to do the job, adding the overhead of a fork()+exec(), though only once or nonce, depending on cmdline options. Maybe you already considered all this WRT that cmd_name_rev() call, I don't know. In any case, I think at least the subject line should spell out cmd_diff_index(). Gábor
quoted hunk ↗ jump to hunk
We could invoke it via the run_command() interface, but the diff family of commands have helper functions in diff-lib.c that are meant to be usable as subroutines, and using the latter does not make the resulting code all that longer. Use it. Signed-off-by: Junio C Hamano <redacted> --- builtin/describe.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)diff --git a/builtin/describe.c b/builtin/describe.c index 89ea1cdd60..50263759cb 100644 --- a/builtin/describe.c +++ b/builtin/describe.c@@ -7,12 +7,12 @@ #include "builtin.h" #include "exec_cmd.h" #include "parse-options.h" +#include "revision.h" #include "diff.h" #include "hashmap.h" #include "argv-array.h" #include "run-command.h" -#define SEEN (1u << 0) #define MAX_TAGS (FLAG_BITS - 1) static const char * const describe_usage[] = {@@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix) } } else if (dirty) { static struct lock_file index_lock; - int fd; + struct rev_info revs; + struct argv_array args = ARGV_ARRAY_INIT; + int fd, result; read_cache_preload(NULL); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,@@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (0 <= fd) update_index_if_able(&the_index, &index_lock); - if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, - diff_index_args, prefix)) + init_revisions(&revs, prefix); + argv_array_pushv(&args, diff_index_args); + if (setup_revisions(args.argc, args.argv, &revs, NULL) != 1) + BUG("malformed internal diff-index command line"); + result = run_diff_index(&revs, 0); + + if (!diff_result_code(&revs.diffopt, result)) suffix = NULL; else suffix = dirty;-- 2.15.0-rc0-203-g4c8d0e28b1