Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453213002:
Add support for running git in tests (Closed)
|
Created:
5 years, 1 month ago by Brian Wilkerson Modified:
5 years, 1 month ago Reviewers:
Paul Berry CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd support for running git in tests
Committed: https://github.com/dart-lang/sdk/commit/905cb67e99ba113b9be72d25baf68f77b2fcd423
Patch Set 1 #
Total comments: 26
Messages
Total messages: 6 (1 generated)
brianwilkerson@google.com changed reviewers: + paulberry@google.com
https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... File pkg/analysis_server/test/stress/utilities/git.dart (right): https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:32: * command (the [input]). Git diff has a lot of options controlling its output format. We should probably add some clarifying text specifying what set of options need to be used to create the diff in order for it to be compatible with BlobDiff. For instance, the diff can't have any context lines. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:61: } Diff uses special formatting when the end of the file lacks a newline. Consider adding code to support this. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:102: * command (the [diffResults]). As with BlobDiff, it would be nice to clarify what git options are required to format the diff in the way that CommitDelta expects. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:127: void filterDiffs(List<String> analysisRoots) { This method seems out of place, since it is the only method in the file that encodes analyzer-specific semantics. Everything else is general-purpose enough to be used by any app that wants to interact with git. Is there some way we could generalize this method so that the analyzer-specific logic moves to the caller? https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:205: // 6 What do these numbers mean? https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:235: class CommitHistory { Since git histories frequently have branches, we should clarify that we're not trying to represent the full branching structure, but merely a linear subset of the commit history. Maybe rename this to "LinearCommitHistory"? https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:264: class CommitHistoryIterator { Similar concern here. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:318: * The index of the first line that was changed in the src. Since diffSrcLine and diffDstLine are public, their doc comments should explicitly specify that they follow the conventions used by Git (subtracting 1 if there are no lines on the src/destination side of the diff, respectively). https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:338: * Initialize a newly created hunk. The lines will be Looks like you forgot to finish this comment. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:343: * Return the index of the first line that was changed in the dst. Similarly, the doc comment here should explicitly specify that this getter *doesn't* follow the convention used by git. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:418: bool get isAddition => status == 'A'; Rather than having a bunch of "is" getters that are mutually exclusive by convention, I'd prefer to have a single "kind" getter that returns an enum. That way clients clients can process a diff record using a switch statement rather than having to do a bunch of cascaded "if" tests, and then they get the benefit of the "missing case clause" warnings if they forget to handle one of the possibilities. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:524: [String arg2, These optional args, and the if tree below, seem like a lot of effort to go to just so that we can get behavior that's reminiscent of variable length arguments in Java. Why not just do: ProcessResult _run(List<String> arguments) { return Process.runSync(...); } And then add "[" and "]" characters at the call sites, e.g.: ProcessResult result = _run(['rev-list', '--first-parent', 'HEAD'])
https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... File pkg/analysis_server/test/stress/utilities/git.dart (right): https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:32: * command (the [input]). > Git diff has a lot of options controlling its output format. We should probably > add some clarifying text specifying what set of options need to be used to > create the diff in order for it to be compatible with BlobDiff. I don't really want to deeply document it everywhere, so I took a slightly different tack. > For instance, the diff can't have any context lines. Actually, I think we correctly ignore context lines. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:61: } > Diff uses special formatting when the end of the file lacks a newline. Consider > adding code to support this. I'll need a pointer to understand what needs to happen. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:102: * command (the [diffResults]). Ditto https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:127: void filterDiffs(List<String> analysisRoots) { > Is there some way we could generalize this method so that the analyzer-specific > logic moves to the caller? I can rename the parameter; it's just a list of directory paths (they wouldn't have to be analysis roots). I can also pass in the list of glob patterns. Or just pass in glob patters. I'll do this in a future CL. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:205: // 6 > What do these numbers mean? They're field numbers. Added to the comments to make it more clear. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:235: class CommitHistory { Done https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:264: class CommitHistoryIterator { Done https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:318: * The index of the first line that was changed in the src. Done https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:338: * Initialize a newly created hunk. The lines will be > Looks like you forgot to finish this comment. Yep. Thanks. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:343: * Return the index of the first line that was changed in the dst. Done https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:418: bool get isAddition => status == 'A'; > I'd prefer to have a single "kind" getter that returns an enum. > That way clients clients can process a diff record using a switch statement ... I think you can do that today: switch (record.status.substring(0, 1)) { case 'A': ... If it becomes useful we could add a getter to return the first character. https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:524: [String arg2, > Why not just do: Just following a pattern I saw somewhere else. I've change it.
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 905cb67e99ba113b9be72d25baf68f77b2fcd423 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... File pkg/analysis_server/test/stress/utilities/git.dart (right): https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:32: * command (the [input]). On 2015/11/18 17:36:46, Brian Wilkerson wrote: > Actually, I think we correctly ignore context lines. We ignore them, but that won't give correct results for two reasons: (1) the numbers that appear above the diff hunk (on the line beginning "@@") include context lines, so if we don't account for that we'll apply the diff in the wrong place. (2) if two hunks are closer together than the number of context lines, Git will combine them, and we won't handle that properly. E.g.: @@ -100,5 +100,5 @@ context line -removed line +added line context line -removed line +added line context line https://codereview.chromium.org/1453213002/diff/1/pkg/analysis_server/test/st... pkg/analysis_server/test/stress/utilities/git.dart:61: } On 2015/11/18 17:36:46, Brian Wilkerson wrote: > > Diff uses special formatting when the end of the file lacks a newline. > Consider > > adding code to support this. > > I'll need a pointer to understand what needs to happen. Unfortunately I don't have a pointer to give you (I don't know if there is one). But I think if you just try diffing files that do/don't end in a newline it should be pretty obvious. |
||||||||||||||||||||||||||||||||||||||||||
