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

In affected tests logic, don't do git diff using a A..B range #14093

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Nov 16, 2018

"%s..HEAD" will result in something like git diff abcd1234..HEAD
being called, and this is the same as git diff abcd1234 HEAD, which
in turn is the same as git diff abcd1234.

Also, while git rev-parse can parse A..B, it doesn't refer to
one revision, so isn't appropriate to call a revish.

"%s..HEAD" will result in something like `git diff abcd1234..HEAD`
being called, and this is the same as `git diff abcd1234 HEAD`, which
in turn is the same as `git diff abcd1234`.

Also, while `git rev-parse` can parse `A..B`, it doesn't refer to
*one* revision, so isn't appropriate to call a revish.
@jgraham
Copy link
Contributor

jgraham commented Nov 19, 2018

I'm not sure this is better really; it seems marginally less explicit. I also think git tends to operate transparently across single revisions or ranges, so I'm not bothered by the terminology. But I don't really care either way.

@foolip
Copy link
Member Author

foolip commented Nov 20, 2018

https://git-scm.com/docs/git-diff explains how git diff A..B is a bit of a special case:

For a more complete list of ways to spell <commit>, see "SPECIFYING REVISIONS" section in gitrevisions[7]. However, "diff" is about comparing two endpoints, not ranges, and the range notations ("<commit>..<commit>" and "<commit>...<commit>") do not mean a range as defined in the "SPECIFYING RANGES" section in gitrevisions[7].

But the "revish" wording is used to refer to multiple commits elsewhere too, ./wpt tests-affected --help says "Commits to consider" and ./wpt tests-affected merge_pr_12948..merge_pr_14127 does work. It's not quite sensible since the state of the tree at HEAD will also be used, but I'll close this PR as low value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants