protocol/packp: capabilities new Capability entity and List struct, test improvements #144
Conversation
| @@ -98,7 +98,7 @@ func (c *MockFetchPackSession) AdvertisedReferences() ( | |||
|
|
|||
| h := fixtures.ByURL(c.endpoint.String()).One().Head | |||
|
|
|||
| cap := packp.NewCapabilities() | |||
| cap := capability.NewList() | |||
| cap.Decode("6ecf0ef2c2dffb796033e5a02219af86ec6584e5 HEADmulti_ack thin-pack side-band side-band-64k ofs-delta shallow no-progress include-tag multi_ack_detailed no-done symref=HEAD:refs/heads/master agent=git/2:2.4.8~dbussink-fix-enterprise-tokens-compilation-1167-gc7006cf") | |||
There was a problem hiding this comment.
Decode method changes his input param type from string to byte[].
|
|
||
| // Get returns the values for a capability | ||
| func (l *List) Get(capability Capability) []string { | ||
| return l.m[capability].Values |
There was a problem hiding this comment.
This panics when capability is not found in the map.
| return nil | ||
| } | ||
|
|
||
| func (l *List) validate(c Capability, values []string) error { |
There was a problem hiding this comment.
I think this method is missing the point: why is this a method of the List type?
IMHO it should be a method of the entry type or an external function. And it must be public.
There must be a public List.Validate method thought, but it should not do what this method is doing, instead it should check things like having incompatible capabilities in the list, like sideband and sideband64, for example.
There was a problem hiding this comment.
This is a basic validation preventing values where should not be. The other validation is requested dependant and should be on the request, not here.
| // List represents a list of capabilities | ||
| type List struct { | ||
| m map[Capability]*entry | ||
| o []string |
There was a problem hiding this comment.
What does o means?
What is it purpose? I cannot see how is this useful. If it is here only for easier sorting in the String method, it should be removed.
There was a problem hiding this comment.
Yes is a performance improvement, if not we need to go through all the keys. Yep maybe is useless
| return nil | ||
| } | ||
|
|
||
| if !multipleArgument[c] && len(l.m[c].Values) > 0 { |
There was a problem hiding this comment.
this is already checked at line 88.
There was a problem hiding this comment.
is not the same, this check, if has previous values stored, the other function validate the values you are trying to add
| o []string | ||
| } | ||
|
|
||
| type entry struct { |
There was a problem hiding this comment.
This must be public, as it is what the clients would like to use to add new capabilities, instead of a Capability plus some values.
I would rename entry to Capability and rename Capability to Type.
There was a problem hiding this comment.
I am not agree, is what I removed. The values are vary rarely use only two. So this proposal is worse for the majority of the cases
| } | ||
|
|
||
| for _, value := range cap.Values { | ||
| o += fmt.Sprintf("%s=%s ", key, value) |
There was a problem hiding this comment.
strings should be quoted here to prevent name collisions. Also on the key.
| } | ||
| } | ||
|
|
||
| return strings.Trim(o, " ") |
There was a problem hiding this comment.
use a proper prefix separator, initialized to the empty string on the first iterarion, instead.
There was a problem hiding this comment.
I will use a slice and implode it
| @@ -0,0 +1,158 @@ | |||
| package capability | |||
There was a problem hiding this comment.
The List type will need some more methods in the future:
- Complementary set: to check the validity of client requests: requested caps not present in the announcement
- Announced-requested validity: to check client/server compatibility. Example: PushCert without AllowReachableSHA1InWant
There was a problem hiding this comment.
This is not in the scope of this object this should be checked in the request since are request dependant
Current coverage is 75.56% (diff: 90.66%)@@ master #144 diff @@
==========================================
Files 78 78
Lines 5091 5088 -3
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3838 3845 +7
+ Misses 804 794 -10
Partials 449 449
|
…est improvements (#144) * protocol/pakp: capabilities new Capability entity and List struct, test improvements * etc: example cloud-config file * removing sorting from List.String
Typo fussiness
Capabilitiesstruct moved to a package inside packp, and renamed toListCapabilityand all the consts with the valid capabilities