| Message ID | patch-6.6-5c1694e071e-20210720T113707Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: usage fixes | expand |
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 >
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'.
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
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.
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
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/
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
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
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" &&
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(-)