Basic git show command #48
Conversation
git/show.go
Outdated
| ) | ||
|
|
||
| type ShowOptions struct { | ||
| Pretty string |
There was a problem hiding this comment.
The man page implies this should have a DiffCommonOptions composed in. I also think "Format" is more descriptive than the Pretty alias.
(Maybe we should have a FormatString type to distinguish it from other types of strings too)
git/show.go
Outdated
| } | ||
|
|
||
| // Headers | ||
| fmt.Printf("commit %v\ntree %v\nparent %v\nauthor %s %v +0000\ncommiter %s %v +0000\n\n", cmt, tree, parents[0], author, date.Unix(), author, date.Unix()) |
There was a problem hiding this comment.
I think the parent should use string.Join("\n", ...) rather than silently discarding the other parents. ( In fact, I think this whole bit is unnecessary. You can either just call cat-file -p or call c.GetCommitObject and just print the results using the stringer to get the raw commit.)
There was a problem hiding this comment.
Thanks, I'll have a look at doing that for the raw case.
…output, wrap diff options in show options for the future
cmd/show.go
Outdated
|
|
||
| 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") |
There was a problem hiding this comment.
Is this (multiple flags going to the same place) valid or do they overwrite each other?
There was a problem hiding this comment.
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.
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.
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.
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.
git/show.go
Outdated
| value *string | ||
| } | ||
|
|
||
| func (f *FormatString) FormatCommit(cmt CommitID, c *Client) (string, error) { |
There was a problem hiding this comment.
I think client is first everywhere else in the code
git/show.go
Outdated
| "strings" | ||
| ) | ||
|
|
||
| type FormatString struct { |
There was a problem hiding this comment.
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.
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.
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 type FormatString string to the git package which keeps the remaining FormatCommit method (but it would be simplified and safer by not having to check against nil or dereference the value.)
…gs to the same string pointer detecting duplicates
|
@driusan I think I have found a solution that separates the two concerns fairly nicely. There is a new flags.go file in the cmd package where we can define custom flag value types different dgit specific cases. I have included a string alias value in there that will adjust a string from a pointer and detect if the value is set twice in order to complain. I anticipate that we can add multi-valued string value types in there too and others that we know we will need to address #50 |
|
Thanks, this is more what I had in mind. I suspect we'll need more flag helper types for multi options, but this is definately a cleaner separation of concerns than when it was one type in the git package |
|
Where are we now with t0000-basic.sh? The stat comparison needs to compare ctime, update-index --refresh needs implementation, and long file names need to be stored in the index properly and then are we done? |
The details are in the flags and status.txt. This improve a few of the test cases in the official git test suite.