From 9918ab09050a89054b5ad5f928e4c1dcd1a2bfc8 Mon Sep 17 00:00:00 2001 From: Aleksey Kulikov Date: Tue, 25 Jan 2022 20:16:46 +0300 Subject: [PATCH] refactor(checkout)!: move checkout related methods into Checkout class (#42) --- lib/libgit2dart.dart | 1 + lib/src/bindings/checkout.dart | 2 - lib/src/checkout.dart | 161 +++++++++++++++++++++++++++++++++ lib/src/repository.dart | 91 ------------------- test/blob_test.dart | 2 +- test/checkout_test.dart | 77 ++++++++++------ test/diff_test.dart | 12 +-- test/index_test.dart | 9 +- test/merge_test.dart | 3 +- test/rebase_test.dart | 15 ++- 10 files changed, 238 insertions(+), 135 deletions(-) create mode 100644 lib/src/checkout.dart diff --git a/lib/libgit2dart.dart b/lib/libgit2dart.dart index ff118d8..2679124 100644 --- a/lib/libgit2dart.dart +++ b/lib/libgit2dart.dart @@ -3,6 +3,7 @@ export 'src/blame.dart'; export 'src/blob.dart'; export 'src/branch.dart'; export 'src/callbacks.dart'; +export 'src/checkout.dart'; export 'src/commit.dart'; export 'src/config.dart'; export 'src/credentials.dart'; diff --git a/lib/src/bindings/checkout.dart b/lib/src/bindings/checkout.dart index a41861e..e3849d2 100644 --- a/lib/src/bindings/checkout.dart +++ b/lib/src/bindings/checkout.dart @@ -83,8 +83,6 @@ void index({ /// Updates files in the index and working tree to match the content of the tree /// pointed at by the treeish. -/// -/// Throws a [LibGit2Error] if error occured. void tree({ required Pointer repoPointer, required Pointer treeishPointer, diff --git a/lib/src/checkout.dart b/lib/src/checkout.dart new file mode 100644 index 0000000..eb4aa5c --- /dev/null +++ b/lib/src/checkout.dart @@ -0,0 +1,161 @@ +import 'package:libgit2dart/libgit2dart.dart'; +import 'package:libgit2dart/src/bindings/checkout.dart' as bindings; +import 'package:libgit2dart/src/bindings/object.dart' as object_bindings; + +class Checkout { + const Checkout._(); + + /// Updates files in the index and the working tree to match the content of + /// the commit pointed at by HEAD. + /// + /// Note that this is **not** the correct mechanism used to switch branches; + /// do not change your HEAD and then call this method, that would leave you + /// with checkout conflicts since your working directory would then appear + /// to be dirty. Instead, checkout the target of the branch and then update + /// HEAD using [Repository]'s `setHead` to point to the branch you checked + /// out. + /// + /// [repo] is the repository into which to check out (must be non-bare). + /// + /// Default checkout [strategy] is combination of [GitCheckout.safe] and + /// [GitCheckout.recreateMissing]. + /// + /// [directory] is optional alternative checkout path to workdir. + /// + /// [paths] is optional list of files to checkout from index. + /// + /// Throws a [LibGit2Error] if error occured. + static void head({ + required Repository repo, + Set strategy = const { + GitCheckout.safe, + GitCheckout.recreateMissing + }, + String? directory, + List? paths, + }) { + bindings.head( + repoPointer: repo.pointer, + strategy: strategy.fold(0, (int acc, e) => acc | e.value), + directory: directory, + paths: paths, + ); + } + + /// Updates files in the working tree to match the content of the index. + /// + /// [repo] is the repository into which to check out (must be non-bare). + /// + /// Default checkout [strategy] is combination of [GitCheckout.safe] and + /// [GitCheckout.recreateMissing]. + /// + /// [directory] is optional alternative checkout path to workdir. + /// + /// [paths] is optional list of files to checkout from index. + /// + /// Throws a [LibGit2Error] if error occured. + static void index({ + required Repository repo, + Set strategy = const { + GitCheckout.safe, + GitCheckout.recreateMissing + }, + String? directory, + List? paths, + }) { + bindings.index( + repoPointer: repo.pointer, + strategy: strategy.fold(0, (int acc, e) => acc | e.value), + directory: directory, + paths: paths, + ); + } + + /// Updates files in the working tree to match the content of the tree + /// pointed at by the reference [name] target. + /// + /// [repo] is the repository into which to check out (must be non-bare). + /// + /// [name] is the fully-qualified reference name (e.g. 'refs/heads/master') + /// which target's content will be used to update the working directory; + /// + /// Default checkout [strategy] is combination of [GitCheckout.safe] and + /// [GitCheckout.recreateMissing]. + /// + /// [directory] is optional alternative checkout path to workdir. + /// + /// [paths] is optional list of files to checkout from index. + /// + /// Throws a [LibGit2Error] if error occured. + static void reference({ + required Repository repo, + required String name, + Set strategy = const { + GitCheckout.safe, + GitCheckout.recreateMissing + }, + String? directory, + List? paths, + }) { + final ref = Reference.lookup(repo: repo, name: name); + final treeish = object_bindings.lookup( + repoPointer: repo.pointer, + oidPointer: ref.target.pointer, + type: GitObject.any.value, + ); + + bindings.tree( + repoPointer: repo.pointer, + treeishPointer: treeish, + strategy: strategy.fold(0, (int acc, e) => acc | e.value), + directory: directory, + paths: paths, + ); + + object_bindings.free(treeish); + ref.free(); + } + + /// Updates files in the working tree to match the content of the tree + /// pointed at by the [commit]. + /// + /// [repo] is the repository into which to check out (must be non-bare). + /// + /// [commit] is the commit which content will be used to update the working + /// directory. + /// + /// Default checkout [strategy] is combination of [GitCheckout.safe] and + /// [GitCheckout.recreateMissing]. + /// + /// [directory] is optional alternative checkout path to workdir. + /// + /// [paths] is optional list of files to checkout from index. + /// + /// Throws a [LibGit2Error] if error occured. + static void commit({ + required Repository repo, + required Commit commit, + Set strategy = const { + GitCheckout.safe, + GitCheckout.recreateMissing + }, + String? directory, + List? paths, + }) { + final treeish = object_bindings.lookup( + repoPointer: repo.pointer, + oidPointer: commit.oid.pointer, + type: GitObject.any.value, + ); + + bindings.tree( + repoPointer: repo.pointer, + treeishPointer: treeish, + strategy: strategy.fold(0, (int acc, e) => acc | e.value), + directory: directory, + paths: paths, + ); + + object_bindings.free(treeish); + } +} diff --git a/lib/src/repository.dart b/lib/src/repository.dart index 72b9986..d82799e 100644 --- a/lib/src/repository.dart +++ b/lib/src/repository.dart @@ -4,7 +4,6 @@ import 'package:ffi/ffi.dart'; import 'package:libgit2dart/libgit2dart.dart'; import 'package:libgit2dart/src/bindings/attr.dart' as attr_bindings; -import 'package:libgit2dart/src/bindings/checkout.dart' as checkout_bindings; import 'package:libgit2dart/src/bindings/describe.dart' as describe_bindings; import 'package:libgit2dart/src/bindings/graph.dart' as graph_bindings; import 'package:libgit2dart/src/bindings/libgit2_bindings.dart'; @@ -552,96 +551,6 @@ class Repository { } } - /// Checkouts the provided [target] using the given strategy, and updates the - /// HEAD. - /// - /// [target] can be null, 'HEAD', reference name or commit [Oid]. - /// - /// If no [target] is given, updates files in the working tree to match the - /// content of the index. - /// - /// If [target] is 'HEAD' string, updates files in the index and the working - /// tree to match the content of the commit pointed at by HEAD. - /// - /// If [target] is reference name, updates files in the index and working - /// tree to match the content of the tree pointed at by the reference. - /// - /// If [target] is commit oid, updates files in the index and working - /// tree to match the content of the tree pointed at by the commit. - /// - /// Default checkout strategy is combination of [GitCheckout.safe] and - /// [GitCheckout.recreateMissing]. - /// - /// [directory] is alternative checkout path to workdir. - /// - /// [paths] is list of files to checkout from provided [target]. - /// If paths are provided, HEAD will not be set to the [target]. - /// - /// Throws [ArgumentError] if provided [target] is not String or [Oid]. - void checkout({ - Object? target, - Set strategy = const { - GitCheckout.safe, - GitCheckout.recreateMissing - }, - String? directory, - List? paths, - }) { - final strat = strategy.fold(0, (int acc, e) => acc | e.value); - - if (target == null) { - checkout_bindings.index( - repoPointer: _repoPointer, - strategy: strat, - directory: directory, - paths: paths, - ); - } else if (target == 'HEAD') { - checkout_bindings.head( - repoPointer: _repoPointer, - strategy: strat, - directory: directory, - paths: paths, - ); - } else if (target is String) { - final ref = Reference.lookup(repo: this, name: target); - final treeish = object_bindings.lookup( - repoPointer: _repoPointer, - oidPointer: ref.target.pointer, - type: GitObject.any.value, - ); - checkout_bindings.tree( - repoPointer: _repoPointer, - treeishPointer: treeish, - strategy: strat, - directory: directory, - paths: paths, - ); - if (paths == null) { - setHead(target); - } - ref.free(); - } else if (target is Oid) { - final treeish = object_bindings.lookup( - repoPointer: _repoPointer, - oidPointer: target.pointer, - type: GitObject.any.value, - ); - checkout_bindings.tree( - repoPointer: _repoPointer, - treeishPointer: treeish, - strategy: strat, - directory: directory, - paths: paths, - ); - if (paths == null) { - setHead(target); - } - } else { - throw ArgumentError.value('$target must be either String or Oid'); - } - } - /// Sets the current head to the specified commit [oid] and optionally resets /// the index and working tree to match. /// diff --git a/test/blob_test.dart b/test/blob_test.dart index 286d386..0fdbe4d 100644 --- a/test/blob_test.dart +++ b/test/blob_test.dart @@ -134,7 +134,7 @@ void main() { }); test('filters content of a blob with provided commit for attributes', () { - repo.checkout(target: 'refs/tags/v0.2'); + Checkout.reference(repo: repo, name: 'refs/tags/v0.2'); final blobOid = Blob.create(repo: repo, content: 'clrf\nclrf\n'); final blob = Blob.lookup(repo: repo, oid: blobOid); diff --git a/test/checkout_test.dart b/test/checkout_test.dart index fd14cc7..4c6b318 100644 --- a/test/checkout_test.dart +++ b/test/checkout_test.dart @@ -27,8 +27,8 @@ void main() { File(p.join(tmpDir.path, 'feature_file')).writeAsStringSync('edit'); expect(repo.status, contains('feature_file')); - repo.checkout( - target: 'HEAD', + Checkout.head( + repo: repo, strategy: {GitCheckout.force}, paths: ['feature_file'], ); @@ -39,8 +39,8 @@ void main() { 'throws when trying to checkout head with invalid alternative ' 'directory', () { expect( - () => repo.checkout( - target: 'HEAD', + () => Checkout.head( + repo: repo, directory: 'not/there', ), throwsA(isA()), @@ -51,7 +51,8 @@ void main() { File(p.join(repo.workdir, 'feature_file')).writeAsStringSync('edit'); expect(repo.status, contains('feature_file')); - repo.checkout( + Checkout.index( + repo: repo, strategy: { GitCheckout.force, GitCheckout.conflictStyleMerge, @@ -65,7 +66,7 @@ void main() { 'throws when trying to checkout index with invalid alternative ' 'directory', () { expect( - () => repo.checkout(directory: 'not/there'), + () => Checkout.index(repo: repo, directory: 'not/there'), throwsA(isA()), ); }); @@ -78,12 +79,12 @@ void main() { false, ); - repo.checkout(target: 'refs/heads/feature'); + Checkout.reference(repo: repo, name: 'refs/heads/feature'); final featureHead = Commit.lookup(repo: repo, oid: repo['5aecfa0']); final featureTree = featureHead.tree; final repoHead = repo.head; - expect(repoHead.target, featureHead.oid); - expect(repo.status, isEmpty); + // does not change HEAD + expect(repoHead.target, isNot(featureHead.oid)); expect( featureTree.entries.any((e) => e.name == 'another_feature_file'), true, @@ -100,8 +101,9 @@ void main() { 'throws when trying to checkout reference with invalid alternative ' 'directory', () { expect( - () => repo.checkout( - target: 'refs/heads/feature', + () => Checkout.reference( + repo: repo, + name: 'refs/heads/feature', directory: 'not/there', ), throwsA(isA()), @@ -113,11 +115,11 @@ void main() { expect(index.find('another_feature_file'), equals(false)); final featureHead = Commit.lookup(repo: repo, oid: repo['5aecfa0']); - repo.checkout(target: featureHead.oid); + Checkout.commit(repo: repo, commit: featureHead); final repoHead = repo.head; - expect(repoHead.target, featureHead.oid); - expect(repo.status, isEmpty); + // does not change HEAD + expect(repoHead.target, isNot(featureHead.oid)); expect(index.find('another_feature_file'), equals(true)); repoHead.free(); @@ -127,10 +129,14 @@ void main() { test('checkouts commit with provided path', () { final featureHead = Commit.lookup(repo: repo, oid: repo['5aecfa0']); - repo.checkout(target: featureHead.oid, paths: ['another_feature_file']); + Checkout.commit( + repo: repo, + commit: featureHead, + paths: ['another_feature_file'], + ); final repoHead = repo.head; - // When path is provided HEAD will not be set to target; + // does not change HEAD expect(repoHead.target, isNot(featureHead.oid)); expect( repo.status, @@ -143,6 +149,23 @@ void main() { featureHead.free(); }); + test( + 'throws when trying to checkout commit with invalid alternative ' + 'directory', () { + final commit = Commit.lookup(repo: repo, oid: repo['5aecfa0']); + + expect( + () => Checkout.commit( + repo: repo, + commit: commit, + directory: 'not/there', + ), + throwsA(isA()), + ); + + commit.free(); + }); + test('checkouts with alrenative directory', () { final altDir = Directory(p.join(Directory.systemTemp.path, 'alt_dir')); // making sure there is no directory @@ -152,7 +175,11 @@ void main() { altDir.createSync(); expect(altDir.listSync().length, 0); - repo.checkout(target: 'refs/heads/feature', directory: altDir.path); + Checkout.reference( + repo: repo, + name: 'refs/heads/feature', + directory: altDir.path, + ); expect(altDir.listSync().length, isNot(0)); altDir.deleteSync(recursive: true); @@ -165,8 +192,9 @@ void main() { ).target; expect(repo.status, isEmpty); - repo.checkout( - target: 'refs/heads/feature', + Checkout.reference( + repo: repo, + name: 'refs/heads/feature', paths: ['another_feature_file'], ); expect( @@ -175,7 +203,7 @@ void main() { 'another_feature_file': {GitStatus.indexNew} }, ); - // When path is provided HEAD will not be set to target; + // does not change HEAD expect(repo.head.target, isNot(featureTip)); }); @@ -185,8 +213,9 @@ void main() { final file = File(p.join(repo.workdir, 'another_feature_file')); expect(file.existsSync(), false); - repo.checkout( - target: 'refs/heads/feature', + Checkout.reference( + repo: repo, + name: 'refs/heads/feature', strategy: {GitCheckout.dryRun}, ); expect(index.length, 4); @@ -194,9 +223,5 @@ void main() { index.free(); }); - - test('throws when provided target is not String or Oid', () { - expect(() => repo.checkout(target: 1), throwsA(isA())); - }); }); } diff --git a/test/diff_test.dart b/test/diff_test.dart index 0c9044a..1d27832 100644 --- a/test/diff_test.dart +++ b/test/diff_test.dart @@ -323,7 +323,7 @@ index e69de29..c217c63 100644 ); final diff2 = Diff.parse(patchText); - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); expect( diff2.applies(repo: repo, location: GitApplyLocation.both), true, @@ -345,7 +345,7 @@ index e69de29..c217c63 100644 final diff2 = Diff.parse(patchText); final hunk = diff2.patches.first.hunks.first; - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); expect( diff2.applies( repo: repo, @@ -364,7 +364,7 @@ index e69de29..c217c63 100644 final diff = Diff.parse(patchText); final file = File(p.join(tmpDir.path, 'subdir', 'modified_file')); - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); expect(file.readAsStringSync(), ''); diff.apply(repo: repo); @@ -396,7 +396,7 @@ index e69de29..c217c63 100644 final hunk = diff.patches.first.hunks.first; final file = File(p.join(tmpDir.path, 'subdir', 'modified_file')); - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); expect(file.readAsStringSync(), ''); diff.apply(repo: repo, hunkIndex: hunk.index); @@ -408,7 +408,7 @@ index e69de29..c217c63 100644 test('applies diff to tree', () { final diff = Diff.parse(patchText); - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); final head = repo.head; final commit = Commit.lookup(repo: repo, oid: head.target); final tree = commit.tree; @@ -441,7 +441,7 @@ index e69de29..c217c63 100644 final diff = Diff.parse(patchText); final hunk = diff.patches.first.hunks.first; - repo.checkout(target: 'HEAD', strategy: {GitCheckout.force}); + Checkout.head(repo: repo, strategy: {GitCheckout.force}); final head = repo.head; final commit = Commit.lookup(repo: repo, oid: head.target); final tree = commit.tree; diff --git a/test/index_test.dart b/test/index_test.dart index 9dc6317..608e316 100644 --- a/test/index_test.dart +++ b/test/index_test.dart @@ -364,7 +364,8 @@ void main() { oid: conflictBranch.target, ); - conflictRepo.checkout(target: 'refs/heads/feature'); + Checkout.reference(repo: conflictRepo, name: 'refs/heads/feature'); + conflictRepo.setHead('refs/heads/feature'); Merge.commit(repo: conflictRepo, commit: commit); @@ -424,7 +425,8 @@ void main() { oid: conflictBranch.target, ); - conflictRepo.checkout(target: 'refs/heads/our-conflict'); + Checkout.reference(repo: conflictRepo, name: 'refs/heads/our-conflict'); + conflictRepo.setHead('refs/heads/our-conflict'); Merge.commit(repo: conflictRepo, commit: commit); @@ -455,7 +457,8 @@ void main() { oid: conflictBranch.target, ); - conflictRepo.checkout(target: 'refs/heads/feature'); + Checkout.reference(repo: conflictRepo, name: 'refs/heads/feature'); + conflictRepo.setHead('refs/heads/feature'); Merge.commit(repo: conflictRepo, commit: commit); diff --git a/test/merge_test.dart b/test/merge_test.dart index 34e25e6..7de6a58 100644 --- a/test/merge_test.dart +++ b/test/merge_test.dart @@ -172,7 +172,8 @@ Another feature edit repo: repo, oid: conflictBranch.target, ); - repo.checkout(target: 'refs/heads/feature'); + Checkout.reference(repo: repo, name: 'refs/heads/feature'); + repo.setHead('refs/heads/feature'); final index = repo.index; Merge.commit(repo: repo, commit: commit); diff --git a/test/rebase_test.dart b/test/rebase_test.dart index 69b7085..f63bb78 100644 --- a/test/rebase_test.dart +++ b/test/rebase_test.dart @@ -43,7 +43,8 @@ void main() { reference: feature, ); - repo.checkout(target: feature.name); + Checkout.reference(repo: repo, name: feature.name); + repo.setHead(feature.name); expect(() => repo.index['.gitignore'], throwsA(isA())); final rebase = Rebase.init( @@ -141,7 +142,8 @@ void main() { final feature = Reference.lookup(repo: repo, name: 'refs/heads/feature'); final upstream = AnnotatedCommit.lookup(repo: repo, oid: repo[shas[1]]); - repo.checkout(target: feature.name); + Checkout.reference(repo: repo, name: feature.name); + repo.setHead(feature.name); expect(() => repo.index['conflict_file'], throwsA(isA())); final rebase = Rebase.init( @@ -197,7 +199,8 @@ void main() { ); final ontoHead = AnnotatedCommit.lookup(repo: repo, oid: conflict.target); - repo.checkout(target: conflict.name); + Checkout.reference(repo: repo, name: conflict.name); + repo.setHead(conflict.name); final rebase = Rebase.init( repo: repo, @@ -237,7 +240,8 @@ void main() { ); final ontoHead = AnnotatedCommit.lookup(repo: repo, oid: conflict.target); - repo.checkout(target: conflict.name); + Checkout.reference(repo: repo, name: conflict.name); + repo.setHead(conflict.name); final rebase = Rebase.init( repo: repo, @@ -266,7 +270,8 @@ void main() { ); final ontoHead = AnnotatedCommit.lookup(repo: repo, oid: conflict.target); - repo.checkout(target: conflict.name); + Checkout.reference(repo: repo, name: conflict.name); + repo.setHead(conflict.name); final rebase = Rebase.init( repo: repo,