Skip to content
Snippets Groups Projects

Fix gitaly ref encoding bugs

Merged Jacob Vosmaer requested to merge jacobvosmaer-gitlab/gitlab-ce:gitaly-encodings into master
1 unresolved thread

What does this MR do?

Adds missing encoding coercion calls on Gitaly ref responses.

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

We were injecting ASCII-8BIT strings into GitLab, that doesn't work. It has to be UTF-8.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/34156

Edited by Jacob Vosmaer

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Jacob Vosmaer resolved all discussions

    resolved all discussions

  • Jacob Vosmaer changed milestone to %9.4

    changed milestone to %9.4

  • Jacob Vosmaer changed milestone to %9.3

    changed milestone to %9.3

  • This is a regression that prevents us from delivering 4 Gitaly features.

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 9b2bf24c - Fix gitaly ref encoding bugs

    Compare with previous version

  • Jacob Vosmaer changed the description

    changed the description

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • d3bcf8ac - Fix gitaly ref encoding bugs

    Compare with previous version

  • Jacob Vosmaer resolved all discussions

    resolved all discussions

  • Some polish in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12526; that's separate because I want to get the bug fix in as a small commit.

  • Jacob Vosmaer mentioned in merge request !12526 (merged)

    mentioned in merge request !12526 (merged)

  • Robert Speicher
  • Robert Speicher resolved all discussions

    resolved all discussions

  • 64 65 end
    65 66
    66 67 def consume_branches_response(response)
    67 response.flat_map { |r| r.branches }
    68 response.flat_map do |message|
    69 message.branches.map do |gitaly_branch|
    70 Gitlab::Git::Branch.new(
    71 @repository,
    72 encode!(gitaly_branch.name.dup),
    73 gitaly_branch.commit_id
  • Robert Speicher approved this merge request

    approved this merge request

  • Robert Speicher resolved all discussions

    resolved all discussions

  • Robert Speicher mentioned in commit 9f44687a

    mentioned in commit 9f44687a

  • Robert Speicher mentioned in commit 20bb1c99

    mentioned in commit 20bb1c99

  • @jacobvosmaer-gitlab Attempting to pick this into https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12508 for 9.3.3 resulted in conflicts. I attempted to fix them with https://gitlab.com/gitlab-org/gitlab-ce/commit/20bb1c99ca4d8fb3b57e034325f6bb1fce179753, but that resulted in the following failures:

                                                                                                                                                                                                     
      1) Gitlab::Git::Repository#ref_name_for_sha returns an empty name if the ref doesn't exist
         Failure/Error: expect(repository.ref_name_for_sha(ref_path, "000000")).to eq("")
         
           expected: ""
                got: "commit"
         
           (compared using ==)
         # ./spec/lib/gitlab/git/repository_spec.rb:1089:in `block (3 levels) in <top (required)>'
         # -e:1:in `<main>'
    
                                                                                                                                                                                                     
      2) Gitlab::Git::Repository#local_branches with gitaly enabled returns a Branch with UTF-8 fields
         Failure/Error: consume_branches_response(stub.find_local_branches(request))
           #<Double :stub> received unexpected message :find_local_branches with (<Gitaly::FindLocalBranchesRequest: repository: <Gitaly::Repository: path: "/Users/jedwardsjones/gitla...le-repo.git/.git", storage_name: "default", relative_path: "mutable-repo.git/.git">, sort_by: :NAME>)
         # ./lib/gitlab/gitaly_client/ref.rb:53:in `local_branches'
         # ./lib/gitlab/git/repository.rb:116:in `block in local_branches'
         # ./lib/gitlab/gitaly_client.rb:77:in `block in migrate'
         # ./lib/gitlab/metrics/influx_db.rb:92:in `measure'
         # ./lib/gitlab/gitaly_client.rb:76:in `migrate'
         # ./lib/gitlab/git/repository.rb:1245:in `gitaly_migrate'
         # ./lib/gitlab/git/repository.rb:114:in `local_branches'
         # ./spec/lib/gitlab/git/repository_spec.rb:1301:in `block (4 levels) in <top (required)>'
         # -e:1:in `<main>'
  • James Edwards-Jones mentioned in merge request !12508 (merged)

    mentioned in merge request !12508 (merged)

  • I can't reproduce that test failure at https://gitlab.com/gitlab-org/gitlab-ce/commit/20bb1c99ca4d8fb3b57e034325f6bb1fce179753 locally (yet).

    Edit: also, worth pointing out: that commit is green in CI.

    Edited by Jacob Vosmaer
  • Robert Speicher mentioned in commit 00b03d9f

    mentioned in commit 00b03d9f

  • Confirmed that these failures were only local to my machine. Fixed with a gdk reconfigure which needed an updated GO to compile with.

  • Picked into https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12508, will merge into 9.3-stable ready for 9.3.3

  • Please register or sign in to reply
    Loading