Update to 2.3.0

[dscho]

This updates our code base to Git 2.3.0.

[dscho]

@sschuberth @t-b This is the big update that targets MSys2. However, I need your help: two tests fail, still.

The first one, t0027, seems to be (at least partially) due to the warning being printed that LF will be replaced by CRLF. The fix might be simply to disable the test, but I would really like to understand first why the same four tests pass on Linux but not with Windows.

The second one, t0204, is even more curious. Apparently the Icelandic messages are mangled into some ISO encoding even when LC_ALL=is_IS.utf8. My hunch is that it has something to do with the console's encoding that overrides LC_ALL or some such.

I disabled t9020 wholesale because I think it is too much work to work around the path mangling issues. If you disagree, please feel very free to drop this commit and to work on making it pass.

[dscho]

BTW I do not think that our merging rebase script would handle it well if we merged this PR; I think the reviewer should push directly to git-for-windows' master when satisfied.

[nalla]

I was able to pass some tests in t0204 by changing the gettext command from gettext to /bin/gettext

eg.

test_expect_success GETTEXT_LOCALE 'gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic' '
    printf "TILRAUN: Halló Heimur!" >expect &&
    LANGUAGE=is LC_ALL="$is_IS_locale" /bin/gettext "TEST: Hello World!" >actual &&
    test_cmp expect actual

Maybe that'll help you identifying the problem.

[dscho]

@nalla oh, but those tests pass for me (but then, I ran them in a slightly odd way...)! The test that does not pass is the gettext.c: git init UTF-8 -> UTF-8 one. For some strange reason, the output of git init repo does not use UTF-8 encoding, but in ISO-8859 encoding (according to the file utility). I imagine that it has something to do with a hard-coded encoding of the terminal.

But you're correct, if I run the tests again with an MSys2 32-bit shell, there are more failures in t0204... Odd.

[dscho]

I was able to pass some tests in t0204 by changing the gettext command from gettext to /bin/gettext

Ah, I now notice that I ran my tests with /usr/bin/ before /mingw32/bin/ in the PATH...

[dscho]

Ah, and now I see that there is no locale.exe in /mingw32/bin/, but only in /usr/bin/! That means that the gettext.exe and the locale.exe executables are incompatible, unless I get a locale.exe to compile in the mingw-w64-i686-gettext package.

[nalla]

Maybe the missing locale.exe is also the problem for LANG=is LC_ALL=is_IS.utf-8 git init repo then.

I dont think its a terminal issue. Because if you use the msys2 git (LANG=is LC_ALL=is_IS.utf-8 /bin/git init repo) it works.

[dscho]

Maybe the missing locale.exe is also the problem for LANG=is LC_ALL=is_IS.utf-8 git init repo then.

Yep, it appears you are correct. See the latest commit. BTW are you okay with the credit? Or do you prefer to go by "Manuel"?

[nalla]

마누엘 is ok. Yeah i also found out that locale is msys2 only by running pkgfile --update and pkgfile locale.exe

[t-b]

@dscho Jeep I'd also say that the failings in t0027 stem from the recent changes 0291973.

I've just installed msys2 according to the docs and run the git tests on win7pro x64. Only t0027 fails. I ran the tests with prove, so I needed to call export LC_ALL=C before as otherwise perl always complained that is does not know my locale ("DE").

[t-b]

So the test failure in t0027 is as follows:
With the gitattributes

$ cat .gitattributes
*.txt text=auto

when adding the file

 ../../git -c core.autocrlf=false add crlf_false_attr_auto_LF.txt

a warning message like

warning: LF will be replaced by CRLF in crlf_false_attr_auto_LF.txt.
The file will have its original line endings in your working directory.

is outputted whereas the test expects no warning message here.

[nalla]

I looked in convert.c for an answer. the function output_eol returns somehow EOL_CRLF. Thats the cause of the warning.

But EOL_CRLF is only returned when the crlf_action is CRLF_AUTO && core.autocrlf true or CRLF_CRLF. I believe the 2nd one is the the case.

I dug a little deeper. If you set core.eol to lf before you add the file you won't get a warning. Maybe the test is presuming that core.eol is set to lf by default. Maybe on MinGW that is not the case?

[t-b]

@nalla Good call. On mingw we have crlf as native eol. See https://github.com/git-for-windows/git/blob/master/config.mak.uname#L515.

[nalla]

So we have two possibilites now? Either adapt the test when MinGW is used or do not run it when using MinGW.

[dscho]

Yep, in my brief debugging tests, I saw that the NATIVE EOL was returned by output_eol. The funny thing is that even if I recompiled GIt on Linux with the native eol set to crlf, the test passed.

Therefore, I am really puzzled and I think we'll have to single-step the problem on MSys2 and Linux simultaneously to figure out what is going wrong.

If nobody beats me to it, I will do that next week.

[lygstate]

Hope there will be a binary release of MSYS2 with git 2.3.0, really great work from @dscho

[dscho]

BTW I just checked with msysGit, and it looks as if the same four test cases of t0027 fail there, too.

[dscho]

Success! d4dbf57 addresses the test failures; They were indeed caused by flawed assumptions of the test script.

Before merging, I would like to reorder the new commits into the appropriate branches of the thicket (e.g. the test fixes into the win-test-fixes branch).

To make it easier to see what I did, I think it is better to write a little helper to compare thickets: list commits that were dropped, ones that were added, ones that were replaced/split/squashed and reordered, and ones whose diffs or commit message had to be adjusted. I hope that it will not take a long time to write that helper ;-)

[t-b]

@dscho Cool!

[nalla]

On msys64 t7800 still fails me. Can anyone confirm this? The test tries to run git difftool --no-prompt --extcmd sh -c "cat $2" branch >actual. cat fails with C:UsersNallaAppDataLocalTemp/2f0vd9_file: No such file or directory. Seems to be a pathing issue when C:UsersNallaAppDataLocalTemp/2f0vd9_file was put into $2.

Installed msys2-runtime: msys2-runtime 2.0.16587.894e323-1

[dscho]

@nalla hmm. I put in a work-around (60048d2) and hoped that that would suffice. Maybe you can put in some debug statements to find out? I.e. something like:

diff --git a/compat/mingw.c b/compat/mingw.c
index 3437810..ea9b8d0 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2241,9 +2241,11 @@ void mingw_startup()
             * for escape characters.
             */
            char *p;
+error("TMP before: %s", environ[i]);
            for (p = environ[i]; *p; p++)
                if (*p == '\\')
                    *p = '/';
+error("TMP after: %s", environ[i]);
        }
    }
    environ[i] = NULL;

[nalla]

Mmh. I get a segmentation fault when compiling git with that patch...

[dscho]

I get a segmentation fault when compiling git with that patch...

Strange indeed. You should be able to identify which git call causes that segfault, and then run it in gdb by prefixing the call with TEST_GDB_GIT=1 and running the test via sh -x t7800*.sh -i -v.

[nalla]

I think i know what's the problem now.

When I start up the mingw64_shell, I have four different environment variables:

$ sh -c "echo $tmp"
C:UsersNallaAppDataLocalTemp
$ sh -c "echo $temp"
C:UsersNallaAppDataLocalTemp
$ sh -c "echo $TMP"
/tmp
$ sh -c "echo $TEMP"
/tmp

When I change if (starts_with(environ[i], "TMP=")) { into if (starts_with(environ[i], "tmp=")) { within mingw.cand then recompile, the test passes.

@dscho I believe that is that case sensitive environment variable thing again. Maybe the replace function should check for both variables. upper and undercase?

[sschuberth]

Right. On Windows, environment variable names are case-insensitive, just like file names.

[nalla]

I believe https://github.com/dscho/git/blob/update-to-2.3.0/compat/mingw.c#L2258 then needs an update too then.

[dscho]

I thought I saw that the environment variable keys are up-cased somewhere... but maybe that was in the msys2-runtime source code...

[dscho]

For the record, I squashed these two commits in, too: dscho/git@11ef22a^^...11ef22a (this lets the test suite pass also on MacOSX).

[sschuberth]

Doh, you're right, I was somehow looking at the wrong diffs. My patch is in upstream git-gui, but Junio did not pull that yet.

But we do not have the -f flag in the kill.exe executable shipping with msysGit-based Git for Windows, therefore I would be reluctant to drop the commit.

It's the other way around: In msysgit we have a kill.exe that understands -f, see https://github.com/msysgit/msysgit/blob/master/src/rt/patches/0004-Added-Cygwin-implementation-of-kill.patch. We used to have that in the MSYS1-based Git SDK, too, but I've removed it from there since we have 218f9b1 now.

Long story short, I we must keep the patch here as neither the MSYS1-based nor the MSYS2-based Git SDK now ship with a kill.exe that understands -f.

[nalla]

@dscho I created a patch for t9118, t9124 and t9130 - https://gist.github.com/nalla/b1a9c7575546ca99ae09. If you seem it fit you may just include the changes. Bigger patch for t9100 is in the pipeline..

[dscho]

@nalla very nice! Let's include your work before merging. (No stress with t9100, I am currently fixing the problem where OpenSSH finds the wrong home directory, and after that I will still be busy with the portable application release script).

[nalla]

@dscho wile trying to fix t9100, I believe I stumbled upon a possible bug. The test tries to do a git init --separate-git-dir ../gitdir and fails, while the builtin msys2 git succeeds. You can reproduce this with the following setup:

$ mkdir w g
$ cd w
$ /usr/src/git/git.exe init
$ /usr/src/git/git.exe init --separate-git-dir ../g
# fatal: unable to move C:/path/to/w/.git to C:/path/to/g: Is a directory
$ git init --separate-git-dir ../g
# Reinitialized existing Git repository in /path/to/g/

https://github.com/git/git/blob/18d0fec24027ac226dc2c4df2b955eef2a16462a/builtin/init-db.c#L366 is the line that throws the error. somehow rename(src, git_dir) does not succeed.

[dscho]

@nalla indeed, it looks as if the rename() call succeeds on MacOSX, but only if the target directory is empty. This agrees with the man page:

The rename() system call causes the link named old to be renamed as new.
If new exists, it is first removed. [...]

However, Git for Windows has to use the mingw_rename() emulation of that syscall which just errors out if the target exists and is a directory.

So indeed, you found a bug, and the proper solution is to try to delete the directory (which will fail if the directory is not empty) and repeat. Something like this (beware: completely untested):

diff --git a/compat/mingw.c b/compat/mingw.c
index fac8a11..ee5947e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1655,7 +1655,12 @@ repeat:
    if (gle == ERROR_ACCESS_DENIED &&
        (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
        if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
-           errno = EISDIR;
+           DWORD attrsold = GetFileAttributes(wpold);
+           if (attrsold == INVALID_FILE_ATTRIBUTES ||
+                   !(attrsold & FILE_ATTRIBUTE_DIRECTORY))
+               errno = EISDIR;
+           else if (!_wrmdir(wpnew))
+               goto repeat;
            return -1;
        }
        if ((attrs & FILE_ATTRIBUTE_READONLY) &&

[dscho]

@nalla would you have time to mangle this diff into a proper commit, test, and possibly fix? (I am awfully short on time today.)

[nalla]

I'll do that, is a git format-patch per gist like the last one ok for you?

[dscho]

@nalla for me, the easiest would be a Pull Request (like the ones you already opened). But I can also use a formatted patch if that is more convenient for you.

[nalla]

I issued PR dscho#8. It includes the svn test fixups and the fix for the mingw rename.

[dscho]

@sschuberth @t-b I really would like to move forward with this, in particular because version 2.3.1 is already out and I should continue with a merging rebase to said version...

Is it okay with you? As discussed earlier, I would then simply push to (rather than merge with) master, to make subsequent merging rebases easier.

[sschuberth]

@dscho Yes, please go ahead. I think this is good enough already.

[dscho]

@dscho Yes, please go ahead. I think this is good enough already.

Okay, I actually already finished the merging rebase to 2.3.1 (these rebases are really much easier if you do them regularly), and the test suite passes on Linux (in just under 7.5 minutes); the test suite still runs on the Windows VM (I run it with prove for the first time, woohoo!). Once that's done, I'll push directly to git-for-windows' master.

[dscho]

... and on the Windows VM, after almost 160 minutes real time (~30 minutes user, ~55 minutes sys), the test suite passed! So I pushed the update-to-2.3.1 branch to master and tagged it for good measure. More on Sunday.

[sschuberth]

For the record, the MSYS(1)-based CI only reports 3 failures for Git 2.3.1 compared to 8 before. In particular, t9020 is not failing anymore, as expected. But t5000 and t7400 still fail partly.

[dscho]

Yep, the 2 t5000 failures are due to my 3153820. My hunch is that we probably need something like this:

if test_have_prereq MINGW
then
    case "$(uname -r)" in 1.0*)
        function tr () {
            /bin/tr.exe "$@" | dos2unix --force
        }
        ;
    esac
fi

But I am reluctant to work on letting the test suite pass with the obsolete msysGit-built Git now, in particular when the issues are known to be fixed with MSys2-built Git...

[sschuberth]

Ok, so it's clear now why t5000 fails with MSYS1 (details here), and about t7400 with MSYS1 the details are here.

[dscho]

Thanks for investigating! I had a brief look and had to give up for the moment...