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

Fix wrong format specifier in debug message #2645

Merged
merged 1 commit into from
Nov 29, 2020
Merged

Fix wrong format specifier in debug message #2645

merged 1 commit into from
Nov 29, 2020

Conversation

eht16
Copy link
Member

@eht16 eht16 commented Nov 7, 2020

This led to crashes on Windows when starting without an existing
configuration file.

Closes #2639.

elextr
elextr reviewed Nov 7, 2020
View reviewed changes
src/libmain.c Outdated Show resolved Hide resolved
@eht16 eht16 mentioned this pull request Nov 7, 2020
Closed
b4n
b4n reviewed Nov 7, 2020
View reviewed changes
src/libmain.c Outdated
@@ -406,7 +406,7 @@ static gint get_windows_socket_port(void)
if (! g_file_test(configfile, G_FILE_TEST_IS_REGULAR))
Copy link
Member

Choose a reason for hiding this comment

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

This code could actually be simplified by basically having the same code path for the with and without config cases: just don't load the config if it doesn't exist, and then utils_get_setting_integer() will return the default. But that's not for a point release.

Copy link
Member Author

@eht16 eht16 Nov 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done in 58c5e34.
Thanks!

b4n
b4n approved these changes Nov 7, 2020
View reviewed changes
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

LGTM. Changing the wording might be good and I'm not against it here, but as a rule of the thumb I usually prefer to avoid unnecessary changes in bugfix releases.

@b4n b4n added this to the 1.37.1 milestone Nov 7, 2020
@eht16
Copy link
Member Author

eht16 commented Nov 7, 2020

LGTM. Changing the wording might be good and I'm not against it here, but as a rule of the thumb I usually prefer to avoid unnecessary changes in bugfix releases.

Yeah, right you are.
What about cherry-picking 3ab2518 into the 1.37 branch, update NEWS et. al and release.
In this PR we could then afterwards simplify the code as suggested for 1.38+.

@b4n
Copy link
Member

b4n commented Nov 7, 2020

What about cherry-picking 3ab2518 into the 1.37 branch, update NEWS et. al and release.
In this PR we could then afterwards simplify the code as suggested for 1.38+.

Sounds good to me. I prep'd the 1.37 branch you created, and cherry-picked the first commit you mentioned. I won't do the whole release stuff tonight, but probably doable tomorrow.

@eht16 eht16 force-pushed the issue2639_fix_windows_debug_message_crash branch from 3efe4a8 to 58c5e34 Compare November 15, 2020 11:52
b4n
b4n approved these changes Nov 15, 2020
View reviewed changes
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

LGTM, and I trust you have actually tested this on Windows, with and without initial config 😉

@eht16
Copy link
Member Author

eht16 commented Nov 15, 2020

I trust you have actually tested this on Windows, with and without initial config wink

Oh yes. I'm constantly trying to do the same mistake only once :).

@eht16 eht16 merged commit d2740f2 into geany:master Nov 29, 2020
@eht16 eht16 deleted the issue2639_fix_windows_debug_message_crash branch November 29, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elextr elextr elextr left review comments

@b4n b4n b4n approved these changes

Assignees
No one assigned
Labels
bug windows
Projects
None yet
Milestone
1.37.1
Development

Successfully merging this pull request may close these issues.

geany would not open (Windows 10)
3 participants
@eht16 @b4n @elextr