Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust support for revision ranges #565

Merged
merged 3 commits into from
Apr 21, 2019

Conversation

eatdrinksleepcode
Copy link
Contributor

This is the first step in adding greater support for revision ranges (see #561). Currently, only log supports revision ranges, it only supports specific kinds (single tip includes and excludes), and only in very limited formats ("git log include" or "git log include ^exclude"). With this PR, log now supports any number of single tip includes or excludes, in any order, as real git does.

This PR also introduces support for the "rev-list" command, which uses the same revision range capability. In addition to increased git compatibility, the rev-list command also provides much simpler output with which to test the output ordering of revision ranges.

Compatibility Note: this PR changes the output ordering of the log command. The previous ordering was essentially a breadth-first search. The new ordering follows the git default, which is based on commit date.

This commit introduces the RevisionRange, which more closely follows real git
in terms of identifying a range of revisions than the current implementation of
log. RevisionRange is being used in the new rev-list command first because it
is easy to test for ordering. A future commit will replace the existing
implementation of log with RevisionRange.
This changes the implementation of the "log" command to use the RevisionRange
functionality. RevisionRange sorts the results in order of reverse create time,
to match real git. This is a change from the previous implementation of log,
which essentially produced a breadth-first ordering.
Copy link
Owner

@pcottle pcottle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! thanks for the test coverage

var msg = null;

deferred.promise.then(function(commands) {
msg = commands[commands.length - 1].get('error').get('msg');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i vaguely remember why i store all command output in the error field, but its certainly silly

});

describe('Log supports', function() {
var SETUP = 'git co -b left C0; gc; git merge master; git co -b right C0; gc; git merge master; git co -b all left; git merge right; ';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes i use serialized tree states for this type fo setup, but this works as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, serialized tree state would probably be more in keeping with the way the other tests are setup. I'll be perfectly honest: just entering the commands was easier :)

return thisDeferred.promise;
}.bind(this));
}, this);

chain.then(function() {
var nowTime = new Date().getTime();
if (entireCommandPromise) {
entireCommandPromise.resolve();
entireCommandPromise.resolve(commands);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh so this adds a return arg to the promise? good call

});

exclusions.forEach(function(exclusion) {
self.addExcluded(Graph.getUpstreamSet(self.engine, exclusion));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad you found these helpers

@@ -151,6 +147,12 @@ var Command = Backbone.Model.extend({
this.validateArgBounds(args, 0, 2);
},

impliedHead: function(args, min) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pcottle pcottle merged commit 06e0f29 into pcottle:master Apr 21, 2019
@pcottle
Copy link
Owner

pcottle commented Apr 21, 2019

Pushed the site with these changes, it should be live :)

@eatdrinksleepcode eatdrinksleepcode deleted the revision-range branch April 22, 2019 18:12
@pcottle pcottle mentioned this pull request Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants