patterngoMinor
A minimal version control system
Viewed 0 times
minimalversionsystemcontrol
Problem
I just finished writing a simple version control. You register files using "add", and then "com" will save them inside a directory, with an ID attached to it, same ID for all files. With "rev" it will copy back the content of the files, but won't delete any file other than the one in the "commitdir".
```
package main
import (
"encoding/gob"
"errors"
"fmt"
//"github.com/davecgh/go-spew/spew"
//"io"
"io/ioutil"
"log"
"os"
"strconv"
)
type DB struct {
filename string
dirname string
CommitList map[string]bool
ID int
file *os.File
}
func LoadDB(filename, dirname string) (*DB, error) {
er := ger("Loading DB")
file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0666)
if err != nil {
return nil, er("Opening file", err)
}
dec := gob.NewDecoder(file)
db := &DB{}
stats, err := file.Stat()
if err != nil {
return nil, er("Getting file stats", err)
}
if stats.Size() > 0 {
err = dec.Decode(db)
if err != nil {
return nil, er("Decoding non empty db", err)
}
} else {
db = &DB{}
db.CommitList = make(map[string]bool)
}
db.filename = filename
db.dirname = dirname
db.file = file
return db, nil
}
func (db *DB) Write() error {
er := ger("Writing DB")
_, err := db.file.Seek(0, 0)
if err != nil {
return er("File seek", err)
}
enc := gob.NewEncoder(db.file)
err = enc.Encode(db)
if err != nil {
return er("Encoding", err)
}
err = db.file.Close()
if err != nil {
return er("Closing file", err)
}
return nil
}
func (db *DB) Add(name string) error {
if _, ok := db.CommitList[name]; ok {
return fmt.Errorf("%s already in the list", name)
}
db.CommitList[name] = true
return nil
}
func (db *DB) Rem(name string) error {
if _, ok := db.CommitList[name]; !ok {
return fmt.Errorf("%s
```
package main
import (
"encoding/gob"
"errors"
"fmt"
//"github.com/davecgh/go-spew/spew"
//"io"
"io/ioutil"
"log"
"os"
"strconv"
)
type DB struct {
filename string
dirname string
CommitList map[string]bool
ID int
file *os.File
}
func LoadDB(filename, dirname string) (*DB, error) {
er := ger("Loading DB")
file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0666)
if err != nil {
return nil, er("Opening file", err)
}
dec := gob.NewDecoder(file)
db := &DB{}
stats, err := file.Stat()
if err != nil {
return nil, er("Getting file stats", err)
}
if stats.Size() > 0 {
err = dec.Decode(db)
if err != nil {
return nil, er("Decoding non empty db", err)
}
} else {
db = &DB{}
db.CommitList = make(map[string]bool)
}
db.filename = filename
db.dirname = dirname
db.file = file
return db, nil
}
func (db *DB) Write() error {
er := ger("Writing DB")
_, err := db.file.Seek(0, 0)
if err != nil {
return er("File seek", err)
}
enc := gob.NewEncoder(db.file)
err = enc.Encode(db)
if err != nil {
return er("Encoding", err)
}
err = db.file.Close()
if err != nil {
return er("Closing file", err)
}
return nil
}
func (db *DB) Add(name string) error {
if _, ok := db.CommitList[name]; ok {
return fmt.Errorf("%s already in the list", name)
}
db.CommitList[name] = true
return nil
}
func (db *DB) Rem(name string) error {
if _, ok := db.CommitList[name]; !ok {
return fmt.Errorf("%s
Solution
Version control system principle
Your "version control system" principle is a little strange for me.
You don't store which files belong to a revision (
Also since every commit makes a copy of all the files that are under version control, it seems redundant and unneccessary to add the id to each of the file name. It would be easier and cleaner to just create a subfolder named after the revision or ID in the commit folder, and copy the files under that without renaming.
This ID-subfolder implementation gives you more than the db file: it gives you the file list for every single revision, while the db file can be used to provide the "staged", to-be-committed file list. A folder listing of any ID subfolder gives you the file list, the folder name itself gives you the ID, the biggest ID gives you the latest commit. Of course if you would want to improve the application to handle comments, you could use a db or meta file in every ID subfolder.
When you do a revert (
If you would want to go serious about your version control tool, I would recommend to first get to know existing, popular version control systems to get ideas and to know what works and what not.
Now on to your code
It's hard to follow when and if the file stored in your
For example if opening the file succeeds in
To address this, you could just remove the
Your
Whenever I would need to do something using
But of course it looks nicer this way:
Also note that your code does not work properly on Windows systems as you append file paths to the commit dir, and if the file path contains a drive letter (e.g.
Also note that your
Copying files:
In your
Your "version control system" principle is a little strange for me.
You don't store which files belong to a revision (
ID) and so for example if you add a file and you remove it later, the db file will have no knowledge of it. And so if you perform a revert operation, it will not delete previously committed instances of that file.Also since every commit makes a copy of all the files that are under version control, it seems redundant and unneccessary to add the id to each of the file name. It would be easier and cleaner to just create a subfolder named after the revision or ID in the commit folder, and copy the files under that without renaming.
This ID-subfolder implementation gives you more than the db file: it gives you the file list for every single revision, while the db file can be used to provide the "staged", to-be-committed file list. A folder listing of any ID subfolder gives you the file list, the folder name itself gives you the ID, the biggest ID gives you the latest commit. Of course if you would want to improve the application to handle comments, you could use a db or meta file in every ID subfolder.
When you do a revert (
"rev"), your code deletes files from the commit folder that have higher revision. This is a very deviant and weird implementation, it should just reproduce the revision in the local folder and leave the commit folder untouched.If you would want to go serious about your version control tool, I would recommend to first get to know existing, popular version control systems to get ideas and to know what works and what not.
Now on to your code
It's hard to follow when and if the file stored in your
DB struct is closed. The LoadDB function does not close it. If you create a resource which is returned and not closed, you should provide a Close() method on it which makes it clear to the users (which may only be you) that it has resources which should be closed properly.For example if opening the file succeeds in
LoadDB() but reading and decoding its content fails, LoadDB() returns an error and does not close it. And your main() function which called LoadDB() in this calse will do a log.Fatal() call and not close it, because defer db.file.Close() is only called after this. Although this won't cause any problems, it's not nice to sometimes properly close a resource and other times leave it hang and put it in the hands of the OS to release.To address this, you could just remove the
DB.file field (for which I don't see much purpose - both performance wise and otherwise), and open it when/where you need it (and use defer Close()). Or - as mentioned earlier - provide a DB.Close() method and use defer db.Close() in the main() function.Your
DB.Com() method:Whenever I would need to do something using
defer, I would warp in in a separate function, so it is clear when the defer needs to run and what it does. And also so that the deferred function runs as soon as possible and not hold on to resources when they are not needed anymore. In your Com() method you are using defer inside a loop, and also doing it manually at the end of each iteration. Even if you don't want to separate code (to a distinct function), the least I would do is use an anonymous function which would designate the point were defers used in that are called, and so you can also avoid duplication, for example:for name := range something {
func() {
res, err := ... // Open some resource
if err != nil {
// Handle error
}
defer res.Close()
// Do something with res
}()
}But of course it looks nicer this way:
func handleRes(name string) error {
res, err := ... // Open some resource
if err != nil {
return err
}
defer res.Close()
// Do something with res...
return nil
}()
for name := range something {
handleRes(name)
}Also note that your code does not work properly on Windows systems as you append file paths to the commit dir, and if the file path contains a drive letter (e.g.
C:\my\file.txt), the concatenation will result in an invalid path. A (not neccessarily the best) solution to this problem could be to introduce a "drive" level inside the commit dir, so for example C:\my\file.txt would go to "commitdir/c/my/file.txt". This is very easily implemented by first converting filepaths to absolute, and removing the ':' character, and then proceeding to concatenate as before.Also note that your
DB.Rev() method does not create subdirectories when trying to copy back the files, if subfolders are not present, will stop with an error. You should also use os.MkdirAll() before copying back.Copying files:
In your
DB.Com() method you copy files by reading their full content into memory (with ioutil.ReadAll()), and then you write the content from memory to the destination file. This is inefficient and becomes quite uCode Snippets
for name := range something {
func() {
res, err := ... // Open some resource
if err != nil {
// Handle error
}
defer res.Close()
// Do something with res
}()
}func handleRes(name string) error {
res, err := ... // Open some resource
if err != nil {
return err
}
defer res.Close()
// Do something with res...
return nil
}()
for name := range something {
handleRes(name)
}// filea and fileb are opened:
if _, err := io.Copy(fileb, filea); err != nil {
// Failed to copy
}Context
StackExchange Code Review Q#100525, answer score: 6
Revisions (0)
No revisions yet.