Fix bug in Config LookupString #258

2015-09-26T18:00:06-05:00

TheDahv commented

2015-09-26 18:00:06 -05:00

(Migrated from github.com)

A solution to get_string called on a live config object errors when attempting a string lookup on a config object.

A solution to `get_string called on a live config object` errors when attempting a string lookup on a config object.

TheDahv commented

2015-09-26 18:04:51 -05:00

(Migrated from github.com)

@carlosmn, @vmg - Not sure what the best way to contact you for discussion or questions. Am I just using the Config methods wrong? I kept running into errors when doing something along these lines:

path, _ := ConfigFindGlobal()
conf, _ = OpenOndisk(nil, path)
val, err = c.LookupString("foo.bar")
// err should complain about performing a lookup on a live config object

If that's supposed to work, then adding this snapshot method on config objects fixes the problem.

@carlosmn, @vmg - Not sure what the best way to contact you for discussion or questions. Am I just using the Config methods wrong? I kept running into errors when doing something along these lines: ``` go path, _ := ConfigFindGlobal() conf, _ = OpenOndisk(nil, path) val, err = c.LookupString("foo.bar") // err should complain about performing a lookup on a live config object ``` If that's supposed to work, then adding this snapshot method on config objects fixes the problem.

carlosmn commented

2015-09-26 19:53:12 -05:00

(Migrated from github.com)

The snapshotting solves a different problem. We simply should not be using git_config_get_string() at all, but git_config_get_string_buf(). The former is something you can do if you know the lifetimes of your objects, but that's not something you can really know in Go.

The snapshotting solves a different problem. We simply should not be using `git_config_get_string()` at all, but `git_config_get_string_buf()`. The former is something you can do if you know the lifetimes of your objects, but that's not something you can really know in Go.

TheDahv commented

2015-09-27 17:56:41 -05:00

(Migrated from github.com)

@carlosmn thanks for the tip and explanation. I updated my approach to use git_config_get_string_buf(). I also added some tests for the other Lookup methods. Hopefully they might serve as documentation of example usage for future users of this library.

@carlosmn thanks for the tip and explanation. I updated my approach to use `git_config_get_string_buf()`. I also added some tests for the other Lookup methods. Hopefully they might serve as documentation of example usage for future users of this library.

TheDahv commented

2015-10-02 18:41:36 -05:00

(Migrated from github.com)

@carlosmn any further feedback or shall I just wait until more time frees up for you?

carlosmn commented

2015-10-03 05:11:51 -05:00

(Migrated from github.com)

Just to clean up the history here. This PR still contains commits to add and remove snapshotting capabilities. If all you're aiming for is to get string lookups to work, that's the only one it should have.

TheDahv commented

2015-10-08 11:48:44 -05:00

(Migrated from github.com)

Sorry, got caught up for a few days. Commits cleaned up and squashed.

Sign in to join this conversation.

No reviewers