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.

Sign up for GitHub

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

Jump to bottom

Installer: stop complaining about "downgrading" when upgrading from an rc version #264

Merged
merged 4 commits into from
Oct 31, 2019
Merged

Installer: stop complaining about "downgrading" when upgrading from an rc version #264

merged 4 commits into from
Oct 31, 2019

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 30, 2019

There is a pretty old bug where the Git for Windows installer warns about a "downgrade" when upgrading from, say, v2.23.0-rc2 to v2.23.0, because the versions that are actually compared are of the format N.N.N.N (by stripping out non-numerical letters, in the example, the compared versions would be 2.23.0.2 vs 2.23.0.0).

Let's fix this by trying to be a bit more careful when we're about to warn about a downgrade, and compare the output of the installed git version vs the one we're about to install.

dscho added 3 commits October 30, 2019 13:55
@dscho
installer: when reading files' contents, chomp the trailing newline
327adad 
This makes things easier when reading back the output from a command
that we just let redirect its output into a file.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
installer: be prepared for Git tags when comparing versions
4908879 
A Git tag has a slightly different format from the one
`VersionCompare()` currently expects: A Git for Windows tag usually has
the `.windows.<patch-level>` suffix, and Git versions can have the
`.rc<N>` suffix.

Let's just skip identical non-numerical substrings when comparing
versions.

This also fixes the (theoretical) bug where `2.19a` and `2.19b` would
have been treated as identical versions.

Note: to allow for comparing, say, `v2.24.0.rc1.windows.1` to
`v2.24.0.windows.1`, we willfully rely on the fact that `r` has a lower
numerical value in ASCII than `w`.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
installer: do not warn about a "downgrade" from -rc<N> versions
53c07da 
When upgrading from, say, v2.24.0-rc2 to v2.24.0, we definitely do _not_
want a bogus "do you want to downgrade" warning.

To help the existing test, let's include the output of the `git version`
command and compare it with the just-enhanced `VersionCompare()`
function to the output of the to-be-upgraded Git for Windows version.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho requested a review from PhilipOakley October 30, 2019 13:22
@dscho
Copy link
Member Author

dscho commented Oct 30, 2019

@webstech could you have a look, too? You've been the second-most active developer in this area in the past year...

@PhilipOakley
Copy link
Contributor

I had a quick read through https://patch-diff.githubusercontent.com/raw/git-for-windows/build-extra/pull/264.patch and saw nothing objectionable.

There were a couple of bits I wasn't familiar with (i.e. stuff I could use as a learning point, e.g. why the #ifdef GIT_VERSION) but nothing that would stop its acceptance.

@dscho
Copy link
Member Author

dscho commented Oct 30, 2019

There were a couple of bits I wasn't familiar with (i.e. stuff I could use as a learning point, e.g. why the #ifdef GIT_VERSION) but nothing that would stop its acceptance.

Cool, cool!

The #ifdef GIT_VERSION is about a defined constant called GIT_VERSION, much like the C preprocessor constructs in Git's source code.

The GIT_VERSION in question is defined in config.iss which is generated by installer/release.sh.

There are more things that are defined in config.iss, it is essentially the dynamically-generated part of the installer.

I did realize on a walk I just took that I failed to account for downgrades to rc versions. In those cases, we would compare the installed version (e.g. 2.24.0, I think) to the filtered rc version (e.g. 2.4.0.1.1) and the current VersionCompare() would mislabel that not to be a downgrade.

Will work on that and update the PR.

@dscho
installer: do detect downgrades _to_ -rc<N> versions
dbbfe25 
The `-rc<N>` versions reduce to a registry value `CurrentVersion` that
looks like it is a _later_ version than the final version. Example:
v2.24.0.windows.1 would reduce to `2.24.0.1` while
v2.24.0.rc2.windows.1` would reduce to `2.24.0.2.1`, i.e. the -rc2
version would be mistaken for being an _upgrade_ relative to the final
v2.24.0.

To fix this, let's detect whether the version we are about to install,
or the version we want to upgrade, are potentially `-rc<N>` versions,
simply by counting the dots, and fall back to the (a little bit more
expensive) test via `git version` if that is the case.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Oct 30, 2019

I did realize on a walk I just took that I failed to account for downgrades to rc versions. In those cases, we would compare the installed version (e.g. 2.24.0, I think) to the filtered rc version (e.g. 2.4.0.1.1) and the current VersionCompare() would mislabel that not to be a downgrade.

@PhilipOakley there you go: dbbfe25

@dscho
Copy link
Member Author

dscho commented Oct 30, 2019

FWIW I want to merge this in time for v2.24.0 (and designed the PR that way, my initial idea was to write a registry entry in -rc<N> versions to indicate the "rc-ness" and consume that, but that would already break if anybody has installed -rc0 and wants to upgrade to the final release).

To make sure that this gets a lot of eyes, I will request reviews from a range of potential reviewers, hoping that this PR will get a lot of scrutiny.

@bbolli
Copy link
Collaborator

bbolli commented Oct 30, 2019

I find this way of doing version comparisons a bit brittle and hardly extensible (imagine if we had alpha and beta version numbers). The "right" way would be to build a tuple (prio, number) out of each segment of the version string. Stand-alone numbers get a high prio, rcN gets one less, beta and alpha one less each as well. Then you compare the list of tuples from left to right.

Other than that, the patches seem to do what you intend :-)

@PhilipOakley
Copy link
Contributor

The #ifdef GIT_VERSION is about..

.. it is essentially the dynamically-generated part of the installer.

Hadn't appreciated that part (I think you are saying this is dynamic at install time..?)

Hmm, re-reading, I see that that part (the GIT_VERSION is defined in the config.iss file, rather than being an if within the installer interrogating the installation (two different places, needing two different types of if) - mainly writing this out long hand for other readers (assuming I got it right 😉)

@dscho
Copy link
Member Author

dscho commented Oct 30, 2019

I find this way of doing version comparisons a bit brittle and hardly extensible (imagine if we had alpha and beta version numbers). The "right" way would be to build a tuple (prio, number) out of each segment of the version string. Stand-alone numbers get a high prio, rcN gets one less, beta and alpha one less each as well. Then you compare the list of tuples from left to right.

I'd rather cross that bridge when (if?) we need to... Think YAGNI. It would require a ton of work: we would need to build a new class for that, non-trivial parsing, etc. And: no regression test suite we can use to build/maintain confidence...

Other than that, the patches seem to do what you intend :-)

👍

the GIT_VERSION is defined in the config.iss file, rather than being an if within the installer interrogating the installation

Yes, GIT_VERSION reflects the version reported by git version as of the git.exe about to be installed, as opposed to the version reported by the installed git version.

And yes, the two different types of if reflect compile time vs run time.

@dscho
Copy link
Member Author

dscho commented Oct 30, 2019 via email •
edited
Loading

@dscho dscho merged commit 2e2d2bf into git-for-windows:master Oct 31, 2019
@dscho dscho deleted the upgrading-from-rc-versions-isnt-a-downgrade branch October 31, 2019 10:44
@dscho
Copy link
Member Author

dscho commented Oct 31, 2019

Thank you all, reviewers!

dscho added a commit that referenced this pull request Nov 4, 2019
@dscho
Mention Bug Fix in release notes
2623b41 
The Git for Windows installer [no longer complains about a
downgrade](#264)
when upgrading from an `-rc` version, i.e. from a pre-release leading
up to the next major version.

Signed-off-by: Johannes Schindelin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webstech webstech webstech left review comments

@nalla nalla nalla approved these changes

@PhilipOakley PhilipOakley Awaiting requested review from PhilipOakley

@bbolli bbolli Awaiting requested review from bbolli

@derrickstolee derrickstolee Awaiting requested review from derrickstolee

@drizzd drizzd Awaiting requested review from drizzd

@elieux elieux Awaiting requested review from elieux

@fourpastmidnight fourpastmidnight Awaiting requested review from fourpastmidnight

@kgybels kgybels Awaiting requested review from kgybels

@landstander668 landstander668 Awaiting requested review from landstander668

@orgads orgads Awaiting requested review from orgads

@shiftkey shiftkey Awaiting requested review from shiftkey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dscho @PhilipOakley @bbolli @nalla @webstech