code review 161890044: flag: roll back 156390043 (flag setting)

flag: roll back 156390043 (flag setting)
Shell scripts depend on the old behavior too often.
It's too late to make this change.

[r, 2014-10-20 20:26:32]

Hello rsc (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-10-20 20:27:39]

LGTM
 On Oct 20, 2014 10:26 PM, <r@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> flag: roll back 156390043 (flag setting)
> Shell scripts depend on the old behavior too often.
> It's too late to make this change.
>
> Please review this at https://codereview.appspot.com/161890044/
>
> Affected files (+0, -89 lines):
>   M src/flag/flag.go
>   M src/flag/flag_test.go
>
>
> Index: src/flag/flag.go
> ===================================================================
> --- a/src/flag/flag.go
> +++ b/src/flag/flag.go
> @@ -235,9 +235,6 @@
>  // If a Value has an IsBoolFlag() bool method returning true,
>  // the command-line parser makes -name equivalent to -name=true
>  // rather than using the next command-line argument.
> -//
> -// It is the implementer's responsibility to verify, if required, that the
> -// flag has been set only once.
>  type Value interface {
>         String() string
>         Set(string) error
> @@ -273,7 +270,6 @@
>         parsed        bool
>         actual        map[string]*Flag
>         formal        map[string]*Flag
> -       set           map[*Flag]bool
>         args          []string // arguments after flags
>         errorHandling ErrorHandling
>         output        io.Writer // nil means stderr; use out() accessor
> @@ -721,25 +717,6 @@
>         }
>  }
>
> -// alreadySet reports whether the flag has already been set during
> parsing.
> -// Because user-defined flags may legally be set multiple times, it only
> -// complains about flags defined in this package.
> -func (f *FlagSet) alreadySet(flag *Flag) bool {
> -       if f.set == nil {
> -               f.set = make(map[*Flag]bool)
> -       }
> -       switch flag.Value.(type) {
> -       case *boolValue, *intValue, *int64Value, *uintValue, *uint64Value,
> *stringValue, *float64Value, *durationValue:
> -       default:
> -               return false // Not one of ours.
> -       }
> -       if f.set[flag] {
> -               return true
> -       }
> -       f.set[flag] = true
> -       return false
> -}
> -
>  // parseOne parses one flag. It reports whether a flag was seen.
>  func (f *FlagSet) parseOne() (bool, error) {
>         if len(f.args) == 0 {
> @@ -784,11 +761,6 @@
>                 return false, f.failf("flag provided but not defined:
> -%s", name)
>         }
>
> -       // has it already been set?
> -       if f.alreadySet(flag) {
> -               return false, f.failf("flag set multiple times: -%s", name)
> -       }
> -
>         if fv, ok := flag.Value.(boolFlag); ok && fv.IsBoolFlag() { //
> special case: doesn't need an arg
>                 if has_value {
>                         if err := fv.Set(value); err != nil {
> @@ -824,7 +796,6 @@
>  // The return value will be ErrHelp if -help or -h were set but not
> defined.
>  func (f *FlagSet) Parse(arguments []string) error {
>         f.parsed = true
> -       f.set = nil
>         f.args = arguments
>         for {
>                 seen, err := f.parseOne()
> Index: src/flag/flag_test.go
> ===================================================================
> --- a/src/flag/flag_test.go
> +++ b/src/flag/flag_test.go
> @@ -377,63 +377,3 @@
>                 t.Fatal("help was called; should not have been for defined
> help flag")
>         }
>  }
> -
> -// Test that a standard flag can be set only once. Need to verify every
> flag type.
> -// User-defined flags can be set multiple times, and this is verified in
> the tests above.
> -func TestErrorOnMultipleSettings(t *testing.T) {
> -       check := func(typ string, err error) {
> -               if err == nil {
> -                       t.Errorf("%s flag can be set multiple times")
> -               } else if !strings.Contains(err.Error(), "flag set
> multiple") {
> -                       t.Fatalf("expected multiple setting error, got
> %q", err)
> -               }
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Bool("v", false, "usage")
> -               check("bool", flags.Parse([]string{"-v", "-v"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Int("v", 0, "usage")
> -               check("int", flags.Parse([]string{"-v", "1", "-v", "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Int64("v", 0, "usage")
> -               check("int64", flags.Parse([]string{"-v", "1", "-v", "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Uint("v", 0, "usage")
> -               check("uint", flags.Parse([]string{"-v", "1", "-v", "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Uint64("v", 0, "usage")
> -               check("uint64", flags.Parse([]string{"-v", "1", "-v",
> "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.String("v", "", "usage")
> -               check("string", flags.Parse([]string{"-v", "1", "-v",
> "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Float64("v", 0, "usage")
> -               check("float64", flags.Parse([]string{"-v", "1", "-v",
> "2"}))
> -       }
> -       {
> -               var flags FlagSet
> -               flags.Init("test", ContinueOnError)
> -               flags.Duration("v", 0, "usage")
> -               check("duration", flags.Parse([]string{"-v", "1s", "-v",
> "2s"}))
> -       }
> -}
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-10-20 20:28:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=8b92bde49be1 ***

flag: roll back 156390043 (flag setting)
Shell scripts depend on the old behavior too often.
It's too late to make this change.

LGTM=bradfitz
R=rsc, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/161890044

[gobot, 2014-10-20 20:33:21]

This CL appears to have broken the plan9-amd64-aram builder.
See http://build.golang.org/log/f656085c8ba8e5c22da9c856e3b9bf63a7435079

[minux, 2014-10-21 00:05:45]

don't forget to rollback CL 161820043.

[r, 2014-10-21 00:20:10]

No need. That file gets deleted anyway, and I'm about to do that deletion.


On Mon, Oct 20, 2014 at 5:05 PM, minux <minux@golang.org> wrote:

> don't forget to rollback CL 161820043.
>