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

[JENKINS-40834] Add an API to resolve symbolic refs #236

Merged
merged 4 commits into from Mar 24, 2017

Conversation

@stephenc
Copy link
Member

commented Mar 21, 2017

  • Sadly JGit needs to try and guess

@reviewbybees

(Downstream PR for git-plugin to use the API will follow - based on WIP in https://github.com/jenkinsci/git-plugin/tree/jenkins-40834 that needed this change to have a hope of working)

- Sadly JGit needs to try and guess
@reviewbybees

This comment has been minimized.

Copy link

commented Mar 21, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Contributor

left a comment

My review is incomplete, but day is done, so I thought it was better to provide my incomplete comments. I'll work more on this tomorrow.

public Map<String, String> getRemoteSymbolicReferences(String url, String pattern)
throws GitException, InterruptedException {
ArgumentListBuilder args = new ArgumentListBuilder("ls-remote");
args.add("--symref");

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 22, 2017

Contributor

Command line git versions through at least 2.1.4 don't understand the symref argument to git ls-remote. Unfortunately, that means the git ls-remote --symref command won't work on machines running the default git version bundled with CentOS 6, CentOS 7, Debian 7, Debian 8, or Ubuntu 14.

That's particularly difficult, since the Jenkins standard Docker instance is based on Debian 8.

If I run the following command:

docker run -i --rm jenkins \
git ls-remote --symref https://github.com/jenkinsci/git-client-plugin HEAD

it reports

usage: git ls-remote [--heads] [--tags]  [-u <exec> | --upload-pack <exec>]
                     [-q|--quiet] [--exit-code] [--get-url] [<repository> [<refs>...]]

I think the same thing can be accomplished on the older git versions with two commands rather than one. The commands (shell level for simplification):

repo=https://github.com/jenkinsci/git-client-plugin
sha1=$(git ls-remote $repo HEAD | awk '{print $1}')
git ls-remote $repo | grep -w $sha1

Unfortunately, when the repository contains multiple remotes (as I use with some of my caching repositories), the two step process provides additional output like "refs/remotes/origin/master" in addition to "refs/heads/master". It may be enough to filter the output to only those which start with refs/heads. I think that is the technique you're using in the JGit implementation, and which you commented about its weaknesses in the JGit implementation.

As another alternative, could the command line git implementation check for command line git version and fall back to use the "guesser" implementation in JGit if the command line git version does not support the symref argument?

Matcher matcher = symRefPattern.matcher(line);
if (matcher.matches()) {
references.put(matcher.group(2), matcher.group(1));
} else if (line.length() < 41) {

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 22, 2017

Contributor

Would you be willing to not throw the exception if ! line.isEmpty()?

That would make this implementation consistent with the JGit implementation and would avoid throwing an exception in the degenerate case where a repository has no commits.

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 22, 2017

Contributor

If it helps, you can refer to my https://github.com/MarkEWaite/git-client-plugin/tree/master-PR236-resolve-symbolic-refs branch for some additional tests I added and for the line.isEmpty() change.

…pending their implementation of same
stephenc added a commit to jenkinsci/git-plugin that referenced this pull request Mar 22, 2017
stephenc added 2 commits Mar 22, 2017
… what to do if the is no symbolc ref support
@stephenc

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2017

@MarkEWaite I have reworked this somewhat based on your feedback.

Now the API will return an empty map if the client or remote server does not support symref reporting.

Consumers will have to decide what they want to do in the absence of the symref information, which I think is better than trying to fake out the information in the Git 1.8.4 way

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2017

Thanks @stephenc , I'll review it and post my feedback.

Map<String, String> references = new HashMap<>();
String regexPattern = null;
if (pattern != null) {
regexPattern = createRefRegexFromGlob(pattern);

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 22, 2017

Contributor

This call to createRefRegexFromGlob creates a regex pattern which won't match "HEAD". Line 805 seems to mandate that only HEAD is accepted as a parameter to JGit for non-empty results, but if I pass "HEAD", the results are empty for JGit but have a value for CliGit.

Refer to e897850 for a test to show the problem, and to aa92d89 for one alternative to fix it.

* Test getRemoteSymbolicReferences with listing all references
*/
public void test_getRemoteSymbolicReferences_withMatchingPattern() throws Exception {
assumeThat(hasWorkingGetRemoteSymbolicReferences(), is(true));

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2017

Contributor

Unfortunately, this file is JUnit 3. It doesn't act correctly for an assumeThat. Need to replace it with a simple conditional. See the test failures.

* Test getRemoteSymbolicReferences with listing all references
*/
public void test_getRemoteSymbolicReferences() throws Exception {
assumeThat(hasWorkingGetRemoteSymbolicReferences(), is(true));

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2017

Contributor

Unfortunately, this file is JUnit 3. It doesn't act correctly for an assumeThat. Need to replace it with a simple conditional. See the test failures.

if (pattern != null) {
regexPattern = createRefRegexFromGlob(pattern);
}
// HACK ALERT... JGit doesn't give the info we require hard-coding HEAD symref support only based on pre 1.8.5

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2017

Contributor

I think this comment is now outdated, isn't it?

The later comment describes the current implementation, while I think this comment describes something previous

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2017

@stephenc , I think this is ready to merge, so long as you are OK with me adding the changes I've implemented in https://github.com/MarkEWaite/git-client-plugin/commits/master-PR236-resolve-symbolic-refs . I'd merge this to the master branch, then squash my changes into a more coherent set of a few commits and merge them to the master branch.

Is that OK with you, or would you prefer to make changes based on my last review?

I've confirmed that the tests pass on CentOS 6 (git 1.7.1), CentOS 7 (git 1.8.3.1) , Debian 7 (git 1.7.10.4), Debian 8 (git 2.1.4), Ubuntu 14 (git 1.9.1), Ubuntu 16 (git 2.1.4), Ubuntu 16 with the latest ppa (git 2.11), and windows (git 2.12.1).

@MarkEWaite MarkEWaite merged commit 8550139 into jenkinsci:master Mar 24, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/jenkins/pr-merge This commit has test failures
Details
Jenkins Jenkins is validating pull request ...
Details
@stephenc

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2017

All fine by me

@stephenc stephenc deleted the stephenc:jenkins-40834 branch Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.