-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
runcom
commented
Jul 7, 2016
•
edited
Loading
edited
- First commit deals with an O(N) routine and removes it (the last layer ID index takes N-1 cycle to be found)
- Second commit remove a reader close call because the reader is used after the function returns
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@@ -512,7 +504,6 @@ func (rb *RepositoryBackend) getLayerV2(layerID string, dockerURL *types.ParsedD | |||
if err != nil { | |||
return nil, nil, err | |||
} | |||
defer res.Body.Close() |
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 just noticed that the original res doesn't get closed if this if block gets dropped into. Would you mind adding a res.Body.Close()
around like 507 while you're here?
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.
there's a problem however, response body closing is an issue in all of this routine in case of any errors during the function, I'll add a defer
LGTM |
@dgonyeo reworked the second commit so that if there's an error and the |
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
defer func() { | ||
if err != 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.
Just "and" those to save indentation?
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.
changed to and
One nit but LGTM afterwards. |
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Thanks! |