Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-67281

Commits since last build is wrong when last build commit no longer exists

      The "Commits since last build" shows 200 when it can not determine the commits since last build.

      Steps to reproduce:

      1. Push new branch with 1 commit
      2. Amend commit (replace commit) and force-push

      Instead of nothing or unable to determine, it shows 200 - which is wrong.

      • Commits since last build: 200

          [JENKINS-67281] Commits since last build is wrong when last build commit no longer exists

          Jan Klass added a comment -

          I see now that this is probably not a forensics plugin issue, but git SCM instead.

          Jan Klass added a comment - I see now that this is probably not a forensics plugin issue, but git SCM instead.

          Ulli Hafner added a comment -

          I think the component was correct. The output is from my plugin.

          The algorithm currently scans the log until it finds a matching commit (it stops after a configurable limit of 200 commits).

          Do know a way to detect that we have a rewriting of the commit history?

          Ulli Hafner added a comment - I think the component was correct. The output is from my plugin. The algorithm currently scans the log until it finds a matching commit (it stops after a configurable limit of 200 commits). Do know a way to detect that we have a rewriting of the commit history?

          Jan Klass added a comment - - edited

          I don’t think identifying discarded commits is viable. With a force push, the commit is discarded from the ref/branch. I am not sure how Git hosting reflog and discarded objects may be accessible in general or between platforms (GitHub vs GitLab etc).

          I think a simple "try to find" approach where a force-push would mean no "x commits since" can be determined is fine. A force push is a rewriting of history, so determining a commit number/scalar diff value requires a well thought out concept which I would only do as a second step, if at all.

          If I force-push IMO it should display either no "commits since last build" or "could not determine commits since last build because last build commit was not found within the last 200 commits". I think given how long the second is, that may be reasonable as a log message rather than a displayed short info.

          If you really wanted to determine e.g. "discarded x commits, added y commits since last build", this may work for repositories already cloned and reused on a node, as it could still refer to and log-diff to the old object. You open a whole bunch of complexity there though with handling this, and communicating to the user what the dependency or platform conditions and consequences are, and how to display this and/or discard+add adequately.

          I am not that familiar with how Jenkins master tracks refs to determine diffs, so I can not really make guesses there. Maybe it would be reasonably possible with that - at least to a degree where it is viable.

          Jan Klass added a comment - - edited I don’t think identifying discarded commits is viable. With a force push, the commit is discarded from the ref/branch. I am not sure how Git hosting reflog and discarded objects may be accessible in general or between platforms (GitHub vs GitLab etc). I think a simple "try to find" approach where a force-push would mean no "x commits since" can be determined is fine. A force push is a rewriting of history, so determining a commit number/scalar diff value requires a well thought out concept which I would only do as a second step, if at all. If I force-push IMO it should display either no "commits since last build" or "could not determine commits since last build because last build commit was not found within the last 200 commits". I think given how long the second is, that may be reasonable as a log message rather than a displayed short info. If you really wanted to determine e.g. "discarded x commits, added y commits since last build", this may work for repositories already cloned and reused on a node, as it could still refer to and log-diff to the old object. You open a whole bunch of complexity there though with handling this, and communicating to the user what the dependency or platform conditions and consequences are, and how to display this and/or discard+add adequately. I am not that familiar with how Jenkins master tracks refs to determine diffs, so I can not really make guesses there. Maybe it would be reasonably possible with that - at least to a degree where it is viable.

          Jan Klass added a comment -

          To summarize: To fix this bug I would not show any "since last build" number/info if the previous build commit can not be located. Given that there is a limit (200), a log message informing about this and the limit would be helpful.

          Jan Klass added a comment - To summarize: To fix this bug I would not show any "since last build" number/info if the previous build commit can not be located. Given that there is a limit (200), a log message informing about this and the limit would be helpful.

          Ulli Hafner added a comment -

          Yes, that makes sense! We need to change that in three different steps but it shouldn't be hard to improve that. One idea is to toggle a flag in BuildCommits if the max number of commits has been reached.

          If you are interested in helping with that fix feel free to provide a PR.

          Ulli Hafner added a comment - Yes, that makes sense! We need to change that in three different steps but it shouldn't be hard to improve that. One idea is to toggle a flag in BuildCommits if the max number of commits has been reached. If you are interested in helping with that fix feel free to provide a PR.

          Jan Klass added a comment -

          Not currently.

          Jan Klass added a comment - Not currently.

          Marijn van Zon added a comment - - edited

          Today I ran into an issue which looks like it might have the same root cause as this one.

          Scenario:

          1. Create a new job that includes the gitDiffStat() step
          2. Assume the current HEAD of the branch is at 'abc123'
          3. Run the job
            1. It will show the message "Skipping step since no previous build has been completed yet"
          4. Amend the commit in the branch and do a force-push, overwriting the previous HEAD commit. Now the HEAD commit is 'def456'
          5. Run the job again
            1. It will say: No reference build found, using previous build 'myjob #1' as baseline. Found latest previous commit 'abc123'
            2. Because the new HEAD 'def456' was a force push, the commit from the previous build 'abc123' will not be in the history of the current HEAD.
            3. It says: Found 74 commits
            4. That's all the commits from the initial commit

          That's a minor inconvenience for small repositories, but for repositories containing thousands of commits this becomes a problem. For one of my repositories it basically got stuck (the process was automatically killed after an hour).

          drulli your comment suggests there should be a 200 commit limit, but considering mine was still running an hour later I'm not sure if that limit is working properly. A simple guess could be that it will always traverses the thousands of commits in the repo trying to find a common parent, and only after it collected all commits does it take the newest 200? Aside from the previously suggested fix that if it hits 200 it should consider it as no changes could be located, it also needs to stop after examining 200 commits (any results beyond that will get thrown away anyways).

          I don't have much Java experience, so not sure how easy it'll be to fix, but I'm willing to take a short at making a PR if you (or anyone with sufficient knowledge) can help out with some guidance on where the code changes need to be done. I did do some debugging on a different plugin previously, so I do know the basics on how to make code changes, build it and test them locally.

          Currently also implementing a workaround for my specific use case to skip gitDiffStat() when there is no reference build.

          Marijn van Zon added a comment - - edited Today I ran into an issue which looks like it might have the same root cause as this one. Scenario: Create a new job that includes the gitDiffStat() step Assume the current HEAD of the branch is at 'abc123' Run the job It will show the message "Skipping step since no previous build has been completed yet" Amend the commit in the branch and do a force-push, overwriting the previous HEAD commit. Now the HEAD commit is 'def456' Run the job again It will say: No reference build found, using previous build 'myjob #1' as baseline. Found latest previous commit 'abc123' Because the new HEAD 'def456' was a force push, the commit from the previous build 'abc123' will not be in the history of the current HEAD. It says: Found 74 commits That's all the commits from the initial commit That's a minor inconvenience for small repositories, but for repositories containing thousands of commits this becomes a problem. For one of my repositories it basically got stuck (the process was automatically killed after an hour). drulli your comment suggests there should be a 200 commit limit, but considering mine was still running an hour later I'm not sure if that limit is working properly. A simple guess could be that it will always traverses the thousands of commits in the repo trying to find a common parent, and only after it collected all commits does it take the newest 200? Aside from the previously suggested fix that if it hits 200 it should consider it as no changes could be located, it also needs to stop after examining 200 commits (any results beyond that will get thrown away anyways). I don't have much Java experience, so not sure how easy it'll be to fix, but I'm willing to take a short at making a PR if you (or anyone with sufficient knowledge) can help out with some guidance on where the code changes need to be done. I did do some debugging on a different plugin previously, so I do know the basics on how to make code changes, build it and test them locally. Currently also implementing a workaround for my specific use case to skip gitDiffStat() when there is no reference build.

            Unassigned Unassigned
            jk Jan Klass
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated: