Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

formats/config: Added encoder/decoder for git config files. #97

Merged
merged 14 commits into from
Oct 26, 2016
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions formats/config/common.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// Package config implements decoding, encoding and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package doc description usually is filled in a doc.go and we copy the reference there: https://github.com/src-d/go-git/blob/master/formats/idxfile/doc.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// manipulation git config files.
//
// Reference: https://git-scm.com/docs/git-config
package config

import "strings"

// New creates a new config instance.
func New() *Config {
return &Config{}
}

type Config struct {
Comment *Comment
Sections []*Section
Includes []*Include
}

type Section struct {
Name string
Options []*Option
Subsections []*Subsection
}

type Subsection struct {
Name string
Options []*Option
}

type Option struct {
// Key preserving original caseness.
// Use IsKey instead to compare key regardless of caseness.
Key string
// Original value as string, could be not notmalized.
Value string
}

// A reference to a included configuration.
type Include struct {
Path string
Config *Config
}

type Comment string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, did you forgot to add some methods to this type? otherwise, why can we use the string type instead of Comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It's still missing comment handling code such as returning comment without comment symbols (;. #).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 👍


// IsKey returns true if the given key matches
// this options' key in a case-insensitive comparison.
func (o *Option) IsKey(key string) bool {
return strings.ToLower(o.Key) == strings.ToLower(key)
}

func (c *Config) Section(name string) *Section {
for i := len(c.Sections) - 1; i >= 0; i-- {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not you prefer range ? Is there any special meaning iterating in reverse order ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it has to do with disambiguating sections with the same name to the one that was appended last?, is name supposed to be an unique id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have multiple options with the same key, the standard git config behaviour for Get is that the last value wins. So we iterate in reverse order and return the first value.

For Sections/Subsections we do the same. Although this will probably have to be changed in the future if we want to handle merging of repeated sections. At the moment, we do not require such functionality, but we'll have to handle this when we want to work with git config files in the wild (at the moment, we just want to get/set/delete remotes for our storage backend).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

s := c.Sections[i]
if s.Name == name {
return s
}
}
s := &Section{Name: name}
c.Sections = append(c.Sections, s)
return s
}

func (s *Section) Subsection(name string) *Subsection {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment the public methods

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

for i := len(s.Subsections) - 1; i >= 0; i-- {
ss := s.Subsections[i]
if ss.Name == name {
return ss
}
}
ss := &Subsection{Name: name}
s.Subsections = append(s.Subsections, ss)
return ss
}

func (s *Section) Option(key string) string {
return getOption(s.Options, key)
}

func (s *Subsection) Option(key string) string {
return getOption(s.Options, key)
}

func getOption(opts []*Option, key string) string {
for i := len(opts) - 1; i >= 0; i-- {
o := opts[i]
if o.IsKey(key) {
return o.Value
}
}
return ""
}

func (s *Config) AddOption(section string, subsection string, key string, value string) *Config {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method returns its receiver to be chainable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if subsection == "" {
s.Section(section).AddOption(key, value)
} else {
s.Section(section).Subsection(subsection).AddOption(key, value)
}

return s
}

func (s *Config) SetOption(section string, subsection string, key string, value string) *Config {
if subsection == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to what happen with flush-pkt in the past, that we should pass a constant instead of making the user aware of the specific value. How about a NoSection constant with the value of ""?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

s.Section(section).SetOption(key, value)
} else {
s.Section(section).Subsection(subsection).SetOption(key, value)
}

return s
}

func (s *Section) AddOption(key string, value string) *Section {
s.Options = append(s.Options, &Option{key, value})
return s
}

func (s *Subsection) AddOption(key string, value string) *Subsection {
s.Options = append(s.Options, &Option{key, value})
return s
}

func (s *Section) SetOption(key string, value string) *Section {
for i := len(s.Options) - 1; i >= 0; i-- {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code from SubSction.SetOption, is exactly the same? Maybe we can find another abstraction to avoid having duplicate code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, subsection is a section that has no children.

That's why I think we can remove subsection and put children (subsections) as a reference to itself ([]*Section). As a result it will be defined fully recursive and there will be no repetition.

o := s.Options[i]
if o.IsKey(key) {
o.Value = value
return s
}
}

return s.AddOption(key, value)
}

func (s *Subsection) SetOption(key string, value string) *Subsection {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a lot of duplicated code here.

Would it be possible to avoid it by embedding subsection into section or something along those lines...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved duplicated code to a separate function.

I'd like to avoid embedding at the moment, since this keeps the API easier when using no constructors. Unless we move everything to methods and make the values opaque.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

for i := len(s.Options) - 1; i >= 0; i-- {
o := s.Options[i]
if o.IsKey(key) {
o.Value = value
return s
}
}

return s.AddOption(key, value)
}
103 changes: 103 additions & 0 deletions formats/config/common_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package config_test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not having _test packages, and using the same package

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


import (
"testing"

"gopkg.in/src-d/go-git.v4/formats/config"

. "gopkg.in/check.v1"
)

func Test(t *testing.T) { TestingT(t) }

type CommonSuite struct {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same link of the open brace

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


var _ = Suite(&CommonSuite{})

func (s *CommonSuite) TestConfig_SetOption(c *C) {
obtained := config.New().SetOption("section", "", "key1", "value1")
expected := &config.Config{
Sections: []*config.Section{
{
Name: "section",
Options: []*config.Option{
{Key: "key1", Value: "value1"},
},
},
},
}
c.Assert(obtained, DeepEquals, expected)
obtained = obtained.SetOption("section", "", "key1", "value1")
c.Assert(obtained, DeepEquals, expected)

obtained = config.New().SetOption("section", "subsection", "key1", "value1")
expected = &config.Config{
Sections: []*config.Section{
{
Name: "section",
Subsections: []*config.Subsection{
{
Name: "subsection",
Options: []*config.Option{
{Key: "key1", Value: "value1"},
},
},
},
},
},
}
c.Assert(obtained, DeepEquals, expected)
obtained = obtained.SetOption("section", "subsection", "key1", "value1")
c.Assert(obtained, DeepEquals, expected)
}

func (s *CommonSuite) TestConfig_AddOption(c *C) {
obtained := config.New().AddOption("section", "", "key1", "value1")
expected := &config.Config{
Sections: []*config.Section{
{
Name: "section",
Options: []*config.Option{
{Key: "key1", Value: "value1"},
},
},
},
}
c.Assert(obtained, DeepEquals, expected)
}

func (s *CommonSuite) TestSection_Option(c *C) {
sect := &config.Section{
Options: []*config.Option{
{Key: "key1", Value: "value1"},
{Key: "key2", Value: "value2"},
{Key: "key1", Value: "value3"},
},
}
c.Assert(sect.Option("otherkey"), Equals, "")
c.Assert(sect.Option("key2"), Equals, "value2")
c.Assert(sect.Option("key1"), Equals, "value3")
}

func (s *CommonSuite) TestSubsection_Option(c *C) {
sect := &config.Subsection{
Options: []*config.Option{
{Key: "key1", Value: "value1"},
{Key: "key2", Value: "value2"},
{Key: "key1", Value: "value3"},
},
}
c.Assert(sect.Option("otherkey"), Equals, "")
c.Assert(sect.Option("key2"), Equals, "value2")
c.Assert(sect.Option("key1"), Equals, "value3")
}

func (s *CommonSuite) TestOption_IsKey(c *C) {
c.Assert((&config.Option{Key: "key"}).IsKey("key"), Equals, true)
c.Assert((&config.Option{Key: "key"}).IsKey("KEY"), Equals, true)
c.Assert((&config.Option{Key: "KEY"}).IsKey("key"), Equals, true)
c.Assert((&config.Option{Key: "key"}).IsKey("other"), Equals, false)
c.Assert((&config.Option{Key: "key"}).IsKey(""), Equals, false)
c.Assert((&config.Option{Key: ""}).IsKey("key"), Equals, false)
}
Loading