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

Use signed version comparisons before recommending upgrade #358

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

derrickstolee
Copy link
Contributor

This is not super-important for git-for-windows/git because it always uses -rcX text in its pre-releases. However, in microsoft/git we use regular-versioned tags as prereleases that we hand to dogfooders before promoting that same tag as a full release. This has led to users being prompted for upgrades because their version doesn't match the latest, even though their current version is newer.

Always use a version comparison that compares the components carefully. I tested the new version_compare very carefully, but am not sure of how to test the change in the second commit.

@derrickstolee derrickstolee requested a review from dscho June 21, 2021 19:27
@derrickstolee derrickstolee self-assigned this Jun 21, 2021
dscho and others added 4 commits June 21, 2021 22:23
Currently, this is failing, but it documents what we want.

Signed-off-by: Johannes Schindelin <[email protected]>
Swapping the arguments should yield the negative result.

Signed-off-by: Johannes Schindelin <[email protected]>
…ifferently

Instead of insisting on single period characters to separate the numbers
(and ignoring everything after an unexpected character), let's just
ignore the non-numeric parts altogether (with one special exception for
`-rc` versions).

This fixes our "unit" test.

Signed-off-by: Johannes Schindelin <[email protected]>
Whenever we have unmatched versions, do a signed comparison. A user
might have installed a prerelease version that is not yet a full
release. This is particularly important in the microsoft/git world.

Signed-off-by: Derrick Stolee <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the version-compare branch from eb65251 to 5f7b932 Compare June 21, 2021 20:36
fi

return 0
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I dislike the complexity of having to do lexicographic comparisons... Let me try whether I can come up with an alternative solution real quick.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to comment immediately but forgot to "finalize my review".

@dscho
Copy link
Member

dscho commented Jun 21, 2021

@derrickstolee I think I came up with a simpler solution. We do not really need to take care of all possible version number formats, after all, just the ones we use in Git for Windows (and in microsoft/git).

What do you think about my alternative solution? (I force-pushed it to your branch so that it is easier to compare the two.)

Copy link
Contributor Author

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dscho I like yours better! Much cleaner, thanks. I'll rebase.

done
}

test "--test-version-compare" != "$*" || {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this!

@derrickstolee
Copy link
Contributor Author

@dscho I like yours better! Much cleaner, thanks. I'll rebase.

Nevermind, you already rebased. Thanks!

@dscho dscho merged commit 1702020 into git-for-windows:main Jun 21, 2021
@PhilipOakley
Copy link
Contributor

(duplicated from the merged commit)
+1
This looks good.
It moves the special .rc* case into the version_compare() function, and will almost certainly resolve my issue git-for-windows/git#1843 (comment)

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.

3 participants