JENKINS-20991 - changed firstLine in revParse to use firstNonBlankLine

[Evildethow]

@reviewbybees

@ndeloof This is the one I mentioned to you

Basically, git rev-parse has been observed to return a leading blank line in its results. Not sure why. This change basically just covers for it by returning the first non blank line in the result or null if no result (which is existing behaviour).

[ghost]

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.

[jenkinsadmin]

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

[Evildethow]

@MarkEWaite

Why only guard one of the two calls to rev-parse in CliGitAPIImpl?

Wasn't a conscious decision. Simply missed those cases.

I've updated isBareRepository and validateRevision to use the new method. Let me know if there are any others I've missed.

Cheers

[MarkEWaite]

While evaluating the pull request in my testing environment, I found the tests pass on all the Linux variants I tested (Ubuntu 12, 13, 14, CentOS 6, 7, and Debian 6, 7, and 8). Unfortunately, they fail on the two Windows platforms (Windows 7 and Windows 10).

One failure was:

Error Message

expected:<[58c8401641511340b7d919a4e6b805d9f3416d3f]> but was:<[
58c8401641511340b7d919a4e6b805d9f3416d3f
61c8401641511340b7d919a4e6b805d9f3416d1c]>

Stacktrace

org.junit.ComparisonFailure: expected:<[58c8401641511340b7d919a4e6b805d9f3416d3f]> but was:<[
58c8401641511340b7d919a4e6b805d9f3416d3f
61c8401641511340b7d919a4e6b805d9f3416d1c]>
at org.junit.Assert.assertEquals(Assert.java:115)
at org.junit.Assert.assertEquals(Assert.java:144)
at     org.jenkinsci.plugins.gitclient.utils.GitOutputUtilsTest.testFirstNonBlankLine_TwoResults(GitOutputUtilsTest.java:27)

The other failure was:

Error Message

expected:<[58c8401641511340b7d919a4e6b805d9f3416d3f]> but was:<[
58c8401641511340b7d919a4e6b805d9f3416d3f
61c8401641511340b7d919a4e6b805d9f3416d1c]>

Stacktrace

org.junit.ComparisonFailure: expected:<[58c8401641511340b7d919a4e6b805d9f3416d3f]> but was:<[
58c8401641511340b7d919a4e6b805d9f3416d3f
61c8401641511340b7d919a4e6b805d9f3416d1c]>
at org.junit.Assert.assertEquals(Assert.java:115)
at org.junit.Assert.assertEquals(Assert.java:144)
at org.jenkinsci.plugins.gitclient.utils.GitOutputUtilsTest.testFirstNonBlankLine_TwoResults(GitOutputUtilsTest.java:27)`

Could you investigate those Windows test failures before the code is merged?

[Evildethow]

@MarkEWaite

Nice catch. My guess is its this line - https://github.com/jenkinsci/git-client-plugin/pull/199/files#diff-2a3315af197d8897c3ed957db727fb37R21

The code is using Java's line.separator. Obviously this is wrong. Forgot to circle back to that.

Maybe the line separator should be passed into firstNonBlankLine as a parameter?

Not sure how git would return the result with blank line on Windows. Would it use the native CRLF or just LF? Maybe querying core.autocrlf would work. Not sure the correct approach here.. Thoughts?

Maybe your original idea of simply trimming the output is the better approach. Although then we don't cover the scenario where multiple results are returned (which should never happen right?).

Obviously I can change the test to use Java's line.separator instead of simply doing "\n" and it will pass but that seems wrong.

Or maybe the test is just rubbish.. Or both :)

Thoughts?

@ndeloof Maybe you have some thoughts here as well?

Cheers

[MarkEWaite]

I hadn't considered making the change in the test until you suggested it. I replaced the test references to "\n" with EOL, a constant copy of line.separator. That was enough to allow the tests to pass on Windows and Linux.

It is especially challenging that the change is fixing a problem which we don't know how to duplicate. Any further ideas of ways to duplicate the actual conditions that the customer is seeing?

Refer to c167579 to see if you think my change is of interest.

[MarkEWaite]

After reading stackoverflow split Java string I think we may need to reconsider the use of the line.separator property. Isn't the use of line.separator assuming that the "git rev-parse" command will output lines separated by the same line separator that java uses? Is that a safe assumption?

Since this is always parsing the output of "git rev-parse", I don't think it is as risky as the conditions described in the article. The article talks about reading files from arbitrary sources, etc. That's not the case which this code needs to support.

[Evildethow]

@MarkEWaite

Isn't the use of line.separator assuming that the "git rev-parse" command will output lines separated by the same line separator that java uses? Is that a safe assumption?

Yeah this is my concern as well. I have no idea if the assumption is safe at this point. I can experiment in a Windows VM to see which line ending git uses by default but I do agree that simply trimming output as you originally suggested is a better option here, especially, as you mentioned, we are only using this for git rev-parse at the moment.

I'll try and find some time today to make the changes if you agree?

[MarkEWaite]

Yes, I think a trim of the output may be the safest thing in this context.

[Evildethow]

Hey @MarkEWaite,

I've removed the new classes and basically replaced the instances where I was calling firstNonBlankLine to use Apache Commons StringUtils.trimToNull().

I have no idea how to test this scenario without extracting the trim call out to test in isolation, which seems a bit redundant.

Any further ideas of ways to duplicate the actual conditions that the customer is seeing?

Sorry no clue here. This also makes writing an end-to-end test case pretty hard :(.

Any suggestions for how to add a test case for this (without just mocking out everything I guess) ?

[MarkEWaite]

I squashed and rebased the change so that it would appear as a single change rather than the series of commits used to arrive at the final solution. It is now on the master branch and will be included in the next release of the git client plugin.