| Message ID | xmqqttocp98r.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/PATCH] archive: "--list" does not take further options | expand |
Am 21.12.23 um 03:41 schrieb Junio C Hamano: > "git archive --list blah" should notice an extra command line > parameter that goes unused. Make it so. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * This was done to convince myself that even though cmd_archive() > calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then > uses the resulting argc/argv without apparent filtering of the > "--end-of-options" thing, it is correctly handling it, either > locally or remotely. > > - locally, write_archive() uses parse_archive_args(), which calls > parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options" > is handled there just fine. > > - remotely, run_remote_archiver() relays the local parameters, > including "--end-of-options" via the "argument" packet. Then > the arguments are assembled into a strvec and used by the > upload-archive running on the other side to spawn an > upload-archive--writer process with. > cmd_upload_archive_writer() then makes the same write_archive() > call; "--end-of-options" would still be in argv[] if the > original "git archive --remote" invocation had one, but it is > consumed the same way as the local case in write_archive() by > calling parse_archive_args(). > > I do not like the remote error behaviour this one adds at all. > Do we use a more proper mechanism to propagate a remote error > back for other subcommands we can reuse here? Don't we have one? It would affect other unsupported options as well, and this seems to work just fine, e.g.: $ git archive --remote=. --format=foo HEAD remote: fatal: Unknown archive format 'foo' remote: git upload-archive: archiver died with error fatal: sent error to the client: git upload-archive: archiver died with error > > archive.c | 7 +++++++ > t/t5000-tar-tree.sh | 14 ++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git c/archive.c w/archive.c > index 9aeaf2bd87..3244e9f9f2 100644 > --- c/archive.c > +++ w/archive.c > @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv, > base = ""; > > if (list) { > + if (argc) { > + if (!is_remote) > + die(_("extra command line parameter '%s'"), *argv); > + else > + printf("!ERROR! extra command line parameter '%s'\n", > + *argv); > + } So just call die() here? > for (i = 0; i < nr_archivers; i++) > if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE) > printf("%s\n", archivers[i]->name); > diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh > index 918a2fc7c6..04592f45b0 100755 > --- c/t/t5000-tar-tree.sh > +++ w/t/t5000-tar-tree.sh > @@ -124,6 +124,20 @@ test_expect_success 'setup' ' > EOF > ' > > +test_expect_success '--list notices extra parameters' ' > + test_must_fail git archive --list blah && > + # NEEDSWORK: remote error does not result in non-zero > + # exit, which we might want to change later. > + git archive --remote=. --list blah >remote-out && > + grep "!ERROR! " remote-out ... and use test_must_fail in both cases? > +' > + > +test_expect_success 'end-of-options is correctly eaten' ' > + git archive --list --end-of-options && > + git archive --remote=. --list --end-of-options >remote-out && > + ! grep "!ERROR! " remote-out > +' > + > test_expect_success 'populate workdir' ' > mkdir a && > echo simple textfile >a/a &&
On Wed, Dec 20, 2023 at 06:41:56PM -0800, Junio C Hamano wrote: > --- > * This was done to convince myself that even though cmd_archive() > calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then > uses the resulting argc/argv without apparent filtering of the > "--end-of-options" thing, it is correctly handling it, either > locally or remotely. > > - locally, write_archive() uses parse_archive_args(), which calls > parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options" > is handled there just fine. Right. Not only is it handled by the second parser, but _not_ keeping it would be a bug, because that second parser would have no idea that we saw end-of-options (and so "git archive --end-of-options --foo" would try to treat "--foo" as an option). The recent commit 9385174627 did fix a case like this for fast-export, but git-archive was not changed. It passed KEEP_DASHDASH along with KEEP_UNKNOWN_OPT, so it already retained and passed along --end-of-options. > - remotely, run_remote_archiver() relays the local parameters, > including "--end-of-options" via the "argument" packet. Then > the arguments are assembled into a strvec and used by the > upload-archive running on the other side to spawn an > upload-archive--writer process with. > cmd_upload_archive_writer() then makes the same write_archive() > call; "--end-of-options" would still be in argv[] if the > original "git archive --remote" invocation had one, but it is > consumed the same way as the local case in write_archive() by > calling parse_archive_args(). Right, and this is just the same case but with a lot of complicated network-ferrying in between. :) We must retain --end-of-options so that the next parser knows to stop treating them as arguments. And because it doesn't use KEEP_UNKNOWN_OPT, the ferried "--end-of-options" is removed then. > diff --git c/archive.c w/archive.c > index 9aeaf2bd87..3244e9f9f2 100644 > --- c/archive.c > +++ w/archive.c > @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv, > base = ""; > > if (list) { > + if (argc) { > + if (!is_remote) > + die(_("extra command line parameter '%s'"), *argv); > + else > + printf("!ERROR! extra command line parameter '%s'\n", > + *argv); > + } This general direction seems reasonable to me, since we're letting the user know that their extra argument was ignored (though note that if it was a misspelled option, like "--otuput", we would complain about that). It's largely orthogonal to end-of-options, but I see how you got here by wondering about it. :) -Peff
On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote: > > I do not like the remote error behaviour this one adds at all. > > Do we use a more proper mechanism to propagate a remote error > > back for other subcommands we can reuse here? > > Don't we have one? It would affect other unsupported options as well, > and this seems to work just fine, e.g.: > > $ git archive --remote=. --format=foo HEAD > remote: fatal: Unknown archive format 'foo' > remote: git upload-archive: archiver died with error > fatal: sent error to the client: git upload-archive: archiver died with error Right. The whole idea of upload-archive is to spawn a separate writer process and mux the conversation (including errors) back over the wire. There are a zillion reasons it can die (including bad arguments) and we catch and report them in the muxing process. > > if (list) { > > + if (argc) { > > + if (!is_remote) > > + die(_("extra command line parameter '%s'"), *argv); > > + else > > + printf("!ERROR! extra command line parameter '%s'\n", > > + *argv); > > + } > > So just call die() here? Yes, exactly. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Dec 21, 2023 at 08:30:36AM +0100, René Scharfe wrote: >> ... >> Don't we have one? It would affect other unsupported options as well, >> and this seems to work just fine, e.g.: >> ... > Right. The whole idea of upload-archive is to spawn a separate writer > process and mux the conversation (including errors) back over the wire. Thanks, both. Just to tie the loose end, let me queue this and merge it to 'next'. ----- >8 --------- >8 --------- >8 ----- Subject: [PATCH] archive: "--list" does not take further options "git archive --list blah" should notice an extra command line parameter that goes unused. Make it so. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- archive.c | 2 ++ t/t5000-tar-tree.sh | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/archive.c b/archive.c index ca11db185b..8da820d1ce 100644 --- a/archive.c +++ b/archive.c @@ -685,6 +685,8 @@ static int parse_archive_args(int argc, const char **argv, base = ""; if (list) { + if (argc) + die(_("extra command line parameter '%s'"), *argv); for (i = 0; i < nr_archivers; i++) if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE) printf("%s\n", archivers[i]->name); diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index 4b4c3315d8..72b8d0ff02 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -124,6 +124,16 @@ test_expect_success 'setup' ' EOF ' +test_expect_success '--list notices extra parameters' ' + test_must_fail git archive --list blah && + test_must_fail git archive --remote=. --list blah +' + +test_expect_success 'end-of-options is correctly eaten' ' + git archive --list --end-of-options && + git archive --remote=. --list --end-of-options +' + test_expect_success 'populate workdir' ' mkdir a && echo simple textfile >a/a &&
On Thu, Dec 21, 2023 at 10:13:58AM -0800, Junio C Hamano wrote: > Thanks, both. Just to tie the loose end, let me queue this and > merge it to 'next'. > > ----- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] archive: "--list" does not take further options This looks fine to me. It does mean that this sequence of commands no longer works: [let's try to make a "foo" archive] $ git archive --format=foo HEAD fatal: Unknown archive format 'foo' [oops, that didn't work. What formats are supported?] $ !! --list git archive --format=foo HEAD --list tar tgz tar.gz zip I think that's OK in practice. The increased error checking is worth it (and matches many other commands, which tend to warn about confusing extra bits). -Peff
diff --git c/archive.c w/archive.c index 9aeaf2bd87..3244e9f9f2 100644 --- c/archive.c +++ w/archive.c @@ -641,6 +641,13 @@ static int parse_archive_args(int argc, const char **argv, base = ""; if (list) { + if (argc) { + if (!is_remote) + die(_("extra command line parameter '%s'"), *argv); + else + printf("!ERROR! extra command line parameter '%s'\n", + *argv); + } for (i = 0; i < nr_archivers; i++) if (!is_remote || archivers[i]->flags & ARCHIVER_REMOTE) printf("%s\n", archivers[i]->name); diff --git c/t/t5000-tar-tree.sh w/t/t5000-tar-tree.sh index 918a2fc7c6..04592f45b0 100755 --- c/t/t5000-tar-tree.sh +++ w/t/t5000-tar-tree.sh @@ -124,6 +124,20 @@ test_expect_success 'setup' ' EOF ' +test_expect_success '--list notices extra parameters' ' + test_must_fail git archive --list blah && + # NEEDSWORK: remote error does not result in non-zero + # exit, which we might want to change later. + git archive --remote=. --list blah >remote-out && + grep "!ERROR! " remote-out +' + +test_expect_success 'end-of-options is correctly eaten' ' + git archive --list --end-of-options && + git archive --remote=. --list --end-of-options >remote-out && + ! grep "!ERROR! " remote-out +' + test_expect_success 'populate workdir' ' mkdir a && echo simple textfile >a/a &&
"git archive --list blah" should notice an extra command line parameter that goes unused. Make it so. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This was done to convince myself that even though cmd_archive() calls parse_options with PARSE_OPT_KEEP_UNKNOWN_OPT and then uses the resulting argc/argv without apparent filtering of the "--end-of-options" thing, it is correctly handling it, either locally or remotely. - locally, write_archive() uses parse_archive_args(), which calls parse_options() without KEEP_UNKNOWN_OPT and "--end-of-options" is handled there just fine. - remotely, run_remote_archiver() relays the local parameters, including "--end-of-options" via the "argument" packet. Then the arguments are assembled into a strvec and used by the upload-archive running on the other side to spawn an upload-archive--writer process with. cmd_upload_archive_writer() then makes the same write_archive() call; "--end-of-options" would still be in argv[] if the original "git archive --remote" invocation had one, but it is consumed the same way as the local case in write_archive() by calling parse_archive_args(). I do not like the remote error behaviour this one adds at all. Do we use a more proper mechanism to propagate a remote error back for other subcommands we can reuse here? archive.c | 7 +++++++ t/t5000-tar-tree.sh | 14 ++++++++++++++ 2 files changed, 21 insertions(+)