Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Oct 10, 2017, 13:43 UTC
- Message-ID
- <20171010134309.13803-1-szeder.dev@gmail.com>
- In-Reply-To
- <20171010040604.26029-2-gitster@pobox.com>
Show 16 quoted lines
> 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
Show 57 quoted lines
> 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 <gitster@pobox.com>
> ---
> 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