threads / patch / 19188

patchFix sloppy Getopt::Long.

Subject: [PATCH] Fix sloppy Getopt::Long.

## tl;dr

6 messages between May 5, 2009 and May 8, 2009. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Robin H. Johnson· May 5, 2009, 18:16 UTC · lore

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 <robbat2@gentoo.org>
===
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(-)
Show changes to git-svn.perl +1 −1
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· May 5, 2009, 19:37 UTC · re: Robin H. Johnson · lore

Re: [PATCH] Fix sloppy Getopt::Long.

"Robin H. Johnson" <robbat2@gentoo.org> 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· May 5, 2009, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix sloppy Getopt::Long.

On Tue, May 05, 2009 at 12:37:34PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> "Robin H. Johnson" <robbat2@gentoo.org> 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.

Show 6 quoted lines
> 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:
Show changes to diff +1 −1
@@ -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· May 6, 2009, 16:13 UTC · lore

Re: [PATCH] Fix sloppy Getopt::Long.

On Tue, May 05, 2009 at 11:49:49PM -0700, Eric Wong wrote:
Show 18 quoted lines
> Junio C Hamano <gitster@pobox.com> wrote:
> > "Robin H. Johnson" <robbat2@gentoo.org> 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 <robbat2@gentoo.org>
> > 
> > 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· May 6, 2009, 17:24 UTC · re: Robin H. Johnson · lore

Re: [PATCH] Fix sloppy Getopt::Long.

"Robin H. Johnson" <robbat2@gentoo.org> writes:
Show 21 quoted lines
> On Tue, May 05, 2009 at 11:49:49PM -0700, Eric Wong wrote:
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > "Robin H. Johnson" <robbat2@gentoo.org> 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 <robbat2@gentoo.org>
>> > 
>> > 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· May 8, 2009, 18:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix sloppy Getopt::Long.

On Wed, May 06, 2009 at 10:24:50AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> >> +			{ '' => \$_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

← back to recent threads