3.6 - GitHub và Pull Request

Tóm tắt: Pull request thường bị xem là cổng gác — ai đó phải bấm Approve thì code mới vào được. Nhưng giá trị lớn hơn nằm ở chỗ khác: nó là nơi kiến thức về hệ thống lan ra khỏi đầu một người. Từ góc nhìn đó suy ra cách làm hiệu quả: PR nhỏ (dưới 400 dòng, vì quá ngưỡng đó khả năng phát hiện lỗi của người review tụt hẳn), mô tả trả lời vì sao chứ không kể lại diff, và nhận xét nhắm vào code chứ không nhắm vào người viết. Bài này cũng làm rõ ba kiểu merge của GitHub và hệ quả rất khác nhau của chúng lên lịch sử.

Mục tiêu bài học

Sau bài này bạn có thể:

• Mở một pull request mà người khác review được trong 15 phút.
• Chọn đúng giữa merge commit, squash merge và rebase merge.
• Viết nhận xét review cụ thể và không mang tính công kích.
• Thiết lập branch protection cho nhánh chính.
• Dựng một quy trình CI tối thiểu cho dự án .NET bằng GitHub Actions.

Cần biết trước: bài 3.4 (push, remote), bài 3.5 (nhánh, merge, rebase). Bài tập 3 cần một project .NET có unit test như ở bài 0.4.

Nội dung bài học

3.6.1 — Vòng đời một pull request

git switch -c feature/revenue-report
# ... làm việc, commit ...
git push -u origin feature/revenue-report
# GitHub in ra một đường dẫn tạo PR ngay trong output của lệnh push

Một PR chưa sẵn sàng để review có thể mở ở dạng Draft pull request: CI vẫn chạy, mọi người xem được, nhưng không merge được cho tới khi bạn bấm Ready for review.

3.6.2 — PR nhỏ là PR được review tử tế

Đây là yếu tố ảnh hưởng nhiều nhất tới chất lượng review, và nó không liên quan gì tới kỹ năng của người review.

Kích thước PR	Chuyện thường xảy ra
Dưới 100 dòng	Review kỹ, nhận xét cụ thể
100–400 dòng	Vẫn ổn, cần tập trung
400–1000 dòng	Bắt đầu đọc lướt
Trên 1000 dòng	"LGTM 👍"

PR càng to, người review càng khó giữ toàn bộ bối cảnh trong đầu, và xác suất họ chỉ duyệt cho xong càng cao. Một PR 2000 dòng thường nhận được ít nhận xét hơn một PR 200 dòng — không phải vì nó tốt hơn.

Cách giữ PR nhỏ:

• Tách tái cấu trúc ra khỏi thay đổi hành vi. Hai PR, review độc lập.
• Gửi PR "dọn đường" trước (đổi tên, di chuyển file), rồi mới gửi PR logic.
• Tính năng lớn thì chia theo lát cắt dọc: mỗi PR là một phần chạy được từ đầu tới cuối.

3.6.3 — Mô tả PR

## Vì sao

Khách hàng nhập số lượng âm ở form đặt hàng, hệ thống vẫn tạo đơn
với tổng tiền âm. Tuần trước có 3 đơn như vậy lọt vào báo cáo doanh thu.

## Làm gì

- Thêm kiểm tra ở tầng domain (`Order.AddLine`), không đặt ở controller
  vì còn luồng tạo đơn từ API công khai và từ job nhập liệu.
- Thêm test cho số lượng âm, bằng không, và vượt tồn kho.

## Cách kiểm tra

1. Gọi `POST /api/orders` với `quantity: -5`
2. Kỳ vọng `422` kèm `ProblemDetails`
3. Kiểm tra bảng `orders` không có bản ghi mới

## Lưu ý cho người review

Phần `OrderValidator` có thay đổi hành vi với đơn hàng đã tạo trước đây —
xem `Migrations/20260924_FixNegativeTotals.cs`.

Mục cuối là mục ít người viết nhất và có giá trị cao nhất. Nó chỉ thẳng vào chỗ cần soi kỹ, thay vì để người review tự mò trong 300 dòng diff.

3.6.4 — Ba kiểu merge của GitHub

Kiểu	Lịch sử main	Dùng khi
Create a merge commit	Giữ mọi commit của nhánh, thêm một merge commit	Muốn giữ chi tiết quá trình
Squash and merge	Gộp tất cả thành một commit	Nhánh có nhiều commit "wip", "fix"
Rebase and merge	Đặt từng commit lên đầu main, lịch sử thẳng	Các commit đã sạch sẵn

Squash là mặc định hợp lý cho hầu hết đội: main có lịch sử sạch, mỗi dòng là một tính năng hoàn chỉnh, và git bisect chạy rất hiệu quả trên đó.

Đánh đổi của squash: mất chi tiết quá trình. Với PR lớn, sáu tháng sau bạn chỉ thấy một commit 800 dòng, không còn biết thứ tự người viết đã làm. Đây là một lý do nữa để giữ PR nhỏ — với PR nhỏ, đánh đổi này gần như bằng không.

Lưu ý kỹ thuật: cả squash và rebase đều tạo mã commit mới, nên nhánh cũ trên máy bạn trở nên lỗi thời và git branch -d sẽ báo not fully merged (bài 3.5 giải thích vì sao). Sau khi PR đã merged, xoá nhánh cục bộ bằng git branch -D và chạy git fetch --prune.

Với squash merge, message của commit trên main mặc định lấy từ commit message nếu PR chỉ có một commit, và từ tiêu đề PR kèm danh sách commit nếu PR có nhiều commit. Quản trị kho có thể đổi mặc định này trong Settings > General > Pull Requests. Bài 3.10 cho thấy vì sao chi tiết này quan trọng.

3.6.5 — Review: nhắm vào code, không nhắm vào người

❌ "Code này viết tệ quá."
❌ "Sao lại làm thế này?"
❌ "Bạn không hiểu async à?"

✅ "Chỗ này `await` trong vòng lặp sẽ gọi database N lần.
    Gợi ý: lấy toàn bộ trong một query rồi tra bằng Dictionary.
    Tham khảo: /blog/ef-core-n-plus-1-query"

✅ "Mình chưa rõ vì sao cần cache ở đây — dữ liệu này đổi mỗi phút mà?
    Có bối cảnh nào mình đang thiếu không?"

Bốn nguyên tắc dùng được ngay:

1. Nói về code, không nói về người. "Hàm này" chứ không phải "bạn".
2. Kèm lý do và gợi ý. Nhận xét không có đường đi tiếp chỉ làm người ta bế tắc.
3. Phân loại mức độ. Đánh dấu rõ cái nào phải sửa, cái nào chỉ là đề xuất:

**Phải sửa:** SQL injection ở dòng 42
**Nên sửa:** hàm này 80 dòng, tách được thành ba
**Tuỳ bạn:** mình hay đặt tên biến này là `orderTotal`

4. Khen chỗ đáng khen. Nếu ai đó xử lý một ca biên khéo, nói ra. Review chỉ toàn chê là review mà người ta sợ.

Và một quy tắc cho người nhận review: nếu một nhận xét khiến bạn phải giải thích dài dòng, rất có thể code cần một comment — vì người đọc tiếp theo cũng sẽ hỏi đúng câu đó.

3.6.6 — Branch protection

Branch protection (hoặc bản mới hơn là ruleset) là luật GitHub áp lên một nhánh: ai được push, cần gì mới được merge. Cấu hình ở Settings > Branches (kiểu cũ) hoặc Settings > Rules > Rulesets.

NOTE

Với tài khoản cá nhân gói GitHub Free, branch protection chỉ dùng được cho kho công khai. Muốn bật cho kho riêng tư cần GitHub Pro — sinh viên được dùng miễn phí qua GitHub Education. Để luyện tập, dùng kho công khai là đơn giản nhất.

Thiết lập tối thiểu cho nhánh main của một dự án thật:

• ☑ Require a pull request before merging
• ☑ Require approvals (ít nhất 1)
• ☑ Require status checks to pass — build và test phải xanh
• ☑ Require branches to be up to date before merging
• ☑ Do not allow bypassing the above settings

Mục thứ tư đáng chú ý: nó chặn tình huống PR của bạn xanh khi tách ra, nhưng main đã đổi ở giữa và hai thay đổi xung đột về mặt logic dù không xung đột về mặt văn bản.

Kèm theo đó, một quy trình CI (continuous integration — tự động build và chạy test mỗi khi có thay đổi) tối thiểu cho .NET bằng GitHub Actions:

# .github/workflows/ci.yml
name: CI
on: [pull_request]
jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v7
      - uses: actions/setup-dotnet@v6
        with:
          dotnet-version: '10.0.x'
      - run: dotnet restore
      - run: dotnet build --no-restore
      - run: dotnet test --no-build --verbosity normal

Số sau @ là phiên bản lớn của action, tại thời điểm viết là checkout@v7 và setup-dotnet@v6; xem trang phát hành của từng action khi bạn đọc bài này. Chi tiết: GitHub Actions.

3.6.7 — Rà lại quy trình của bạn

Danh sách rà soát pull request

• PR dưới 400 dòng thay đổi; lớn hơn thì đã tách nhỏ.
• Mô tả PR trả lời vì sao, không kể lại diff.
• Có mục hướng dẫn kiểm tra để người review chạy thử được.
• Có chỉ rõ chỗ cần soi kỹ nhất.
• Tái cấu trúc và thay đổi hành vi nằm ở hai PR khác nhau.
• Nhận xét review phân loại rõ phải sửa / nên sửa / tuỳ bạn.
• Nhánh main đã bật branch protection và yêu cầu CI xanh.
• Nhánh được xoá sau khi merge (dùng -D nếu squash merge), và đã fetch --prune ở máy cục bộ.

Bài tập áp dụng

Bài 1 — Đo kích thước pull request (làm theo cặp)

Làm cùng một bạn trong CLB. Mỗi người tạo một kho công khai crm-git-lab trên GitHub, thêm bạn kia làm Collaborator (Settings > Collaborators). Trong kho của mình, mở hai PR:

• PR nhỏ: khoảng 20–40 dòng, ví dụ thêm một hàm và chú thích.
• PR lớn: trên 400 dòng, ví dụ thêm nhiều file cùng lúc.

Cố ý cài ba lỗi nhỏ cùng loại vào mỗi PR (một lỗi chính tả trong chuỗi, một điều kiện > lẽ ra là >=, một biến khai báo mà không dùng). Mỗi người review hai PR của bạn kia, mỗi PR trong 15 phút, không biết trước lỗi nằm đâu.

Tiêu chí hoàn thành: mỗi người có bảng hai dòng (số dòng thay đổi, số nhận xét, số lỗi cài sẵn bị phát hiện) và ba câu kết luận về quan hệ giữa kích thước PR và chất lượng review.

Gợi ý và lời giải — Bài 1

Gợi ý. Lấy số liệu bằng GitHub CLI thay vì đếm tay:

gh pr list --state all --limit 10 \
  --json number,title,additions,deletions,reviews \
  --jq '.[] | "\(.number)\t\(.additions + .deletions)\t\(.reviews | length)\t\(.title)"'

Số nhận xét trên từng dòng code xem ở tab Files changed của PR.

Lời giải — cách đọc kết quả. Không có con số "đúng" chung cho mọi cặp. Hãy so hai tỉ lệ giữa PR nhỏ và PR lớn: số lỗi phát hiện / số lỗi cài sẵn và số nhận xét / 100 dòng. Cùng 15 phút, người review đọc kỹ được 30 dòng nhưng chỉ lướt được 400 dòng, nên thường PR lớn có cả hai tỉ lệ thấp hơn hẳn. Nếu PR lớn của bạn vẫn được soi kỹ, hãy hỏi người review đã thật sự mất bao lâu.

Vì sao. Review là công việc tốn sức tập trung. Người review phải dựng lại trong đầu ngữ cảnh của toàn bộ thay đổi trước khi đánh giá được từng dòng. Vượt một ngưỡng nhất định, việc đó trở nên bất khả thi trong một lần ngồi, và phản ứng tự nhiên là duyệt cho xong thay vì thừa nhận mình không đọc nổi. Các khuyến nghị trong công nghiệp (ví dụ nghiên cứu của SmartBear tại Cisco) cũng là mỗi lần chỉ review vài trăm dòng và không quá khoảng một giờ.

Cách chia PR lớn — ba hướng, theo thứ tự nên thử:

1. Chia theo tầng. Thay đổi database thành một PR, tầng nghiệp vụ một PR, API một PR. Mỗi PR merge được độc lập nếu dùng mẫu mở rộng rồi thu hẹp ở bài 3.4.
2. Tách riêng phần dọn dẹp. Đổi tên, định dạng lại, sắp xếp using — gộp chung với thay đổi logic là cách chắc chắn nhất để không ai review được. Tách thành PR riêng, người review duyệt trong hai phút.
3. Dùng feature flag. Merge code chưa hoàn thiện nhưng chưa bật cho người dùng, nên PR nhỏ lại mà không cần chờ tính năng xong — cách làm ở bài 3.8.

Bài 2 — Viết lại một mô tả PR

Lấy PR lớn ở Bài 1 (hoặc bất kỳ PR nào có mô tả sơ sài). Viết lại mô tả theo cấu trúc ở mục 3.6.3, kèm mục "lưu ý cho người review". Hỏi bạn cùng cặp: bản nào dễ review hơn, vì sao?

Tiêu chí hoàn thành: mô tả mới trả lời được vì sao thay đổi này cần thiết, không chỉ nó làm gì.

Gợi ý và lời giải — Bài 2

Gợi ý. Diff đã nói cái gì thay đổi. Mô tả PR tồn tại để nói những thứ diff không nói được: vì sao cần, đã cân nhắc phương án nào khác, chỗ nào rủi ro, kiểm thử ra sao.

Lời giải — trước:

Sửa bug tính doanh thu

Sau:

## Vấn đề

Báo cáo doanh thu theo chi nhánh tính cả đơn hàng đã huỷ, nên số liệu
tháng 8 cao hơn thực tế khoảng 12%. Phòng kinh doanh phát hiện khi đối
chiếu với sổ kế toán (CRM-142).

## Cách xử lý

Thêm điều kiện lọc `IsCancelled == false` ở tầng truy vấn thay vì lọc
trong bộ nhớ, để database làm việc lọc và không kéo về dữ liệu thừa.

## Phương án đã cân nhắc

- Lọc trong bộ nhớ sau khi lấy dữ liệu: đơn giản hơn nhưng vẫn truyền
  toàn bộ đơn huỷ qua mạng, với chi nhánh lớn là khoảng 40% dữ liệu.
- Thêm một view trong database: sạch hơn nhưng phải thêm migration và
  đội vận hành đang tạm dừng thay đổi lược đồ trong tuần này.

## Lưu ý cho người review

- Hàm `TinhDoanhThu` được dùng ở **ba** chỗ khác: dashboard, xuất Excel,
  và job gửi email hằng tuần. Đã kiểm tra cả ba, nhưng đây là chỗ rủi ro nhất.
- Chưa xử lý đơn hoàn tiền một phần — sẽ làm ở CRM-148, đã tạo issue.

## Kiểm thử

- Thêm 3 unit test cho các trường hợp: không có đơn huỷ, toàn đơn huỷ, hỗn hợp.
- Chạy lại báo cáo tháng 8 trên dữ liệu staging: khớp với sổ kế toán.

Closes #142

Năm phần và lý do tồn tại của từng phần:

Phần	Trả lời câu hỏi	Vì sao cần
Vấn đề	Vì sao thay đổi này tồn tại	Sáu tháng sau, đây là thứ duy nhất còn giải thích được
Cách xử lý	Đã chọn hướng nào	Người review đánh giá được lựa chọn, không chỉ đánh giá code
Phương án đã cân nhắc	Vì sao không làm cách khác	Chặn trước câu "sao không dùng cách X"
Lưu ý cho người review	Nên soi kỹ chỗ nào	Hướng sự tập trung vào phần rủi ro nhất
Kiểm thử	Làm sao biết nó đúng	Người review không phải tự đoán

Mục "lưu ý cho người review" là mục có giá trị cao nhất, và cũng là mục hay thiếu nhất. Nó thừa nhận rằng người review có thời gian hữu hạn, và hướng thời gian đó vào nơi đáng dùng. Nói thẳng "chỗ này tôi không chắc" không phải điểm yếu — nó là cách hiệu quả nhất để nhận được review thực chất.

Tự động hoá bằng mẫu. Tạo file .github/pull_request_template.md với các tiêu đề trên; GitHub tự điền vào mỗi PR mới. Công sức bỏ ra một lần, cả đội hưởng lợi lâu dài.

Liên hệ với commit message. Cùng một nguyên tắc ở hai quy mô: commit message trả lời "vì sao" cho một thay đổi, mô tả PR trả lời "vì sao" cho một nhóm thay đổi. Bài 3.10 trình bày cách chuẩn hoá phần này để sinh changelog tự động.

Bài 3 — Dựng CI tối thiểu và chặn merge

Đưa project .NET có unit test (ví dụ solution ở bài 0.4) lên một kho công khai. Thêm file workflow, bật bảo vệ nhánh main, rồi mở một PR cố ý làm hỏng một test và xác nhận GitHub chặn không cho merge.

Tiêu chí hoàn thành: nút Merge bị vô hiệu hoá, và bạn giải thích được vì sao cấu hình bảo vệ nhánh quan trọng hơn bản thân file workflow.

Gợi ý và lời giải — Bài 3

Gợi ý. Chỉ thêm workflow thôi thì CI chạy nhưng không chặn được gì — nó chỉ hiện dấu đỏ mà mọi người vẫn merge được. Phần chặn nằm ở cấu hình bảo vệ nhánh trên GitHub.

Lời giải — bước 1, file .github/workflows/ci.yml:

name: CI

on:
  pull_request:
    branches: [main]
  push:
    branches: [main]

jobs:
  build-and-test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v7

      - uses: actions/setup-dotnet@v6
        with:
          dotnet-version: '10.0.x'

      - name: Restore packages
        run: dotnet restore

      - name: Build
        run: dotnet build --no-restore --configuration Release

      - name: Test
        run: dotnet test --no-build --configuration Release --verbosity normal

Commit file này lên main và đợi lần chạy đầu tiên xanh ở tab Actions. Tên job build-and-test sẽ là tên "status check" ở bước 2.

Bước 2 — bật bảo vệ nhánh. Trên GitHub: Settings > Branches > Add classic branch protection rule cho main (hoặc tạo ruleset với các tuỳ chọn tương đương), rồi bật:

[x] Require a pull request before merging
    [x] Require approvals: 1
[x] Require status checks to pass before merging
    [x] Require branches to be up to date before merging
    -> chọn check "build-and-test"
[x] Do not allow bypassing the above settings

Nếu làm một mình, bỏ "Require approvals" (GitHub không cho tự duyệt PR của chính mình), hoặc nhờ bạn cùng cặp ở Bài 1 duyệt.

Bước 3 — kiểm chứng. Tạo nhánh, sửa một test cho nó hỏng (ví dụ đổi giá trị mong đợi trong Assert.Equal), push và mở PR. Kết quả mong đợi: check build-and-test đỏ và nút Merge bị khoá vì một status check bắt buộc chưa đạt.

Vì sao bảo vệ nhánh quan trọng hơn file workflow. Không có nó, CI chỉ là một tín hiệu mang tính gợi ý. Thực tế lặp lại ở mọi đội: khi đang gấp, người ta merge đè lên dấu đỏ với lý do "test này hay hỏng vặt thôi". Vài lần như vậy là cả đội quen với việc bỏ qua dấu đỏ, và CI mất sạch giá trị.

Ba tuỳ chọn đáng bật thêm:

Tuỳ chọn	Ngăn được gì
Require branches to be up to date	Xung đột ngữ nghĩa ở bài 3.7 — buộc CI chạy trên kết quả merge thật
Require conversation resolution	Merge khi nhận xét review chưa được xử lý
Do not allow bypassing	Người có quyền quản trị tự ý bỏ qua mọi quy tắc

Tuỳ chọn đầu tiên đáng giá nhất, và cũng là tuỳ chọn hay bị bỏ qua nhất.

Giữ CI dưới 10 phút. Đây là điều kiện mà bài 3.8 nêu cho mô hình trunk-based. CI chạy 40 phút khiến người ta gộp nhiều việc vào một PR để đỡ phải chờ nhiều lần — đúng thứ mà Bài 1 ở trên cho thấy là có hại. Hai cách rút ngắn hiệu quả nhất: lưu đệm gói NuGet giữa các lần chạy, và tách bộ test chậm sang một workflow chạy theo lịch thay vì chạy trên mỗi PR.

Lưu đệm NuGet cần lock file packages.lock.json (file ghi chính xác phiên bản mọi gói, xem bài 3.13), vì khoá của bộ đệm được tính từ file này. Bật lock file trong .csproj, chạy dotnet restore một lần rồi commit file sinh ra:

<PropertyGroup>
  <RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>
</PropertyGroup>

      - uses: actions/setup-dotnet@v6
        with:
          dotnet-version: '10.0.x'
          cache: true                       # lưu đệm NuGet, khoá theo packages.lock.json
      - run: dotnet restore --locked-mode   # báo lỗi nếu lock file lệch với .csproj

Chưa có lock file thì đừng thêm bộ đệm: khoá không đổi theo gói thì bộ đệm vô nghĩa.

Tự kiểm tra

Vì sao PR nhỏ lại được review tốt hơn?

Vì người review phải giữ toàn bộ bối cảnh trong đầu, và khả năng đó giảm nhanh theo kích thước. Trên khoảng 400 dòng thì người ta bắt đầu đọc lướt, trên 1000 dòng thì thường chỉ duyệt cho xong. Một PR 2000 dòng thường nhận ít nhận xét hơn một PR 200 dòng, không phải vì nó tốt hơn.

Ba kiểu merge của GitHub khác nhau ra sao?

Merge commit giữ mọi commit của nhánh và thêm một commit hợp nhất. Squash and merge gộp tất cả thành một commit duy nhất trên main. Rebase and merge đặt từng commit lên đầu main cho lịch sử thẳng. Squash là mặc định hợp lý cho hầu hết đội vì main có lịch sử sạch và git bisect chạy rất hiệu quả.

Đánh đổi của squash merge là gì?

Mất chi tiết quá trình. Với PR lớn, sáu tháng sau bạn chỉ thấy một commit 800 dòng và không còn biết thứ tự người viết đã làm. Đây là thêm một lý do để giữ PR nhỏ — với PR nhỏ thì đánh đổi này gần như bằng không.

Mục nào trong mô tả PR có giá trị cao nhất?

Mục lưu ý cho người review, chỉ thẳng vào chỗ cần soi kỹ nhất. Đó là mục ít người viết nhất nhưng tiết kiệm nhiều thời gian nhất, vì nó thay việc người review tự mò trong hàng trăm dòng diff bằng một chỉ dẫn cụ thể.

Nhận xét review nên viết thế nào?

Nói về code chứ không nói về người, kèm lý do và một hướng đi tiếp cụ thể, và phân loại rõ mức độ: phải sửa, nên sửa, hay chỉ là đề xuất tuỳ chọn. Cũng nên khen chỗ đáng khen, vì review chỉ toàn chê là review mà người ta sợ.

Require branches to be up to date before merging chặn được gì?

Chặn tình huống PR của bạn xanh khi tách ra, nhưng nhánh main đã thay đổi ở giữa, và hai thay đổi xung đột về mặt logic dù không xung đột về mặt văn bản. Tuỳ chọn này buộc nhánh phải cập nhật theo main rồi chạy lại CI trước khi được merge.

Kết luận

Ba điều đáng nhớ nhất:

1. PR là nơi kiến thức lan ra, không chỉ là cổng gác.
2. 400 dòng là ngưỡng thực tế. Vượt qua nó, review biến thành nghi lễ.
3. Nhận xét cần kèm đường đi tiếp. Chỉ ra vấn đề mà không gợi ý giải pháp chỉ làm người ta bế tắc.

Tham khảo

• GitHub Pull requests — vòng đời và tuỳ chọn merge
• GitHub Actions — dựng CI cho PR
• git-merge — phần Git bên dưới
• Conventional Commits — hữu ích khi dùng squash merge

Điều hướng

• Bài trước: 3.5 — Branching và Merging
• Bài tiếp theo: 3.7 — Git Conflicts
• Về module: Trang mục lục