{"thread":{"id":"53045","subject":"[PATCH] test-lib: allow short options to be stacked","startedAt":"2020-03-21T03:07:28Z","lastAt":"2020-03-23T20:18:19Z","messageCount":16,"participants":["Matheus Tavares","Junio C Hamano","Jeff King","Johannes Sixt","Matheus Tavares Bernardino","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393572","messageId":"48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br","threadId":"53045","inReplyTo":null,"subject":"[PATCH] test-lib: allow short options to be stacked","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T03:07:05Z","receivedAt":"2020-03-21T03:07:28Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When debugging a test (or a set of tests), it's common to execute it\nwith some combination of short options, such as:\n\n\t$ ./txxx-testname.sh -d -x -i\n\nIn cases like this, CLIs usually allow the short options to be stacked\nin a single argument, for convenience and agility. Let's add this\nfeature to test-lib, allowing the above command to be run as:\n\n\t$ ./txxx-testname.sh -dxi\n\t(or any other permutation, e.g. '-ixd')\n\nSigned-off-by: Matheus Tavares \n---\n t/README | 3 ++-\n t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++--------------\n 2 files changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 9afd61e3ca..c3cf8f617b 100644\n--- a/t/README\n+++ b/t/README\n@@ -69,7 +69,8 @@ You can also run each test individually from command line, like this:\n \n You can pass --verbose (or -v), --debug (or -d), and --immediate\n (or -i) command line argument to the test, or by setting GIT_TEST_OPTS\n-appropriately before running \"make\".\n+appropriately before running \"make\". Short options can be stacked, i.e.\n+'-d -v' is the same as '-dv'.\n \n -v::\n --verbose::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0ea1e5a05e..14363010d2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -72,119 +72,137 @@ then\n \tif test -n \"$GIT_TEST_INSTALLED\"\n \tthen\n \t\techo >&2 \"error: there is no working Git at '$GIT_TEST_INSTALLED'\"\n \telse\n \t\techo >&2 'error: you do not seem to have built git yet.'\n \tfi\n \texit 1\n fi\n \n-# Parse options while taking care to leave $@ intact, so we will still\n-# have all the original command line options when executing the test\n-# script again for '--tee' and '--verbose-log' below.\n-store_arg_to=\n-prev_opt=\n-for opt\n-do\n-\tif test -n \"$store_arg_to\"\n-\tthen\n-\t\teval $store_arg_to=\\$opt\n-\t\tstore_arg_to=\n-\t\tprev_opt=\n-\t\tcontinue\n-\tfi\n+parse_option () {\n+\tlocal opt=\"$@\"\n \n \tcase \"$opt\" in\n \t-d|--d|--de|--deb|--debu|--debug)\n \t\tdebug=t ;;\n \t-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)\n \t\timmediate=t ;;\n \t-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)\n \t\tGIT_TEST_LONG=t; export GIT_TEST_LONG ;;\n \t-r)\n \t\tstore_arg_to=run_list\n \t\t;;\n \t--run=*)\n \t\trun_list=${opt#--*=} ;;\n \t-h|--h|--he|--hel|--help)\n \t\thelp=t ;;\n \t-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)\n \t\tverbose=t ;;\n \t--verbose-only=*)\n \t\tverbose_only=${opt#--*=}\n \t\t;;\n \t-q|--q|--qu|--qui|--quie|--quiet)\n \t\t# Ignore --quiet under a TAP::Harness. Saying how many tests\n \t\t# passed without the ok/not ok details is always an error.\n \t\ttest -z \"$HARNESS_ACTIVE\" && quiet=t ;;\n \t--with-dashes)\n \t\twith_dashes=t ;;\n \t--no-bin-wrappers)\n \t\tno_bin_wrappers=t ;;\n \t--no-color)\n \t\tcolor= ;;\n \t--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)\n \t\tvalgrind=memcheck\n \t\ttee=t\n \t\t;;\n \t--valgrind=*)\n \t\tvalgrind=${opt#--*=}\n \t\ttee=t\n \t\t;;\n \t--valgrind-only=*)\n \t\tvalgrind_only=${opt#--*=}\n \t\ttee=t\n \t\t;;\n \t--tee)\n \t\ttee=t ;;\n \t--root=*)\n \t\troot=${opt#--*=} ;;\n \t--chain-lint)\n \t\tGIT_TEST_CHAIN_LINT=1 ;;\n \t--no-chain-lint)\n \t\tGIT_TEST_CHAIN_LINT=0 ;;\n \t-x)\n \t\ttrace=t ;;\n \t-V|--verbose-log)\n \t\tverbose_log=t\n \t\ttee=t\n \t\t;;\n \t--write-junit-xml)\n \t\twrite_junit_xml=t\n \t\t;;\n \t--stress)\n \t\tstress=t ;;\n \t--stress=*)\n \t\techo \"error: --stress does not accept an argument: '$opt'\" >&2\n \t\techo \"did you mean --stress-jobs=${opt#*=} or --stress-limit=${opt#*=}?\" >&2\n \t\texit 1\n \t\t;;\n \t--stress-jobs=*)\n \t\tstress=t;\n \t\tstress=${opt#--*=}\n \t\tcase \"$stress\" in\n \t\t*[!0-9]*|0*|\"\")\n \t\t\techo \"error: --stress-jobs= requires the number of jobs to run\" >&2\n \t\t\texit 1\n \t\t\t;;\n \t\t*)\t# Good.\n \t\t\t;;\n \t\tesac\n \t\t;;\n \t--stress-limit=*)\n \t\tstress=t;\n \t\tstress_limit=${opt#--*=}\n \t\tcase \"$stress_limit\" in\n \t\t*[!0-9]*|0*|\"\")\n \t\t\techo \"error: --stress-limit= requires the number of repetitions\" >&2\n \t\t\texit 1\n \t\t\t;;\n \t\t*)\t# Good.\n \t\t\t;;\n \t\tesac\n \t\t;;\n \t*)\n \t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n \tesac\n+}\n+\n+# Parse options while taking care to leave $@ intact, so we will still\n+# have all the original command line options when executing the test\n+# script again for '--tee' and '--verbose-log' below.\n+store_arg_to=\n+prev_opt=\n+for opt\n+do\n+\tif test -n \"$store_arg_to\"\n+\tthen\n+\t\teval $store_arg_to=\\$opt\n+\t\tstore_arg_to=\n+\t\tprev_opt=\n+\t\tcontinue\n+\tfi\n+\n+\tcase \"$opt\" in\n+\t--*)\n+\t\tparse_option \"$opt\" ;;\n+\t-?*)\n+\t\t# stacked short options must be fed separately to parse_option\n+\t\tfor c in $(echo \"${opt#-}\" | sed 's/./& /g')\n+\t\tdo\n+\t\t\tparse_option \"-$c\"\n+\t\tdone\n+\t\t;;\n+\t*)\n+\t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n+\tesac\n \n \tprev_opt=$opt\n done\n-- \n2.25.1\n\n"},{"id":"393575","messageId":"xmqqsgi29um6.fsf@gitster.c.googlers.com","threadId":"53045","inReplyTo":"48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-21T04:53:21Z","receivedAt":"2020-03-21T04:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> +parse_option () {\n> +\tlocal opt=\"$@\"\n\nI do not think there is any context in which var=\"$@\" makes sense in\nshell script (var=\"$*\" is understandable, though). \n\nDid you mean opt=$1 here?\n\n> +# Parse options while taking care to leave $@ intact, so we will still\n> +# have all the original command line options when executing the test\n> +# script again for '--tee' and '--verbose-log' below.\n\nThe phrase \"below\" no longer makes much sense after moving lines\naround, does it?\n\n> +store_arg_to=\n> +prev_opt=\n> +for opt\n> +do\n> +\tif test -n \"$store_arg_to\"\n> +\tthen\n> +\t\teval $store_arg_to=\\$opt\n> +\t\tstore_arg_to=\n> +\t\tprev_opt=\n> +\t\tcontinue\n> +\tfi\n> +\n> +\tcase \"$opt\" in\n> +\t--*)\n> +\t\tparse_option \"$opt\" ;;\n> +\t-?*)\n> +\t\t# stacked short options must be fed separately to parse_option\n\nAre you calling concatenated short options, e.g. \"-abc\", as\n\"stacked\"? It sounds like a very unusual phrasing, at least to me.\n\n> +\t\tfor c in $(echo \"${opt#-}\" | sed 's/./& /g')\n> +\t\tdo\n> +\t\t\tparse_option \"-$c\"\n> +\t\tdone\n> +\t\t;;\n> +\t*)\n> +\t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n> +\tesac\n> \n> \tprev_opt=$opt\n> done\n\nI am personally not very enthused (the line counts vs benefit does\nnot feel so great), but as long as it works correctly and\nmaintainable, I won't complain too much.\n"},{"id":"393583","messageId":"20200321062611.GA1441446@coredump.intra.peff.net","threadId":"53045","inReplyTo":"48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-21T06:26:11Z","receivedAt":"2020-03-21T06:26:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2020 at 12:07:05AM -0300, Matheus Tavares wrote:\n\n> When debugging a test (or a set of tests), it's common to execute it\n> with some combination of short options, such as:\n> \n> \t$ ./txxx-testname.sh -d -x -i\n> \n> In cases like this, CLIs usually allow the short options to be stacked\n> in a single argument, for convenience and agility. Let's add this\n> feature to test-lib, allowing the above command to be run as:\n> \n> \t$ ./txxx-testname.sh -dxi\n> \t(or any other permutation, e.g. '-ixd')\n\nYay. I've grown accustomed to the lack of this feature in the test\nscripts, but I'll be happy to have it. :)\n\nMost getopt implementations I've seen call this \"bundling\" rather than\n\"stacking\" (I don't care too much either way, but Junio mentioned being\nconfused at the name).\n\n> +\tcase \"$opt\" in\n> +\t--*)\n> +\t\tparse_option \"$opt\" ;;\n> +\t-?*)\n> +\t\t# stacked short options must be fed separately to parse_option\n> +\t\tfor c in $(echo \"${opt#-}\" | sed 's/./& /g')\n> +\t\tdo\n> +\t\t\tparse_option \"-$c\"\n> +\t\tdone\n\nI wondered if we could do this without the extra process. This works:\n\n opt=${opt#-}\n while test -n \"$opt\"\n do\n\textra=${opt#?}\n\tthis=${opt%$extra}\n\topt=$extra\n\tparse_option \"-$this\"\n done\n\nIt's a little convoluted. I'm not sure if saving a process per unbundled\nshort option is worth it.\n\nWhat happens to bundled short options with arguments? I think \"-r\" is\nthe only one. We don't allow \"stuck\" short options like \"-r5\", so we\ndon't have to worry about feeding non-option bits to parse_option(). It\nlooks like we'd only examine $store_arg_to outside of the short-option\nloop, so we'd treat:\n\n ./t1234-foo.sh -vrix 5\n\nthe same as:\n\n ./t1234-foo.sh -v -r 5 -i -x\n\nwhich seems reasonable. But:\n\n ./t1234-foo.sh -rr 5 6\n\nwould get garbled. We'd come out of \"-rr\" parsing with $store_arg_to\nset, but only grab the first argument. I think I could live with that,\nconsidering this is just the test scripts. Fixing it would require\nstore_arg_to becoming a space-separated list.\n\n-Peff\n"},{"id":"393586","messageId":"2a54d866-c40e-da5e-4c0c-6bfcb56eb8f7@kdbg.org","threadId":"53045","inReplyTo":"48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-03-21T08:55:02Z","receivedAt":"2020-03-21T08:55:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.03.20 um 04:07 schrieb Matheus Tavares:\n> +\n> +# Parse options while taking care to leave $@ intact, so we will still\n> +# have all the original command line options when executing the test\n> +# script again for '--tee' and '--verbose-log' below.\n> +store_arg_to=\n> +prev_opt=\n> +for opt\n> +do\n> +\tif test -n \"$store_arg_to\"\n> +\tthen\n> +\t\teval $store_arg_to=\\$opt\n> +\t\tstore_arg_to=\n> +\t\tprev_opt=\n> +\t\tcontinue\n> +\tfi\n> +\n> +\tcase \"$opt\" in\n> +\t--*)\n\nCan we please have a case arm for -? here:\n\n\t--* | -?)\n\nso that we do not pay the price of many sub-processes when options are\n_not_ stacked?\n\n> +\t\tparse_option \"$opt\" ;;\n> +\t-?*)\n> +\t\t# stacked short options must be fed separately to parse_option\n> +\t\tfor c in $(echo \"${opt#-}\" | sed 's/./& /g')\n> +\t\tdo\n> +\t\t\tparse_option \"-$c\"\n> +\t\tdone\n> +\t\t;;\n> +\t*)\n> +\t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n> +\tesac\n> \n> \tprev_opt=$opt\n> done\n> \n\n-- Hannes\n"},{"id":"393629","messageId":"20200321172752.3268-1-matheus.bernardino@usp.br","threadId":"53045","inReplyTo":"xmqqsgi29um6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T17:27:52Z","receivedAt":"2020-03-21T17:28:02Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Mar 21, 2020 at 1:53 AM Junio C Hamano wrote:\n> Matheus Tavares writes:\n>\n> > +parse_option () {\n> > +\tlocal opt=\"$@\"\n>\n> I do not think there is any context in which var=\"$@\" makes sense in\n> shell script (var=\"$*\" is understandable, though). \n>\n> Did you mean opt=$1 here?\n\nRight, it should be $1. Thanks.\n\n> > +# Parse options while taking care to leave $@ intact, so we will still\n> > +# have all the original command line options when executing the test\n> > +# script again for '--tee' and '--verbose-log' below.\n>\n> The phrase \"below\" no longer makes much sense after moving lines\n> around, does it?\n\nOh, I thought \"below\" referred to the later usage of $@ (when --tee or\n--verbose-log are set). I.e. not the parsing section we moved up, but this\none:\n\nelif test -n \"$tee\"\nthen\n\t...\n\t(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} \"$0\" \"$@\" 2>&1;\n\nMaybe change \"below\" for \"later in the code\"?\n\n> > +store_arg_to=\n> > +prev_opt=\n> > +for opt\n> > +do\n> > +\tif test -n \"$store_arg_to\"\n> > +\tthen\n> > +\t\teval $store_arg_to=\\$opt\n> > +\t\tstore_arg_to=\n> > +\t\tprev_opt=\n> > +\t\tcontinue\n> > +\tfi\n> > +\n> > +\tcase \"$opt\" in\n> > +\t--*)\n> > +\t\tparse_option \"$opt\" ;;\n> > +\t-?*)\n> > +\t\t# stacked short options must be fed separately to parse_option\n>\n> Are you calling concatenated short options, e.g. \"-abc\", as\n> \"stacked\"? It sounds like a very unusual phrasing, at least to me.\n\nYeah, I wasn't really sure about the naming for this. Thanks, \"concatenated\"\n(or \"bundled\", as Peff suggested in another reply) does sound better.\n\n"},{"id":"393652","messageId":"CAHd-oW4cGEwyge+BPRGiOsmMxr5ne9Ufk-BOM4EgG_i6qTxxMg@mail.gmail.com","threadId":"53045","inReplyTo":"20200321062611.GA1441446@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T18:50:55Z","receivedAt":"2020-03-21T18:51:10Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Mar 21, 2020 at 3:26 AM Jeff King wrote:\n>\n> On Sat, Mar 21, 2020 at 12:07:05AM -0300, Matheus Tavares wrote:\n>\n> > In cases like this, CLIs usually allow the short options to be stacked\n> > in a single argument, for convenience and agility. Let's add this\n> > feature to test-lib, allowing the above command to be run as:\n>\n> Most getopt implementations I've seen call this \"bundling\" rather than\n> \"stacking\" (I don't care too much either way, but Junio mentioned being\n> confused at the name).\n\nYeah, \"stacking\" wasn't the best word choice. I will replace it by\n\"bundling\" then, thanks.\n\n>\n> > + case \"$opt\" in\n> > + --*)\n> > + parse_option \"$opt\" ;;\n> > + -?*)\n> > + # stacked short options must be fed separately to parse_option\n> > + for c in $(echo \"${opt#-}\" | sed 's/./& /g')\n> > + do\n> > + parse_option \"-$c\"\n> > + done\n>\n> I wondered if we could do this without the extra process. This works:\n>\n> opt=${opt#-}\n> while test -n \"$opt\"\n> do\n> extra=${opt#?}\n> this=${opt%$extra}\n> opt=$extra\n> parse_option \"-$this\"\n> done\n>\n> It's a little convoluted. I'm not sure if saving a process per unbundled\n> short option is worth it.\n\nI quite liked this alternative with builtins. It's a little more\nverbose, but it remains very clear.\n\n> What happens to bundled short options with arguments? I think \"-r\" is\n> the only one. We don't allow \"stuck\" short options like \"-r5\", so we\n> don't have to worry about feeding non-option bits to parse_option(). It\n> looks like we'd only examine $store_arg_to outside of the short-option\n> loop, so we'd treat:\n>\n> ./t1234-foo.sh -vrix 5\n>\n> the same as:\n>\n> ./t1234-foo.sh -v -r 5 -i -x\n\nYes. I just thought, though, that if another \"short option with\narguments\" gets added in the future, the bundle would not work\ncorrectly. I don't think we need to be prepared for such a scenario\nnow, but ...\n\n> which seems reasonable. But:\n>\n> ./t1234-foo.sh -rr 5 6\n>\n> would get garbled.\n\n... we could prohibit using more than one \"short option with\narguments\" in the same bundle. This would not only solve the problem\nfor \"-rr 5 6\"[1] but also for the scenario of future new options. And\nit's quite simple to implement, we just have to check whether\n$store_arg_to is set before setting it to another value. I'll try that\nfor v2.\n\n[1]: Someone that used '-rr 5 6' might have wanted the script to run\n*both* tests 5 and 6. But I don't think we need to support that now,\nsince '-r 5 -r 6' doesn't do that as well (instead, the last value\noverrides all previous ones).\n"},{"id":"393653","messageId":"CAHd-oW73xp2=XcFr1F57fo2UtAyuZ2aeXVSc_A50bYKKVN7fJw@mail.gmail.com","threadId":"53045","inReplyTo":"2a54d866-c40e-da5e-4c0c-6bfcb56eb8f7@kdbg.org","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T18:55:27Z","receivedAt":"2020-03-21T18:55:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Mar 21, 2020 at 5:55 AM Johannes Sixt wrote:\n>\n> Am 21.03.20 um 04:07 schrieb Matheus Tavares:\n> >\n> > +for opt\n> > +do\n> > + if test -n \"$store_arg_to\"\n> > + then\n> > + eval $store_arg_to=\\$opt\n> > + store_arg_to=\n> > + prev_opt=\n> > + continue\n> > + fi\n> > +\n> > + case \"$opt\" in\n> > + --*)\n>\n> Can we please have a case arm for -? here:\n>\n> --* | -?)\n>\n> so that we do not pay the price of many sub-processes when options are\n> _not_ stacked?\n\nMakes sense, thanks. However, using Peff's suggestion[1] for the\ncharacter iteration already eliminates the need for extra processes.\nSo it will cover this case as well.\n\n[1]: https://lore.kernel.org/git/20200321062611.GA1441446@coredump.intra.peff.net/\n"},{"id":"393654","messageId":"xmqqblopa63n.fsf@gitster.c.googlers.com","threadId":"53045","inReplyTo":"20200321062611.GA1441446@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-21T18:57:32Z","receivedAt":"2020-03-21T18:57:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wondered if we could do this without the extra process. This works:\n>\n> opt=${opt#-}\n> while test -n \"$opt\"\n> do\n> \textra=${opt#?}\n> \tthis=${opt%$extra}\n> \topt=$extra\n> \tparse_option \"-$this\"\n> done\n>\n> It's a little convoluted. I'm not sure if saving a process per unbundled\n> short option is worth it.\n\nI was wondering if I should suggest something similar to the above\nwhen I wrote my response. Yours actually does not look as bad as\nwhat mine would have been ;-)\n\n> What happens to bundled short options with arguments? I think \"-r\" is\n> the only one. We don't allow \"stuck\" short options like \"-r5\", so we\n> don't have to worry about feeding non-option bits to parse_option(). It\n> looks like we'd only examine $store_arg_to outside of the short-option\n> loop, so we'd treat:\n>\n> ./t1234-foo.sh -vrix 5\n>\n> the same as:\n>\n> ./t1234-foo.sh -v -r 5 -i -x\n>\n> which seems reasonable. But:\n>\n> ./t1234-foo.sh -rr 5 6\n>\n> would get garbled.\n\nAnd also we declare we will never add any option that takes an\nargument with this patch? ... Ah, no, it is just store_arg_to is\ntaking a short-cut assuming the current lack of bundled options.\n\nOK, so yeah, I am not sure if this half-way \"solution\" is worth it.\nIt is just the test scripts, sure, that we have this strange\nlimitation that \"-rr 5 6\" is not parsed correctly (i.e. \"do not use\nthe bundled form if -r is involved\" is something developers can live\nwith), but then it is just the test scripts so \"do not use the\nbundled form at al\" is not too bad, either. It is just one less\nthing for developers to remember, compared to having to remember\n\"you can, but except for this special case\".\n\nSo, I dunno.\n\nThanks.\n"},{"id":"393658","messageId":"7a6a8197dcd58e8690892d03cb904dd1eec5d7c1.1584818457.git.matheus.bernardino@usp.br","threadId":"53045","inReplyTo":"48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br","subject":"[PATCH v2] test-lib: allow short options to be bundled","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T19:57:31Z","receivedAt":"2020-03-21T19:57:47Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When debugging a test (or a set of tests), it's common to execute it\nwith some combination of short options, such as:\n\n\t$ ./txxx-testname.sh -d -x -i\n\nIn cases like this, CLIs usually allow the short options to be bundled\nin a single argument, for convenience and agility. Let's add this\nfeature to test-lib, allowing the above command to be run as:\n\n\t$ ./txxx-testname.sh -dxi\n\t(or any other permutation, e.g. '-ixd')\n\nHelped-by: Jeff King \nSigned-off-by: Matheus Tavares \n---\n\nChanges since v1:\n- Added a check for bundles containing more than one \"option that\n requires args\" (e.g. '-rr'), in which case we error-out. We could\n interpret '-rr 1 2' as 'run tests 1 _and_ 2', but the unbundled\n format, '-r 1 -r 2', is not currently interpreted like that (the last\n just overrides the previous). So, for simplicity, let's only forbid\n such bundles for now.\n- Used \"$1\" instead of \"$@\" to get the argument in parse_option()\n- Replaced occurrences of \"stacked options\" to \"bundled options\"\n- Eliminated spawning of extra processes in the bundled options parser\n- s/below/later/ in the parser loop comment, to make it clearer\n\n t/README | 3 ++-\n t/test-lib.sh | 62 +++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 50 insertions(+), 15 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 9afd61e3ca..080bc59fc4 100644\n--- a/t/README\n+++ b/t/README\n@@ -69,7 +69,8 @@ You can also run each test individually from command line, like this:\n \n You can pass --verbose (or -v), --debug (or -d), and --immediate\n (or -i) command line argument to the test, or by setting GIT_TEST_OPTS\n-appropriately before running \"make\".\n+appropriately before running \"make\". Short options can be bundled, i.e.\n+'-d -v' is the same as '-dv'.\n \n -v::\n --verbose::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0ea1e5a05e..dda770ec94 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -78,20 +78,24 @@ then\n \texit 1\n fi\n \n-# Parse options while taking care to leave $@ intact, so we will still\n-# have all the original command line options when executing the test\n-# script again for '--tee' and '--verbose-log' below.\n store_arg_to=\n-prev_opt=\n-for opt\n-do\n-\tif test -n \"$store_arg_to\"\n+opt_required_arg=\n+# $1: option string\n+# $2: name of the var where the arg will be stored\n+mark_option_requires_arg ()\n+{\n+\tif test -n \"$opt_required_arg\"\n \tthen\n-\t\teval $store_arg_to=\\$opt\n-\t\tstore_arg_to=\n-\t\tprev_opt=\n-\t\tcontinue\n+\t\techo \"error: options that require args cannot be bundled\" \\\n+\t\t\t\"together: '$opt_required_arg' and '$1'\" >&2\n+\t\texit 1\n \tfi\n+\topt_required_arg=$1\n+\tstore_arg_to=$2\n+}\n+\n+parse_option () {\n+\tlocal opt=\"$1\"\n \n \tcase \"$opt\" in\n \t-d|--d|--de|--deb|--debu|--debug)\n@@ -101,7 +105,7 @@ do\n \t-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)\n \t\tGIT_TEST_LONG=t; export GIT_TEST_LONG ;;\n \t-r)\n-\t\tstore_arg_to=run_list\n+\t\tmark_option_requires_arg \"$opt\" run_list\n \t\t;;\n \t--run=*)\n \t\trun_list=${opt#--*=} ;;\n@@ -185,12 +189,42 @@ do\n \t*)\n \t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n \tesac\n+}\n+\n+# Parse options while taking care to leave $@ intact, so we will still\n+# have all the original command line options when executing the test\n+# script again for '--tee' and '--verbose-log' later.\n+for opt\n+do\n+\tif test -n \"$store_arg_to\"\n+\tthen\n+\t\teval $store_arg_to=\\$opt\n+\t\tstore_arg_to=\n+\t\topt_required_arg=\n+\t\tcontinue\n+\tfi\n \n-\tprev_opt=$opt\n+\tcase \"$opt\" in\n+\t--*)\n+\t\tparse_option \"$opt\" ;;\n+\t-?*)\n+\t\t# bundled short options must be fed separately to parse_option\n+\t\topt=${opt#-}\n+\t\twhile test -n \"$opt\"\n+\t\tdo\n+\t\t\textra=${opt#?}\n+\t\t\tthis=${opt%$extra}\n+\t\t\topt=$extra\n+\t\t\tparse_option \"-$this\"\n+\t\tdone\n+\t\t;;\n+\t*)\n+\t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n+\tesac\n done\n if test -n \"$store_arg_to\"\n then\n-\techo \"error: $prev_opt requires an argument\" >&2\n+\techo \"error: $opt_required_arg requires an argument\" >&2\n \texit 1\n fi\n \n-- \n2.25.1\n\n"},{"id":"393659","messageId":"xmqqtv2h8oac.fsf@gitster.c.googlers.com","threadId":"53045","inReplyTo":"7a6a8197dcd58e8690892d03cb904dd1eec5d7c1.1584818457.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2] test-lib: allow short options to be bundled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-21T20:07:39Z","receivedAt":"2020-03-21T20:07:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> - Added a check for bundles containing more than one \"option that\n> requires args\" (e.g. '-rr'), in which case we error-out. We could\n> interpret '-rr 1 2' as 'run tests 1 _and_ 2', but the unbundled\n> format, '-r 1 -r 2', is not currently interpreted like that (the last\n> just overrides the previous). So, for simplicity, let's only forbid\n> such bundles for now.\n\nMakes sense. I think this is the best we can do at this moment.\n\n> +opt_required_arg=\n> +# $1: option string\n> +# $2: name of the var where the arg will be stored\n> +mark_option_requires_arg ()\n> +{\n\n\"{\" on the same line, just like you did for parse_option below.\n\n> +\tif test -n \"$opt_required_arg\"\n> \tthen\n> +\t\techo \"error: options that require args cannot be bundled\" \\\n> +\t\t\t\"together: '$opt_required_arg' and '$1'\" >&2\n> +\t\texit 1\n> \tfi\n> +\topt_required_arg=$1\n> +\tstore_arg_to=$2\n> +}\n> +\n> +parse_option () {\n> +\tlocal opt=\"$1\"\n> ...\n> +\tcase \"$opt\" in\n> +\t--*)\n> +\t\tparse_option \"$opt\" ;;\n\nI think J6t's suggestion to the previous round still has merit here.\n\n> +\t-?*)\n> +\t\t# bundled short options must be fed separately to parse_option\n> +\t\topt=${opt#-}\n> +\t\twhile test -n \"$opt\"\n> +\t\tdo\n> +\t\t\textra=${opt#?}\n\nTake the rest of the string after stripping the first one in $extra ...\n\n> +\t\t\tthis=${opt%$extra}\n\n... and then strip that tail part from the end, which would give the\nfirst letter in $this.\n\n> +\t\t\topt=$extra\n\nAnd the next round will use the remainder after taking $this out\nof the bundled options from the front.\n\nMakes sense.\n\n> +\t\t\tparse_option \"-$this\"\n> +\t\tdone\n\nThanks\n"},{"id":"393660","messageId":"xmqqpnd58o3z.fsf@gitster.c.googlers.com","threadId":"53045","inReplyTo":"CAHd-oW73xp2=XcFr1F57fo2UtAyuZ2aeXVSc_A50bYKKVN7fJw@mail.gmail.com","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-21T20:11:28Z","receivedAt":"2020-03-21T20:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> Makes sense, thanks. However, using Peff's suggestion[1] for the\n> character iteration already eliminates the need for extra processes.\n\nEven without an extra process, having to strip \"-\", assign an empty\nstring to $extra, and then strip that empty string from the tail to\ncome up with a single letter in $this, all are consuming cycles.\nEven though these wasted cycles are now much smaller, having an arm\nthat specifically catches unbundled case and avoid doing anything\nextra makes the codeflow clear, I would think.\n"},{"id":"393665","messageId":"CAHd-oW6V4cTpf4L-usGYEOMrn_VOY7ajMpBeLzfs_NTyi8pbxg@mail.gmail.com","threadId":"53045","inReplyTo":"xmqqtv2h8oac.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] test-lib: allow short options to be bundled","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-21T22:42:49Z","receivedAt":"2020-03-21T22:43:04Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Mar 21, 2020 at 5:07 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > +opt_required_arg=\n> > +# $1: option string\n> > +# $2: name of the var where the arg will be stored\n> > +mark_option_requires_arg ()\n> > +{\n>\n> \"{\" on the same line, just like you did for parse_option below.\n\nThanks. Will fix.\n\n> > + if test -n \"$opt_required_arg\"\n> > then\n> > + echo \"error: options that require args cannot be bundled\" \\\n> > + \"together: '$opt_required_arg' and '$1'\" >&2\n> > + exit 1\n> > fi\n> > + opt_required_arg=$1\n> > + store_arg_to=$2\n> > +}\n> > +\n> > +parse_option () {\n> > + local opt=\"$1\"\n> > ...\n> > + case \"$opt\" in\n> > + --*)\n> > + parse_option \"$opt\" ;;\n>\n> I think J6t's suggestion to the previous round still has merit here.\n\nYeah, I agree with your last reply that it makes the codeflow clearer.\nI'll wait a bit to see if anyone else has other comments about this\niteration and then send v3 with both changes.\n\nThanks.\n"},{"id":"393691","messageId":"20200322064940.GA599744@coredump.intra.peff.net","threadId":"53045","inReplyTo":"CAHd-oW4cGEwyge+BPRGiOsmMxr5ne9Ufk-BOM4EgG_i6qTxxMg@mail.gmail.com","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-22T06:49:40Z","receivedAt":"2020-03-22T06:49:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2020 at 03:50:55PM -0300, Matheus Tavares Bernardino wrote:\n\n> > which seems reasonable. But:\n> >\n> > ./t1234-foo.sh -rr 5 6\n> >\n> > would get garbled.\n> \n> ... we could prohibit using more than one \"short option with\n> arguments\" in the same bundle. This would not only solve the problem\n> for \"-rr 5 6\"[1] but also for the scenario of future new options. And\n> it's quite simple to implement, we just have to check whether\n> $store_arg_to is set before setting it to another value. I'll try that\n> for v2.\n\nYeah, I'd be perfectly happy with that. This bundling is a new format\nthat did not exist before, so we are not taking away anything you could\npreviously do. As long as we don't produce a wrong or confusing result\n(and instead say \"don't do that; we don't support it\", anybody else is\nfree to come along later and make it work. :)\n\n> [1]: Someone that used '-rr 5 6' might have wanted the script to run\n> *both* tests 5 and 6. But I don't think we need to support that now,\n> since '-r 5 -r 6' doesn't do that as well (instead, the last value\n> overrides all previous ones).\n\nHeh, that's what I assumed \"-r 5 -r 6\" would do, but I guess it goes to\nshow that I do not use that option very much. :)\n\n-Peff\n"},{"id":"393694","messageId":"20200322081417.GA2224@szeder.dev","threadId":"53045","inReplyTo":"CAHd-oW4cGEwyge+BPRGiOsmMxr5ne9Ufk-BOM4EgG_i6qTxxMg@mail.gmail.com","subject":"Re: [PATCH] test-lib: allow short options to be stacked","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-03-22T08:14:17Z","receivedAt":"2020-03-22T08:14:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Mar 21, 2020 at 03:50:55PM -0300, Matheus Tavares Bernardino wrote:\n> [1]: Someone that used '-rr 5 6' might have wanted the script to run\n> *both* tests 5 and 6. But I don't think we need to support that now,\n> since '-r 5 -r 6' doesn't do that as well (instead, the last value\n> overrides all previous ones).\n\nWell, that '-r 5 -r 6' should be written as '-r 5,6', but it shouldn't\nbe terribly difficult to concatenate the args of multiple '-r' options\nwith a comma, I suppose.\n\nBut '-rr 5 6' just looks wrong.\n\n"},{"id":"393720","messageId":"cec4bbcc4270914a729a2c65d9c0efc4a45742ce.1584854619.git.matheus.bernardino@usp.br","threadId":"53045","inReplyTo":"7a6a8197dcd58e8690892d03cb904dd1eec5d7c1.1584818457.git.matheus.bernardino@usp.br","subject":"[PATCH v3] test-lib: allow short options to be bundled","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-22T15:58:57Z","receivedAt":"2020-03-22T15:59:17Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When debugging a test (or a set of tests), it's common to execute it\nwith some combination of short options, such as:\n\n\t$ ./txxx-testname.sh -d -x -i\n\nIn cases like this, CLIs usually allow the short options to be bundled\nin a single argument, for convenience and agility. Let's add this\nfeature to test-lib, allowing the above command to be run as:\n\n\t$ ./txxx-testname.sh -dxi\n\t(or any other permutation, e.g. '-ixd')\n\nNote: Short options that require an argument can also be used in a\nbundle, in any position. So, for example, '-r 5 -x', '-xr 5' and '-rx 5'\nare all valid and equivalent. A special case would be having a bundle\nwith more than one of such options. To keep things simple, this case is\nnot allowed for now. This shouldn't be a major limitation, though, as\nthe only short option that requires an argument today is '-r'. And\nconcatenating '-r's as in '-rr 5 6' would probably not be very\npractical: its unbundled format would be '-r 5 -r 6', for which test-lib\ncurrently considers only the last argument. Therefore, if '-rr 5 6' were\nto be allowed, it would have the same effect as just typing '-r 6'.\n\nNote: the test-lib currently doesn't support '-r5', as an alternative\nfor '-r 5', so the former is not supported in bundles as well.\n\nHelped-by: Jeff King \nSigned-off-by: Matheus Tavares \n---\n\nChanges since v2:\n- Fixed style in mark_option_requires_arg(), moving \"{\" to the same line\n- In the main options parser loop, joined long options and unbundled\n short options in the same case arm, as suggested by J6t. This provides\n a clearer codeflow.\n- Summarized, in the commit message, the approach chosen on bundling\n options that require args. Since this was an important point in the\n last rounds, I thought it would be good to include our decisions (and\n justification) in the commit body.\n\n t/README | 3 ++-\n t/test-lib.sh | 61 +++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 49 insertions(+), 15 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 9afd61e3ca..080bc59fc4 100644\n--- a/t/README\n+++ b/t/README\n@@ -69,7 +69,8 @@ You can also run each test individually from command line, like this:\n \n You can pass --verbose (or -v), --debug (or -d), and --immediate\n (or -i) command line argument to the test, or by setting GIT_TEST_OPTS\n-appropriately before running \"make\".\n+appropriately before running \"make\". Short options can be bundled, i.e.\n+'-d -v' is the same as '-dv'.\n \n -v::\n --verbose::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0ea1e5a05e..a6178e9fac 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -78,20 +78,23 @@ then\n \texit 1\n fi\n \n-# Parse options while taking care to leave $@ intact, so we will still\n-# have all the original command line options when executing the test\n-# script again for '--tee' and '--verbose-log' below.\n store_arg_to=\n-prev_opt=\n-for opt\n-do\n-\tif test -n \"$store_arg_to\"\n+opt_required_arg=\n+# $1: option string\n+# $2: name of the var where the arg will be stored\n+mark_option_requires_arg () {\n+\tif test -n \"$opt_required_arg\"\n \tthen\n-\t\teval $store_arg_to=\\$opt\n-\t\tstore_arg_to=\n-\t\tprev_opt=\n-\t\tcontinue\n+\t\techo \"error: options that require args cannot be bundled\" \\\n+\t\t\t\"together: '$opt_required_arg' and '$1'\" >&2\n+\t\texit 1\n \tfi\n+\topt_required_arg=$1\n+\tstore_arg_to=$2\n+}\n+\n+parse_option () {\n+\tlocal opt=\"$1\"\n \n \tcase \"$opt\" in\n \t-d|--d|--de|--deb|--debu|--debug)\n@@ -101,7 +104,7 @@ do\n \t-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)\n \t\tGIT_TEST_LONG=t; export GIT_TEST_LONG ;;\n \t-r)\n-\t\tstore_arg_to=run_list\n+\t\tmark_option_requires_arg \"$opt\" run_list\n \t\t;;\n \t--run=*)\n \t\trun_list=${opt#--*=} ;;\n@@ -185,12 +188,42 @@ do\n \t*)\n \t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n \tesac\n+}\n+\n+# Parse options while taking care to leave $@ intact, so we will still\n+# have all the original command line options when executing the test\n+# script again for '--tee' and '--verbose-log' later.\n+for opt\n+do\n+\tif test -n \"$store_arg_to\"\n+\tthen\n+\t\teval $store_arg_to=\\$opt\n+\t\tstore_arg_to=\n+\t\topt_required_arg=\n+\t\tcontinue\n+\tfi\n \n-\tprev_opt=$opt\n+\tcase \"$opt\" in\n+\t--*|-?)\n+\t\tparse_option \"$opt\" ;;\n+\t-?*)\n+\t\t# bundled short options must be fed separately to parse_option\n+\t\topt=${opt#-}\n+\t\twhile test -n \"$opt\"\n+\t\tdo\n+\t\t\textra=${opt#?}\n+\t\t\tthis=${opt%$extra}\n+\t\t\topt=$extra\n+\t\t\tparse_option \"-$this\"\n+\t\tdone\n+\t\t;;\n+\t*)\n+\t\techo \"error: unknown test option '$opt'\" >&2; exit 1 ;;\n+\tesac\n done\n if test -n \"$store_arg_to\"\n then\n-\techo \"error: $prev_opt requires an argument\" >&2\n+\techo \"error: $opt_required_arg requires an argument\" >&2\n \texit 1\n fi\n \n-- \n2.25.1\n\n"},{"id":"393812","messageId":"xmqqv9mu4ygs.fsf@gitster.c.googlers.com","threadId":"53045","inReplyTo":"cec4bbcc4270914a729a2c65d9c0efc4a45742ce.1584854619.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v3] test-lib: allow short options to be bundled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-23T20:18:11Z","receivedAt":"2020-03-23T20:18:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> Helped-by: Jeff King \n> Signed-off-by: Matheus Tavares \n> ---\n>\n> Changes since v2:\n\nQueued. Thanks.\n"}]}