Skip to content

Commit 0d9442e

Browse files
a1012112796a1012112796
committed
Apply suggestions from code review. Thanks @jiangxin 
1 parent c1291fa commit 0d9442e

File tree

4 files changed

+109
-104
lines changed

4 files changed

+109
-104
lines changed

‎cmd/hook.go

Lines changed: 91 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -503,52 +503,43 @@ Gitea or set your environment appropriately.`, "")
503503
// H: PKT-LINE(version=1\0push-options...)
504504
// H: flush-pkt
505505

506-
rs, err := readPktLine(reader)
507-
if err != nil {
508-
fail("Internal Server Error", "Pkt-Line format is wrong :%v", err)
509-
}
510-
if rs.Type != pktLineTypeData {
511-
fail("Internal Server Error", "Pkt-Line format is wrong. get %v", rs)
512-
}
506+
rs := readPktLine(reader, pktLineTypeData)
513507

514508
const VersionHead string = "version=1"
515509

516-
if !strings.HasPrefix(rs.Data, VersionHead) {
517-
fail("Internal Server Error", "Pkt-Line format is wrong. get %v", rs)
518-
}
519-
520510
hasPushOptions := false
521511
response := []byte(VersionHead)
522-
if strings.Contains(rs.Data, "push-options") {
523-
response = append(response, byte(0))
524-
response = append(response, []byte("push-options")...)
525-
hasPushOptions = true
526-
}
527-
response = append(response, []byte("\n")...)
512+
var requestOptions []string
528513

529-
rs, err = readPktLine(reader)
530-
if err != nil {
531-
fail("Internal Server Error", "Pkt-Line format is wrong :%v", err)
532-
}
533-
if rs.Type != pktLineTypeFlush {
534-
fail("Internal Server Error", "Pkt-Line format is wrong. get %v", rs)
514+
for i := range rs.Data {
515+
if rs.Data[i] == byte(0) {
516+
if string(rs.Data[0:i]) != VersionHead {
517+
fail("Internal Server Error", "Received an not supported version: %s", string(rs.Data[0:i]))
518+
}
519+
requestOptions = strings.Split(string(rs.Data[i+1:]), " ")
520+
break
521+
}
535522
}
536523

537-
err = writePktLine(os.Stdout, pktLineTypeData, response)
538-
if err != nil {
539-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
524+
for _, option := range requestOptions {
525+
if strings.HasPrefix(option, "push-options") {
526+
response = append(response, byte(0))
527+
response = append(response, []byte("push-options")...)
528+
hasPushOptions = true
529+
}
540530
}
531+
response = append(response, []byte("\n")...)
541532

542-
err = writePktLine(os.Stdout, pktLineTypeFlush, nil)
543-
if err != nil {
544-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
545-
}
533+
rs = readPktLine(reader, pktLineTypeFlush)
534+
535+
writePktLine(os.Stdout, pktLineTypeData, response)
536+
writePktLine(os.Stdout, pktLineTypeFlush, nil)
546537

547538
// 2. receive commands from server.
548539
// S: PKT-LINE(<old-oid> <new-oid> <ref>)
549540
// S: ... ...
550541
// S: flush-pkt
551-
// # receive push-options
542+
// # [receive push-options]
552543
// S: PKT-LINE(push-option)
553544
// S: ... ...
554545
// S: flush-pkt
@@ -561,14 +552,13 @@ Gitea or set your environment appropriately.`, "")
561552
hookOptions.RefFullNames = make([]string, 0, hookBatchSize)
562553

563554
for {
564-
rs, err = readPktLine(reader)
565-
if err != nil {
566-
fail("Internal Server Error", "Pkt-Line format is wrong :%v", err)
567-
}
555+
// note: pktLineTypeUnknow means pktLineTypeFlush and pktLineTypeData all allowed
556+
rs = readPktLine(reader, pktLineTypeUnknow)
557+
568558
if rs.Type == pktLineTypeFlush {
569559
break
570560
}
571-
t := strings.SplitN(rs.Data, " ", 3)
561+
t := strings.SplitN(string(rs.Data), " ", 3)
572562
if len(t) != 3 {
573563
continue
574564
}
@@ -581,29 +571,27 @@ Gitea or set your environment appropriately.`, "")
581571

582572
if hasPushOptions {
583573
for {
584-
rs, err = readPktLine(reader)
585-
if err != nil {
586-
fail("Internal Server Error", "Pkt-Line format is wrong :%v", err)
587-
}
574+
rs = readPktLine(reader, pktLineTypeUnknow)
575+
588576
if rs.Type == pktLineTypeFlush {
589577
break
590578
}
591579

592-
kv := strings.SplitN(rs.Data, "=", 2)
580+
kv := strings.SplitN(string(rs.Data), "=", 2)
593581
if len(kv) == 2 {
594582
hookOptions.GitPushOptions[kv[0]] = kv[1]
595583
}
596584
}
597585
}
598586

599-
// run hook
587+
// 3. run hook
600588
resp, err := private.HookProcReceive(repoUser, repoName, hookOptions)
601589
if err != nil {
602590
fail("Internal Server Error", "run proc-receive hook failed :%v", err)
603591
}
604592

605-
// 3 response result to service.
606-
// # OK, but has an alternate reference. The alternate reference name
593+
// 4. response result to service
594+
// # a. OK, but has an alternate reference. The alternate reference name
607595
// # and other status can be given in option directives.
608596
// H: PKT-LINE(ok <ref>)
609597
// H: PKT-LINE(option refname <refname>)
@@ -612,28 +600,24 @@ Gitea or set your environment appropriately.`, "")
612600
// H: PKT-LINE(option forced-update)
613601
// H: ... ...
614602
// H: flush-pkt
603+
// # b. NO, I reject it.
604+
// H: PKT-LINE(ng <ref> <reason>)
605+
615606
for _, rs := range resp.Results {
616-
err = writePktLine(os.Stdout, pktLineTypeData, []byte("ok "+rs.OrignRef))
617-
if err != nil {
618-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
619-
}
620-
err = writePktLine(os.Stdout, pktLineTypeData, []byte("option refname "+rs.Ref))
621-
if err != nil {
622-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
623-
}
624-
err = writePktLine(os.Stdout, pktLineTypeData, []byte("option old-oid "+rs.OldOID))
625-
if err != nil {
626-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
607+
if len(rs.Err) > 0 {
608+
writePktLine(os.Stdout, pktLineTypeData, []byte("ng "+rs.OrignRef+" "+rs.Err))
609+
continue
627610
}
628-
err = writePktLine(os.Stdout, pktLineTypeData, []byte("option new-oid "+rs.NewOID))
629-
if err != nil {
630-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
611+
612+
writePktLine(os.Stdout, pktLineTypeData, []byte("ok "+rs.OrignRef))
613+
writePktLine(os.Stdout, pktLineTypeData, []byte("option refname "+rs.Ref))
614+
if rs.OldOID != git.EmptySHA {
615+
writePktLine(os.Stdout, pktLineTypeData, []byte("option old-oid "+rs.OldOID))
631616
}
617+
writePktLine(os.Stdout, pktLineTypeData, []byte("option new-oid "+rs.NewOID))
632618
}
633-
err = writePktLine(os.Stdout, pktLineTypeFlush, nil)
634-
if err != nil {
635-
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
636-
}
619+
writePktLine(os.Stdout, pktLineTypeFlush, nil)
620+
637621
return nil
638622
}
639623

@@ -653,75 +637,93 @@ const (
653637
// gitPktLine pkt-line api
654638
type gitPktLine struct {
655639
Type pktLineType
656-
Length int64
657-
Data string
640+
Length uint64
641+
Data []byte
658642
}
659643

660-
func readPktLine(in *bufio.Reader) (r *gitPktLine, err error) {
644+
func readPktLine(in *bufio.Reader, requestType pktLineType) (r *gitPktLine) {
645+
var err error
646+
661647
// read prefix
662648
lengthBytes := make([]byte, 4)
663649
for i := 0; i < 4; i++ {
664650
lengthBytes[i], err = in.ReadByte()
665651
if err != nil {
666-
return nil, err
652+
fail("Internal Server Error", "Pkt-Line: read stdin failed : %v", err)
667653
}
668654
}
655+
669656
r = new(gitPktLine)
670-
r.Length, err = strconv.ParseInt(string(lengthBytes), 16, 64)
657+
r.Length, err = strconv.ParseUint(string(lengthBytes), 16, 32)
671658
if err != nil {
672-
return nil, err
659+
fail("Internal Server Error", "Pkt-Line format is wrong :%v", err)
673660
}
674661

675662
if r.Length == 0 {
663+
if requestType == pktLineTypeData {
664+
fail("Internal Server Error", "Pkt-Line format is wrong")
665+
}
676666
r.Type = pktLineTypeFlush
677-
return r, nil
667+
return r
678668
}
679669

680-
if r.Length <= 4 || r.Length > 65520 {
681-
r.Type = pktLineTypeUnknow
682-
return r, nil
670+
if r.Length <= 4 || r.Length > 65520 || requestType == pktLineTypeFlush {
671+
fail("Internal Server Error", "Pkt-Line format is wrong")
683672
}
684673

685-
tmp := make([]byte, r.Length-4)
686-
for i := range tmp {
687-
tmp[i], err = in.ReadByte()
674+
r.Data = make([]byte, r.Length-4)
675+
for i := range r.Data {
676+
r.Data[i], err = in.ReadByte()
688677
if err != nil {
689-
return nil, err
678+
fail("Internal Server Error", "Pkt-Line: read stdin failed : %v", err)
690679
}
691680
}
692681

693682
r.Type = pktLineTypeData
694-
r.Data = string(tmp)
695683

696-
return r, nil
684+
return r
697685
}
698686

699-
func writePktLine(out io.Writer, typ pktLineType, data []byte) error {
687+
func writePktLine(out io.Writer, typ pktLineType, data []byte) {
700688
if typ == pktLineTypeFlush {
701689
l, err := out.Write([]byte("0000"))
702690
if err != nil {
703-
return err
691+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
704692
}
705693
if l != 4 {
706-
return fmt.Errorf("real write length is different with request, want %v, real %v", 4, l)
694+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
707695
}
708696
}
709697

710698
if typ != pktLineTypeData {
711-
return nil
699+
return
700+
}
701+
702+
hexchar := []byte("0123456789abcdef")
703+
hex := func(n uint64) byte {
704+
return hexchar[(n)&15]
712705
}
713706

714-
l := len(data) + 4
715-
tmp := []byte(fmt.Sprintf("%04x", l))
716-
tmp = append(tmp, data...)
707+
length := uint64(len(data) + 4)
708+
tmp := make([]byte, 4)
709+
tmp[0] = hex(length >> 12)
710+
tmp[1] = hex(length >> 8)
711+
tmp[2] = hex(length >> 4)
712+
tmp[3] = hex(length)
717713

718714
lr, err := out.Write(tmp)
719715
if err != nil {
720-
return err
716+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
721717
}
722-
if l != lr {
723-
return fmt.Errorf("real write length is different with request, want %v, real %v", l, lr)
718+
if 4 != lr {
719+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
724720
}
725721

726-
return nil
722+
lr, err = out.Write(data)
723+
if err != nil {
724+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
725+
}
726+
if int(length-4) != lr {
727+
fail("Internal Server Error", "Pkt-Line response failed: %v", err)
728+
}
727729
}

‎cmd/hook_test.go

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,21 @@ func TestPktLine(t *testing.T) {
1717
// test read
1818
s := strings.NewReader("0000")
1919
r := bufio.NewReader(s)
20-
result, err := readPktLine(r)
21-
assert.NoError(t, err)
20+
result := readPktLine(r, pktLineTypeFlush)
2221
assert.Equal(t, pktLineTypeFlush, result.Type)
2322

2423
s = strings.NewReader("0006a\n")
2524
r = bufio.NewReader(s)
26-
result, err = readPktLine(r)
27-
assert.NoError(t, err)
25+
result = readPktLine(r, pktLineTypeData)
2826
assert.Equal(t, pktLineTypeData, result.Type)
29-
assert.Equal(t, "a\n", result.Data)
27+
assert.Equal(t, []byte("a\n"), result.Data)
3028

3129
// test write
3230
w := bytes.NewBuffer([]byte{})
33-
err = writePktLine(w, pktLineTypeFlush, nil)
34-
assert.NoError(t, err)
31+
writePktLine(w, pktLineTypeFlush, nil)
3532
assert.Equal(t, []byte("0000"), w.Bytes())
3633

3734
w.Reset()
38-
err = writePktLine(w, pktLineTypeData, []byte("a\nb"))
39-
assert.NoError(t, err)
35+
writePktLine(w, pktLineTypeData, []byte("a\nb"))
4036
assert.Equal(t, []byte("0007a\nb"), w.Bytes())
4137
}

‎modules/private/hook.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ type HockProcReceiveRefResult struct {
8686
NewOID string
8787
Ref string
8888
OrignRef string
89+
Err string
8990
}
9091

9192
// HookPreReceive check whether the provided commits are allowed

‎routers/private/hook.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -620,19 +620,25 @@ func HookProcReceive(ctx *gitea_context.PrivateContext) {
620620

621621
for i := range opts.OldCommitIDs {
622622
if opts.NewCommitIDs[i] == git.EmptySHA {
623-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
624-
"err": "Can't delete not exist branch",
623+
results = append(results, private.HockProcReceiveRefResult{
624+
OrignRef: opts.RefFullNames[i],
625+
OldOID: opts.OldCommitIDs[i],
626+
NewOID: opts.NewCommitIDs[i],
627+
Err: "Can't delete not exist branch",
625628
})
626-
return
629+
continue
627630
}
628631

629632
baseBranchName := opts.RefFullNames[i][len(git.PullRequestPrefix):]
630633
if !gitRepo.IsBranchExist(baseBranchName) {
631-
ctx.JSON(http.StatusNotFound, map[string]interface{}{
632-
"Err": fmt.Sprintf("target branch %s is not exist in %s/%s",
634+
results = append(results, private.HockProcReceiveRefResult{
635+
OrignRef: opts.RefFullNames[i],
636+
OldOID: opts.OldCommitIDs[i],
637+
NewOID: opts.NewCommitIDs[i],
638+
Err: fmt.Sprintf("target branch %s is not exist in %s/%s",
633639
baseBranchName, ownerName, repoName),
634640
})
635-
return
641+
continue
636642
}
637643

638644
if len(topicBranch) == 0 {

0 commit comments

Comments
 (0)