From be26d2b29b6a58fc53ed9d38c3f36b195f8a7cc9 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 Oct 2017 12:42:58 +0900 Subject: [PATCH 1/2] describe: do not use cmd_*() as a subroutine 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. 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. Note that there is also an invocation of cmd_name_rev() at the end; "git describe --contains" massages its command line arguments to be suitable for "git name-rev" invocation and jumps to it, never to regain control. This call is left as-is as an exception to the rule. When we start to allow calling name-rev repeatedly as a helper function, we would be able to remove this call as well. Signed-off-by: Junio C Hamano --- 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; From a92b1095d190ea348d365baeb9ff5c5023d0efd2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 Oct 2017 12:04:48 +0900 Subject: [PATCH 2/2] merge-ours: do not use cmd_*() as a subroutine The call to cmd_diff_index() "git merge-ours" makes has been working by accident that the function did not call exit(3), and the caller exited almost immediately after making a call, but it sets a bad precedent for people to cut and paste. For finding out if the index exactly matches the HEAD (or a given tree-ish), there is index_differs_from() which is exactly written for that purpose. Signed-off-by: Junio C Hamano --- builtin/merge-ours.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c index 684411694f..beb0623d56 100644 --- a/builtin/merge-ours.c +++ b/builtin/merge-ours.c @@ -9,26 +9,24 @@ */ #include "git-compat-util.h" #include "builtin.h" +#include "diff.h" static const char builtin_merge_ours_usage[] = "git merge-ours ... -- HEAD ..."; -static const char *diff_index_args[] = { - "diff-index", "--quiet", "--cached", "HEAD", "--", NULL -}; -#define NARGS (ARRAY_SIZE(diff_index_args) - 1) - int cmd_merge_ours(int argc, const char **argv, const char *prefix) { if (argc == 2 && !strcmp(argv[1], "-h")) usage(builtin_merge_ours_usage); /* - * We need to exit with 2 if the index does not match our HEAD tree, - * because the current index is what we will be committing as the - * merge result. + * The contents of the current index becomes the tree we + * commit. The index must match HEAD, or this merge cannot go + * through. */ - if (cmd_diff_index(NARGS, diff_index_args, prefix)) + if (read_cache() < 0) + die_errno("read_cache failed"); + if (index_differs_from("HEAD", 0, 0)) exit(2); exit(0); }