* [PATCH v3 1/6] commit-graph: define common usage with a macro
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 2/6] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
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=<n>', 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 <avarab@gmail.com>
---
builtin/commit-graph.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd863152216..5af3cd7178f 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 <objdir>] [--shallow] [--[no-]progress]"),
- N_("git commit-graph write [--object-dir <objdir>] [--append] "
- "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
- "<split options>"),
+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
+ N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
+
+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
+ N_("git commit-graph write [--object-dir <objdir>] [--append] " \
+ "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
+ "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
+ "<split options>")
+
+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 <objdir>] [--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 <objdir>] [--append] "
- "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
- "<split options>"),
- 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 {
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 2/6] commit-graph: remove redundant handling of -h
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 3/6] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 5af3cd7178f..3cf18dc5345 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,
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 3/6] commit-graph: use parse_options_concat()
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 2/6] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 11:39 ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
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 <me@ttaylorr.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3cf18dc5345..6e49184439f 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 @@ static int graph_write(int argc, const char **argv)
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,
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-07-20 11:39 ` [PATCH v3 3/6] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 18:14 ` Taylor Blau
2021-07-20 11:39 ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) to maintain the same brevity, but doesn't run
afoul of the recommendation in CodingGuidelines about braces:
When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency[...]
Let's also change "argv == 0" to juts "!argv", per:
Do not explicitly compare an integral value with constant 0 or
'\0', or a pointer value with constant NULL[...]
I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/multi-pack-index.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fdb..2952388a8eb 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
if (!opts.object_dir)
opts.object_dir = get_object_directory();
- if (argc == 0)
+ if (!argc)
goto usage;
if (!strcmp(argv[0], "repack"))
@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
return cmd_multi_pack_index_verify(argc, argv);
else if (!strcmp(argv[0], "expire"))
return cmd_multi_pack_index_expire(argc, argv);
- else {
+
usage:
- error(_("unrecognized subcommand: %s"), argv[0]);
- usage_with_options(builtin_multi_pack_index_usage,
- builtin_multi_pack_index_options);
- }
+ error(_("unrecognized subcommand: %s"), argv[0]);
+ usage_with_options(builtin_multi_pack_index_usage,
+ builtin_multi_pack_index_options);
}
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern
2021-07-20 11:39 ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
@ 2021-07-20 18:14 ` Taylor Blau
0 siblings, 0 replies; 40+ messages in thread
From: Taylor Blau @ 2021-07-20 18:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak
On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Refactor the "goto usage" pattern added in
> cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
> command, 2021-03-30) to maintain the same brevity, but doesn't run
> afoul of the recommendation in CodingGuidelines about braces:
>
> When there are multiple arms to a conditional and some of them
> require braces, enclose even a single line block in braces for
> consistency[...]
>
> Let's also change "argv == 0" to juts "!argv", per:
>
> Do not explicitly compare an integral value with constant 0 or
> '\0', or a pointer value with constant NULL[...]
>
> I'm changing this because in a subsequent commit I'll make
> builtin/commit-graph.c use the same pattern, having the two similarly
> structured commands match aids readability.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/multi-pack-index.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 5d3ea445fdb..2952388a8eb 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
> if (!opts.object_dir)
> opts.object_dir = get_object_directory();
>
> - if (argc == 0)
> + if (!argc)
> goto usage;
>
> if (!strcmp(argv[0], "repack"))
> @@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
> return cmd_multi_pack_index_verify(argc, argv);
> else if (!strcmp(argv[0], "expire"))
> return cmd_multi_pack_index_expire(argc, argv);
> - else {
> +
> usage:
> - error(_("unrecognized subcommand: %s"), argv[0]);
> - usage_with_options(builtin_multi_pack_index_usage,
> - builtin_multi_pack_index_options);
> - }
> + error(_("unrecognized subcommand: %s"), argv[0]);
Not the fault of this patch, but since we jump to this from the "if
(!argc)" conditional, reading "argv[0]" is UB. Some compilers will print
out:
error: unrecognized subcommand: (null)
which at least doesn't segfault, but is still ugly. I don't mind calling
it ugly since I was the one to introduce this behavior (by accident, of
course).
I sent a patch [1] yesterday to squash this, but we should consider
combining the two (or dropping this patch from the series and then
coming back to it later on).
If you want to combine forces, here's what I think that could look like
(without your s-o-b, which you'll have to reattach).
[1]: https://lore.kernel.org/git/8c0bb3e0dc121bd68f7014000fbb60b28750a0fe.1626715096.git.me@ttaylorr.com/
--- >8 ---
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Subject: [PATCH] multi-pack-index: refactor "goto usage" pattern
Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) to maintain the same brevity, but doesn't run
afoul of the recommendation in CodingGuidelines about braces:
When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency[...]
Let's also change "argv == 0" to juts "!argv", per:
Do not explicitly compare an integral value with constant 0 or
'\0', or a pointer value with constant NULL[...]
I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.
While here, fix an issue since where cd57bc41bb
(builtin/multi-pack-index.c: display usage on unrecognized command,
2021-03-30) we sometimes jump to 'usage' without any arguments.
Many compilers will save us from a segfault here, but the end result is
ugly, since it mentions an unrecognized subcommand when we didn't even
pass one, and (on GCC) includes "(null)" in its output.
Move the "usage" label down past the error about unrecognized
subcommands so that it is only triggered when it should be. While we're
at it, bulk up our test coverage in this area, too.
Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/multi-pack-index.c | 11 +++++------
t/t5319-multi-pack-index.sh | 6 ++++++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fd..e3a684c842 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
if (!opts.object_dir)
opts.object_dir = get_object_directory();
- if (argc == 0)
+ if (!argc)
goto usage;
if (!strcmp(argv[0], "repack"))
@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
return cmd_multi_pack_index_verify(argc, argv);
else if (!strcmp(argv[0], "expire"))
return cmd_multi_pack_index_expire(argc, argv);
- else {
-usage:
+ else
error(_("unrecognized subcommand: %s"), argv[0]);
- usage_with_options(builtin_multi_pack_index_usage,
- builtin_multi_pack_index_options);
- }
+usage:
+ usage_with_options(builtin_multi_pack_index_usage,
+ builtin_multi_pack_index_options);
}
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 5641d158df..fcde6dcded 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -824,4 +824,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
)
'
+
+test_expect_success 'usage shown without sub-command' '
+ test_expect_code 129 git multi-pack-index 2>err &&
+ ! test_i18ngrep "unrecognized subcommand" err
+'
+
test_done
--
2.31.1.163.ga65ce7f831
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-07-20 11:39 ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 18:17 ` Taylor Blau
2021-07-20 11:39 ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
6 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 6e49184439f..bf34aa43f22 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);
}
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc
2021-07-20 11:39 ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-20 18:17 ` Taylor Blau
0 siblings, 0 replies; 40+ messages in thread
From: Taylor Blau @ 2021-07-20 18:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak
On Tue, Jul 20, 2021 at 01:39:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
> builtin/commit-graph.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 6e49184439f..bf34aa43f22 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);
Nice, what you wrote here is definitely an improvement in readability.
This could potentially also have an else like I suggested in the
multi-pack-index patch earlier (later?) in the thread. Maybe something
like:
else if (strcmp(argv[0], "..."))
return graph_xyz(...);
else
error(_("unrecognized subcommand: %s"), argv[0]);
usage:
usage_with_options(...);
Arguably that could be done in this patch, separately, or not at all
;).
Thanks,
Taylor
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-07-20 11:39 ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
2021-07-20 17:47 ` SZEDER Gábor
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
6 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
Ævar Arnfjörð Bjarmason
Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.
But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 10 ++++++++--
t/t5318-commit-graph.sh | 5 +++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
opts.progress = isatty(2);
argc = parse_options(argc, argv, NULL,
options,
- builtin_commit_graph_verify_usage, 0);
+ builtin_commit_graph_verify_usage,
+ PARSE_OPT_KEEP_UNKNOWN);
+ if (argc)
+ usage_with_options(builtin_commit_graph_verify_usage, options);
if (!opts.obj_dir)
opts.obj_dir = get_object_directory();
@@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)
argc = parse_options(argc, argv, NULL,
options,
- builtin_commit_graph_write_usage, 0);
+ builtin_commit_graph_write_usage,
+ PARSE_OPT_KEEP_UNKNOWN);
+ 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"));
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index af88f805aa2..09a2ccd2920 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -5,6 +5,11 @@ test_description='commit graph'
GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
+test_expect_success 'usage' '
+ test_expect_code 129 git commit-graph write blah &&
+ test_expect_code 129 git commit-graph write verify
+'
+
test_expect_success 'setup full repo' '
mkdir full &&
cd "$TRASH_DIRECTORY/full" &&
--
2.32.0.874.ge7a9d58bfcf
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 11:39 ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-07-20 17:47 ` SZEDER Gábor
2021-07-20 17:55 ` SZEDER Gábor
0 siblings, 1 reply; 40+ messages in thread
From: SZEDER Gábor @ 2021-07-20 17:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak
On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Change the parse_options() invocation in the commit-graph code to make
> sense. We're calling it twice, once for common options parsing, and
> then for the sub-commands.
>
> But we never checked if we had something leftover in argc in "write"
> or "verify", as a result we'd silently accept garbage in these
> subcommands. Let's not do that.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/commit-graph.c | 10 ++++++++--
> t/t5318-commit-graph.sh | 5 +++++
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index bf34aa43f22..88cbcb5a64f 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
> opts.progress = isatty(2);
> argc = parse_options(argc, argv, NULL,
> options,
> - builtin_commit_graph_verify_usage, 0);
> + builtin_commit_graph_verify_usage,
> + PARSE_OPT_KEEP_UNKNOWN);
> + if (argc)
> + usage_with_options(builtin_commit_graph_verify_usage, options);
Checking 'argc' alone is sufficient to catch unsupported parameters.
Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here, because 'git commit-graph write --foo' won't print "error:
unknown option `foo'", and we don't want to pass the remaining
unrecognized options to a different command, like e.g. 'git difftool',
or another parse_options(), like e.g. 'git archive'.
> if (!opts.obj_dir)
> opts.obj_dir = get_object_directory();
> @@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)
>
> argc = parse_options(argc, argv, NULL,
> options,
> - builtin_commit_graph_write_usage, 0);
> + builtin_commit_graph_write_usage,
> + PARSE_OPT_KEEP_UNKNOWN);
> + 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"));
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index af88f805aa2..09a2ccd2920 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -5,6 +5,11 @@ test_description='commit graph'
>
> GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
>
> +test_expect_success 'usage' '
> + test_expect_code 129 git commit-graph write blah &&
> + test_expect_code 129 git commit-graph write verify
> +'
> +
> test_expect_success 'setup full repo' '
> mkdir full &&
> cd "$TRASH_DIRECTORY/full" &&
> --
> 2.32.0.874.ge7a9d58bfcf
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 17:47 ` SZEDER Gábor
@ 2021-07-20 17:55 ` SZEDER Gábor
2021-07-20 18:24 ` Taylor Blau
0 siblings, 1 reply; 40+ messages in thread
From: SZEDER Gábor @ 2021-07-20 17:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak
On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:
> On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > Change the parse_options() invocation in the commit-graph code to make
> > sense. We're calling it twice, once for common options parsing, and
> > then for the sub-commands.
> >
> > But we never checked if we had something leftover in argc in "write"
> > or "verify", as a result we'd silently accept garbage in these
> > subcommands. Let's not do that.
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > ---
> > builtin/commit-graph.c | 10 ++++++++--
> > t/t5318-commit-graph.sh | 5 +++++
> > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> > index bf34aa43f22..88cbcb5a64f 100644
> > --- a/builtin/commit-graph.c
> > +++ b/builtin/commit-graph.c
> > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
> > opts.progress = isatty(2);
> > argc = parse_options(argc, argv, NULL,
> > options,
> > - builtin_commit_graph_verify_usage, 0);
> > + builtin_commit_graph_verify_usage,
> > + PARSE_OPT_KEEP_UNKNOWN);
> > + if (argc)
> > + usage_with_options(builtin_commit_graph_verify_usage, options);
>
> Checking 'argc' alone is sufficient to catch unsupported parameters.
>
> Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
> wrong here
And it's wrong in 'multi-pack-index' and 'env--helper' as well.
> because 'git commit-graph write --foo' won't print "error:
> unknown option `foo'", and we don't want to pass the remaining
> unrecognized options to a different command, like e.g. 'git difftool',
> or another parse_options(), like e.g. 'git archive'.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 17:55 ` SZEDER Gábor
@ 2021-07-20 18:24 ` Taylor Blau
2021-07-20 21:17 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 40+ messages in thread
From: Taylor Blau @ 2021-07-20 18:24 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Derrick Stolee, Taylor Blau, Andrei Rybak
On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:
> On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:
> > On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > > Change the parse_options() invocation in the commit-graph code to make
> > > sense. We're calling it twice, once for common options parsing, and
> > > then for the sub-commands.
> > >
> > > But we never checked if we had something leftover in argc in "write"
> > > or "verify", as a result we'd silently accept garbage in these
> > > subcommands. Let's not do that.
> > >
> > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > > ---
> > > builtin/commit-graph.c | 10 ++++++++--
> > > t/t5318-commit-graph.sh | 5 +++++
> > > 2 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> > > index bf34aa43f22..88cbcb5a64f 100644
> > > --- a/builtin/commit-graph.c
> > > +++ b/builtin/commit-graph.c
> > > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
> > > opts.progress = isatty(2);
> > > argc = parse_options(argc, argv, NULL,
> > > options,
> > > - builtin_commit_graph_verify_usage, 0);
> > > + builtin_commit_graph_verify_usage,
> > > + PARSE_OPT_KEEP_UNKNOWN);
> > > + if (argc)
> > > + usage_with_options(builtin_commit_graph_verify_usage, options);
> >
> > Checking 'argc' alone is sufficient to catch unsupported parameters.
> >
> > Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
> > wrong here
>
> And it's wrong in 'multi-pack-index' and 'env--helper' as well.
Thanks for spotting. Obviously this one is new, but the one in the midx
builtin is my fault; I'm not sure why it's there, because it's clearly
wrong.
So we should definitely fix this instance via a reroll of this series,
but that still leaves the others up for grabs. Ævar, are those (the ones
in the 'multi-pack-index' and 'env--helper' builtins) something that you
want to clean up while you're working in this area, or would you rather
that I take care of it?
I don't mind either way, just want to make sure that we don't duplicate
effort.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 18:24 ` Taylor Blau
@ 2021-07-20 21:17 ` Ævar Arnfjörð Bjarmason
2021-07-20 21:47 ` Taylor Blau
0 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 21:17 UTC (permalink / raw)
To: Taylor Blau
Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee,
Andrei Rybak
On Tue, Jul 20 2021, Taylor Blau wrote:
> On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:
>> On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:
>> > On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > > Change the parse_options() invocation in the commit-graph code to make
>> > > sense. We're calling it twice, once for common options parsing, and
>> > > then for the sub-commands.
>> > >
>> > > But we never checked if we had something leftover in argc in "write"
>> > > or "verify", as a result we'd silently accept garbage in these
>> > > subcommands. Let's not do that.
>> > >
>> > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> > > ---
>> > > builtin/commit-graph.c | 10 ++++++++--
>> > > t/t5318-commit-graph.sh | 5 +++++
>> > > 2 files changed, 13 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>> > > index bf34aa43f22..88cbcb5a64f 100644
>> > > --- a/builtin/commit-graph.c
>> > > +++ b/builtin/commit-graph.c
>> > > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
>> > > opts.progress = isatty(2);
>> > > argc = parse_options(argc, argv, NULL,
>> > > options,
>> > > - builtin_commit_graph_verify_usage, 0);
>> > > + builtin_commit_graph_verify_usage,
>> > > + PARSE_OPT_KEEP_UNKNOWN);
>> > > + if (argc)
>> > > + usage_with_options(builtin_commit_graph_verify_usage, options);
>> >
>> > Checking 'argc' alone is sufficient to catch unsupported parameters.
>> >
>> > Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
>> > wrong here
>>
>> And it's wrong in 'multi-pack-index' and 'env--helper' as well.
>
> Thanks for spotting. Obviously this one is new, but the one in the midx
> builtin is my fault; I'm not sure why it's there, because it's clearly
> wrong.
>
> So we should definitely fix this instance via a reroll of this series,
> but that still leaves the others up for grabs. Ævar, are those (the ones
> in the 'multi-pack-index' and 'env--helper' builtins) something that you
> want to clean up while you're working in this area, or would you rather
> that I take care of it?
>
> I don't mind either way, just want to make sure that we don't duplicate
> effort.
I'm all for you picking it up :)
If you wanted to pick up these patches (or some of them) and
partially/entirely replace this series I'd be happy with that too,
i.e. if it makes conflicts etc. easier.
I just re-submitted this now because it's been staring at me in my
"should re-roll at somep point" list for a while...
FWIW if you're poking at this area more generally we really could do
with some standardization around these built-in sub-commands:
git built-in --here subcommand
git built-in subcommand --or-here
Various commands support one or the other as some confusing amalgamation
of either taking options to "built-in" and/or "subcommand". The sane
thing to do is:
git --git-options built-in --built-in-options subcommand --just-for-the-subcommand
But not everything does that, and some things that should be
--git-options or --built-in-options we take as subcommand options,
e.g. --object-format or whatever.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 21:17 ` Ævar Arnfjörð Bjarmason
@ 2021-07-20 21:47 ` Taylor Blau
2021-07-21 7:26 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 40+ messages in thread
From: Taylor Blau @ 2021-07-20 21:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee,
Andrei Rybak
On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > So we should definitely fix this instance via a reroll of this series,
> > but that still leaves the others up for grabs. Ævar, are those (the ones
> > in the 'multi-pack-index' and 'env--helper' builtins) something that you
> > want to clean up while you're working in this area, or would you rather
> > that I take care of it?
> >
> > I don't mind either way, just want to make sure that we don't duplicate
> > effort.
>
> I'm all for you picking it up :)
>
> If you wanted to pick up these patches (or some of them) and
> partially/entirely replace this series I'd be happy with that too,
> i.e. if it makes conflicts etc. easier.
I think either is fine; there shouldn't be any conflicts in the
multi-pack-index code just eyeballing based on what you wrote.
I started working on it (which I suppose can count for me volunteering
to patch it up myself ;-)), but I wondered why we have env--helper at
all. When you wrote it back in b4f207f339 (env--helper: new undocumented
builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added
as a test-tool because it had some uses outside of tests.
But I can't find any locations. We do use env--helper (via
test_bool_env, which you also introduced) in a couple of t/lib-*.sh
files, but this would be far from the first test-tool that has been used
in a test library.
So ISTM that this could be converted to a test-tool and removed from the
list of builtins. *And* we could do that without a deprecation warning,
because it was never documented in the first place.
Can you double check my thinking and/or let me know if there is a
compelling reason to keep it as a builtin?
> I just re-submitted this now because it's been staring at me in my
> "should re-roll at somep point" list for a while...
>
> FWIW if you're poking at this area more generally we really could do
> with some standardization around these built-in sub-commands:
>
> git built-in --here subcommand
> git built-in subcommand --or-here
That's probably a step too far for this loose end for me, so if you want
to work on that please be my guest, but I probably don't have time for
it in the near future.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-20 21:47 ` Taylor Blau
@ 2021-07-21 7:26 ` Ævar Arnfjörð Bjarmason
2021-07-21 8:08 ` Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 7:26 UTC (permalink / raw)
To: Taylor Blau
Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee,
Andrei Rybak
On Tue, Jul 20 2021, Taylor Blau wrote:
> On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > So we should definitely fix this instance via a reroll of this series,
>> > but that still leaves the others up for grabs. Ævar, are those (the ones
>> > in the 'multi-pack-index' and 'env--helper' builtins) something that you
>> > want to clean up while you're working in this area, or would you rather
>> > that I take care of it?
>> >
>> > I don't mind either way, just want to make sure that we don't duplicate
>> > effort.
>>
>> I'm all for you picking it up :)
>>
>> If you wanted to pick up these patches (or some of them) and
>> partially/entirely replace this series I'd be happy with that too,
>> i.e. if it makes conflicts etc. easier.
>
> I think either is fine; there shouldn't be any conflicts in the
> multi-pack-index code just eyeballing based on what you wrote.
>
> I started working on it (which I suppose can count for me volunteering
> to patch it up myself ;-)), but I wondered why we have env--helper at
> all. When you wrote it back in b4f207f339 (env--helper: new undocumented
> builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added
> as a test-tool because it had some uses outside of tests.
>
> But I can't find any locations. We do use env--helper (via
> test_bool_env, which you also introduced) in a couple of t/lib-*.sh
FWIW test_bool_env is SZEDER'S, see 43a2afee82a (tests: add
'test_bool_env' to catch non-bool GIT_TEST_* values, 2019-11-22).
> files, but this would be far from the first test-tool that has been used
> in a test library.
>
> So ISTM that this could be converted to a test-tool and removed from the
> list of builtins. *And* we could do that without a deprecation warning,
> because it was never documented in the first place.
>
> Can you double check my thinking and/or let me know if there is a
> compelling reason to keep it as a builtin?
Unlike the test-tools it was used by installed git, namely
git-sh-i18n.sh before d162b25f956 (tests: remove support for
GIT_TEST_GETTEXT_POISON, 2021-01-20), but yes, now it could just be
migrated to a test-tool.
I suppose it never really *needed* to be a built-in, i.e. the
git-sh-i18n.sh code could have just died without the helper if
GIT_TEST_GETTEXT_POISON was set, anyway...
...yes, nowadays it can simply be moved to t/helper.
<digression>
I do think in general this recent proliferation of t/helper over new
plumbing built-ins has sent git a bit in the wrong direction.
E.g. I think the likes of t/helper/test-pkt-line.c should really be a
plumbing tool, the same goes for many (but not all) the test tool, we
could just document them as being "unstable plumbing" or whatever.
But I think I've been losing that argument recently, e.g. after [1]
(which I argued we should put into git-ls-files) even things like git's
basic idea of the state of the index are exposed in some helpers, but
not corresponding plumbing.
Anyway, even if we assume that's an argument that would carry the day in
general I'd find it hard to justify git-env--helper being a thing that
should be exposed to users or post-"make install", it's purely for the
use of our own test suite.
</digression>
So yeah, it can just be moved to t/helper if you want to do that.
>> I just re-submitted this now because it's been staring at me in my
>> "should re-roll at somep point" list for a while...
>>
>> FWIW if you're poking at this area more generally we really could do
>> with some standardization around these built-in sub-commands:
>>
>> git built-in --here subcommand
>> git built-in subcommand --or-here
>
> That's probably a step too far for this loose end for me, so if you want
> to work on that please be my guest, but I probably don't have time for
> it in the near future.
*nod*, but just to be clear, were you going to pick up &
re-roll/re-submit the patches I have in this series? That would be good
or me, but I don't mind either way, just wondering what state this
should be on my TODO list.
1. https://lore.kernel.org/git/a1b8135c0fc890b2c2c0271c923b2f8efa8d1465.1617109864.git.gitgitgadget@gmail.com/
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-21 7:26 ` Ævar Arnfjörð Bjarmason
@ 2021-07-21 8:08 ` Jeff King
2021-07-21 16:54 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2021-07-21 8:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Taylor Blau, SZEDER Gábor, git, Junio C Hamano,
Derrick Stolee, Andrei Rybak
On Wed, Jul 21, 2021 at 09:26:38AM +0200, Ævar Arnfjörð Bjarmason wrote:
> <digression>
>
> I do think in general this recent proliferation of t/helper over new
> plumbing built-ins has sent git a bit in the wrong direction.
>
> E.g. I think the likes of t/helper/test-pkt-line.c should really be a
> plumbing tool, the same goes for many (but not all) the test tool, we
> could just document them as being "unstable plumbing" or whatever.
FWIW, I agree with you here. These kind of "inspection" tools are handy
when you are debugging something. Building a copy of test-tool on a
production system is only a mild inconvenience for me, but not being
able to ask a user things like "what does git pack-bitmap --dump
.git/objects/pack*.bitmap say" is occasionally quite annoying.
The flip side is that we expect the overall quality of user-visible
tools to be a bit higher, and we're generally on the hook to keep
supporting them. Maybe that's solvable with documentation. I dunno.
> But I think I've been losing that argument recently, e.g. after [1]
> (which I argued we should put into git-ls-files) even things like git's
> basic idea of the state of the index are exposed in some helpers, but
> not corresponding plumbing.
Yeah. I wish "ls-files --debug" showed more of the extension data, for
example.
> Anyway, even if we assume that's an argument that would carry the day in
> general I'd find it hard to justify git-env--helper being a thing that
> should be exposed to users or post-"make install", it's purely for the
> use of our own test suite.
Yeah, I'd agree with that. The most valuable helpers to me are the ones
that help us understand what Git is seeing, or what's in a binary file
format. Obscure-case "functional" helpers are less likely to be
generally useful.
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-07-21 8:08 ` Jeff King
@ 2021-07-21 16:54 ` Junio C Hamano
0 siblings, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-07-21 16:54 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, Taylor Blau,
SZEDER Gábor, git, Derrick Stolee, Andrei Rybak
Jeff King <peff@peff.net> writes:
>> But I think I've been losing that argument recently, e.g. after [1]
>> (which I argued we should put into git-ls-files) even things like git's
>> basic idea of the state of the index are exposed in some helpers, but
>> not corresponding plumbing.
>
> Yeah. I wish "ls-files --debug" showed more of the extension data, for
> example.
Let me second that ;-) With extensions and even more drastic things
like sparse index entries, "pretend that the index is a flat list of
<mode, object, path>" is sometimes not useful in debugging (as bugs
and design mistakes might lie in the code that makes us pretend).
Even with packed objects, we still "pretetend that an object file is
a single line '<type> SP <len> NUL' followed by payload bytes,
deflated", as if packed objects do not exist. I do not offhand
recall we have a good debugging option in the plumbing, or a
dedicated debugging tool, but because the format for packed objects
has been stable, we are probably OK. Contrast to that, the index is
designed to be more ephemeral and its format is subject to change,
so we may in more need for a good debugging option.
> Yeah, I'd agree with that. The most valuable helpers to me are the ones
> that help us understand what Git is seeing, or what's in a binary file
> format. Obscure-case "functional" helpers are less likely to be
> generally useful.
Yup
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v4 0/7] commit-graph: usage fixes
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-07-20 11:39 ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 1/7] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
` (7 more replies)
6 siblings, 8 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
A re-roll of v3 which should fix all outstanding issues &
feedback. Thanks SZEDER & Taylor:
https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/
I dropped this myself because per
https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was
under the impression that Taylor was intending to pick up these
patches as part of some more generale usage() fixes of his (which also
touched multi-pack-index.c), but his recent changes to fix
multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.
Ævar Arnfjörð Bjarmason (7):
commit-graph: define common usage with a macro
commit-graph: remove redundant handling of -h
commit-graph: use parse_options_concat()
multi-pack-index: refactor "goto usage" pattern
commit-graph: early exit to "usage" on !argc
commit-graph: show usage on "commit-graph [write|verify] garbage"
commit-graph: show "unexpected subcommand" error
builtin/commit-graph.c | 90 +++++++++++++++++++++-----------------
builtin/multi-pack-index.c | 11 +++--
t/t5318-commit-graph.sh | 19 ++++++++
3 files changed, 74 insertions(+), 46 deletions(-)
Range-diff against v3:
1: 1b9b4703ce2 = 1: ef37a8243c8 commit-graph: define common usage with a macro
2: 8f50750ae5e = 2: bbed18ff193 commit-graph: remove redundant handling of -h
3: f02da994363 = 3: 32cc0d1c7bc commit-graph: use parse_options_concat()
4: 6e9bd877866 ! 4: 087f98bbec6 multi-pack-index: refactor "goto usage" pattern
@@ Commit message
Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
- command, 2021-03-30) to maintain the same brevity, but doesn't run
- afoul of the recommendation in CodingGuidelines about braces:
+ command, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential
+ segfault without sub-command, 2021-07-19) to maintain the same
+ brevity, but in a form that doesn't run afoul of the recommendation in
+ CodingGuidelines about braces:
When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
@@ builtin/multi-pack-index.c: int cmd_multi_pack_index(int argc, const char **argv
else if (!strcmp(argv[0], "expire"))
return cmd_multi_pack_index_expire(argc, argv);
- else {
+- error(_("unrecognized subcommand: %s"), argv[0]);
+
++ error(_("unrecognized subcommand: %s"), argv[0]);
usage:
-- error(_("unrecognized subcommand: %s"), argv[0]);
- usage_with_options(builtin_multi_pack_index_usage,
- builtin_multi_pack_index_options);
- }
-+ error(_("unrecognized subcommand: %s"), argv[0]);
+ usage_with_options(builtin_multi_pack_index_usage,
+ builtin_multi_pack_index_options);
}
5: 7acb4bd75ce = 5: 2983e16ba69 commit-graph: early exit to "usage" on !argc
6: 5c1694e071e ! 6: 85552a6f88c commit-graph: show usage on "commit-graph [write|verify] garbage"
@@ Metadata
## Commit message ##
commit-graph: show usage on "commit-graph [write|verify] garbage"
- Change the parse_options() invocation in the commit-graph code to make
- sense. We're calling it twice, once for common options parsing, and
- then for the sub-commands.
+ 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.
- But we never checked if we had something leftover in argc in "write"
- or "verify", as a result we'd silently accept garbage in these
- subcommands. Let's not do that.
+ 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 <avarab@gmail.com>
## builtin/commit-graph.c ##
@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
- opts.progress = isatty(2);
argc = parse_options(argc, argv, NULL,
options,
-- builtin_commit_graph_verify_usage, 0);
-+ builtin_commit_graph_verify_usage,
-+ PARSE_OPT_KEEP_UNKNOWN);
+ 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();
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
-
argc = parse_options(argc, argv, NULL,
options,
-- builtin_commit_graph_write_usage, 0);
-+ builtin_commit_graph_write_usage,
-+ PARSE_OPT_KEEP_UNKNOWN);
+ builtin_commit_graph_write_usage, 0);
+ if (argc)
+ usage_with_options(builtin_commit_graph_write_usage, options);
-: ----------- > 7: 962521cfa17 commit-graph: show "unexpected subcommand" error
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v4 1/7] commit-graph: define common usage with a macro
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 2/7] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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=<n>', 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 <avarab@gmail.com>
---
builtin/commit-graph.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd863152216..5af3cd7178f 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 <objdir>] [--shallow] [--[no-]progress]"),
- N_("git commit-graph write [--object-dir <objdir>] [--append] "
- "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
- "<split options>"),
+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
+ N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
+
+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
+ N_("git commit-graph write [--object-dir <objdir>] [--append] " \
+ "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
+ "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
+ "<split options>")
+
+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 <objdir>] [--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 <objdir>] [--append] "
- "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
- "<split options>"),
- 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 {
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 2/7] commit-graph: remove redundant handling of -h
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 1/7] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 3/7] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 5af3cd7178f..3cf18dc5345 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,
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 3/7] commit-graph: use parse_options_concat()
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 1/7] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 2/7] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 4/7] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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 <me@ttaylorr.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3cf18dc5345..6e49184439f 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 @@ static int graph_write(int argc, const char **argv)
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,
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 4/7] multi-pack-index: refactor "goto usage" pattern
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-08-23 12:30 ` [PATCH v4 3/7] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 5/7] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential
segfault without sub-command, 2021-07-19) to maintain the same
brevity, but in a form that doesn't run afoul of the recommendation in
CodingGuidelines about braces:
When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency[...]
Let's also change "argv == 0" to juts "!argv", per:
Do not explicitly compare an integral value with constant 0 or
'\0', or a pointer value with constant NULL[...]
I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/multi-pack-index.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 8ff0dee2ecb..649aa5f9ab2 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
if (!opts.object_dir)
opts.object_dir = get_object_directory();
- if (argc == 0)
+ if (!argc)
goto usage;
if (!strcmp(argv[0], "repack"))
@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
return cmd_multi_pack_index_verify(argc, argv);
else if (!strcmp(argv[0], "expire"))
return cmd_multi_pack_index_expire(argc, argv);
- else {
- error(_("unrecognized subcommand: %s"), argv[0]);
+
+ error(_("unrecognized subcommand: %s"), argv[0]);
usage:
- usage_with_options(builtin_multi_pack_index_usage,
- builtin_multi_pack_index_options);
- }
+ usage_with_options(builtin_multi_pack_index_usage,
+ builtin_multi_pack_index_options);
}
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 5/7] commit-graph: early exit to "usage" on !argc
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-08-23 12:30 ` [PATCH v4 4/7] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 6/7] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 6e49184439f..bf34aa43f22 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);
}
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 6/7] commit-graph: show usage on "commit-graph [write|verify] garbage"
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-08-23 12:30 ` [PATCH v4 5/7] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-23 12:30 ` [PATCH v4 7/7] commit-graph: show "unexpected subcommand" error Ævar Arnfjörð Bjarmason
2021-08-30 23:54 ` [PATCH v4 0/7] commit-graph: usage fixes Taylor Blau
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 4 ++++
t/t5318-commit-graph.sh | 5 +++++
2 files changed, 9 insertions(+)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..0457903f18f 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"));
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index af88f805aa2..09a2ccd2920 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -5,6 +5,11 @@ test_description='commit graph'
GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
+test_expect_success 'usage' '
+ test_expect_code 129 git commit-graph write blah &&
+ test_expect_code 129 git commit-graph write verify
+'
+
test_expect_success 'setup full repo' '
mkdir full &&
cd "$TRASH_DIRECTORY/full" &&
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v4 7/7] commit-graph: show "unexpected subcommand" error
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-08-23 12:30 ` [PATCH v4 6/7] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-08-23 12:30 ` Ævar Arnfjörð Bjarmason
2021-08-30 23:54 ` [PATCH v4 0/7] commit-graph: usage fixes Taylor Blau
7 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-23 12:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
SZEDER Gábor, Jeff King,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/commit-graph.c | 1 +
t/t5318-commit-graph.sh | 16 +++++++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 0457903f18f..21fc6e934b0 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);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 09a2ccd2920..295c5bd94d2 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -6,10 +6,24 @@ test_description='commit graph'
GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
test_expect_success 'usage' '
- test_expect_code 129 git commit-graph write blah &&
+ test_expect_code 129 git commit-graph write blah 2>err &&
test_expect_code 129 git commit-graph write verify
'
+test_expect_success 'usage shown without sub-command' '
+ test_expect_code 129 git commit-graph 2>err &&
+ ! grep error: err
+'
+
+test_expect_success 'usage shown with an error on unknown sub-command' '
+ cat >expect <<-\EOF &&
+ error: unrecognized subcommand: unknown
+ EOF
+ test_expect_code 129 git commit-graph unknown 2>stderr &&
+ grep error stderr >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'setup full repo' '
mkdir full &&
cd "$TRASH_DIRECTORY/full" &&
--
2.33.0.662.gbc81f8cbdca
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v4 0/7] commit-graph: usage fixes
2021-08-23 12:30 ` [PATCH v4 0/7] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-08-23 12:30 ` [PATCH v4 7/7] commit-graph: show "unexpected subcommand" error Ævar Arnfjörð Bjarmason
@ 2021-08-30 23:54 ` Taylor Blau
2021-08-30 23:58 ` Junio C Hamano
7 siblings, 1 reply; 40+ messages in thread
From: Taylor Blau @ 2021-08-30 23:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Andrei Rybak,
SZEDER Gábor, Jeff King
On Mon, Aug 23, 2021 at 02:30:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
> A re-roll of v3 which should fix all outstanding issues &
> feedback. Thanks SZEDER & Taylor:
> https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/
>
> I dropped this myself because per
> https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was
> under the impression that Taylor was intending to pick up these
> patches as part of some more generale usage() fixes of his (which also
> touched multi-pack-index.c), but his recent changes to fix
> multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.
Thanks, and sorry for totally dropping this on the floor even though I
said that I would pick it up. Since it had been a while, I re-read
through the patches and they all look good to me.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v4 0/7] commit-graph: usage fixes
2021-08-30 23:54 ` [PATCH v4 0/7] commit-graph: usage fixes Taylor Blau
@ 2021-08-30 23:58 ` Junio C Hamano
0 siblings, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-08-30 23:58 UTC (permalink / raw)
To: Taylor Blau
Cc: Ævar Arnfjörð Bjarmason, git, Derrick Stolee,
Andrei Rybak, SZEDER Gábor, Jeff King
Taylor Blau <me@ttaylorr.com> writes:
> On Mon, Aug 23, 2021 at 02:30:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> A re-roll of v3 which should fix all outstanding issues &
>> feedback. Thanks SZEDER & Taylor:
>> https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/
>>
>> I dropped this myself because per
>> https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was
>> under the impression that Taylor was intending to pick up these
>> patches as part of some more generale usage() fixes of his (which also
>> touched multi-pack-index.c), but his recent changes to fix
>> multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.
>
> Thanks, and sorry for totally dropping this on the floor even though I
> said that I would pick it up. Since it had been a while, I re-read
> through the patches and they all look good to me.
>
> Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread