Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine

["SZEDER Gábor" <szeder.dev@gmail.com>]

> 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


> 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