# [PATCH] Fix sloppy Getopt::Long. 6 messages from 2009-05-05 to 2009-05-08. Participants: Robin H. Johnson, Junio C Hamano. Thread: https://gitlist.dev/t/19188 ## Robin H. Johnson, 2009-05-05 18:16 Subject: [PATCH] Fix sloppy Getopt::Long. Message-ID: <1241547374-6737-1-git-send-email-robbat2@gentoo.org> URL: https://gitlist.dev/e/1241547374-6737-1-git-send-email-robbat2%40gentoo.org ``` Getopt-Long v2.38 is much stricter about sloppy getopt usage. The trailing pipe causes git-svn testcases to fail for all of the --stdin argument calls. Signed-off-by: Robin H. Johnson === Should be applied to both the stable 1.6.2.x tree and the new 1.6.3 tree. --- git-svn.perl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index c5965c9..ef1d30d 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -147,7 +147,7 @@ my %cmd = ( 'dry-run|n' => \$_dry_run } ], 'set-tree' => [ \&cmd_set_tree, "Set an SVN repository to a git tree-ish", - { 'stdin|' => \$_stdin, %cmt_opts, %fc_opts, } ], + { 'stdin' => \$_stdin, %cmt_opts, %fc_opts, } ], 'create-ignore' => [ \&cmd_create_ignore, 'Create a .gitignore per svn:ignore', { 'revision|r=i' => \$_revision -- 1.6.2.3 ``` ## Junio C Hamano, 2009-05-05 19:37 Subject: Re: [PATCH] Fix sloppy Getopt::Long. Message-ID: <7vfxfj1gu9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfxfj1gu9.fsf%40alter.siamese.dyndns.org In-Reply-To: <1241547374-6737-1-git-send-email-robbat2@gentoo.org> ``` "Robin H. Johnson" writes: > Getopt-Long v2.38 is much stricter about sloppy getopt usage. The > trailing pipe causes git-svn testcases to fail for all of the --stdin > argument calls. I am not objecting at all; just asking for clarification. > - { 'stdin|' => \$_stdin, %cmt_opts, %fc_opts, } ], > + { 'stdin' => \$_stdin, %cmt_opts, %fc_opts, } ], Is this "pipe" supposed to be followed by an alternative spelling of the option, as in 'stdin|standard-input' => \$_stdin, ... and is the sloppyness that it would be crazy to accept either --stdin or just -- (without actual option name) for this option? Could an older version of Getopt::Long() have accepted $ command --foo --bar - other args to set $_stdin to true with that "sloppy" syntax? If so people could have relied on such a behaviour, which is a bit worrying. ``` ## Robin H. Johnson, 2009-05-05 20:21 Subject: Re: [PATCH] Fix sloppy Getopt::Long. Message-ID: URL: https://gitlist.dev/e/robbat2.20090505T200652.976942941Z%40orbis-terrarum.net In-Reply-To: <7vfxfj1gu9.fsf@alter.siamese.dyndns.org> ``` On Tue, May 05, 2009 at 12:37:34PM -0700, Junio C Hamano wrote: > "Robin H. Johnson" writes: > > > Getopt-Long v2.38 is much stricter about sloppy getopt usage. The > > trailing pipe causes git-svn testcases to fail for all of the --stdin > > argument calls. > > I am not objecting at all; just asking for clarification. > > > - { 'stdin|' => \$_stdin, %cmt_opts, %fc_opts, } ], > > + { 'stdin' => \$_stdin, %cmt_opts, %fc_opts, } ], > > Is this "pipe" supposed to be followed by an alternative spelling of the > option, as in > 'stdin|standard-input' => \$_stdin, ... Yes. Short form or alternative long form. > and is the sloppyness that it would be crazy to accept either --stdin or > just -- (without actual option name) for this option? Within the main loop, both '--' and '-' are treated as special cases earlier on before the matching of options is done. '--' is the explicit separator, while '-' is an argument (or a value to an option), not an option in itself. > Could an older version of Getopt::Long() have accepted > > $ command --foo --bar - other args > > to set $_stdin to true with that "sloppy" syntax? If so people could have > relied on such a behaviour, which is a bit worrying. As far as I can follow in the Getopt::Long code, with the old case of 'stdin|', the empty string case would never have matched anyway. The v2.38 change in respect to this is not described in the upstream CHANGES, but boils down to this single modification: Getopt-Long-2.38/lib/Getopt/Long.pm: @@ -777,7 +776,7 @@ # Option name (?: \w+[-\w]* ) # Alias names, or "?" - (?: \| (?: \? | \w[-\w]* )? )* + (?: \| (?: \? | \w[-\w]* ) )* )? -- Robin Hugh Johnson Gentoo Linux Developer & Infra Guy E-Mail : robbat2@gentoo.org GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85 ``` ## Robin H. Johnson, 2009-05-06 16:13 Subject: Re: [PATCH] Fix sloppy Getopt::Long. Message-ID: <20090506161309.GC10702@curie-int> URL: https://gitlist.dev/e/20090506161309.GC10702%40curie-int In-Reply-To: <20090506064949.GB29479@dcvr.yhbt.net> ``` On Tue, May 05, 2009 at 11:49:49PM -0700, Eric Wong wrote: > Junio C Hamano wrote: > > "Robin H. Johnson" writes: > > > > > Getopt-Long v2.38 is much stricter about sloppy getopt usage. The > > > trailing pipe causes git-svn testcases to fail for all of the --stdin > > > argument calls. > > > > > > Signed-off-by: Robin H. Johnson > > > > Eric, I'll take this directly to my tree. Ok? > > The empty "" after the "|" was intended for the set-tree command to take > a lone "-" as a parameter to also mean "--stdin". > > The following should work, too, but I don't have time to test right now: > > + { '' => \$_stdin, 'stdin' => \$_stdin, > + %cmt_opts, %fc_opts, } ], I confirm that it does correctly set the $_stdin variable (tested briefly). Testcase: ===== use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/; my $_stdin; my $rc = GetOptions( 'stdin' => \$_stdin, '' => \$_stdin); printf "rc:%s s:%s\n",$rc,$_stdin; ===== -- Robin Hugh Johnson Gentoo Linux Developer & Infra Guy E-Mail : robbat2@gentoo.org GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85 ``` ## Junio C Hamano, 2009-05-06 17:24 Subject: Re: [PATCH] Fix sloppy Getopt::Long. Message-ID: <7v63gew3dp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63gew3dp.fsf%40alter.siamese.dyndns.org In-Reply-To: <20090506161309.GC10702@curie-int> ``` "Robin H. Johnson" writes: > On Tue, May 05, 2009 at 11:49:49PM -0700, Eric Wong wrote: >> Junio C Hamano wrote: >> > "Robin H. Johnson" writes: >> > >> > > Getopt-Long v2.38 is much stricter about sloppy getopt usage. The >> > > trailing pipe causes git-svn testcases to fail for all of the --stdin >> > > argument calls. >> > > >> > > Signed-off-by: Robin H. Johnson >> > >> > Eric, I'll take this directly to my tree. Ok? >> >> The empty "" after the "|" was intended for the set-tree command to take >> a lone "-" as a parameter to also mean "--stdin". >> >> The following should work, too, but I don't have time to test right now: >> >> + { '' => \$_stdin, 'stdin' => \$_stdin, >> + %cmt_opts, %fc_opts, } ], > I confirm that it does correctly set the $_stdin variable (tested > briefly). Wait a minute. Do you mean we would also need the above "explicit empty argument sets $_stdin"? Wasn't it your earlier analysis/claim that the caller already takes care of a lone "-"? Or do you mean "yes it would also work but it is not necessary"? ``` ## Robin H. Johnson, 2009-05-08 18:28 Subject: Re: [PATCH] Fix sloppy Getopt::Long. Message-ID: URL: https://gitlist.dev/e/robbat2.20090508T182516.673117391Z%40orbis-terrarum.net In-Reply-To: <7v63gew3dp.fsf@alter.siamese.dyndns.org> ``` On Wed, May 06, 2009 at 10:24:50AM -0700, Junio C Hamano wrote: > >> + { '' => \$_stdin, 'stdin' => \$_stdin, > >> + %cmt_opts, %fc_opts, } ], > > I confirm that it does correctly set the $_stdin variable (tested > > briefly). > Wait a minute. Do you mean we would also need the above "explicit empty > argument sets $_stdin"? Wasn't it your earlier analysis/claim that the > caller already takes care of a lone "-"? I'd originally considered a single '-' as an argument not an option, meaning stdin, not being processed within the getopt framework, but instead being handled later. > Or do you mean "yes it would also work but it is not necessary"? Using Eric's change makes it explicit what is expected: Passing '--stdin' is the same as passing '-'. -- Robin Hugh Johnson Gentoo Linux Developer & Infra Guy E-Mail : robbat2@gentoo.org GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85 ```