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

git-update-git-for-windows: strip 'v' from fork tag #382

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

derrickstolee
Copy link
Contributor

When checking for a Git version from a fork such as microsoft/git, we
need to extract the tag name from a JSON object that we get from a REST
API. This differs from the git-for-windows/git mechanism of using the
gitforwindows.org/latest-tag.txt file. In the case of the latest-tag.txt
file, the letter 'v' is stripped from the tag name, allowing comparison
to the installed Git version, which does not include the 'v'.

The 'v' was not previously stripped from the tag name from the fork.
Rectify that, allowing comparisons that present an upgrade.


What confuses me about this change is that I don't understand how this ever worked.

To test, I inserted this change into my installed script:

@@ -253,6 +253,7 @@ update_git_for_windows () {
                # We are not testing and we don't have exact equality,
                # so do a careful comparison and look to see if the
                # latest release is strictly newer than ours.
+               echo "Comparing $version to $latest"
                if test 0 -lt "$(version_compare "$version" "$latest")"
                then
                        return

And I saw that $latest included a v while $version did not. This causes the first iteration of version_compare to think there is no number in the $b1 position, so it quits without presenting a new version.

When checking for a Git version from a fork such as microsoft/git, we
need to extract the tag name from a JSON object that we get from a REST
API. This differs from the git-for-windows/git mechanism of using the
gitforwindows.org/latest-tag.txt file. In the case of the latest-tag.txt
file, the letter 'v' is stripped from the tag name, allowing comparison
to the installed Git version, which does not include the 'v'.

The 'v' was not previously stripped from the tag name from the fork.
Rectify that, allowing comparisons that present an upgrade.

Signed-off-by: Derrick Stolee <[email protected]>
@dscho
Copy link
Member

dscho commented Oct 6, 2021

We could make it even safer by scraping off that v in a separate latest="${latest#v}". That way, we don't _expect_ a leading v` but if it's there, we remove it?

The previous change fixed a bug regarding a leading 'v' in a tag name.
Let's be extra careful to make sure our latest version does not have a
leading 'v'.

Signed-off-by: Derrick Stolee <[email protected]>
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

An even better place for the statement than I would have used! Thank you so much.

@dscho dscho merged commit 4676f28 into git-for-windows:main Oct 7, 2021
@PhilipOakley
Copy link
Contributor

Aside: Also is there a potential confusion between the dash-rc and dot.rc tag and version strings?

I had noticed that my previous rc2 version didn't auto-update, even if I ran the new update command, so I ended up by updating manually. I didn't manage to replicate the problem (only spend a few minutes on the re-installs to try it), but while looking at the code and the strings did notice the dot/dash difference.

It's also not clear how the rc candidates should be sorted anyway, depending on whether the user already has an rc candidate installed and/or a minor release appears. (Users opt in to rc candidates, but otherwise they by-pass them)

@derrickstolee
Copy link
Contributor Author

@PhilipOakley some of the -rc handling was done in #358. It does not expect a .rc, though.

derrickstolee added a commit to microsoft/git that referenced this pull request Oct 7, 2021
In 'git-update-git-for-windows', there is a recently_seen variable that
is loaded from Git config. This is intended to allow users to say "No, I
don't want that version of Git for Windows." If users say no, then they
are not reminded. Ever.

We want users of microsoft/git to be notified repeately until they
upgrade. The first notification might be dismissed because they don't
want to interrupt their work. They should get the picture within a few
reminders and upgrade in a timely fashion.

---

I tested this by running the `sed` command in my copy of `git-for-windows/build-extra`, and then replacing my installed version of `git-update-git-for-windows` in `C:\Program Files\Git\mingw64\bin`. Along the way, I discovered a bug in the script and created git-for-windows/build-extra#382. If you do your own testing, you will likely need that fix, too.

I also noticed a misplaced `&&` due to copying some existing scripts out of the workflow into a bash script so I could test all of the scripts simultaneously.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants