-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic git show command #48
Changes from 11 commits
c0bcdec
a1f3c85
81b924f
b1008ff
c0b08d9
6c5f8db
ee1a978
4dd570d
6647264
7695d1d
16f93bf
13db2c3
b8f115e
54b86bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package cmd | ||
|
||
import ( | ||
"flag" | ||
"fmt" | ||
|
||
"github.com/driusan/dgit/git" | ||
) | ||
|
||
func Show(c *git.Client, args []string) error { | ||
flags := flag.NewFlagSet("show", flag.ExitOnError) | ||
flags.SetOutput(flag.CommandLine.Output()) | ||
flags.Usage = func() { | ||
flag.Usage() | ||
fmt.Fprintf(flag.CommandLine.Output(), "\n\nOptions:\n") | ||
flags.PrintDefaults() | ||
} | ||
|
||
opts := git.ShowOptions{} | ||
flags.Var(&opts.Format, "format", "Print the contents of commit logs in a specified format") | ||
flags.Var(&opts.Format, "pretty", "Alias for --format") | ||
flags.Parse(args) | ||
|
||
objects := flags.Args() | ||
return git.Show(c, opts, objects) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
package git | ||
|
||
import ( | ||
"fmt" | ||
"strings" | ||
) | ||
|
||
type FormatString struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, now that I understand what this is doing it explains some mysteries like why the pointer and how we could use flag for multiple options but this seems to be leaking implementation details of the flag package into library code. Can we make this just a type of string in the git package, but have a type in the cmd package to handle the flag interfaces? I think that would be cleaner. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll give this some thought again tomorrow. I see where you're coming from. Technically, there is no leaking of the flag package here into the git package. It just happens to conform to the interface defined there. Some of those functions may have limited usefulness outside of the context of flags. The choice of the pointer in the struct does seem to make it clear that there is an explicit unset state of nil, which I think is nice in some ways. Maybe there's a cleaner way to separate the two concerns. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using pointers as Maybe types often leads to nil pointer dereferences down the line when someone forgets to check it, and always results in having two incompatible zero values (like on line 13). I don't think this would be hard to fix. You just need to move your type def, Get, Set, and String methods to the cmd package as-is, then add a |
||
value *string | ||
} | ||
|
||
func (f *FormatString) FormatCommit(cmt CommitID, c *Client) (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think client is first everywhere else in the code |
||
if f == nil || f.value == nil || *f.value == "medium" { | ||
output, err := formatCommitMedium(cmt, c) | ||
if err != nil { | ||
return "", err | ||
} | ||
return fmt.Sprintf("%v\n", output), nil | ||
} | ||
if *f.value == "raw" { | ||
output := fmt.Sprintf("commit %v\n", cmt) | ||
cmtObject, err := c.GetCommitObject(cmt) | ||
if err != nil { | ||
return "", err | ||
} | ||
return fmt.Sprintf("%v%v\n", output, cmtObject), nil | ||
} | ||
|
||
return "", fmt.Errorf("Format %s is not supported.\n", *f.value) | ||
} | ||
|
||
func (f *FormatString) String() string { | ||
if f == nil || f.value == nil { | ||
return "" | ||
} | ||
|
||
return *f.value | ||
} | ||
|
||
func (f *FormatString) Set(s string) error { | ||
if f.value != nil { | ||
return fmt.Errorf("Format already set to %s\n", *f.value) | ||
} | ||
|
||
if s != "raw" && s != "medium" { | ||
return fmt.Errorf("Unsupported format: %s\n", s) | ||
} | ||
|
||
f.value = &s | ||
return nil | ||
} | ||
|
||
func (f *FormatString) Get() interface{} { | ||
return f | ||
} | ||
|
||
type ShowOptions struct { | ||
DiffOptions | ||
Format FormatString | ||
} | ||
|
||
// Show implementes the "git show" command. | ||
func Show(c *Client, opts ShowOptions, objects []string) error { | ||
if len(objects) < 1 { | ||
return fmt.Errorf("Provide at least one commit.") | ||
} | ||
|
||
commitIds := []CommitID{} | ||
|
||
for _, object := range objects { | ||
// Commits only for now | ||
commit, err := RevParseCommit(c, &RevParseOptions{}, object) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
commitIds = append(commitIds, commit) | ||
} | ||
|
||
for _, commit := range commitIds { | ||
output, err := opts.Format.FormatCommit(commit, c) | ||
if err != nil { | ||
return err | ||
} | ||
fmt.Printf("%v", output) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func formatCommitMedium(cmt CommitID, c *Client) (string, error) { | ||
author, err := cmt.GetAuthor(c) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
date, err := cmt.GetDate(c) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
msg, err := cmt.GetCommitMessage(c) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
// Headers | ||
output := fmt.Sprintf("commit %v\nAuthor: %s\nDate: %v\n\n", cmt, author, date.Format("Mon Jan 2 15:04:05 2006 -0700")) | ||
|
||
// Commit message body | ||
lines := strings.Split(strings.TrimSpace(msg.String()), "\n") | ||
for _, l := range lines { | ||
output = fmt.Sprintf("%v %v\n", output, l) | ||
} | ||
|
||
return output, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this (multiple flags going to the same place) valid or do they overwrite each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They go to the same place. If there is a collision then an error is produced. See git/show.go FormatString type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the error propagate back to us? Do we need to ensure we check the return of flags.parse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that it is handled the same way as if you were to using flags.IntVar() and the user provides a non-integer value for the flag. flags.Parse() will detect it, print the defaults out and os.Exit(2) according to the documentation. We do get an opportunity to provide an error message so that we can tell the user what was wrong with their value. In this case it complains that the format was already set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to check the return of flags.Parse() because we have chosen a flags policy that does an os.Exit(2) before flags.Parse() returns.