From ab1e20773c6a300b546841f79adf8dd6e707250e Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:36:34 -0700 Subject: [PATCH 1/8] fix: remove deprecation from Git::Path This class is internal only as well as the classes that inherit from it (Git::Index and Git::WorkingTree). Remove the deprecation warning and just remove the deprecated code. --- lib/git/path.rb | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/lib/git/path.rb b/lib/git/path.rb index 32b3baa4..066f39db 100644 --- a/lib/git/path.rb +++ b/lib/git/path.rb @@ -9,17 +9,7 @@ module Git class Path attr_accessor :path - def initialize(path, check_path = nil, must_exist: nil) - unless check_path.nil? - Git::Deprecation.warn( - 'The "check_path" argument is deprecated and ' \ - 'will be removed in a future version. Use "must_exist:" instead.' - ) - end - - # default is true - must_exist = must_exist.nil? && check_path.nil? ? true : must_exist || check_path - + def initialize(path, must_exist: true) path = File.expand_path(path) raise ArgumentError, 'path does not exist', [path] if must_exist && !File.exist?(path) From 9da1e9112e38c0e964dd2bc638bda7aebe45ba91 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:37:48 -0700 Subject: [PATCH 2/8] fix: remove deprecation from Git::Stash Since Git::Stash#initialize is an internal only method, remove the deprecation warning and removed the deprecated code. --- lib/git/stash.rb | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/lib/git/stash.rb b/lib/git/stash.rb index 2e9af43b..4762fe2a 100644 --- a/lib/git/stash.rb +++ b/lib/git/stash.rb @@ -3,16 +3,7 @@ module Git # A stash in a Git repository class Stash - def initialize(base, message, existing = nil, save: nil) - unless existing.nil? - Git::Deprecation.warn( - 'The "existing" argument is deprecated and will be removed in a future version. Use "save:" instead.' - ) - end - - # default is false - save = existing.nil? && save.nil? ? false : save | existing - + def initialize(base, message, save: false) @base = base @message = message self.save unless save From abb0efbdb3b6bb49352d097b1fece708477d4362 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:39:23 -0700 Subject: [PATCH 3/8] test: verify deprecated Git::Log methods emit a deprecation warning --- tests/test_helper.rb | 6 +- tests/units/test_log_deprecations.rb | 82 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 tests/units/test_log_deprecations.rb diff --git a/tests/test_helper.rb b/tests/test_helper.rb index aa42eedd..0e6ac89f 100644 --- a/tests/test_helper.rb +++ b/tests/test_helper.rb @@ -13,14 +13,14 @@ $stderr.sync = true # Make tests that emit a deprecation warning fail - +# # Deprecation warnings should not be ignored. - +# # This is important so that: # * when a user sees a deprecation warning, they can be confident it is coming from # their code and not this gem # * test output is clean and does not contain noisey deprecation warnings - +# # Tests whose purpose is to test that a deprecation warning is issued in the right # circumstance should mock Git::Deprecation#warn to avoid raising an error. # diff --git a/tests/units/test_log_deprecations.rb b/tests/units/test_log_deprecations.rb new file mode 100644 index 00000000..ed945a99 --- /dev/null +++ b/tests/units/test_log_deprecations.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'git' +require 'fileutils' +require 'tmpdir' + +# A test case to verify the deprecation warnings for methods on the Git::Log class. +class LogDeprecationsTest < Test::Unit::TestCase + # Set up a temporary Git repository with a single commit before each test. + def setup + @repo_path = Dir.mktmpdir('git_test') + @repo = Git.init(@repo_path) + + # Create a commit so the log has an entry to work with. + Dir.chdir(@repo_path) do + File.write('file.txt', 'content') + @repo.add('file.txt') + @repo.commit('Initial commit') + end + + @log = @repo.log + @first_commit = @repo.gcommit('HEAD') + end + + # Clean up the temporary repository after each test. + def teardown + FileUtils.rm_rf(@repo_path) + end + + # Test the deprecation warning and functionality of Git::Log#each + def test_each_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#each is deprecated. Call #execute and then #each on the result object.' + ) + + commits = @log.map { |c| c } + + assert_equal(1, commits.size, 'The #each method should still yield the correct number of commits.') + assert_equal(@first_commit.sha, commits.first.sha, 'The yielded commit should have the correct SHA.') + end + + # Test the deprecation warning and functionality of Git::Log#size + def test_size_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#size is deprecated. Call #execute and then #size on the result object.' + ) + assert_equal(1, @log.size, 'The #size method should still return the correct number of commits.') + end + + # Test the deprecation warning and functionality of Git::Log#to_s + def test_to_s_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#to_s is deprecated. Call #execute and then #to_s on the result object.' + ) + assert_equal(@first_commit.sha, @log.to_s, 'The #to_s method should return the commit SHA.') + end + + # Test the deprecation warning and functionality of Git::Log#first + def test_first_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#first is deprecated. Call #execute and then #first on the result object.' + ) + assert_equal(@first_commit.sha, @log.first.sha, 'The #first method should return the correct commit.') + end + + # Test the deprecation warning and functionality of Git::Log#last + def test_last_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#last is deprecated. Call #execute and then #last on the result object.' + ) + assert_equal(@first_commit.sha, @log.last.sha, 'The #last method should return the correct commit.') + end + + # Test the deprecation warning and functionality of Git::Log#[] + def test_indexer_deprecation + Git::Deprecation.expects(:warn).with( + 'Calling Git::Log#[] is deprecated. Call #execute and then #[] on the result object.' + ) + assert_equal(@first_commit.sha, @log[0].sha, 'The #[] method should return the correct commit at the specified index.') + end +end From ab17621d65a02b70844fde3127c9cbb219add7f5 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:40:38 -0700 Subject: [PATCH 4/8] test: add tests to verify Git::Object.new creates the right type of object --- tests/units/test_object_new.rb | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 tests/units/test_object_new.rb diff --git a/tests/units/test_object_new.rb b/tests/units/test_object_new.rb new file mode 100644 index 00000000..052bf728 --- /dev/null +++ b/tests/units/test_object_new.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'git' +require 'fileutils' +require 'tmpdir' + +# A test case to verify the functionality of the Git::Object.new factory method. +class ObjectNewTest < Test::Unit::TestCase + # Set up a temporary Git repository with objects of different types. + def setup + @repo_path = Dir.mktmpdir('git_test') + @repo = Git.init(@repo_path) + + Dir.chdir(@repo_path) do + File.write('file.txt', 'This is a test file.') + @repo.add('file.txt') + @repo.commit('Initial commit') + @repo.add_tag('v1.0', message: 'Version 1.0', annotate: true) + end + + @commit = @repo.gcommit('HEAD') + @tree = @commit.gtree + @blob = @tree.blobs['file.txt'] + @tag = @repo.tag('v1.0') + end + + # Clean up the temporary repository after each test. + def teardown + FileUtils.rm_rf(@repo_path) + end + + # Test that the factory method creates a Git::Object::Commit for a commit SHA. + def test_new_creates_commit_object + object = Git::Object.new(@repo, @commit.sha) + assert_instance_of(Git::Object::Commit, object, 'Should create a Commit object.') + assert(object.commit?, 'Object#commit? should be true.') + end + + # Test that the factory method creates a Git::Object::Tree for a tree SHA. + def test_new_creates_tree_object + object = Git::Object.new(@repo, @tree.sha) + assert_instance_of(Git::Object::Tree, object, 'Should create a Tree object.') + assert(object.tree?, 'Object#tree? should be true.') + end + + # Test that the factory method creates a Git::Object::Blob for a blob SHA. + def test_new_creates_blob_object + object = Git::Object.new(@repo, @blob.sha) + assert_instance_of(Git::Object::Blob, object, 'Should create a Blob object.') + assert(object.blob?, 'Object#blob? should be true.') + end + + # Test that using the deprecated `is_tag` argument creates a Tag object + # and issues a deprecation warning. + def test_new_with_is_tag_deprecation + # Set up the mock expectation for the deprecation warning. + Git::Deprecation.expects(:warn).with( + 'Git::Object.new with is_tag argument is deprecated. Use Git::Object::Tag.new instead.' + ) + + # Action: Call the factory method with the deprecated argument. + # The `objectish` here is the tag name, as was the old pattern. + tag_object = Git::Object.new(@repo, 'v1.0', nil, true) + + # Verification + assert_instance_of(Git::Object::Tag, tag_object, 'Should create a Tag object.') + assert(tag_object.tag?, 'Object#tag? should be true.') + assert_equal('v1.0', tag_object.name, 'Tag object should have the correct name.') + # Mocha automatically verifies the expectation at the end of the test. + end +end From e6ccb11830a794f12235e47032235c3284c84cf6 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:41:08 -0700 Subject: [PATCH 5/8] test: add tests for Git::Base#set_index including deprecation --- tests/units/test_set_index.rb | 167 ++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 tests/units/test_set_index.rb diff --git a/tests/units/test_set_index.rb b/tests/units/test_set_index.rb new file mode 100644 index 00000000..e8721bfa --- /dev/null +++ b/tests/units/test_set_index.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'git' +require 'fileutils' +require 'tmpdir' + +# A test case to demonstrate the use of Git::Base#set_index +# +# This test case will to programmatically create a new commit without affecting the +# main working directory or index. +# +class SetIndexTest < Test::Unit::TestCase + # Set up a temporary Git repository before each test. + def setup + # Create a temporary directory for the repository + @repo_path = Dir.mktmpdir('git_test') + + # Initialize a new Git repository in the temporary directory + @repo = Git.init(@repo_path) + + # Change into the repo directory to perform file operations + Dir.chdir(@repo_path) do + # Create and commit an initial file to establish a HEAD and a root tree. + # This gives us a base state to work from. + File.write('file1.txt', 'This is the first file.') + @repo.add('file1.txt') + @repo.commit('Initial commit') + end + end + + attr_reader :repo_path, :repo + + # Clean up the temporary repository after each test. + def teardown + FileUtils.rm_rf(repo_path) + end + + # Tests that `set_index` can point to a new, non-existent index file + # when `must_exist: false` is specified. + def test_set_index_with_must_exist_false_for_new_path + custom_index_path = File.join(repo_path, 'custom_index') + assert(!File.exist?(custom_index_path), 'Precondition: Custom index file should not exist.') + + # Action: Set the index to a new path, allowing it to not exist. + repo.set_index(custom_index_path, must_exist: false) + + # Verification: The repo object should now point to the new index path. + assert_equal(custom_index_path, repo.index.path, 'Index path should be updated to the custom path.') + end + + # Tests that `set_index` successfully points to an existing index file + # when `must_exist: true` is specified. + def test_set_index_with_must_exist_true_for_existing_path + original_index_path = repo.index.path + assert(File.exist?(original_index_path), 'Precondition: Original index file should exist.') + + # Action: Set the index to the same, existing path, explicitly requiring it to exist. + repo.set_index(original_index_path, must_exist: true) + + # Verification: The index path should remain unchanged. + assert_equal(original_index_path, repo.index.path, 'Index path should still be the original path.') + end + + # Tests that `set_index` raises an ArgumentError when trying to point to a + # non-existent index file with the default behavior (`must_exist: true`). + def test_set_index_with_must_exist_true_raises_error_for_new_path + non_existent_path = File.join(repo_path, 'no_such_file') + assert(!File.exist?(non_existent_path), 'Precondition: The target index path should not exist.') + + # Action & Verification: Assert that an ArgumentError is raised. + assert_raise(ArgumentError, 'Should raise ArgumentError for a non-existent index path.') do + repo.set_index(non_existent_path) # must_exist defaults to true + end + end + + # Tests that using the deprecated `check` argument issues a warning via mocking. + def test_set_index_with_deprecated_check_argument + custom_index_path = File.join(repo_path, 'custom_index') + assert(!File.exist?(custom_index_path), 'Precondition: Custom index file should not exist.') + + # Set up the mock expectation. + # We expect Git::Deprecation.warn to be called once with a message + # matching the expected deprecation warning. + Git::Deprecation.expects(:warn).with( + regexp_matches(/The "check" argument is deprecated/) + ) + + # Action: Use the deprecated positional argument `check = false` + repo.set_index(custom_index_path, false) + + # Verification: The repo object should still point to the new index path. + assert_equal(custom_index_path, repo.index.path, 'Index path should be updated even with deprecated argument.') + # Mocha automatically verifies the expectation at the end of the test. + end + + # This test demonstrates creating a new commit on a new branch by + # manipulating a custom, temporary index file. This allows for building + # commits programmatically without touching the working directory or the + # default .git/index. + def test_programmatic_commit_with_set_index + # 1. Get the initial commit object to use as a parent for our new commit. + main_commit = repo.gcommit('main') + assert(!main_commit.nil?, 'Initial commit should exist.') + + # 2. Define a path for a new, temporary index file within the repo directory. + custom_index_path = File.join(repo_path, 'custom_index') + assert(!File.exist?(custom_index_path), 'Custom index file should not exist yet.') + + # 3. Point the git object to use our custom index file. + # Since the file doesn't exist yet, we must pass `must_exist: false`. + repo.set_index(custom_index_path, must_exist: false) + assert_equal(custom_index_path, repo.index.path, 'The git object should now be using the custom index.') + + # 4. Populate the new index by reading the tree from our initial commit into it. + # This stages all the files from the 'main' commit in our custom index. + repo.read_tree(main_commit.gtree.sha) + + # 5. Programmatically create a new file blob and add it to our custom index. + # This simulates `git add` for a new file, but operates directly on the index. + new_content = 'This is a brand new file.' + blob_sha = Tempfile.create('new_blob_content') do |file| + file.write(new_content) + file.rewind + # Use `git hash-object -w` to write the blob to the object database and get its SHA + repo.lib.send(:command, 'hash-object', '-w', file.path) + end + repo.lib.send(:command, 'update-index', '--add', '--cacheinfo', "100644,#{blob_sha},new_file.txt") + + # 6. Write the state of the custom index to a new tree object in the Git database. + new_tree_sha = repo.write_tree + assert_match(/^[0-9a-f]{40}$/, new_tree_sha, 'A new tree SHA should be created.') + + # 7. Create a new commit object from the new tree. + # This commit will have the initial commit as its parent. + new_commit = repo.commit_tree( + new_tree_sha, + parents: [main_commit.sha], + message: 'Commit created programmatically via custom index' + ) + assert(new_commit.commit?, 'A new commit object should be created.') + + # 8. Create a new branch pointing to our new commit. + repo.branch('feature-branch').update_ref(new_commit) + assert(repo.branch('feature-branch').gcommit.sha == new_commit.sha, 'feature-branch should point to the new commit.') + + # --- Verification --- + # Verify the history of the new branch + feature_log = repo.log.object('feature-branch').execute + main_commit_sha = repo.rev_parse('main') # Get SHA directly for reliable comparison + + assert_equal(2, feature_log.size, 'Feature branch should have two commits.') + assert_equal(new_commit.sha, feature_log.first.sha, 'HEAD of feature-branch should be our new commit.') + assert_equal(main_commit_sha, feature_log.last.sha, 'Parent of new commit should be the initial commit.') + + # Verify that the main branch is unchanged + main_log = repo.log.object('main').execute + assert_equal(1, main_log.size, 'Main branch should still have one commit.') + assert_equal(main_commit_sha, main_log.first.sha, 'Main branch should still point to the initial commit.') + + # Verify the contents of the new commit's tree + new_commit_tree = new_commit.gtree + assert(new_commit_tree.blobs.key?('file1.txt'), 'Original file should exist in the new tree.') + assert(new_commit_tree.blobs.key?('new_file.txt'), 'New file should exist in the new tree.') + assert_equal(new_content, new_commit_tree.blobs['new_file.txt'].contents, 'Content of new file should match.') + end +end From ee1113706a8e34e9631f0e2d89bd602bca87f05f Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 16:41:25 -0700 Subject: [PATCH 6/8] test: add tests for Git::Base#set_working including deprecation --- tests/units/test_set_working.rb | 83 +++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 tests/units/test_set_working.rb diff --git a/tests/units/test_set_working.rb b/tests/units/test_set_working.rb new file mode 100644 index 00000000..dfe781e1 --- /dev/null +++ b/tests/units/test_set_working.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'git' +require 'fileutils' +require 'tmpdir' + +# A test case to demonstrate the use of Git::Base#set_working +class SetWorkingTest < Test::Unit::TestCase + # Set up a temporary Git repository before each test. + def setup + # Create a temporary directory for the repository + @repo_path = Dir.mktmpdir('git_test') + + # Initialize a new Git repository in the temporary directory + @repo = Git.init(@repo_path) + end + + attr_reader :repo_path, :repo + + # Clean up the temporary repository after each test. + def teardown + FileUtils.rm_rf(repo_path) + end + + # Tests that `set_working` can point to a new, non-existent directory + # when `must_exist: false` is specified. + def test_set_working_with_must_exist_false_for_new_path + custom_working_path = File.join(repo_path, 'custom_work_dir') + assert(!File.exist?(custom_working_path), 'Precondition: Custom working directory should not exist.') + + # Action: Set the working directory to a new path, allowing it to not exist. + repo.set_working(custom_working_path, must_exist: false) + + # Verification: The repo object should now point to the new working directory path. + assert_equal(custom_working_path, repo.dir.path, 'Working directory path should be updated to the custom path.') + end + + # Tests that `set_working` successfully points to an existing directory + # when `must_exist: true` is specified. + def test_set_working_with_must_exist_true_for_existing_path + original_working_path = repo.dir.path + assert(File.exist?(original_working_path), 'Precondition: Original working directory should exist.') + + # Action: Set the working directory to the same, existing path, explicitly requiring it to exist. + repo.set_working(original_working_path, must_exist: true) + + # Verification: The working directory path should remain unchanged. + assert_equal(original_working_path, repo.dir.path, 'Working directory path should still be the original path.') + end + + # Tests that `set_working` raises an ArgumentError when trying to point to a + # non-existent directory with the default behavior (`must_exist: true`). + def test_set_working_with_must_exist_true_raises_error_for_new_path + non_existent_path = File.join(repo_path, 'no_such_dir') + assert(!File.exist?(non_existent_path), 'Precondition: The target working directory path should not exist.') + + # Action & Verification: Assert that an ArgumentError is raised. + assert_raise(ArgumentError, 'Should raise ArgumentError for a non-existent working directory path.') do + repo.set_working(non_existent_path) # must_exist defaults to true + end + end + + # Tests that using the deprecated `check` argument issues a warning via mocking. + def test_set_working_with_deprecated_check_argument + custom_working_path = File.join(repo_path, 'custom_work_dir') + assert(!File.exist?(custom_working_path), 'Precondition: Custom working directory should not exist.') + + # Set up the mock expectation. + # We expect Git::Deprecation.warn to be called once with a message + # matching the expected deprecation warning. + Git::Deprecation.expects(:warn).with( + regexp_matches(/The "check" argument is deprecated/) + ) + + # Action: Use the deprecated positional argument `check = false` + repo.set_working(custom_working_path, false) + + # Verification: The repo object should still point to the new working directory path. + assert_equal(custom_working_path, repo.dir.path, 'Working directory path should be updated even with deprecated argument.') + # Mocha automatically verifies the expectation at the end of the test. + end +end From df3ea35ebfa007809ff0c700c505781b38be74c5 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 17:18:48 -0700 Subject: [PATCH 7/8] chore: release v4.0.4 --- .release-please-manifest.json | 2 +- CHANGELOG.md | 16 ++++++++++++++++ lib/git/version.rb | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 453aad70..b3c3305f 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,3 +1,3 @@ { - ".": "4.0.3" + ".": "4.0.4" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 154ee8e8..439b428a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ # Change Log +## [4.0.4](https://github.com/ruby-git/ruby-git/compare/v4.0.3...v4.0.4) (2025-07-09) + + +### Bug Fixes + +* Remove deprecation from Git::Path ([ab1e207](https://github.com/ruby-git/ruby-git/commit/ab1e20773c6a300b546841f79adf8dd6e707250e)) +* Remove deprecation from Git::Stash ([9da1e91](https://github.com/ruby-git/ruby-git/commit/9da1e9112e38c0e964dd2bc638bda7aebe45ba91)) + + +### Other Changes + +* Add tests for Git::Base#set_index including deprecation ([e6ccb11](https://github.com/ruby-git/ruby-git/commit/e6ccb11830a794f12235e47032235c3284c84cf6)) +* Add tests for Git::Base#set_working including deprecation ([ee11137](https://github.com/ruby-git/ruby-git/commit/ee1113706a8e34e9631f0e2d89bd602bca87f05f)) +* Add tests to verify Git::Object.new creates the right type of object ([ab17621](https://github.com/ruby-git/ruby-git/commit/ab17621d65a02b70844fde3127c9cbb219add7f5)) +* Verify deprecated Git::Log methods emit a deprecation warning ([abb0efb](https://github.com/ruby-git/ruby-git/commit/abb0efbdb3b6bb49352d097b1fece708477d4362)) + ## [4.0.3](https://github.com/ruby-git/ruby-git/compare/v4.0.2...v4.0.3) (2025-07-08) diff --git a/lib/git/version.rb b/lib/git/version.rb index 0b43866a..dde1e521 100644 --- a/lib/git/version.rb +++ b/lib/git/version.rb @@ -3,5 +3,5 @@ module Git # The current gem version # @return [String] the current gem version. - VERSION = '4.0.3' + VERSION = '4.0.4' end From e27255ad6d06fbf84c1bc32efc2e0f8eb48290a7 Mon Sep 17 00:00:00 2001 From: James Couball Date: Tue, 8 Jul 2025 17:41:13 -0700 Subject: [PATCH 8/8] docs: document and announce the proposed architectural redesign --- README.md | 34 ++++++ redesign/1_architecture_existing.md | 66 +++++++++++ redesign/2_architecture_redesign.md | 130 ++++++++++++++++++++ redesign/3_architecture_implementation.md | 138 ++++++++++++++++++++++ 4 files changed, 368 insertions(+) create mode 100644 redesign/1_architecture_existing.md create mode 100644 redesign/2_architecture_redesign.md create mode 100644 redesign/3_architecture_implementation.md diff --git a/README.md b/README.md index 3a9a65ed..415e48d6 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ [![Build Status](https://github.com/ruby-git/ruby-git/workflows/CI/badge.svg?branch=main)](https://github.com/ruby-git/ruby-git/actions?query=workflow%3ACI) [![Conventional Commits](https://img.shields.io/badge/Conventional%20Commits-1.0.0-%23FE5196?logo=conventionalcommits&logoColor=white)](https://conventionalcommits.org) +- [📢 Architectural Redesign 📢](#-architectural-redesign-) - [📢 We Now Use RuboCop 📢](#-we-now-use-rubocop-) - [📢 Default Branch Rename 📢](#-default-branch-rename-) - [📢 We've Switched to Conventional Commits 📢](#-weve-switched-to-conventional-commits-) @@ -24,6 +25,39 @@ - [Ruby version support policy](#ruby-version-support-policy) - [License](#license) +## 📢 Architectural Redesign 📢 + +The git gem is undergoing a significant architectural redesign for the upcoming +v5.0.0 release. The current architecture has several design challenges that make it +difficult to maintain and evolve. This redesign aims to address these issues by +introducing a clearer, more robust, and more testable structure. + +We have prepared detailed documents outlining the analysis of the current +architecture and the proposed changes. We encourage our community and contributors to +review them: + +1. [Analysis of the Current Architecture](redesign/1_architecture_existing.md): A + breakdown of the existing design and its challenges. +2. [The Proposed Redesign](redesign/2_architecture_redesign.md): An overview of the + new three-layered architecture. +3. [Implementation Plan](redesign/3_architecture_implementation.md): The step-by-step + plan for implementing the redesign. + +Your feedback is welcome! Please feel free to open an issue to discuss the proposed +changes. + +> **DON'T PANIC!** +> +> While this is a major internal refactoring, our goal is to keep the primary public +API on the main repository object as stable as possible. Most users who rely on +documented methods like `g.commit`, `g.add`, and `g.status` should find the +transition to v5.0.0 straightforward. +> +> The breaking changes will primarily affect users who have been relying on the +internal g.lib accessor, which will be removed as part of this cleanup. For more +details, please see the "Impact on Users" section in [the redesign +document](redesign/2_architecture_redesign.md). + ## 📢 We Now Use RuboCop 📢 To improve code consistency and maintainability, the `ruby-git` project has now diff --git a/redesign/1_architecture_existing.md b/redesign/1_architecture_existing.md new file mode 100644 index 00000000..36c4d2f5 --- /dev/null +++ b/redesign/1_architecture_existing.md @@ -0,0 +1,66 @@ +# Analysis of the Current Git Gem Architecture and Its Challenges + +This document provides an in-depth look at the current architecture of the `git` gem, outlining its primary components and the design challenges that have emerged over time. Understanding these challenges is the key motivation for the proposed architectural redesign. + +- [1. Overview of the Current Architecture](#1-overview-of-the-current-architecture) +- [2. Key Architectural Challenges](#2-key-architectural-challenges) + - [A. Unclear Separation of Concerns](#a-unclear-separation-of-concerns) + - [B. Circular Dependency](#b-circular-dependency) + - [C. Undefined Public API Boundary](#c-undefined-public-api-boundary) + - [D. Slow and Brittle Test Suite](#d-slow-and-brittle-test-suite) + +## 1. Overview of the Current Architecture + +The gem's current design is centered around three main classes: `Git`, `Git::Base`, and `Git::Lib`. + +- **`Git` (Top-Level Module)**: This module serves as the primary public entry point for creating repository objects. It contains class-level factory methods like `Git.open`, `Git.clone`, and `Git.init`. It also provides an interface for accessing global git configuration settings. + +**`Git::Base`**: This is the main object that users interact with after creating or opening a repository. It holds the high-level public API for most git operations (e.g., `g.commit`, `g.add`, `g.status`). It is responsible for managing the repository's state, such as the paths to the working directory and the `.git` directory. + +**`Git::Lib`**: This class is intended to be the low-level wrapper around the `git` command-line tool. It contains the methods that build the specific command-line arguments and execute the `git` binary. In practice, it also contains a significant amount of logic for parsing the output of these commands. + +## 2. Key Architectural Challenges + +While this structure has been functional, several significant design challenges make the codebase difficult to maintain, test, and evolve. + +### A. Unclear Separation of Concerns + +The responsibilities between Git::Base and Git::Lib are "muddy" and overlap significantly. + +- `Git::Base` sometimes contains logic that feels like it should be lower-level. + +- `Git::Lib`, which should ideally only be concerned with command execution, is filled with high-level logic for parsing command output into specific Ruby objects (e.g., parsing log output, diff stats, and branch lists). + +This blending of responsibilities makes it hard to determine where a specific piece of logic should reside, leading to an inconsistent and confusing internal structure. + +### B. Circular Dependency + +This is the most critical architectural flaw in the current design. + +- A `Git::Base` instance is created. + +- The first time a command is run, `Git::Base` lazily initializes a `Git::Lib` instance via its `.lib` accessor method. + +- The `Git::Lib` constructor is passed the `Git::Base` instance (`self`) so that it can read the repository's path configuration back from the object that is creating it. + +This creates a tight, circular coupling: `Git::Base` depends on `Git::Lib` to execute commands, but `Git::Lib` depends on `Git::Base` for its own configuration. This pattern makes the classes difficula to instantiate or test in isolation and creates a fragile system where changes in one class can have unexpected side effects in the other. + +### C. Undefined Public API Boundary + +The gem lacks a formally defined public interface. Because `Git::Base` exposes its internal `Git::Lib` instance via the public `g.lib` accessor, many users have come to rely on `Git::Lib` and its methods as if they were part of the public API. + +This has two negative consequences: + +1. It prevents the gem's maintainers from refactoring or changing the internal implementation of `Git::Lib` without causing breaking changes for users. + +2. It exposes complex, internal methods to users, creating a confusing and inconsistent user experience. + +### D. Slow and Brittle Test Suite + +The current testing strategy, built on `TestUnit`, suffers from two major issues: + +- **Over-reliance on Fixtures**: Most tests depend on having a complete, physical git repository on the filesystem to run against. Managing these fixtures is cumbersome. + +- **Excessive Shelling Out**: Because the logic for command execution and output parsing are tightly coupled, nearly every test must shell out to the actual `git` command-line tool. + +This makes the test suite extremely slow, especially on non-UNIX platforms like Windows where process creation is more expensive. The slow feedback loop discourages frequent testing and makes development more difficult. The brittleness of filesystem-dependent tests also leads to flickering or unreliable test runs. \ No newline at end of file diff --git a/redesign/2_architecture_redesign.md b/redesign/2_architecture_redesign.md new file mode 100644 index 00000000..a7be3237 --- /dev/null +++ b/redesign/2_architecture_redesign.md @@ -0,0 +1,130 @@ +# Proposed Redesigned Architecture for the Git Gem + +This issue outlines a proposal for a major redesign of the git gem, targeted for version 5.0.0. The goal of this redesign is to modernize the gem's architecture, making it more robust, maintainable, testable, and easier for new contributors to understand. + +- [1. Motivation](#1-motivation) +- [2. The New Architecture: A Three-Layered Approach](#2-the-new-architecture-a-three-layered-approach) +- [3. Key Design Principles](#3-key-design-principles) + - [A. Clear Public vs. Private API](#a-clear-public-vs-private-api) + - [B. Dependency Injection](#b-dependency-injection) + - [C. Immutable Return Values](#c-immutable-return-values) + - [D. Clear Naming for Path Objects](#d-clear-naming-for-path-objects) +- [4. Testing Strategy Overhaul](#4-testing-strategy-overhaul) +- [5. Impact on Users: Breaking Changes for v5.0.0](#5-impact-on-users-breaking-changes-for-v500) + +## 1. Motivation + +The current architecture, while functional, has several design issues that have accrued over time, making it difficult to extend and maintain. + +- **Unclear Separation of Concerns**: The responsibilities of the `Git`, `Git::Base`, and `Git::Lib` classes are "muddy." `Git::Base` acts as both a high-level API and a factory, while `Git::Lib` contains a mix of low-level command execution and high-level output parsing. + +- **Circular Dependency**: A key architectural flaw is the circular dependency between `Git::Base` and `Git::Lib`. `Git::Base` creates and depends on `Git::Lib`, but `Git::Lib`'s constructor requires an instance of Git::Base to access configuration. This tight coupling makes the classes difficult to reason about and test in isolation. + +- **Undefined Public API**: The boundary between the gem's public API and its internal implementation is not clearly defined. This has led some users to rely on internal classes like `Git::Lib`, making it difficult to refactor the internals without introducing breaking changes. + +- **Slow and Brittle Test Suite**: The current tests rely heavily on filesystem fixtures and shelling out to the git command line for almost every test case. This makes the test suite slow and difficult to maintain, especially on non-UNIX platforms. + +## 2. The New Architecture: A Three-Layered Approach + +The new design is built on a clear separation of concerns, dividing responsibilities into three distinct layers: a Facade, an Execution Context, and Command Objects. + +1. The Facade Layer: Git::Repository + + This is the primary public interface that users will interact with. + + **Renaming**: `Git::Base` will be renamed to `Git::Repository`. This name is more descriptive and intuitive. + + **Responsibility**: It will serve as a clean, high-level facade for all common git operations. Its methods will be simple, one-line calls that delegate the actual work to an appropriate command object. + + **Scalability**: To prevent this class from growing too large, its methods will be organized into logical modules (e.g., `Git::Repository::Branching`, `Git::Repository::History`) which are then included in the main class. This keeps the core class definition small and the features well-organized. These categories will be inspired by (but not slavishly follow) the git command line reference in [this page](https://git-scm.com/docs). + +2. The Execution Layer: Git::ExecutionContext + + This is the low-level, private engine for running commands. + + **Renaming**: `Git::Lib` will be renamed to `Git::ExecutionContext`. + + **Responsibility**: Its sole purpose is to execute raw git commands. It will manage the repository's environment (working directory, .git path, logger) and use the existing `Git::CommandLine` class to interact with the system's git binary. It will have no knowledge of any specific git command's arguments or output. + +3. The Logic Layer: Git::Commands + + This is where all the command-specific implementation details will live. + + **New Classes**: For each git operation, a new command class will be created within the `Git::Commands` namespace (e.g., `Git::Commands::Commit`, `Git::Commands::Diff`). + + **Dual Responsibility**: Each command class will be responsible for: + + 1. **Building Arguments**: Translating high-level Ruby options into the specific command-line flags and arguments that git expects. + + 2. **Parsing Output**: Taking the raw string output from the ExecutionContext and converting it into rich, structured Ruby objects. + + **Handling Complexity**: For commands with multiple behaviors (like git diff), we can use specialized subclasses (e.g., Git::Commands::Diff::NameStatus, Git::Commands::Diff::Stats) to keep each class focused on a single responsibility. + +## 3. Key Design Principles + +The new architecture will be guided by the following modern design principles. + +### A. Clear Public vs. Private API + +A primary goal of this redesign is to establish a crisp boundary between the public API and internal implementation details. + +- **Public Interface**: The public API will consist of the `Git` module (for factories), the `Git::Repository` class, and the specialized data/query objects it returns (e.g., `Git::Log`, `Git::Status`, `Git::Object::Commit`). + +- **Private Implementation**: All other components, including `Git::ExecutionContext` and all classes within the `Git::Commands` namespace, will be considered internal. They will be explicitly marked with the `@api private` YARD tag to discourage external use. + +### B. Dependency Injection + +The circular dependency will be resolved by implementing a clear, one-way dependency flow. + +1. The factory methods (`Git.open`, `Git.clone`) will create and configure an instance of `Git::ExecutionContext`. + +2. This `ExecutionContext` instance will then be injected into the constructor of the `Git::Repository` object. + +This decouples the `Repository` from its execution environment, making the system more modular and easier to test. + +### C. Immutable Return Values + +To create a more predictable and robust API, methods will return structured, immutable data objects instead of raw strings or hashes. + +This will be implemented using `Data.define` or simple, frozen `Struct`s. + +For example, instead of returning a raw string, `repo.config('user.name')` will return a `Git::Config::Value` object containing the key, value, scope, and source path. + +### D. Clear Naming for Path Objects + +To improve clarity, all classes that represent filesystem paths will be renamed to follow a consistent `...Path` suffix convention. + +- `Git::WorkingDirectory` -> `Git::WorkingTreePath` + +- `Git::Index` -> `Git::IndexPath` + +- The old `Git::Repository` (representing the .git directory/file) -> `Git::RepositoryPath` + +## 4. Testing Strategy Overhaul + +The test suite will be modernized to be faster, more reliable, and easier to work with. + +- **Migration to RSpec**: The entire test suite will be migrated from TestUnit to RSpec to leverage its modern tooling and expressive DSL. + +- **Layered Testing**: A three-layered testing strategy will be adopted: + + 1. **Unit Tests**: The majority of tests will be fast, isolated unit tests for the `Command` classes, using mock `ExecutionContexts`. + + 2. **Integration Tests**: A small number of integration tests will verify that `ExecutionContext` correctly interacts with the system's `git` binary. + + 3. **Feature Tests**: A minimal set of high-level tests will ensure the public facade on `Git::Repository` works end-to-end. + +- **Reduced Filesystem Dependency**: This new structure will dramatically reduce the suite's reliance on slow and brittle filesystem fixtures. + +## 5. Impact on Users: Breaking Changes for v5.0.0 + +This redesign is a significant undertaking and will be released as version 5.0.0. It includes several breaking changes that users will need to be aware of when upgrading. + +- **`Git::Lib` is Removed**: Any code directly referencing `Git::Lib` will break. + +- **g.lib Accessor is Removed**: The `.lib` accessor on repository objects will be removed. + +- **Internal Methods Relocated**: Methods that were previously accessible via g.lib will now be private implementation details of the new command classes and will not be directly reachable. + +Users should only rely on the newly defined public interface. + diff --git a/redesign/3_architecture_implementation.md b/redesign/3_architecture_implementation.md new file mode 100644 index 00000000..c4dc28da --- /dev/null +++ b/redesign/3_architecture_implementation.md @@ -0,0 +1,138 @@ +# Implementation Plan for Git Gem Redesign (v5.0.0) + +This document outlines a step-by-step plan to implement the proposed architectural redesign. The plan is structured to be incremental, ensuring that the gem remains functional and passes its test suite after each major step. This approach minimizes risk and allows for a gradual, controlled migration to the new architecture. + +- [Phase 1: Foundation and Scaffolding](#phase-1-foundation-and-scaffolding) +- [Phase 2: The Strangler Fig Pattern - Migrating Commands](#phase-2-the-strangler-fig-pattern---migrating-commands) +- [Phase 3: Refactoring the Public Interface](#phase-3-refactoring-the-public-interface) +- [Phase 4: Final Cleanup and Release Preparation](#phase-4-final-cleanup-and-release-preparation) + +## Phase 1: Foundation and Scaffolding + +***Goal**: Set up the new file structure and class names without altering existing logic. The gem will be fully functional after this phase.* + +1. **Create New Directory Structure**: + + - Create the new directories that will house the refactored components: + + - `lib/git/commands/` + + - `lib/git/repository/` (for the facade modules) + +2. **Rename Path Classes**: + + - Perform a project-wide, safe rename of the existing path-related classes. This is a low-risk mechanical change. + + - `Git::WorkingDirectory` -> `Git::WorkingTreePath` + + - `Git::Index` -> `Git::IndexPath` + + - `Git::Repository` -> `Git::RepositoryPath` + + - Run the test suite to ensure everything still works as expected. + +3. **Introduce New Core Classes (Empty Shells)**: + + - Create the new `Git::ExecutionContext` class in `lib/git/execution_context.rb`. For now, its implementation can be a simple shell or a thin wrapper around the existing `Git::Lib`. + + - Create the new `Git::Repository` class in `lib/git/repository.rb`. This will initially be an empty class. + +4. **Set Up RSpec Environment**: + + - Add rspec dependencies to the `Gemfile` as a development dependency. + + - Configure the test setup to allow both TestUnit and RSpec tests to run concurrently. + +## Phase 2: The Strangler Fig Pattern - Migrating Commands + +***Goal**: Incrementally move the implementation of each git command from `Git::Lib` to a new `Command` class, strangling the old implementation one piece at a time using a Test-Driven Development workflow.* + +- **1. Migrate the First Command (`config`)**: + + - **Write Unit Tests First**: Write comprehensive RSpec unit tests for the *proposed* `Git::Commands::Config` class. These tests will fail initially because the class doesn't exist yet. The tests should be fast and mock the `ExecutionContext`. + + - **Create Command Class**: Implement `Git::Commands::Config` to make the tests pass. This class will contain all the logic for building git config arguments and parsing its output. It will accept an `ExecutionContext` instance in its constructor. + + - **Delegate from `Git::Lib`**: Modify the `config_*` methods within the existing `Git::Lib` class. Instead of containing the implementation, they will now instantiate and call the new `Git::Commands::Config` object. + + - **Verify**: Run the full test suite (both TestUnit and RSpec). The existing tests for `g.config` should still pass, but they will now be executing the new, refactored code. + +- **2. Incrementally Migrate Remaining Commands:** + + - Repeat the process from the previous step for all other commands, one by one or in logical groups (e.g., all `diff` related commands, then all `log` commands). + + - For each command (`add`, `commit`, `log`, `diff`, `status`, etc.): + + 1. Create the corresponding Git::Commands::* class. + + 2. Write isolated RSpec unit tests for the new class. + + 3. Change the method in Git::Lib to delegate to the new command object. + + 4. Run the full test suite to ensure no regressions have been introduced. + +## Phase 3: Refactoring the Public Interface + +***Goal**: Switch the public-facing classes to use the new architecture directly, breaking the final ties to the old implementation.* + +1. **Refactor Factory Methods**: + + - Modify the factory methods in the top-level `Git` module (`.open`, `.clone`, etc.). + + - These methods will now be responsible for creating an instance of `Git::ExecutionContext` and injecting it into the constructor of a `Git::Repository` object. + + The return value of these factories will now be a `Git::Repository` instance, not a `Git::Base` instance. + +2. **Implement the Facade**: + + - Populate the `Git::Repository` class with the simple, one-line facade methods that delegate to the `Command` objects. For example: + + ```ruby + def commit(msg) + Git::Commands::Commit.new(@execution_context, msg).run + end + ``` + + - Organize these facade methods into modules as planned (`lib/git/repository/branching.rb`, etc.) and include them in the main `Git::Repository` class. + +3. **Deprecate and Alias `Git::Base`**: + + - To maintain a degree of backward compatibility through the transition, make `Git::Base` a deprecated constant that points to `Git::Repository`. + + ```ruby + Git::Base = ActiveSupport::Deprecation::DeprecatedConstantProxy.new( + 'Git::Base', + 'Git::Repository', + Git::Deprecation + ) + ``` + + - This ensures that any user code checking `is_a?(Git::Base)` will not immediately break. + +## Phase 4: Final Cleanup and Release Preparation + +***Goal**: Remove all old code, finalize the test suite, and prepare for the v5.0.0 release.* + +1. **Remove Old Code**: + + - Delete the `Git::Lib` class entirely. + + - Delete the `Git::Base` class file and remove the deprecation proxy. + + - Remove any other dead code that was part of the old implementation. + +2. **Finalize Test Suite**: + + - Convert any remaining, relevant TestUnit tests to RSpec. + + - Remove the `test-unit` dependency from the `Gemfile`. + + - Ensure the RSpec suite has comprehensive coverage for the new architecture. + +3. **Update Documentation**: + + - Thoroughly document the new public API (`Git`, `Git::Repository`, etc.). + + - Mark all internal classes (`ExecutionContext`, `Commands`, `*Path`) with `@api private` in the YARD documentation. + + - Update the `README.md` and create a `UPGRADING.md` guide explaining the breaking changes for v5.0.0.