Replace int.Parse() with int.TryParse() to avoid exceptions caused by overflowing values in tags

[james-bjss]

Replace int.Parse() with int.TryParse() This is to prevent the tool failing with an overflowing value.

Description

If a tag has a suffix which is a numeric value larger than a 32bit int then gitversion fails with an Overflow error. Whilst the tag can be ignore by using the tag-prefix config It would be preferable if it didn't fall over when it encounters such a tag.

Related Issue

#2390

Motivation and Context

We have some tags in our repository which are suffixed with a timestamp value (created by another build process). We have used the tag-prefix config to ignore these, but without it gitversion falls over with an Int32 Overflow exception.

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

• [X ] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes. ( Will add if this change is deemed acceptable)
• [X ] All new and existing tests passed.

[james-bjss]

Great stuff, I just have some design feedback: Please add the method SemanticVersion.TryParse() instead to deal with this and then use Int32.TryParse() within it to avoid the exceptions.

Sure, will update this evening and add tests. Thanks!

Should we swallow the errors silently or would you like it to print to STDERR?

[asbjornu]

Sure, will update this evening and add tests. Thanks!

👍🏼

Should we swallow the errors silently or would you like it to print to STDERR?

If you're able to inject an instance of ILog somewhere natural, you can always do a log.Error() statement when parsing fails. For more verbosity you can add log.Warning()´ or log.Debug()` statements as well, perhaps on the individual field version part level.

[james-bjss]

Updated to use TryParse(), the code is already within a method called SemanticVersion.TryParse() unless you meant to create a private method for this?

Defaulted the out param to null, the code returns if major version is not matched (there was no default 0 value set for this previously) or if the int.TryParse()s fail.

I looked into the logging but could't see a decent way to provide a logger. I'm wondering how useful it is now to check each field if we can't provide details of the failure to the user?

I noticed the SemanticVersion.Parse() method wraps SemanticVersion.TryParse() and this throws a WarningException. Perhaps it could bubble-up/throw an exception with information about the parsing failure?

   public static SemanticVersion Parse(string version, string tagPrefixRegex)
        {
            if (!TryParse(version, tagPrefixRegex, out var semanticVersion))
                throw new WarningException($"Failed to parse {version} into a Semantic Version");

            return semanticVersion;
        }

[arturcic]

@james-bjss any updates?

[james-bjss]

@james-bjss any updates?

Sorry, have been away on leave and busy with day job. Will try and take a look at this again this week.

[james-bjss]

@asbjornu @arturcic - Have updated the PR with the requested changes.

The WriteVersionInfoTaskShouldLogOutputVariablesToBuildOutputInAzurePipeline is failing though, I noticed these build pipeline tests seem intermittent and seem to fail on other PRs. Not 100% sure this is failing due to my change as previous builds this passed and the GitHub Actions test failed.

Is this a flaky test or do I need to dig deeper?

[arturcic]

@asbjornu @arturcic - Have updated the PR with the requested changes.

The WriteVersionInfoTaskShouldLogOutputVariablesToBuildOutputInAzurePipeline is failing though, I noticed these build pipeline tests seem intermittent and seem to fail on other PRs. Not 100% sure this is failing due to my change as previous builds this passed and the GitHub Actions test failed.

Is this a flaky test or do I need to dig deeper?

It's a flaky test unfortunately.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

[asbjornu]

@james-bjss, could you please go over my feedback and rebase this onto the HEAD of main? 🙏🏼

[asbjornu]

While I think I've solved this by changing int to long in #3028, I like the changes made in this PR and would love to see them rebased and merged.

[james-bjss]

While I think I've solved this by changing int to long in #3028, I like the changes made in this PR and would love to see them rebased and merged.

Hi, Sorry for the late reply. I have had quite a lot on work-wise. Will try to grab some time to revisit this and put this PR to bed.

Just to confirm the outstanding change is to replace all usages with TryParse()?

I suppose that then means adding logic in all the calling code to check the repose and throw an error?

[asbjornu]

I believe a bool TryParse() pattern is better, but I'm not 100% certain it's needed anymore after changing from int to long in #3028. Please have a look at the code and let me know whether you think a TryParse pattern is going to provide an improvement or not still. If it is, I'd love to see this PR rebased and completed.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

[asbjornu]

@james-bjss, can you please complete this PR one way or another? 🙏🏼

[arturcic]

This one is not needed anymore due to recent changes in the main branch