From e1d44d9834168c0d1cf694bd3f3a7cdff67d8c8d Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 14:53:20 +0100 Subject: [PATCH 01/14] cli: optionally show hidden options in usage Callers may wish to show all the options, even hidden ones, when showing usage. In particular, showing generic help for the CLI should show global options (those that are generally "hidden"). But showing help for a particular command should keep them hidden. Instrument a mechanism to deal with this. --- src/cli/cmd_blame.c | 2 +- src/cli/cmd_cat_file.c | 2 +- src/cli/cmd_clone.c | 2 +- src/cli/cmd_config.c | 2 +- src/cli/cmd_hash_object.c | 2 +- src/cli/cmd_help.c | 4 ++-- src/cli/cmd_index_pack.c | 2 +- src/cli/main.c | 2 +- src/cli/opt_usage.c | 38 +++++++++++++++++++++++++++++--------- src/cli/opt_usage.h | 7 ++++++- 10 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/cli/cmd_blame.c b/src/cli/cmd_blame.c index 3e2f5744c..6b720f350 100644 --- a/src/cli/cmd_blame.c +++ b/src/cli/cmd_blame.c @@ -40,7 +40,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Show the origin of each line of a file.\n"); diff --git a/src/cli/cmd_cat_file.c b/src/cli/cmd_cat_file.c index 90ee6033e..4d648d775 100644 --- a/src/cli/cmd_cat_file.c +++ b/src/cli/cmd_cat_file.c @@ -43,7 +43,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Display the content for the given object in the repository.\n"); diff --git a/src/cli/cmd_clone.c b/src/cli/cmd_clone.c index 7d9736fc7..afa71eee1 100644 --- a/src/cli/cmd_clone.c +++ b/src/cli/cmd_clone.c @@ -46,7 +46,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Clone a repository into a new directory.\n"); diff --git a/src/cli/cmd_config.c b/src/cli/cmd_config.c index 6b9d373ce..2d9ec93a2 100644 --- a/src/cli/cmd_config.c +++ b/src/cli/cmd_config.c @@ -68,7 +68,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Query and set configuration options.\n"); diff --git a/src/cli/cmd_hash_object.c b/src/cli/cmd_hash_object.c index 741debbeb..1eee001f7 100644 --- a/src/cli/cmd_hash_object.c +++ b/src/cli/cmd_hash_object.c @@ -36,7 +36,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Compute the object ID for a given file and optionally write that file\nto the object database.\n"); diff --git a/src/cli/cmd_help.c b/src/cli/cmd_help.c index 5e877e06d..ce3565970 100644 --- a/src/cli/cmd_help.c +++ b/src/cli/cmd_help.c @@ -25,7 +25,7 @@ static const cli_opt_spec opts[] = { static int print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, CLI_OPT_USAGE_SHOW_HIDDEN); printf("\n"); printf("Display help information about %s. If a command is specified, help\n", PROGRAM_NAME); @@ -39,7 +39,7 @@ static int print_commands(void) { const cli_cmd_spec *cmd; - cli_opt_usage_fprint(stdout, PROGRAM_NAME, NULL, cli_common_opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, NULL, cli_common_opts, CLI_OPT_USAGE_SHOW_HIDDEN); printf("\n"); printf("These are the %s commands available:\n\n", PROGRAM_NAME); diff --git a/src/cli/cmd_index_pack.c b/src/cli/cmd_index_pack.c index 09685c5d4..c72dd4dcc 100644 --- a/src/cli/cmd_index_pack.c +++ b/src/cli/cmd_index_pack.c @@ -38,7 +38,7 @@ static const cli_opt_spec opts[] = { static void print_help(void) { - cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts); + cli_opt_usage_fprint(stdout, PROGRAM_NAME, COMMAND_NAME, opts, 0); printf("\n"); printf("Indexes a packfile and writes the index to disk.\n"); diff --git a/src/cli/main.c b/src/cli/main.c index 715feab89..b51660481 100644 --- a/src/cli/main.c +++ b/src/cli/main.c @@ -82,7 +82,7 @@ int main(int argc, char **argv) while (cli_opt_parser_next(&opt, &optparser)) { if (!opt.spec) { cli_opt_status_fprint(stderr, PROGRAM_NAME, &opt); - cli_opt_usage_fprint(stderr, PROGRAM_NAME, NULL, cli_common_opts); + cli_opt_usage_fprint(stderr, PROGRAM_NAME, NULL, cli_common_opts, CLI_OPT_USAGE_SHOW_HIDDEN); ret = CLI_EXIT_USAGE; goto done; } diff --git a/src/cli/opt_usage.c b/src/cli/opt_usage.c index 8374f5151..ecb4f1461 100644 --- a/src/cli/opt_usage.c +++ b/src/cli/opt_usage.c @@ -46,7 +46,8 @@ int cli_opt_usage_fprint( FILE *file, const char *command, const char *subcommand, - const cli_opt_spec specs[]) + const cli_opt_spec specs[], + unsigned int print_flags) { git_str usage = GIT_BUF_INIT, opt = GIT_BUF_INIT; const cli_opt_spec *spec; @@ -73,7 +74,8 @@ int cli_opt_usage_fprint( next_choice = !!((spec + 1)->usage & CLI_OPT_USAGE_CHOICE); - if (spec->usage & CLI_OPT_USAGE_HIDDEN) + if ((spec->usage & CLI_OPT_USAGE_HIDDEN) && + !(print_flags & CLI_OPT_USAGE_SHOW_HIDDEN)) continue; if (choice) @@ -140,7 +142,7 @@ int cli_opt_usage_error( const cli_opt *invalid_opt) { cli_opt_status_fprint(stderr, PROGRAM_NAME, invalid_opt); - cli_opt_usage_fprint(stderr, PROGRAM_NAME, subcommand, specs); + cli_opt_usage_fprint(stderr, PROGRAM_NAME, subcommand, specs, 0); return CLI_EXIT_USAGE; } @@ -150,12 +152,19 @@ int cli_opt_help_fprint( { git_str help = GIT_BUF_INIT; const cli_opt_spec *spec; + bool required; int error = 0; /* Display required arguments first */ for (spec = specs; spec->type; ++spec) { - if (! (spec->usage & CLI_OPT_USAGE_REQUIRED) || - (spec->usage & CLI_OPT_USAGE_HIDDEN)) + if ((spec->usage & CLI_OPT_USAGE_HIDDEN) || + (spec->type == CLI_OPT_TYPE_LITERAL)) + continue; + + required = ((spec->usage & CLI_OPT_USAGE_REQUIRED) || + ((spec->usage & CLI_OPT_USAGE_CHOICE) && required)); + + if (!required) continue; git_str_printf(&help, " "); @@ -163,13 +172,22 @@ int cli_opt_help_fprint( if ((error = print_spec_name(&help, spec)) < 0) goto done; - git_str_printf(&help, ": %s\n", spec->help); + if (spec->help) + git_str_printf(&help, ": %s", spec->help); + + git_str_printf(&help, "\n"); } /* Display the remaining arguments */ for (spec = specs; spec->type; ++spec) { - if ((spec->usage & CLI_OPT_USAGE_REQUIRED) || - (spec->usage & CLI_OPT_USAGE_HIDDEN)) + if ((spec->usage & CLI_OPT_USAGE_HIDDEN) || + (spec->type == CLI_OPT_TYPE_LITERAL)) + continue; + + required = ((spec->usage & CLI_OPT_USAGE_REQUIRED) || + ((spec->usage & CLI_OPT_USAGE_CHOICE) && required)); + + if (required) continue; git_str_printf(&help, " "); @@ -177,8 +195,10 @@ int cli_opt_help_fprint( if ((error = print_spec_name(&help, spec)) < 0) goto done; - git_str_printf(&help, ": %s\n", spec->help); + if (spec->help) + git_str_printf(&help, ": %s", spec->help); + git_str_printf(&help, "\n"); } if (git_str_oom(&help) || diff --git a/src/cli/opt_usage.h b/src/cli/opt_usage.h index c752494e1..b9b75b84b 100644 --- a/src/cli/opt_usage.h +++ b/src/cli/opt_usage.h @@ -8,6 +8,10 @@ #ifndef CLI_opt_usage_h__ #define CLI_opt_usage_h__ +typedef enum { + CLI_OPT_USAGE_SHOW_HIDDEN = (1 << 0), +} cli_opt_usage_flags; + /** * Prints usage information to the given file handle. * @@ -21,7 +25,8 @@ int cli_opt_usage_fprint( FILE *file, const char *command, const char *subcommand, - const cli_opt_spec specs[]); + const cli_opt_spec specs[], + unsigned int print_flags); int cli_opt_usage_error( const char *subcommand, From fe66d93b0ecf65c011503c694a1284f76f54a953 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 14:55:13 +0100 Subject: [PATCH 02/14] cli: reorder arguments for help invocation When the `help` command is invoked (because no command was specified) we may need to clean up the arguments, in particular, to avoid passing `--help` (so that the help command isn't confused, and assumes that it's being invoked as `help --help`). --- src/cli/main.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/cli/main.c b/src/cli/main.c index b51660481..15333a45a 100644 --- a/src/cli/main.c +++ b/src/cli/main.c @@ -64,6 +64,17 @@ static void reorder_args(char **argv, size_t first) argv[1] = tmp; } +/* + * When invoked without a command, or just with `--help`, we invoke + * the help command; but we want to preserve only arguments that would + * be useful for that. + */ +static void help_args(int *argc, char **argv) +{ + argv[0] = "help"; + *argc = 1; +} + int main(int argc, char **argv) { const cli_cmd_spec *cmd; @@ -103,6 +114,7 @@ int main(int argc, char **argv) } if (!command) { + help_args(&argc, argv); ret = cmd_help(argc, argv); goto done; } From da6b76d89e73a9b3e2aa43cf689ce332326f47bc Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 14:57:55 +0100 Subject: [PATCH 03/14] cli: refactor common options Move the common option information to a global place, and reuse them. Common options will be global variables. Specify them as _hidden_ for all commands (the main command will pass the SHOW_HIDDEN flag to the usage printer, so that they're visible). --- src/cli/cmd_blame.c | 3 +-- src/cli/cmd_cat_file.c | 3 +-- src/cli/cmd_clone.c | 4 ++-- src/cli/cmd_config.c | 3 +-- src/cli/cmd_hash_object.c | 3 +-- src/cli/cmd_help.c | 3 +-- src/cli/cmd_index_pack.c | 8 +++----- src/cli/common.h | 36 +++++++++++------------------------- src/cli/main.c | 13 ++++++------- 9 files changed, 27 insertions(+), 49 deletions(-) diff --git a/src/cli/cmd_blame.c b/src/cli/cmd_blame.c index 6b720f350..180a948f9 100644 --- a/src/cli/cmd_blame.c +++ b/src/cli/cmd_blame.c @@ -22,7 +22,6 @@ static char *file; static int porcelain, line_porcelain; -static int show_help; static const cli_opt_spec opts[] = { CLI_COMMON_OPT, @@ -254,7 +253,7 @@ int cmd_blame(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/cmd_cat_file.c b/src/cli/cmd_cat_file.c index 4d648d775..95a0240ca 100644 --- a/src/cli/cmd_cat_file.c +++ b/src/cli/cmd_cat_file.c @@ -19,7 +19,6 @@ typedef enum { DISPLAY_TYPE } display_t; -static int show_help; static int display = DISPLAY_CONTENT; static char *type_name, *object_spec; @@ -147,7 +146,7 @@ int cmd_cat_file(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/cmd_clone.c b/src/cli/cmd_clone.c index afa71eee1..c18cb28d4 100644 --- a/src/cli/cmd_clone.c +++ b/src/cli/cmd_clone.c @@ -19,7 +19,7 @@ #define COMMAND_NAME "clone" static char *branch, *remote_path, *local_path, *depth; -static int show_help, quiet, checkout = 1, bare; +static int quiet, checkout = 1, bare; static bool local_path_exists; static cli_progress progress = CLI_PROGRESS_INIT; @@ -133,7 +133,7 @@ int cmd_clone(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/cmd_config.c b/src/cli/cmd_config.c index 2d9ec93a2..9056e81f0 100644 --- a/src/cli/cmd_config.c +++ b/src/cli/cmd_config.c @@ -23,7 +23,6 @@ typedef enum { static action_t action = ACTION_NONE; static int show_origin; static int show_scope; -static int show_help; static int null_separator; static int config_level; static char *config_filename; @@ -180,7 +179,7 @@ int cmd_config(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/cmd_hash_object.c b/src/cli/cmd_hash_object.c index 1eee001f7..94e7eb91f 100644 --- a/src/cli/cmd_hash_object.c +++ b/src/cli/cmd_hash_object.c @@ -13,7 +13,6 @@ #define COMMAND_NAME "hash-object" -static int show_help; static char *type_name; static int write_object, read_stdin, literally; static char **filenames; @@ -103,7 +102,7 @@ int cmd_hash_object(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/cmd_help.c b/src/cli/cmd_help.c index ce3565970..c01163d3c 100644 --- a/src/cli/cmd_help.c +++ b/src/cli/cmd_help.c @@ -13,7 +13,6 @@ #define COMMAND_NAME "help" static char *command; -static int show_help; static const cli_opt_spec opts[] = { CLI_COMMON_OPT, @@ -62,7 +61,7 @@ int cmd_help(int argc, char **argv) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); /* Show the meta-help */ - if (show_help) + if (cli_opt__show_help) return print_help(); /* We were not asked to show help for a specific command. */ diff --git a/src/cli/cmd_index_pack.c b/src/cli/cmd_index_pack.c index c72dd4dcc..b8e9749de 100644 --- a/src/cli/cmd_index_pack.c +++ b/src/cli/cmd_index_pack.c @@ -14,14 +14,12 @@ #define BUFFER_SIZE (1024 * 1024) -static int show_help, verbose, read_stdin; +static int verbose, read_stdin; static char *filename; static cli_progress progress = CLI_PROGRESS_INIT; static const cli_opt_spec opts[] = { - { CLI_OPT_TYPE_SWITCH, "help", 0, &show_help, 1, - CLI_OPT_USAGE_HIDDEN | CLI_OPT_USAGE_STOP_PARSING, NULL, - "display help about the " COMMAND_NAME " command" }, + CLI_COMMON_OPT, { CLI_OPT_TYPE_SWITCH, "verbose", 'v', &verbose, 1, CLI_OPT_USAGE_DEFAULT, NULL, "display progress output" }, @@ -62,7 +60,7 @@ int cmd_index_pack(int argc, char **argv) if (cli_opt_parse(&invalid_opt, opts, argv + 1, argc - 1, CLI_OPT_PARSE_GNU)) return cli_opt_usage_error(COMMAND_NAME, opts, &invalid_opt); - if (show_help) { + if (cli_opt__show_help) { print_help(); return 0; } diff --git a/src/cli/common.h b/src/cli/common.h index 6392ae685..bf7bc3833 100644 --- a/src/cli/common.h +++ b/src/cli/common.h @@ -20,15 +20,20 @@ * Common command arguments. */ +extern int cli_opt__show_help; + #define CLI_COMMON_OPT_HELP \ - CLI_OPT_TYPE_SWITCH, "help", 0, &show_help, 1, \ - CLI_OPT_USAGE_HIDDEN | CLI_OPT_USAGE_STOP_PARSING + CLI_OPT_TYPE_SWITCH, "help", 0, &cli_opt__show_help, 1, \ + CLI_OPT_USAGE_HIDDEN | CLI_OPT_USAGE_STOP_PARSING, \ + NULL, "display help information" #define CLI_COMMON_OPT_CONFIG \ - CLI_OPT_TYPE_VALUE, NULL, 'c', NULL, 0, \ - CLI_OPT_USAGE_HIDDEN + CLI_OPT_TYPE_VALUE, NULL, 'c', NULL, 0, \ + CLI_OPT_USAGE_HIDDEN, \ + "key=value", "add configuration value" #define CLI_COMMON_OPT_CONFIG_ENV \ - CLI_OPT_TYPE_VALUE, "config-env", 0, NULL, 0, \ - CLI_OPT_USAGE_HIDDEN + CLI_OPT_TYPE_VALUE, "config-env", 0, NULL, 0, \ + CLI_OPT_USAGE_HIDDEN, \ + "key=value", "set configuration value to environment variable" #define CLI_COMMON_OPT \ { CLI_COMMON_OPT_HELP }, \ @@ -49,23 +54,4 @@ extern int cli_resolve_path( git_repository *repo, const char *given_path); -/* - * Common command arguments. - */ - -#define CLI_COMMON_OPT_HELP \ - CLI_OPT_TYPE_SWITCH, "help", 0, &show_help, 1, \ - CLI_OPT_USAGE_HIDDEN | CLI_OPT_USAGE_STOP_PARSING -#define CLI_COMMON_OPT_CONFIG \ - CLI_OPT_TYPE_VALUE, NULL, 'c', NULL, 0, \ - CLI_OPT_USAGE_HIDDEN -#define CLI_COMMON_OPT_CONFIG_ENV \ - CLI_OPT_TYPE_VALUE, "config-env", 0, NULL, 0, \ - CLI_OPT_USAGE_HIDDEN - -#define CLI_COMMON_OPT \ - { CLI_COMMON_OPT_HELP }, \ - { CLI_COMMON_OPT_CONFIG }, \ - { CLI_COMMON_OPT_CONFIG_ENV } - #endif /* CLI_common_h__ */ diff --git a/src/cli/main.c b/src/cli/main.c index 15333a45a..e63fd96a5 100644 --- a/src/cli/main.c +++ b/src/cli/main.c @@ -10,18 +10,15 @@ #include "common.h" #include "cmd.h" -static int show_help = 0; +int cli_opt__show_help = 0; + static int show_version = 0; static char *command = NULL; static char **args = NULL; const cli_opt_spec cli_common_opts[] = { - { CLI_OPT_TYPE_SWITCH, "help", 0, &show_help, 1, - CLI_OPT_USAGE_DEFAULT, NULL, "display help information" }, - { CLI_OPT_TYPE_VALUE, NULL, 'c', NULL, 0, - CLI_OPT_USAGE_DEFAULT, "key=value", "add configuration value" }, - { CLI_OPT_TYPE_VALUE, "config-env", 0, NULL, 0, - CLI_OPT_USAGE_DEFAULT, "key=value", "set configuration value to environment variable" }, + CLI_COMMON_OPT, + { CLI_OPT_TYPE_SWITCH, "version", 0, &show_version, 1, CLI_OPT_USAGE_DEFAULT, NULL, "display the version" }, { CLI_OPT_TYPE_ARG, "command", 0, &command, 0, @@ -71,6 +68,8 @@ static void reorder_args(char **argv, size_t first) */ static void help_args(int *argc, char **argv) { + cli_opt__show_help = 0; + argv[0] = "help"; *argc = 1; } From 3ba218c4d5f3bc9139a42356c70205bb07351982 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 14:59:17 +0100 Subject: [PATCH 04/14] cli: add a --no-pager option (currently a noop) Add a `--no-pager` option for git compatibility. --- src/cli/common.h | 8 +++++++- src/cli/main.c | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/cli/common.h b/src/cli/common.h index bf7bc3833..330f776c9 100644 --- a/src/cli/common.h +++ b/src/cli/common.h @@ -21,6 +21,7 @@ */ extern int cli_opt__show_help; +extern int cli_opt__use_pager; #define CLI_COMMON_OPT_HELP \ CLI_OPT_TYPE_SWITCH, "help", 0, &cli_opt__show_help, 1, \ @@ -34,11 +35,16 @@ extern int cli_opt__show_help; CLI_OPT_TYPE_VALUE, "config-env", 0, NULL, 0, \ CLI_OPT_USAGE_HIDDEN, \ "key=value", "set configuration value to environment variable" +#define CLI_COMMON_OPT_NO_PAGER \ + CLI_OPT_TYPE_SWITCH, "no-pager", 0, &cli_opt__use_pager, 0, \ + CLI_OPT_USAGE_HIDDEN, \ + NULL, "don't paginate multi-page output" #define CLI_COMMON_OPT \ { CLI_COMMON_OPT_HELP }, \ { CLI_COMMON_OPT_CONFIG }, \ - { CLI_COMMON_OPT_CONFIG_ENV } + { CLI_COMMON_OPT_CONFIG_ENV }, \ + { CLI_COMMON_OPT_NO_PAGER } typedef struct { char **args; diff --git a/src/cli/main.c b/src/cli/main.c index e63fd96a5..1cc8dd3e2 100644 --- a/src/cli/main.c +++ b/src/cli/main.c @@ -11,6 +11,7 @@ #include "cmd.h" int cli_opt__show_help = 0; +int cli_opt__use_pager = 1; static int show_version = 0; static char *command = NULL; From fddb0bb15376a33801d2823d8e19884b39ca2a26 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 29 Jun 2024 11:14:32 +0100 Subject: [PATCH 05/14] benchmarks: pass options on beyond -- --- tests/benchmarks/benchmark_helpers.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/benchmarks/benchmark_helpers.sh b/tests/benchmarks/benchmark_helpers.sh index 14dbb43c1..939c1a8f0 100644 --- a/tests/benchmarks/benchmark_helpers.sh +++ b/tests/benchmarks/benchmark_helpers.sh @@ -293,6 +293,9 @@ gitbench() { NEXT="prepare" elif [ "${a}" = "--chdir" ]; then NEXT="chdir" + elif [[ "${a}" == "--" ]]; then + shift + break elif [[ "${a}" == "--"* ]]; then echo "unknown argument: \"${a}\"" 1>&2 gitbench_usage 1>&2 From 0dfcb29da2b626882d37944b84b2631bfafca92f Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 29 Jun 2024 11:14:48 +0100 Subject: [PATCH 06/14] benchmarks: clone git or kernel repositories Teach the benchmark script how to clone the git or kernel repositories, which is useful to have a larger corpus of test data. If a benchmark script wants to `clone git` or `clone linux`, then this will be done. Callers probably want to specify `BENCHMARK_GIT_REPOSITORY` to a previously cloned local repository so that the script does not download the repository repeatedly. --- tests/benchmarks/benchmark_helpers.sh | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/benchmarks/benchmark_helpers.sh b/tests/benchmarks/benchmark_helpers.sh index 939c1a8f0..c71d71cbf 100644 --- a/tests/benchmarks/benchmark_helpers.sh +++ b/tests/benchmarks/benchmark_helpers.sh @@ -15,6 +15,11 @@ TEST_CLI="git" JSON= SHOW_OUTPUT= +REPOSITORY_DEFAULTS=$(cat <> "${SANDBOX_DIR}/prepare.sh" << EOF set -e + ${REPOSITORY_DEFAULTS} + SANDBOX_DIR="${SANDBOX_DIR}" RESOURCES_DIR="$(resources_dir)" @@ -218,6 +225,29 @@ create_preparescript() { fi } + clone_repo() { + REPO="\${1}" + + if [ "\${REPO}" = "" ]; then + echo "usage: clone_repo " 1>&2 + exit 1 + fi + + REPO_UPPER=\$(echo "\${1}" | tr '[:lower:]' '[:upper:]') + GIVEN_REPO_URL=\$(eval echo "\\\${BENCHMARK_\${REPO_UPPER}_REPOSITORY}") + DEFAULT_REPO_URL=\$(eval echo "\\\${DEFAULT_\${REPO_UPPER}_REPOSITORY}") + + if [ "\${DEFAULT_REPO_URL}" = "" ]; then + echo "\$0: unknown repository '\${REPO}'" 1>&2 + exit 1 + fi + + REPO_URL="\${GIVEN_REPO_URL:-\${DEFAULT_REPO_URL}}" + + rm -rf "\${SANDBOX_DIR:?}/\${REPO}" + git clone "\${REPO_URL}" "\${SANDBOX_DIR}/\${REPO}" + } + cd "\${SANDBOX_DIR}" EOF From 42bd9df2b463a724616a30b2669a154e367cf798 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 29 Jun 2024 11:35:01 +0100 Subject: [PATCH 07/14] benchmarks: pre-clone git and linux in ci --- .github/workflows/benchmark.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 8b513d94d..44b76b902 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -62,6 +62,11 @@ jobs: run: source/ci/setup-${{ matrix.platform.setup-script }}-benchmark.sh shell: bash if: matrix.platform.setup-script != '' + - name: Clone resource repositories + run: | + mkdir resources + git clone --bare https://github.com/git/git resources/git + git clone --bare https://github.com/torvalds/linux resources/linux - name: Build run: | mkdir build && cd build @@ -69,6 +74,9 @@ jobs: shell: bash - name: Benchmark run: | + export BENCHMARK_GIT_REPOSITORY="$(pwd)/resources/git" + export BENCHMARK_LINUX_REPOSITORY="$(pwd)/resources/linux" + if [[ "$(uname -s)" == MINGW* ]]; then GIT2_CLI="$(cygpath -w $(pwd))\\build\\Release\\git2" else From 03d61cf3219b14bc74e6f3d56d2c103d7e0b756d Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 29 Jun 2024 11:37:36 +0100 Subject: [PATCH 08/14] benchmarks: add blame benchmarks --- tests/benchmarks/blame__git_cached | 9 +++++++++ tests/benchmarks/blame__linux_cached | 9 +++++++++ tests/benchmarks/blame__simple_cached | 9 +++++++++ 3 files changed, 27 insertions(+) create mode 100755 tests/benchmarks/blame__git_cached create mode 100755 tests/benchmarks/blame__linux_cached create mode 100755 tests/benchmarks/blame__simple_cached diff --git a/tests/benchmarks/blame__git_cached b/tests/benchmarks/blame__git_cached new file mode 100755 index 000000000..5a6e2b274 --- /dev/null +++ b/tests/benchmarks/blame__git_cached @@ -0,0 +1,9 @@ +#!/bin/bash -e + +. "$(dirname "$0")/benchmark_helpers.sh" + +gitbench --prepare "clone_repo git && cd git && git reset --hard v2.45.0" \ + --warmup 5 \ + --chdir "git" \ + -- \ + --no-pager blame "git.c" diff --git a/tests/benchmarks/blame__linux_cached b/tests/benchmarks/blame__linux_cached new file mode 100755 index 000000000..ff4902ae9 --- /dev/null +++ b/tests/benchmarks/blame__linux_cached @@ -0,0 +1,9 @@ +#!/bin/bash -e + +. "$(dirname "$0")/benchmark_helpers.sh" + +gitbench --prepare "clone_repo linux && cd linux && git reset --hard v6.9" \ + --warmup 5 \ + --chdir "linux" \ + -- \ + --no-pager blame "Makefile" diff --git a/tests/benchmarks/blame__simple_cached b/tests/benchmarks/blame__simple_cached new file mode 100755 index 000000000..f13e2cb06 --- /dev/null +++ b/tests/benchmarks/blame__simple_cached @@ -0,0 +1,9 @@ +#!/bin/bash -e + +. "$(dirname "$0")/benchmark_helpers.sh" + +gitbench --prepare "sandbox_repo testrepo && cd testrepo && git reset --hard HEAD" \ + --warmup 5 \ + --chdir "testrepo" \ + -- \ + --no-pager blame "branch_file.txt" From d564cc1801e4b9729d2d5ea5237da1fc32c04bb4 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Thu, 4 Jul 2024 07:15:27 +0100 Subject: [PATCH 09/14] benchmark: don't require a default It will never be a good user experience to clone the git or kernel repos from a remote; don't even have a default. --- tests/benchmarks/benchmark_helpers.sh | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/tests/benchmarks/benchmark_helpers.sh b/tests/benchmarks/benchmark_helpers.sh index c71d71cbf..2efcf4705 100644 --- a/tests/benchmarks/benchmark_helpers.sh +++ b/tests/benchmarks/benchmark_helpers.sh @@ -15,11 +15,6 @@ TEST_CLI="git" JSON= SHOW_OUTPUT= -REPOSITORY_DEFAULTS=$(cat <> "${SANDBOX_DIR}/prepare.sh" << EOF set -e - ${REPOSITORY_DEFAULTS} - SANDBOX_DIR="${SANDBOX_DIR}" RESOURCES_DIR="$(resources_dir)" @@ -234,16 +227,13 @@ create_preparescript() { fi REPO_UPPER=\$(echo "\${1}" | tr '[:lower:]' '[:upper:]') - GIVEN_REPO_URL=\$(eval echo "\\\${BENCHMARK_\${REPO_UPPER}_REPOSITORY}") - DEFAULT_REPO_URL=\$(eval echo "\\\${DEFAULT_\${REPO_UPPER}_REPOSITORY}") + REPO_URL=\$(eval echo "\\\${BENCHMARK_\${REPO_UPPER}_REPOSITORY}") - if [ "\${DEFAULT_REPO_URL}" = "" ]; then + if [ "\${REPO_URL}" = "" ]; then echo "\$0: unknown repository '\${REPO}'" 1>&2 exit 1 fi - REPO_URL="\${GIVEN_REPO_URL:-\${DEFAULT_REPO_URL}}" - rm -rf "\${SANDBOX_DIR:?}/\${REPO}" git clone "\${REPO_URL}" "\${SANDBOX_DIR}/\${REPO}" } From 4f40bd9f0964f60558300ccff428f1720a41bf3c Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Thu, 4 Jul 2024 12:33:32 +0100 Subject: [PATCH 10/14] benchmark: show helpful errors w/o local repo When running without a local test repository, show a helpful error message. --- tests/benchmarks/benchmark_helpers.sh | 27 +++++++++++++++++++++++++++ tests/benchmarks/blame__git_cached | 2 ++ tests/benchmarks/blame__linux_cached | 2 ++ 3 files changed, 31 insertions(+) diff --git a/tests/benchmarks/benchmark_helpers.sh b/tests/benchmarks/benchmark_helpers.sh index 2efcf4705..dd84b9cee 100644 --- a/tests/benchmarks/benchmark_helpers.sh +++ b/tests/benchmarks/benchmark_helpers.sh @@ -21,6 +21,9 @@ else OUTPUT_STYLE="auto" fi +HELP_GIT_REMOTE="https://github.com/git/git" +HELP_LINUX_REMOTE="https://github.com/torvalds/linux" + # # parse the arguments to the outer script that's including us; these are arguments that # the `benchmark.sh` passes (or that a user could specify when running an individual test) @@ -384,3 +387,27 @@ gitbench() { hyperfine "${ARGUMENTS[@]}" rm -rf "${SANDBOX_DIR:?}" } + +# helper script to give useful error messages about configuration +needs_repo() { + REPO="${1}" + + if [ "${REPO}" = "" ]; then + echo "usage: needs_repo " 1>&2 + exit 1 + fi + + REPO_UPPER=$(echo "${1}" | tr '[:lower:]' '[:upper:]') + REPO_URL=$(eval echo "\${BENCHMARK_${REPO_UPPER}_REPOSITORY}") + REPO_REMOTE_URL=$(eval echo "\${HELP_${REPO_UPPER}_REMOTE}") + + if [ "${REPO_URL}" = "" ]; then + echo "$0: '${REPO}' repository not configured" 1>&2 + echo "" 1>&2 + echo "This benchmark needs an on-disk '${REPO}' repository. First, clone the" 1>&2 + echo "remote repository ('${REPO_REMOTE_URL}') locally then set," 1>&2 + echo "the 'BENCHMARK_${REPO_UPPER}_REPOSITORY' environment variable to the path that" 1>&2 + echo "contains the repository locally, then run this benchmark again." 1>&2 + exit 1 + fi +} diff --git a/tests/benchmarks/blame__git_cached b/tests/benchmarks/blame__git_cached index 5a6e2b274..1664f737b 100755 --- a/tests/benchmarks/blame__git_cached +++ b/tests/benchmarks/blame__git_cached @@ -2,6 +2,8 @@ . "$(dirname "$0")/benchmark_helpers.sh" +needs_repo git + gitbench --prepare "clone_repo git && cd git && git reset --hard v2.45.0" \ --warmup 5 \ --chdir "git" \ diff --git a/tests/benchmarks/blame__linux_cached b/tests/benchmarks/blame__linux_cached index ff4902ae9..20c5db5bb 100755 --- a/tests/benchmarks/blame__linux_cached +++ b/tests/benchmarks/blame__linux_cached @@ -2,6 +2,8 @@ . "$(dirname "$0")/benchmark_helpers.sh" +needs_repo linux + gitbench --prepare "clone_repo linux && cd linux && git reset --hard v6.9" \ --warmup 5 \ --chdir "linux" \ From 90e659d0b950499984ef26fdfd45205a3113bdd8 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 10:48:25 +0100 Subject: [PATCH 11/14] ci: only publish benchmarks nightly Allow for workflow_dispatch jobs to run, but don't publish their test results; this is useful for testing workflows themselves. --- .github/workflows/benchmark.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 44b76b902..de12739b9 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -97,7 +97,7 @@ jobs: publish: name: Publish results needs: [ build ] - if: always() && github.repository == 'libgit2/libgit2' + if: always() && github.repository == 'libgit2/libgit2' && github.event_name == 'schedule' runs-on: ubuntu-latest steps: - name: Check out benchmark repository From df3d8a6799ffa61316a3c95815c4e333df3b7179 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 11:02:17 +0100 Subject: [PATCH 12/14] ci: allow inputs to benchmark --- .github/workflows/benchmark.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index de12739b9..cf0afe5f5 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -3,6 +3,12 @@ name: Benchmark on: workflow_dispatch: + inputs: + suite: + description: Benchmark suite to run + debug: + type: boolean + description: Debugging output schedule: - cron: '15 4 * * *' @@ -83,8 +89,19 @@ jobs: GIT2_CLI="$(pwd)/build/git2" fi + if [ "${{ github.event.inputs.suite }}" != "" ]; then + SUITE_FLAG="--suite ${{ github.event.inputs.suite }}" + fi + + if [ "${{ github.event.inputs.debug }}" = "true" ]; then + DEBUG_FLAG="--debug" + fi + mkdir benchmark && cd benchmark - ../source/tests/benchmarks/benchmark.sh --baseline-cli "git" --cli "${GIT2_CLI}" --name libgit2 --json benchmarks.json --zip benchmarks.zip + ../source/tests/benchmarks/benchmark.sh \ + ${SUITE_FLAG} ${DEBUG_FLAG} \ + --baseline-cli "git" --cli "${GIT2_CLI}" --name libgit2 \ + --json benchmarks.json --zip benchmarks.zip shell: bash - name: Upload results uses: actions/upload-artifact@v4 From d4222f83215c99fd083904bad482c8817752271a Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 16:28:35 +0100 Subject: [PATCH 13/14] ci: don't run blame on torvalds/linux (yet) blame on torvalds/linux is too punishing for our current implementation; don't run it (yet). --- .github/workflows/benchmark.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index cf0afe5f5..3fb912d40 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -81,7 +81,9 @@ jobs: - name: Benchmark run: | export BENCHMARK_GIT_REPOSITORY="$(pwd)/resources/git" - export BENCHMARK_LINUX_REPOSITORY="$(pwd)/resources/linux" + # avoid linux temporarily; the linux blame benchmarks are simply + # too slow to use + # export BENCHMARK_LINUX_REPOSITORY="$(pwd)/resources/linux" if [[ "$(uname -s)" == MINGW* ]]; then GIT2_CLI="$(cygpath -w $(pwd))\\build\\Release\\git2" From e68d0b4b955cea0a5e9305731eeee693376e90e4 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 21 Oct 2024 17:29:52 +0100 Subject: [PATCH 14/14] benchmark: skip (don't fail) needs_repo tests If a test needs a repo that isn't provide it, mark it as skipped and avoid failing the execution. --- tests/benchmarks/benchmark.sh | 14 ++++++++++++-- tests/benchmarks/benchmark_helpers.sh | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/benchmarks/benchmark.sh b/tests/benchmarks/benchmark.sh index 4cb2b2fbf..6830064e7 100755 --- a/tests/benchmarks/benchmark.sh +++ b/tests/benchmarks/benchmark.sh @@ -213,9 +213,19 @@ for TEST_PATH in "${BENCHMARK_DIR}"/*; do ERROR_FILE="${OUTPUT_DIR}/${TEST_FILE}.err" FAILED= - ${TEST_PATH} --cli "${TEST_CLI}" --baseline-cli "${BASELINE_CLI}" --json "${JSON_FILE}" ${SHOW_OUTPUT} >"${OUTPUT_FILE}" 2>"${ERROR_FILE}" || FAILED=1 + { + ${TEST_PATH} --cli "${TEST_CLI}" --baseline-cli "${BASELINE_CLI}" --json "${JSON_FILE}" ${SHOW_OUTPUT} >"${OUTPUT_FILE}" 2>"${ERROR_FILE}"; + FAILED=$? + } || true - if [ "${FAILED}" = "1" ]; then + if [ "${FAILED}" = "2" ]; then + if [ "${VERBOSE}" != "1" ]; then + echo "skipped!" + fi + + indent < "${ERROR_FILE}" + continue + elif [ "${FAILED}" != "0" ]; then if [ "${VERBOSE}" != "1" ]; then echo "failed!" fi diff --git a/tests/benchmarks/benchmark_helpers.sh b/tests/benchmarks/benchmark_helpers.sh index dd84b9cee..5143a01fc 100644 --- a/tests/benchmarks/benchmark_helpers.sh +++ b/tests/benchmarks/benchmark_helpers.sh @@ -408,6 +408,6 @@ needs_repo() { echo "remote repository ('${REPO_REMOTE_URL}') locally then set," 1>&2 echo "the 'BENCHMARK_${REPO_UPPER}_REPOSITORY' environment variable to the path that" 1>&2 echo "contains the repository locally, then run this benchmark again." 1>&2 - exit 1 + exit 2 fi }