Added short-hash support to downloads #228
Conversation
Current coverage is 3.03% (diff: 100%)@@ master #228 diff @@
========================================
Files 33 33
Lines 8106 8106
Methods 0 0
Messages 0 0
Branches 0 0
========================================
Hits 246 246
Misses 7840 7840
Partials 20 20
|
tboerger
added
the
lgtm/need 2
|
LGTM - this should go in 1.0.0 as bugfix |
tboerger
added
lgtm/need 1
| @@ -311,7 +311,7 @@ func Download(ctx *context.Context) { | |||
| ctx.Handle(500, "GetTagCommit", err) | |||
| return | |||
| } | |||
| } else if len(refName) == 40 { | |||
| } else if len(refName) > 7 && len(refName) <= 40 { | |||
There was a problem hiding this comment.
btw, should that be >= 7 instead ?
|
Adding an automated testcase would be great too |
| @@ -311,7 +311,7 @@ func Download(ctx *context.Context) { | |||
| ctx.Handle(500, "GetTagCommit", err) | |||
| return | |||
| } | |||
| } else if len(refName) == 40 { | |||
| } else if len(refName) > 7 && len(refName) <= 40 { | |||
There was a problem hiding this comment.
It should be == 7 or == 40, beside that I'm not sure if this is the only place that needs to be changed
There was a problem hiding this comment.
nope, should be 7 <= len(refName) <= 40 just like it is, otherwise f00badf00d.tar.gz wouldn't work 🙂
There was a problem hiding this comment.
So how have it worked before with == 40?
|
Currently only a short hash of 8 characters work. But it's not mandatory for a short hash to be x characters long. See http://stackoverflow.com/a/21015031 |
tboerger
added
the
lgtm/need 1
|
LGTM |
tboerger
added
the
lgtm/done
bkcsoft
removed
the
lgtm/need 1
|
Before it didn't work for anything != 40, follow the reference
to the bug (filed by myself, btw).
The new code will still not support a 7-chars long refName
due to `len(refName) > 7` -- should read `len(refName) >= 7`,
or whatever min lenght you want to support.
Question: what happens if a colliding short hash is used ?
|
tboerger
added
the
lgtm/done
|
That's why I suggested this:
Question: what happens if a colliding short hash is used ? Git will produce an ambiguous error:
|
|
On Thu, Nov 24, 2016 at 12:10:32AM -0800, Bwko wrote:
That's why I suggested this:
> Maybe we could set it from >=4 (minimum short hash is 4 characters) to <= 40?
Question: what happens if a colliding short hash is used ?
Git will produce an ambiguous error:
> error: short SHA1 0001 is ambiguous.
Is Gitea code ready to handle that or would it choke ?
|
|
Github allow usage of 7 length commit hashes. No collision encountered on big repo (eg: Linux kernel have 700k commits). By default, GITEA use 10 length short hash on all pages. Maybe we can change routes to accept at least 10 length hash |
|
I think it'll throw a 404 error. When using 7 characters the change of a collision is 1 in 250 million |
|
On Thu, Nov 24, 2016 at 12:20:43AM -0800, Bwko wrote:
I think it'll throw a 404 error.
Could you please check ?
Accept as low as 2 characters and try fetching a tarball with those.
When using 7 characters the change of a collision is 1 in 250 million
Is there any reason for not accepting a few as 4 chars, from an API
point of view ? This is about *accepting*, not *providing*, so we
should be as liberal as possible, for the robustness principle.
I'm fine with starting as low as 7, in any case (but this PR starts
from 8 as far as I understand the code, so should still be changed)
|
Bwko
force-pushed
the
feature/short-hash-download
branch
from
f05c051 to
69db193
Compare
Bwko
force-pushed
the
feature/short-hash-download
branch
from
69db193 to
ff0d1bd
Compare
|
@strk I've lowered the minimum length to 4. I've just tested this and collisions result in a 404 error |
|
LGTM |
|
LGTM, please merge @go-gitea/owners |
Fixes #211