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

smola · 2016-10-25T16:52:53Z

No description provided.

Portions of code taken from: https://github.com/go-gcfg/gcfg/blob/5b9f94ee80b2331c3982477bd84be8edd857df33/read.go

ferhatelmas · 2016-10-26T07:19:06Z

formats/config/decoder.go

+		return true
+	})
+
+	err = decode(c, config, fset, file, src)


simply return decode(c, config, fset, file, src)

Right. Thanks.

ferhatelmas · 2016-10-26T07:19:58Z

formats/config/common.go

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


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

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?

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

alcortesm · 2016-10-26T10:48:34Z

formats/config/common.go

+	return ""
+}
+
+func (s *Config) AddOption(section string, subsection string, key string, value string) *Config {


Does this method returns its receiver to be chainable?

alcortesm · 2016-10-26T10:53:40Z

formats/config/common.go

+	return s.AddOption(key, value)
+}
+
+func (s *Subsection) SetOption(key string, value string) *Subsection {


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

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.

alcortesm · 2016-10-26T10:57:40Z

formats/config/decoder.go

+			}
+		}
+	}
+	panic("never reached")


This function is 150 lines long.

Can we slipt it without impairing its legibility?

Removed. It was a copy of an internal gcfg function that we needed to modify. This is now in our own gcfg fork: https://github.com/src-d/gcfg

ajnavarro · 2016-10-26T12:24:35Z

formats/config/fixtures_test.go

+		`,
+		Text: `[sect1]
+	opt1 = value1
+[sect1 "subsect1"]


That's the expected format?

Yeah, tabs at the left are preserved when using `, and I need no tabs at the left.

ajnavarro · 2016-10-26T12:24:46Z

formats/config/fixtures_test.go

+		Text: `[sect1]
+	opt1 = value1
+	opt1 = value1b
+[sect1 "subsect1"]


That's the expected format?

alcortesm · 2016-10-26T12:25:01Z

formats/config/common.go

+// section and subsection.
+// If subsection is empty, then it's taken as no subsection.
+func (s *Config) SetOption(section string, subsection string, key string, value string) *Config {
+	if subsection == "" {


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

alcortesm · 2016-10-26T12:31:59Z

formats/config/section.go

+	Options Options
+}
+
+type Sections []*Section


Options, Sections and Subsections are currently slices. Willl it make sense to use maps instead, given that we are somewhat assuming names will be unique ids?

There might be multiple sections with the same name. It's up to us to offer convenience methods in the future that hide this for common use case, but the structure needs to preserve this.

alcortesm · 2016-10-26T12:34:54Z

formats/config/common.go

+	Config *Config
+}
+
+type Comment string


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

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

mcuadros · 2016-10-25T18:49:50Z

formats/config/common.go

@@ -0,0 +1,147 @@
+// Package config implements decoding, encoding and


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

mcuadros · 2016-10-25T18:51:09Z

formats/config/common.go

+	return s
+}
+
+func (s *Section) Subsection(name string) *Subsection {


Can you comment the public methods

mcuadros · 2016-10-25T18:53:22Z

formats/config/common.go

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


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

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.

mcuadros · 2016-10-25T18:53:46Z

formats/config/common_test.go

@@ -0,0 +1,103 @@
+package config_test


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

mcuadros · 2016-10-25T18:54:26Z

formats/config/common_test.go

+func Test(t *testing.T) { TestingT(t) }
+
+type CommonSuite struct {
+}


In the same link of the open brace

* WIP: Add config format parser. * add decoder based on gcfg. Portions of code taken from: https://github.com/go-gcfg/gcfg/blob/5b9f94ee80b2331c3982477bd84be8edd857df33/read.go * add git config encoder and config methods. * use format/config in storage/filesystem for read * use format/config in storage/filesystem to write * formats/config: improve docs. * formats/config: improve tests. * formats/config: use our fork of gcfg; improve api. * formats/config: improve api. * storage/filesystem: fix gofmt * formats/config: use NoSubsection constant. * formats/config: add doc.go * formats/config: requested sytle changes. * formats/config: do not use *_test packages.

smola added 7 commits October 25, 2016 14:33

WIP: Add config format parser.

2517001

add decoder based on gcfg.

1a101f0

Portions of code taken from: https://github.com/go-gcfg/gcfg/blob/5b9f94ee80b2331c3982477bd84be8edd857df33/read.go

add git config encoder and config methods.

e83c095

use format/config in storage/filesystem for read

c953939

use format/config in storage/filesystem to write

0850356

formats/config: improve docs.

a47cc39

formats/config: improve tests.

6620dfc

ferhatelmas reviewed Oct 26, 2016

View reviewed changes

alcortesm reviewed Oct 26, 2016

View reviewed changes

formats/config: use our fork of gcfg; improve api.

8bc9d3e

alcortesm reviewed Oct 26, 2016

View reviewed changes

smola added 2 commits October 26, 2016 14:09

formats/config: improve api.

8e203f0

storage/filesystem: fix gofmt

668198f

ajnavarro reviewed Oct 26, 2016

View reviewed changes

alcortesm reviewed Oct 26, 2016

View reviewed changes

formats/config: use NoSubsection constant.

bce963f

alcortesm approved these changes Oct 26, 2016

View reviewed changes

mcuadros suggested changes Oct 26, 2016

View reviewed changes

smola added 3 commits October 26, 2016 18:20

formats/config: add doc.go

270d40e

formats/config: requested sytle changes.

935a6eb

formats/config: do not use *_test packages.

ca8b401

mcuadros merged commit cb29a2c into src-d:master Oct 26, 2016

smola deleted the ini branch October 28, 2016 09:05

		@@ -0,0 +1,147 @@
		// Package config implements decoding, encoding and

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

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

Uh oh!

Conversation

smola commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!