Prepare for release mc-4.8.19

[andrew_b]

Branch: 3693_cleanup.
Initial changeset:c0c14690d8bd81ca980af265230224ead9891b17

mc-3641-cleanup-Wformat-signedness-common_c.patch​: applied.
mc-3641-cleanup-Wformat-signedness-editdraw_c.patch​​: applied.
mc-3641-cleanup-Wformat-signedness-file_c.patch​​: applied.
mc-3641-cleanup-Wformat-signedness-find_c.patch​: applied.
mc-3641-cleanup-Wformat-signedness-ftpfs_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-hex_c.patch: applied with extra small change.
mc-3641-cleanup-Wformat-signedness-info_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-midnight_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-path_c.patch: another way was used.
mc-3641-cleanup-Wformat-signedness-serialize_c.patch: applied.
mc-3641-cleanup-Wunsafe-loop-optimizations.patch: applied.
mc-3641-cleanup-no-attribute-noreturn-utilunix.c.patch: applied.
mc-3641-cleanup-no-attribute-noreturn-utilvfs.c.patch: applied.
mc-3641-cleanup-unreachable-code-warning-utilunix_c.patch: applied.

[andrew_b]

3693-Hex-patterns-fix-manual-page.patch​: applied.

[mooffie]

FWIW: I've read all the commits in this branch (except "Clarify startup" and "fix location of right panel", for lack of time) and they seem alright.

(As for ditching negative numbers in hex patterns (hex.c): that's a blessing (at least as long as nobody volunteers to implement reading words/dwords/etc). Besides, reading them with "%x" meant that if you typed "-10" you got -16.)

[mooffie]

@Andrew:

re: "Clarify usage of assert":

There are two places that use g_assert() (and 3 places that use g_assert_*()).

The purpose of AC_HEADER_ASSERT is to make --disable-assert define the NDEBUG constant, which disables assert(). But to disable g_assert() one has to define the G_DISABLE_ASSERT constant.

So, it seems to me, we should choose either assert() or g_assert(), not both.

It's easier to just rename the two g_assert() we have to assert().

(One may argue that there are a few g_assert_*() functions we may covet (like g_assert_cmpint), and that for the sake of stylistic uniformity we should therefore prefer g_assert() to assert(). But these g_assert_*() bunch can't be disabled, so we may treat them as a category of their own.)

Not that I care too much about any of this. It's just that I'm about to propose a patch in which I was planning to use g_assert() / assert() and I don't want to get some flak ;-)

[andrew_b]

Replying to mooffie:

So, it seems to me, we should choose either assert() or g_assert(), not both.

Indeed.

I prefer g_assert() and I've added the commit.

[zaytsev]

@andrew_b: just a nitpick, the plural of assert is not "Asserts:", but "Assertions:".

[zaytsev]

Here too: "Define to disable asserts" -> "Define to disable assertions".

[andrew_b]

Rebased to current master: [e036c1fb12757dc9d258862736054c33313a51d5].

[mooffie]

This documentation is needed in order to understand the bug(s) in #3720.

[andrew_b]

3693-mc_search_run-document-the-return-value.patch​: applied.

Rebased to current master: [645e88d209a520e6c83eeced710b9376f878c8f8].

[andrew_b]

Merged to master: [24a16003a8c01faaeb4e907e674c7bddb36e1424].

git log --pretty=oneline da538f0..24a1600

[andrew_b]

(In #3703) Fixed as [eec71bccff53f8e73e3bbc27f4c3bfb03221388b].

[andrew_b]

Next series: 3693_cleanup
Initial changeset:9231e10791fa313a04afcf7eb78e671ea0ac3d47

[mooffie]

(This documentation is needed to better understand the patch I'll have for #3589.)

[andrew_b]

3693-mc_search__cond_struct_new_regex_ci_str-add-documentation.patch: applied. Thanks!

[mooffie]

The following patch is too trivial for a separate ticket. It improves the highighting of comments in some syntax files.

# list all syntax files with comments not on 1st column:

$ grep -P '^\s+#' -l *

as.syntax
ebuild.syntax
jal.syntax
js.syntax
lkr.syntax
lua.syntax
m4.syntax
yaml.syntax

(The patch doesn't add support for comments preceded by TABs, though. But that's no biggie.)

[andrew_b]

3693-mcedit-syntax.syntax-highlight-comments-preceded-by-spaces.patch: applied.

[andrew_b]

mc-3693-dialogs.c-Cleanup-maybe-uninitialized-compiler-warni.patch​: [1]
mc-3693-edit.c-Cleanup-unsafe-loop-optimizations-compiler-wa.patch​: [2]
mc-3693-editcmd.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-editcmd_dialogs.c-Cleanup-maybe-uninitialized-compil.patch​: [1]
mc-3693-execute.c-Cleanup-maybe-uninitialized-compiler-warni.patch​: [1]
mc-3693-file.c-Cleanup-maybe-uninitialized-compiler-warning.patch​: [1]
mc-3693-internal.c-Cleanup-format-warning.patch​: applied.
mc-3693-lib.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-main.c-Cleanup-maybe-uninitialized-compiler-warning.patch: [1]
mc-3693-panel.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-search.c-Cleanup-maybe-uninitialized-compiler-warnin.patch​: [1]
mc-3693-syntax.c-Cleanup-unsafe-loop-optimizations-compiler-.patch: [2]

[1] squashed in the single commit.
[2] I can't see something wrong in these loops.
[3] squashed in the single commit.

[andrew_b]

We have strange ​build error:

  CC     file.lo
../../../../src/vfs/sftpfs/file.c: In function ‘sftpfs_open_file’:
../../../../src/vfs/sftpfs/file.c:156:16: error: missing initializer [-Werror=missing-field-initializers]
../../../../src/vfs/sftpfs/file.c:156:16: error: (near initialization for ‘file_info.st_ino’) [-Werror=missing-field-initializers]

gcc-4.6.3 is used.

The code is following:

155     {
156         struct stat file_info = { 0 };
157 
158         if (sftpfs_fstat (file_handler, &file_info, mcerror) == 0)
159             libssh2_sftp_seek64 (file_handler_data->handle, file_info.st_size);
160     }

What's wrong here?

My local build with gcc-5.3.1 is finished successfully.

[zaytsev-work]

Try struct stat file_info = { }; ? I think that it complains, because if you specify the first 0, then it thinks that you should you should go all the way and specify all of them, but this annoying behaviour for this warning was changed in later GCCs.

[andrew_b]

Replying to zaytsev-work:

Try struct stat file_info = { }; ?

This was in the original patch. The result was ​same:

  CC     file.lo
../../../../src/vfs/sftpfs/file.c: In function ‘sftpfs_open_file’:
../../../../src/vfs/sftpfs/file.c:156:16: error: missing initializer [-Werror=missing-field-initializers]
../../../../src/vfs/sftpfs/file.c:156:16: error: (near initialization for ‘file_info.st_dev’) [-Werror=missing-field-initializers]

[zaytsev-work]

Oh, sorry, stupid me, struct stat file_info = { }; isn't even valid C code according to the standard (which can be witnessed via -pedantic):

​http://stackoverflow.com/questions/17589533/is-an-empty-initializer-list-valid-c-code

It's only valid in C++, so your change to struct stat file_info = { 0 }; is absolutely correct.

Anyways, I have found the GCC bug report regarding { 0 } in C, and, unfortunately, it only made it in GCC 4.7.x:

​https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36750

However, it seems a workaround for GCC 4.6.x is possible:

​http://stackoverflow.com/a/27461062

struct stat file_info = { .st_dev = 0 };

Maybe this will work? The only other thing that comes to mind is to mess with pragmas, but it smells so badly that I wouldn't consider this as an option... :-(

[andrew_b]

Replying to zaytsev-work:

Maybe this will work?

Yes, it works. Thanks!

[and]

Replying to andrew_b:

mc-3693-edit.c-Cleanup-unsafe-loop-optimizations-compiler-wa.patch​: [2]

[2] I can't see something wrong in these loops.

GCC complains about unsafe loop optimization when "... <= VALUE" found,
because internal comparing works like "... < (VALUE+1)" and possible VALUE=MAX_INT
will overflow, hence unsafe loop optimization possible.

Last section at
​https://stackoverflow.com/questions/2982507/what-is-the-explanation-for-warning-assuming-that-the-loop-is-not-infinite

[andrew_b]

Replying to and:

Last section at
​https://stackoverflow.com/questions/2982507/what-is-the-explanation-for-warning-assuming-that-the-loop-is-not-infinite

I found this webpage too. I think GCC is some paranoiac here.

[andrew_b]

Rebased: [2b8617122ecdf07ad04350afba6168cf3a5c2e93].

Please review.

[andrew_b]

Merged to master: [9845fa6ce542c5c45ab9a86140052dd3ded03655].

git log --pretty=oneline 43d7fce..9845fa6

[zaytsev]

Andrew, are you fine with committing the patch above?

[mooffie]

Replying to zaytsev:

Andrew, are you fine with committing the patch above?

My patch? No need to. It was eventually committed in #3760 (I posted it here at a time it seemed my solution for #3760 would be rejected). I've just deleted my comment+patch here.

[zaytsev]

Ah, ok thanks, sorry, I've got completely confused in the end.

[zaytsev]

It seems that all is calm, so I'm planning to pull in latest translations and make the release this weekend, probably on Sunday, unless there are objections to this plan.

[egmont]

Sounds great!

[zaytsev]

download PO-translations from Transifex.net

done

store translations in git repo

done

download the hint translations from Transifex.net

done; nothing new

store translations in git repo

skip

create new NEWS wiki page for next version with empty template

done; NEWS-4.8.20

add content of current NEWS wiki page to the doc/NEWS file

done

create new tag in git by command

done

new version in Trac

done

new milestone in Trac

done

create tar.(bz2|xz) package files

done

make checksums for archives

done

upload source packages and checksums to the special upload area

done

developers should download tarballs, verify checksums, compile and install locally; if everything is ok, then developers vote for the ticket

​http://midnight-commander.org/nopaste/tarball/

please!

[egmont]

I hardly ever remember to run "make check", but when I do:

FAIL: utilunix__my_system_fork_child_shell
FAIL: utilunix__my_system_fork_child

The corresponding log files contain this:

Running suite(s): /lib/utilunix


fork () = -1
FAIL utilunix__my_system_fork_child (exit status: 1)

The tests succeed for me with the 4.8.18 tarball.

[zaytsev]

Unfortunately, you are right, good catch! I have reproduced the problem on my Fedora VM, but I have no idea what that is. These tests are failing permanently on Travis, which I why I assumed it has something to do with Travis environment proper. I've started a bisection run between 4.8.19 and 4.8.18 to narrow down the culprit, but it's progressing very slowly.

[egmont]

I'm also running git bisect, it's almost finished :) You can stop yours.

Just wondering, is "make check" not executed in the continuous build, and as part of the release process? If so, at least the release process documentation should be extended with this entry.

[egmont]

b91ab44b43570ebebe58627efedcb2adb4066023 is the first bad commit

[zaytsev]

I'm also running git bisect, it's almost finished :) You can stop yours.

I've got ~7 commits to go, so I guess I'll leave it running anyways just to see if I can match your results ;-)

Just wondering, is "make check" not executed in the continuous build

It definitively is. The problem is, some tests fail on Travis for mysterious reasons and Andrew couldn't reproduce it, and others are failing for reasons that I understood, but was not able to find time to fix so far. Therefore, I had to make Travis builds only fail when the build itself fails, not upon test failures, which obviously make real problems easy to overlook... :-( but it's better than not running them at all, at least I sometimes inspect the logs visually.

as part of the release process? If so, at least the release process documentation should be extended with this entry.

Actually, implicitly it is, I just skipped it because it all takes so long on my underpowered laptop :-/ but you are right, this is not good. I have added it to the guidelines.

[egmont]

Replying to zaytsev:

I've got ~7 commits to go, so I guess I'll leave it running anyways just to see if I can match your results ;-)

Mandatory link: #2252 :P

Actually, implicitly it is, I just skipped it because it all takes so long on my underpowered laptop :-/ but you are right, this is not good. I have added it to the guidelines.

Thanks!

[zaytsev]

b91ab44b43570ebebe58627efedcb2adb4066023 is the first bad commit

Confirm b91ab44b43570ebebe58627efedcb2adb4066023 ! Good thing the commit is so small, that's why we want them to be like this --- upon problems bisect points you directly to where the trouble is. But I'm confused as to why marking noreturn function as such makes the test fail. Need to have a closer look later :-/

[zaytsev]

Mandatory link: #2252 :P

Well, the way I do bisects is as follows, and this is already a shortcut, so in this particular case #2252 is irrelevant

[zaytsev@localhost mc]$ git bisect run sh -c '(./autogen.sh && rm -rf build && mkdir -p build && cd build && ../configure && make check)'

[zaytsev]

Ha, maybe I should have read the comment why this function was introduced in the first place:

 * The _exit() function has gcc's attribute 'noreturn', and this is reason why we can't
 * mock the call.

So apparently the warning should have been suppressed instead!

[egmont]

Replying to zaytsev:

[zaytsev@localhost mc]$ git bisect run sh -c '(./autogen.sh && rm -rf build && mkdir -p build && cd build && ../configure && make check)'

I didn't know about this possibility, I'm always doing it "manually".

But... apparently this construct cannot differentiate between failed tests (where you'd type "git bisect bad" and failed builds ("git bisect skip").

Also it cannot handle failed checkouts which happen every now and then (an autogenerated file becomes a shipped one or vice versa [you prevent these with a separate build directory, I'm lazy and building in the source tree], or .po files sometimes being regenerated locally for reason I haven't tracked down yet).

Anyway, it's always good to learn new practices :)

[zaytsev]

I didn't know about this possibility, I'm always doing it "manually".

w00t!!!11 you are a man of great patience... I always start bisects in the background if results are easily verifiable by script and do other stuff while they are running.

But... apparently this construct cannot differentiate between failed tests (where you'd type "git bisect bad" and failed builds ("git bisect skip").

This is just because I know that there will be no failed builds in master, but otherwise I would add make || exit 125 to tell git to skip the commit if the build fails.

Okay, as to the commit, I propose a revert + a follow up commit to clarify that the comment function should not be decorated.

Then I will make a new tag and rebuild the tarballs. I haven't pushed the tag out yet.

[zaytsev]

Re-built and re-uploaded the tarballs.

[zaytsev]

And again, today is definitively not my day :-(

[egmont]

What was wrong with the comment on the same line?

[zaytsev]

Replying to egmont:

What was wrong with the comment on the same line?

It confused GNU Indent and as it tried to reformat the thing, it was breaking the formatting completely, but somehow I didn't notice it until it was too late :-/ Oh well, the bottom line, it likes it when it's on a separate line and it seems that all is good now.

[zaytsev]

upload source packages and checksums

done

run command: ...

done

update Wiki start page with latest release number

done

write an announcement: list user visible changes (bugs and features)

done

close current milestone

in a moment

create new ticket (type=task, component=adm) for the next release

done; #3780

close ticket for release

done

[zaytsev]

Hmmmm... apparently forgot to close this.