[PATCH] Fix sloppy Getopt::Long.

[PATCH] Fix sloppy Getopt::Long.

[Robin H. Johnson]

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(-)

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

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

[Junio C Hamano]

"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.

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

[Robin H. Johnson]

On Tue, May 05, 2009 at 12:37:34PM -0700, Junio C Hamano 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.
> 
> 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

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

[Robin H. Johnson]

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).

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

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

[Junio C Hamano]

"Robin H. Johnson" <robbat2@gentoo.org> writes:

> 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"?

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]