From 8757b35d443ea01fb6298ae622362c5490a60198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:15 +0200 Subject: commit-graph: define common usage with a macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Share the usage message between these three variables by using a macro. Before this new options needed to copy/paste the usage information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce '--max-new-filters=', 2020-09-18). See b25b727494f (builtin/multi-pack-index.c: define common usage with a macro, 2021-03-30) for another use of this pattern (but on-list this one came first). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index cd86315221..5af3cd7178 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -9,26 +9,29 @@ #include "progress.h" #include "tag.h" -static char const * const builtin_commit_graph_usage[] = { - N_("git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]"), - N_("git commit-graph write [--object-dir ] [--append] " - "[--split[=]] [--reachable|--stdin-packs|--stdin-commits] " - "[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] " - ""), +#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \ + N_("git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]") + +#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \ + N_("git commit-graph write [--object-dir ] [--append] " \ + "[--split[=]] [--reachable|--stdin-packs|--stdin-commits] " \ + "[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] " \ + "") + +static const char * builtin_commit_graph_verify_usage[] = { + BUILTIN_COMMIT_GRAPH_VERIFY_USAGE, NULL }; -static const char * const builtin_commit_graph_verify_usage[] = { - N_("git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]"), +static const char * builtin_commit_graph_write_usage[] = { + BUILTIN_COMMIT_GRAPH_WRITE_USAGE, NULL }; -static const char * const builtin_commit_graph_write_usage[] = { - N_("git commit-graph write [--object-dir ] [--append] " - "[--split[=]] [--reachable|--stdin-packs|--stdin-commits] " - "[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] " - ""), - NULL +static char const * const builtin_commit_graph_usage[] = { + BUILTIN_COMMIT_GRAPH_VERIFY_USAGE, + BUILTIN_COMMIT_GRAPH_WRITE_USAGE, + NULL, }; static struct opts_commit_graph { -- cgit v1.2.3 From 8722f9fb6b7705a3354e2d40d6dba0b86c1dab0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:16 +0200 Subject: commit-graph: remove redundant handling of -h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we don't handle the -h option here like most parse_options() users we'll fall through and it'll do the right thing for us. I think this code added in 4ce58ee38d (commit-graph: create git-commit-graph builtin, 2018-04-02) was always redundant, parse_options() did this at the time, and the commit-graph code never used PARSE_OPT_NO_INTERNAL_HELP. We don't need a test for this, it's tested by the t0012-help.sh test added in d691551192a (t0012: test "-h" with builtins, 2017-05-30). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 5af3cd7178..3cf18dc534 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -321,10 +321,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) OPT_END(), }; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage_with_options(builtin_commit_graph_usage, - builtin_commit_graph_options); - git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, builtin_commit_graph_options, -- cgit v1.2.3 From 84e4484f12895b6acd4047ba6776cede6a627f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:17 +0200 Subject: commit-graph: use parse_options_concat() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make use of the parse_options_concat() so we don't need to copy/paste common options like --object-dir. This is inspired by a similar change to "checkout" in 2087182272 (checkout: split options[] array in three pieces, 2019-03-29), and the same pattern in the multi-pack-index command, see 60ca94769ce (builtin/multi-pack-index.c: split sub-commands, 2021-03-30). A minor behavior change here is that now we're going to list both --object-dir and --progress first, before we'd list --progress along with other options. Co-authored-by: Taylor Blau Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 3cf18dc534..6e49184439 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -46,6 +46,20 @@ static struct opts_commit_graph { int enable_changed_paths; } opts; +static struct option common_opts[] = { + OPT_STRING(0, "object-dir", &opts.obj_dir, + N_("dir"), + N_("the object directory to store the graph")), + OPT_BOOL(0, "progress", &opts.progress, + N_("force progress reporting")), + OPT_END() +}; + +static struct option *add_common_options(struct option *to) +{ + return parse_options_concat(common_opts, to); +} + static struct object_directory *find_odb(struct repository *r, const char *obj_dir) { @@ -79,20 +93,17 @@ static int graph_verify(int argc, const char **argv) int flags = 0; static struct option builtin_commit_graph_verify_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), OPT_BOOL(0, "shallow", &opts.shallow, N_("if the commit-graph is split, only verify the tip file")), - OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_END(), }; + struct option *options = add_common_options(builtin_commit_graph_verify_options); trace2_cmd_mode("verify"); opts.progress = isatty(2); argc = parse_options(argc, argv, NULL, - builtin_commit_graph_verify_options, + options, builtin_commit_graph_verify_usage, 0); if (!opts.obj_dir) @@ -109,6 +120,7 @@ static int graph_verify(int argc, const char **argv) die_errno(_("Could not open commit-graph '%s'"), graph_name); FREE_AND_NULL(graph_name); + FREE_AND_NULL(options); if (open_ok) graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb); @@ -209,9 +221,6 @@ static int graph_write(int argc, const char **argv) struct progress *progress = NULL; static struct option builtin_commit_graph_write_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), OPT_BOOL(0, "reachable", &opts.reachable, N_("start walk at all refs")), OPT_BOOL(0, "stdin-packs", &opts.stdin_packs, @@ -222,7 +231,6 @@ static int graph_write(int argc, const char **argv) N_("include all commits already in the commit-graph file")), OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths, N_("enable computation for changed paths")), - OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL, N_("allow writing an incremental commit-graph file"), PARSE_OPT_OPTARG | PARSE_OPT_NONEG, @@ -238,6 +246,7 @@ static int graph_write(int argc, const char **argv) 0, write_option_max_new_filters), OPT_END(), }; + struct option *options = add_common_options(builtin_commit_graph_write_options); opts.progress = isatty(2); opts.enable_changed_paths = -1; @@ -251,7 +260,7 @@ static int graph_write(int argc, const char **argv) git_config(git_commit_graph_write_config, &opts); argc = parse_options(argc, argv, NULL, - builtin_commit_graph_write_options, + options, builtin_commit_graph_write_usage, 0); if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) @@ -307,6 +316,7 @@ static int graph_write(int argc, const char **argv) result = 1; cleanup: + FREE_AND_NULL(options); string_list_clear(&pack_indexes, 0); strbuf_release(&buf); return result; @@ -314,12 +324,7 @@ cleanup: int cmd_commit_graph(int argc, const char **argv, const char *prefix) { - static struct option builtin_commit_graph_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), - N_("the object directory to store the graph")), - OPT_END(), - }; + struct option *builtin_commit_graph_options = common_opts; git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, -- cgit v1.2.3 From 070e7c5619bfaca2b1e93255d892b68ed455aaa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:19 +0200 Subject: commit-graph: early exit to "usage" on !argc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than guarding all of the !argc with an additional "if" arm let's do an early goto to "usage". This also makes it clear that "save_commit_buffer" is not needed in this case. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 6e49184439..bf34aa43f2 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) builtin_commit_graph_options, builtin_commit_graph_usage, PARSE_OPT_STOP_AT_NON_OPTION); + if (!argc) + goto usage; save_commit_buffer = 0; - if (argc > 0) { - if (!strcmp(argv[0], "verify")) - return graph_verify(argc, argv); - if (!strcmp(argv[0], "write")) - return graph_write(argc, argv); - } + if (!strcmp(argv[0], "verify")) + return graph_verify(argc, argv); + else if (argc && !strcmp(argv[0], "write")) + return graph_write(argc, argv); +usage: usage_with_options(builtin_commit_graph_usage, builtin_commit_graph_options); } -- cgit v1.2.3 From 6d209a01f89fd27d42e0a74a5ada29cef1dcac29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:20 +0200 Subject: commit-graph: show usage on "commit-graph [write|verify] garbage" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the parse_options() invocation in the commit-graph code to error on unknown leftover argv elements, in addition to the existing and implicit erroring via parse_options() on unknown options. We'd already error in cmd_commit_graph() on e.g.: git commit-graph unknown verify git commit-graph --unknown verify But here we're calling parse_options() twice more for the "write" and "verify" subcommands. We did not do the same checking for leftover argv elements there. As a result we'd silently accept garbage in these subcommands, let's not do that. Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index bf34aa43f2..0457903f18 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -105,6 +105,8 @@ static int graph_verify(int argc, const char **argv) argc = parse_options(argc, argv, NULL, options, builtin_commit_graph_verify_usage, 0); + if (argc) + usage_with_options(builtin_commit_graph_verify_usage, options); if (!opts.obj_dir) opts.obj_dir = get_object_directory(); @@ -262,6 +264,8 @@ static int graph_write(int argc, const char **argv) argc = parse_options(argc, argv, NULL, options, builtin_commit_graph_write_usage, 0); + if (argc) + usage_with_options(builtin_commit_graph_write_usage, options); if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1) die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs")); -- cgit v1.2.3 From 367c5f36a6a0fa7f8f706b10f9bb2f0e28baaa1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 14:30:21 +0200 Subject: commit-graph: show "unexpected subcommand" error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring the "commit-graph" command in line with the error output and general pattern in cmd_multi_pack_index(). Let's test for that output, and also cover the same potential bug as was fixed in the multi-pack-index command in 88617d11f9d (multi-pack-index: fix potential segfault without sub-command, 2021-07-19). Signed-off-by: Ævar Arnfjörð Bjarmason Reviewed-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 0457903f18..21fc6e934b 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -345,6 +345,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) else if (argc && !strcmp(argv[0], "write")) return graph_write(argc, argv); + error(_("unrecognized subcommand: %s"), argv[0]); usage: usage_with_options(builtin_commit_graph_usage, builtin_commit_graph_options); -- cgit v1.2.3 From f57a739691e2efa1d71851cd725154a5a760d8f4 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 1 Sep 2021 16:34:01 -0400 Subject: midx: avoid opening multiple MIDXs when writing Opening multiple instance of the same MIDX can lead to problems like two separate packed_git structures which represent the same pack being added to the repository's object store. The above scenario can happen because prepare_midx_pack() checks if `m->packs[pack_int_id]` is NULL in order to determine if a pack has been opened and installed in the repository before. But a caller can construct two copies of the same MIDX by calling get_multi_pack_index() and load_multi_pack_index() since the former manipulates the object store directly but the latter is a lower-level routine which allocates a new MIDX for each call. So if prepare_midx_pack() is called on multiple MIDXs with the same pack_int_id, then that pack will be installed twice in the object store's packed_git pointer. This can lead to problems in, for e.g., the pack-bitmap code, which does something like the following (in pack-bitmap.c:open_pack_bitmap()): struct bitmap_index *bitmap_git = ...; for (p = get_all_packs(r); p; p = p->next) { if (open_pack_bitmap_1(bitmap_git, p) == 0) ret = 0; } which is a problem if two copies of the same pack exist in the packed_git list because pack-bitmap.c:open_pack_bitmap_1() contains a conditional like the following: if (bitmap_git->pack || bitmap_git->midx) { /* ignore extra bitmap file; we can only handle one */ warning("ignoring extra bitmap file: %s", packfile->pack_name); close(fd); return -1; } Avoid this scenario by not letting write_midx_internal() open a MIDX that isn't also pointed at by the object store. So long as this is the case, other routines should prefer to open MIDXs with get_multi_pack_index() or reprepare_packed_git() instead of creating instances on their own. Because get_multi_pack_index() returns `r->object_store->multi_pack_index` if it is non-NULL, we'll only have one instance of a MIDX open at one time, avoiding these problems. To encourage this, drop the `struct multi_pack_index *` parameter from `write_midx_internal()`, and rely instead on the `object_dir` to find (or initialize) the correct MIDX instance. Likewise, replace the call to `close_midx()` with `close_object_store()`, since we're about to replace the MIDX with a new one and should invalidate the object store's memory of any MIDX that might have existed beforehand. Note that this now forbids passing object directories that don't belong to alternate repositories over `--object-dir`, since before we would have happily opened a MIDX in any directory, but now restrict ourselves to only those reachable by `r->objects->multi_pack_index` (and alternate MIDXs that we can see by walking the `next` pointer). As far as I can tell, supporting arbitrary directories with `--object-dir` was a historical accident, since even the documentation says `` when referring to the value passed to this option. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 22 ---------------------- 1 file changed, 22 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index cd86315221..003eaaac5c 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -43,28 +43,6 @@ static struct opts_commit_graph { int enable_changed_paths; } opts; -static struct object_directory *find_odb(struct repository *r, - const char *obj_dir) -{ - struct object_directory *odb; - char *obj_dir_real = real_pathdup(obj_dir, 1); - struct strbuf odb_path_real = STRBUF_INIT; - - prepare_alt_odb(r); - for (odb = r->objects->odb; odb; odb = odb->next) { - strbuf_realpath(&odb_path_real, odb->path, 1); - if (!strcmp(obj_dir_real, odb_path_real.buf)) - break; - } - - free(obj_dir_real); - strbuf_release(&odb_path_real); - - if (!odb) - die(_("could not find object directory matching %s"), obj_dir); - return odb; -} - static int graph_verify(int argc, const char **argv) { struct commit_graph *graph = NULL; -- cgit v1.2.3 From d5fdf3073abaa6b968a7eef2ac09835dc4628fc2 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Sat, 18 Sep 2021 12:02:37 -0400 Subject: builtin/commit-graph.c: don't accept common --[no-]progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In 84e4484f12 (commit-graph: use parse_options_concat(), 2021-08-23) we unified common options of commit-graph's subcommands into a single "common_opts" array. But 84e4484f12 introduced a behavior change which is to accept the "--[no-]progress" option before any sub-commands, e.g., git commit-graph --progress write ... Prior to that commit, the above would error out with "unknown option". There are two issues with this behavior change. First is that the top-level --[no-]progress is not always respected. This is because isatty(2) is performed in the sub-commands, which unconditionally overwrites any --[no-]progress that was given at the top-level. But the second issue is that the existing sub-commands of commit-graph only happen to both have a sensible interpretation of what `--progress` or `--no-progress` means. If we ever added a sub-command which didn't have a notion of progress, we would be forced to ignore the top-level `--[no-]progress` altogether. Since we haven't released a version of Git that supports --[no-]progress as a top-level option for `git commit-graph`, let's remove it. Suggested-by: SZEDER Gábor Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 21fc6e934b..067587a0fd 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -50,8 +50,6 @@ static struct option common_opts[] = { OPT_STRING(0, "object-dir", &opts.obj_dir, N_("dir"), N_("the object directory to store the graph")), - OPT_BOOL(0, "progress", &opts.progress, - N_("force progress reporting")), OPT_END() }; @@ -95,6 +93,8 @@ static int graph_verify(int argc, const char **argv) static struct option builtin_commit_graph_verify_options[] = { OPT_BOOL(0, "shallow", &opts.shallow, N_("if the commit-graph is split, only verify the tip file")), + OPT_BOOL(0, "progress", &opts.progress, + N_("force progress reporting")), OPT_END(), }; struct option *options = add_common_options(builtin_commit_graph_verify_options); @@ -246,6 +246,8 @@ static int graph_write(int argc, const char **argv) OPT_CALLBACK_F(0, "max-new-filters", &write_opts.max_new_filters, NULL, N_("maximum number of changed-path Bloom filters to compute"), 0, write_option_max_new_filters), + OPT_BOOL(0, "progress", &opts.progress, + N_("force progress reporting")), OPT_END(), }; struct option *options = add_common_options(builtin_commit_graph_write_options); -- cgit v1.2.3 From 13d9fcec293ad988c94e81ca7ccc58c76993093a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 8 Oct 2021 21:07:43 +0200 Subject: commit-graph: stop using optname() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop using optname() in builtin/commit-graph.c to emit an error with the --max-new-filters option. This changes code added in 809e0327f57 (builtin/commit-graph.c: introduce '--max-new-filters=', 2020-09-18). See 9440b831ad5 (parse-options: replace opterror() with optname(), 2018-11-10) for why using optname() like this is considered bad, i.e. it's assembling human-readable output piecemeal, and the "option `X'" at the start can't be translated. It didn't matter in this case, but this code was also buggy in its use of "opt->flags" to optname(), that function expects flags, but not *those* flags. Let's pass "max-new-filters" to the new error because the option name isn't translatable, and because we can re-use a translation added in f7e68a08780 (parse-options: check empty value in OPT_INTEGER and OPT_ABBREV, 2019-05-29). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/commit-graph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'builtin/commit-graph.c') diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 0386f5c775..bffbe8a339 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -172,8 +172,8 @@ static int write_option_max_new_filters(const struct option *opt, const char *s; *to = strtol(arg, (char **)&s, 10); if (*s) - return error(_("%s expects a numerical value"), - optname(opt, opt->flags)); + return error(_("option `%s' expects a numerical value"), + "max-new-filters"); } return 0; } -- cgit v1.2.3