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

Fix GIT_DIFF_TREE_PATTERN wrt number of status letters #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krinklesaurus
Copy link

@krinklesaurus krinklesaurus commented Feb 25, 2022

Hi,

we recently saw this failure in our GoCD Pipeline:

The plugin sent a response that could not be understood by Go. Plugin returned with code '500' and the following response: '"Unable to parse git-diff-tree output line: MMMMM\tDockerfile\nFrom output:\n afc1a254a698be419299e6717a6c72c4e365ac8c\nMMMMM\tDockerfile"'

There was a similar issue reported here: ashwanthkumar/gocd-build-github-pull-requests#123 In that case, only 3 Ms were part of the response line, in our case it's 5.

From our investigation it looks like the executed git diff-tree command is

git diff-tree --name-status --root -r -c <revision>

Now according to the git documentation documentation

status is concatenated status characters for each parent

So if a revision has X parents, X status letters are in that line. In that case, the git pattern used here doesn't work anymore since it only accepts up to 3 status letters.

The fix in this PRs is to group anything until the first occurance of a whitespace, then group everything following that whitespace.

Tested with some quick main method:

public static void main(String[] args) {
    final Pattern GIT_DIFF_TREE_PATTERN = Pattern.compile("^([^\\s]+)\\s+(.+)$");

    final String[] resultLines = new String[]{
        "M\tsrc/main/blablabla.json",
        "MMM\tapp/xxx/src/main/webapp/xxxx/xxx.jsp",
        "MMMMM\tDockerfile",
    };

    for(String resultLine : resultLines) {
    Matcher m = GIT_DIFF_TREE_PATTERN.matcher(resultLine);
    if (!m.find()) {
        throw new RuntimeException(String.format("Unable to parse git-diff-tree output line: %s%nFrom output:%n %s", resultLine, String.join(System.lineSeparator())));
    }
    System.err.println(String.format("%s : %s", m.group(1), m.group(2)));
    }
}

@krinklesaurus
Copy link
Author

@ashwanthkumar any update?

@chadlwilson
Copy link
Collaborator

Unfortunately, even if we merge this, the gocd-build-github-pull-requests plugin has never been updated to use v2.x of this base lib, so there's probably a bit of work there too.

I do have collaborator permissions to merge here, but I can't cut new releases and push to Maven central without @ashwanthkumar help as there isn't GitHub Actions automation.

Is there a possibility of adding a test for this do you think?

@krinklesaurus
Copy link
Author

Hey @chadlwilson
sorry I totally missed your answer. I saw that https://github.com/ashwanthkumar/gocd-build-github-pull-requests is using version 1.4 of git-cmd but I don't find the version in this repo. If you point me to the correct revision I would be happy to open the PR again against that version if that helps.

Regarding the test, I'm not familiar with how the sample git directory structure needs to be to cover a merge commit with multiple parents. Even the merge-commit-git-repository is not showing any merge commits to me. The last change to this pattern was approved without any test as well #3 so maybe the existing test coverage is sufficient?

@krinklesaurus
Copy link
Author

Hey @chadlwilson

since our GoCD pipelines are still broken, we

Worked out for us. I leave the PR as it is but it doesn't really matter to us anymore 🙂 Thank you

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