thanks! I left a review comment here: !223849 (comment 3171033058)

@zmartins @albi.yusupova @hacks4oats thanks for the ping for review !

issue (blocking): I think we should address the Danger warnings about commit quantity and structure. Please review the warnings above, and re-ping me once that is addressed and I can continue with the review 👍

issue (blocking): The "How to set up and validate locally" is blank. If the code is not yet reachable, please add that to the description. If it is reachable, please add a section on how to validate locally.

suggestion (non-blocking): The Danger warning "This merge request is quite big (721 lines changed), please consider splitting it into multiple merge requests" also seems relevant. Our docs reinforce this in Recommendations for MR authors:

Consider splitting big MRs into smaller ones. Around 200 lines is a good goal.

And in Keep it simple:

Live by smaller iterations. Keep the amount of changes in a single MR as small as possible.

I see you're already using stacked diffs with !224996 on top of this one. I think stacked diffs is a great approach! What were your thoughts on leveraging it to break this MR into a smaller entry point to merge to master?

I realize this may be difficult to restructure at this juncture, and you already have one approval, but this would be a good practice for the future.

suggestion (non-blocking): Check out !227843 (merged). A where/with_them table could likely reduce the verbosity of the branch name generation specs and the overall cognitive load for reviewers.

Thanks again for the hard work on this ! Let me know if I missed anything or you have any questions! 🏓

@vyaklushin looks good to me 👍

• local verification worked as expected
• code changes look clear
• MR is removing more code than adding

@engwan would you mind doing the backend maintainer review? thanks!

What does this MR do and why?

Contributes to #590000

Problem

The git_ref_validator_custom_validation feature flag was used to derisk the migration from Rugged-based ref name validation to a custom implementation. The flag has been enabled on production and verified stable.

Solution

Remove the feature flag and make custom validation the only code path. Delete legacy Rugged-based validation methods and the FF YAML definition. Flatten and simplify the spec by inlining shared_examples and merging test data that was previously split across FF-conditional contexts.

References

• Rollout issue: #590000
• Feature issue: #430709

Screenshots or screen recordings

Not applicable — no UI changes.

How to set up and validate locally

1. Run bundle exec rspec spec/lib/gitlab/git_ref_validator_spec.rb — all 172 examples should pass
2. Run bundle exec rspec spec/services/tags/create_service_spec.rb — all 8 examples should pass
3. Verify no remaining references: grep -r git_ref_validator_custom_validation . should return nothing

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

@j.seto yeah it seems there's a bit of duplication, which if i remember correctly is part of where i was running into trouble with the undercoverage stuff. I like your idea, and agree it could be a good follow up!

What does this MR do and why?

This MR implements a batched background migration to backfill the subscription_add_on_uid column in the subscription_add_on_purchases table. The migration copies the name enum value from the associated subscription_add_ons record into subscription_add_on_uid, establishing a fixed model ID reference.

This is part of the effort to convert the subscription_add_ons table to be hard-coded in application code (see &19981). By backfilling this column, add-on purchases can reference add-ons by a consistent identifier across all database instances, which is essential for the Cells architecture.

References

• Related to #588162

Query Plan

https://console.postgres.ai/gitlab/gitlab-production-main/sessions/49638/commands/147928

  WITH sub_batch_ids AS MATERIALIZED (
    SELECT "subscription_add_on_purchases"."id"
      FROM "subscription_add_on_purchases"
      WHERE "subscription_add_on_purchases"."id" >= 1
        AND "subscription_add_on_purchases"."id" < 51
      LIMIT 100
  ) UPDATE subscription_add_on_purchases
  SET subscription_add_on_uid = subscription_add_ons.name
    FROM subscription_add_ons
    WHERE subscription_add_on_purchases.subscription_add_on_id = subscription_add_ons.id
      AND subscription_add_on_purchases.id IN (SELECT id FROM sub_batch_ids)
      AND subscription_add_on_purchases.subscription_add_on_uid IS NULL

ModifyTable on public.subscription_add_on_purchases  (cost=4.02..7.08 rows=0 width=0) (actual time=0.030..0.031 rows=0 loops=1)
   Buffers: shared hit=6
   CTE sub_batch_ids
     ->  Limit  (cost=0.42..3.44 rows=1 width=8) (actual time=0.026..0.026 rows=0 loops=1)
           Buffers: shared hit=6
           ->  Index Only Scan using subscription_add_on_purchases_pkey on public.subscription_add_on_purchases subscription_add_on_purchases_1  (cost=0.42..3.44 rows=1 width=8) (actual time=0.025..0.025 rows=0 loops=1)
                 Index Cond: ((subscription_add_on_purchases_1.id >= 1) AND (subscription_add_on_purchases_1.id < 51))
                 Heap Fetches: 0
                 Buffers: shared hit=6
   ->  Nested Loop  (cost=0.58..3.63 rows=1 width=46) (actual time=0.029..0.030 rows=0 loops=1)
         Buffers: shared hit=6
         ->  Nested Loop  (cost=0.45..3.48 rows=1 width=46) (actual time=0.029..0.029 rows=0 loops=1)
               Buffers: shared hit=6
               ->  HashAggregate  (cost=0.02..0.03 rows=1 width=40) (actual time=0.028..0.029 rows=0 loops=1)
                     Group Key: sub_batch_ids.id
                     Buffers: shared hit=6
                     ->  CTE Scan on sub_batch_ids  (cost=0.00..0.02 rows=1 width=40) (actual time=0.027..0.027 rows=0 loops=1)
                           Buffers: shared hit=6
               ->  Index Scan using subscription_add_on_purchases_pkey on public.subscription_add_on_purchases  (cost=0.42..3.44 rows=1 width=22) (actual time=0.000..0.000 rows=0 loops=0)
                     Index Cond: (subscription_add_on_purchases.id = sub_batch_ids.id)
                     Filter: (subscription_add_on_purchases.subscription_add_on_uid IS NULL)
                     Rows Removed by Filter: 0
         ->  Index Scan using subscription_add_ons_pkey on public.subscription_add_ons  (cost=0.13..0.15 rows=1 width=16) (actual time=0.000..0.000 rows=0 loops=0)
               Index Cond: (subscription_add_ons.id = subscription_add_on_purchases.subscription_add_on_id)
Settings: jit = 'off', random_page_cost = '1.5', work_mem = '230MB', seq_page_cost = '4', effective_cache_size = '472585MB'

How to set up and validate locally

1. Ensure you have test data with NULL subscription_add_on_uid values:

   # In Rails console
   add_on = GitlabSubscriptions::AddOn.find_or_create_by!(name: :code_suggestions)
   namespace = Group.first
   
   purchase = GitlabSubscriptions::AddOnPurchase.create!(
     add_on: add_on,
     namespace: namespace,
     organization: namespace.organization,
     quantity: 5,
     expires_on: 1.year.from_now,
     started_at: Time.current,
     purchase_xid: "test-#{SecureRandom.hex(4)}"
   )
   
   # Manually set subscription_add_on_uid to NULL to simulate pre-migration state
   purchase.update_column(:subscription_add_on_uid, nil)

2. Run the migration:

   bundle exec rails db:migrate:up:main VERSION=20260305202643

3. Execute the batched background migration:

   # In Rails console
   Gitlab::Database::BackgroundMigration::BatchedMigrationRunner.new(
     connection: ApplicationRecord.connection
   ).run_entire_migration(
     Gitlab::Database::BackgroundMigration::BatchedMigration.find_by(
       job_class_name: 'BackfillSubscriptionAddOnPurchasesAddOnUid'
     )
   )

4. Verify the backfill:

   # In Rails console
   purchase.reload.subscription_add_on_uid
   # => Should now equal the add_on.name enum value (e.g., 1 for code_suggestions)
   
   # Verify no NULL values remain
   GitlabSubscriptions::AddOnPurchase.where(subscription_add_on_uid: nil).count
   # => Should be 0`

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #588162

@fjedelhauser thanks for doing that! I think we need to update the milestone from 18.10 to 18.11 throughout, correct?

@j.seto @vyaklushin I had to change a couple of things to get proper test coverage across CE/EE. I've just re reviewed it and tested it locally again. Would you mind taking one last look? 🙏 thank you so much

I believe the primary change since you last viewed it is here: !224542 (fb383fd1) which didn't change behavior but just structure (which is why no specs changed in that commit)

Hunter Stewart (fb383fd1) at 16 Mar 18:06

... and 629 more commits

@j.seto thanks for the ping! it looks good to me 👍

@nicolasdular mind doing the backend maintainer review 🙏 ? thanks!

What does this MR do and why?

TimedLogger is used to maintain a list of messages that is used in a response to git push when git access checks time out. GitAccess raises a separate error using these messages.

Start logging the original exception to facilitate debugging timeouts

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #384325

@j.seto This reads better 👍

@j.seto nice, this is less confusing this way

Hunter Stewart (05ddd5e1) at 16 Mar 15:21

@brendan777 I'm thinking we might wants replace the remove multiple missing section with a description of the rake task

@emmaspark thanks for the ping! Are you able to check on the pipeline failures? i spot checked it and it seems there might be some failures around the push rules

for example

 10) PushRule#commit_validation? setting: :max_file_size, value: 1, result: false when rule is enabled at global level returns the default value at project level
      Failure/Error: expect(rule.commit_validation?).to eq(result)
      
      NoMethodError:
        undefined method `commit_validation?' for nil
      Shared Example Group: "commit validated push rule" called from ./ee/spec/models/push_rule_spec.rb:130
      # ./ee/spec/models/push_rule_spec.rb:123:in `block (5 levels) in <top (required)>

Please re-request a review from me once you have a chance to take a look at those 🙏 thanks!

Thanks for this reply! We're working on a new rake task in !226677 (merged) that should be an improvement over copying in the troubleshooting snippet