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

Missing support for ISO 8601 date formats #2495

Closed
GisoBartels opened this issue Jan 28, 2020 · 17 comments
Closed

Missing support for ISO 8601 date formats #2495

GisoBartels opened this issue Jan 28, 2020 · 17 comments
Milestone

Comments

@GisoBartels
Copy link

The following command fails, but works fine on git for Linux and MacOS

$ git show -s --format=%cd --date=format:‘%g.%V’ HEAD
fatal: invalid strftime format: '‘%g.%V’'

$ date +%g.%V
20.05

$ git --version
git version 2.25.0.windows.1

I found a potentially related issue here:
https://stackoverflow.com/questions/42568215/iso-8601-week-number-in-c

Cause could be an outdated MSVCRT.

@rimrul
Copy link
Member

rimrul commented Jan 29, 2020

Could you test link with -lucrtbase? That works around it according to MinGW-w64 issue 793

@dscho
Copy link
Member

dscho commented Jan 29, 2020

link with -lucrtbase?

I guess that would break support for Windows Vista and for Windows 7, though, right?

@rimrul
Copy link
Member

rimrul commented Jan 29, 2020

Should still work with Vista SP2/ 7 SP1/8 if they have update 2999226, according to Microsoft.

@dscho
Copy link
Member

dscho commented Jan 29, 2020

Should still work with Vista SP2/ 7 SP1/8 if they have update 2999226, according to Microsoft.

I don't think that would be easy. You cannot just link to two different MSVC runtimes at the same time. And for compatibility, we have to keep linking to the MSVC runtime that GCC links to.

What might be possible is to use LoadLibrary() to load ucrtbase and then GetProcAddress() to load the function and use it if available, similar to what we do in compat/win32/lazyload.h (we might even use those DECLARE_PROC_ADDR()/INIT_PROC_ADDR() macros, using a file-local variable to keep a copy of the old strftime() just in case we have to fall back to it if we cannot load the new one.

@rimrul wanna give it a try?

@dscho
Copy link
Member

dscho commented Jan 29, 2020

@rimrul wanna give it a try?

(I would have asked @GisoBartels but they seem to be most prolific with Java/Kotlin, not with C.)

@GisoBartels
Copy link
Author

Exactly, thanks. Currently I don't even have a C development environment set up.

@rimrul
Copy link
Member

rimrul commented Jan 30, 2020

I can certainly try. I'm a little bisy until about halfway through february, so it'll probably happen after that.

@dscho
Copy link
Member

dscho commented Jan 30, 2020

Currently I don't even have a C development environment set up.

That's easily fixed, though: download and install the Git for Windows SDK. After that, a Git SDK Bash will pop up and you can build Git via sdk build git.

So if you want to take your fate in your own hands, that'll be easy.

@rimrul
Copy link
Member

rimrul commented Feb 14, 2020

Ok, I have a working prototype, but apparently t0014.4 is currently broken, even on master. There's some rouge line

run-command.c:662 trace: run_command: 'F:\git-sdk-32\mingw32\bin\busybox.exe' --help

in the output of GIT_TRACE=1 git frotz a "" b " " c. I definitely didn't introduce any busybox calls.

The test suite on the 32 bit SDK is really slow for me. t2016 alone is taking over 60 minutes on actual hardware. I dread running it on the vm that I set up for testing without ucrtbase.dll, but I guess it has to pass both with and without that.

Can I run make test with parallel jobs?

@dscho
Copy link
Member

dscho commented Feb 14, 2020

There's some rouge line

run-command.c:662 trace: run_command: 'F:\git-sdk-32\mingw32\bin\busybox.exe' --help

in the output of GIT_TRACE=1 git frotz a "" b " " c. I definitely didn't introduce any busybox calls.

This is Git for Windows trying to figure out whether BusyBox can serve a command that was not found on the PATH: see e41c1e6 for details.

The test suite on the 32 bit SDK is really slow for me. t2016 alone is taking over 60 minutes on actual hardware. I dread running it on the vm that I set up for testing without ucrtbase.dll, but I guess it has to pass both with and without that.

Typically, the reason for that is Windows Defender. I always add exclusions in my test setups.

Can I run make test with parallel jobs?

Yes, absolutely! I frequently run it with make -C t -j$(nproc). We do the something similar in our CI/PR builds, although we use prove, see https://github.com/git-for-windows/git/blob/v2.25.0.windows.1/ci/lib.sh#L134.

@rimrul
Copy link
Member

rimrul commented Feb 14, 2020

Would it make sense to either add an exception to that busybox code for git commands (because I don't think they'd ship such a command) or alternatively teach t0014 about the possibility of busybox showing up in the trace output?

@dscho
Copy link
Member

dscho commented Feb 15, 2020

Would it make sense to either add an exception to that busybox code for git commands

The way I read the code, it should only spawn busybox --help if we try to launch a program that does not exist in the PATH. But git.exe is in the PATH, so either we're looking at a completely different program, or there is a bug in the code. Which one is it?

@rimrul
Copy link
Member

rimrul commented Feb 15, 2020

We're looking for a dashed git command that (intentionally) does not exist. git-frotz.exe

@dscho
Copy link
Member

dscho commented Feb 16, 2020

Ah! Then yes, the busybox --help call here is totally inappropriate. We should probably enhance the "is this not in PATH" check by "does it not start with git-".

@dscho
Copy link
Member

dscho commented Feb 17, 2020

I guess the best solution would be to guard this code behind a config option, e.g. windows.useBusyBox or core.useBusyBox. The ideal place to handle this (which I should also imitate for core.longpaths) should be in one of the read_early_config() calls.

What do you think, @rimrul?

@rimrul
Copy link
Member

rimrul commented Apr 3, 2020

I've finally got around to figuring out what went wrong with my 32bit SDK. #2560. Perl dlls needed to be rebased to fork. Suddenly not every subtest for interactive tests caused dozens of failed fork attempts. That really does wonders for performance. I'm still surprised that only a couple of them broke.

I rebased my prototype onto master and am testing that right now.

@dscho dscho closed this as completed in 6b80b39 Apr 7, 2020
@dscho dscho added this to the Next release milestone Apr 7, 2020
git-for-windows-ci pushed a commit that referenced this issue Apr 7, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 7, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 7, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit to dscho/git that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation
that supports more date formats. We link git against the older
"Microsoft Visual C Runtime Library" (MSVCRT), so to use the UCRT
strftime() we need to load it from ucrtbase.dll using
DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows
update, but in some cases only MSVCRT might be available. In that case
we fall back to using that implementation.

With this change, it is possible to use e.g. the `%g` and `%V` date
format specifiers, e.g.

	git show -s --format=%cd --date=format:‘%g.%V’ HEAD

Without this change, the user would see this error message on Windows:

	fatal: invalid strftime format: '‘%g.%V’'

This fixes git-for-windows#2495

Signed-off-by: Matthias Aßhauer <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
dscho pushed a commit that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
gitster pushed a commit to git/git that referenced this issue Apr 8, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation
that supports more date formats. We link git against the older
"Microsoft Visual C Runtime Library" (MSVCRT), so to use the UCRT
strftime() we need to load it from ucrtbase.dll using
DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows
update, but in some cases only MSVCRT might be available. In that case
we fall back to using that implementation.

With this change, it is possible to use e.g. the `%g` and `%V` date
format specifiers, e.g.

	git show -s --format=%cd --date=format:‘%g.%V’ HEAD

Without this change, the user would see this error message on Windows:

	fatal: invalid strftime format: '‘%g.%V’'

This fixes git-for-windows#2495

Signed-off-by: Matthias Aßhauer <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
dscho pushed a commit that referenced this issue Apr 9, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
@dscho
Copy link
Member

dscho commented Apr 9, 2020

Would it make sense to either add an exception to that busybox code for git commands (because I don't think they'd ship such a command) or alternatively teach t0014 about the possibility of busybox showing up in the trace output?

I guess we should guard this code behind core.useBusyBox (and an accompanying GIT_TEST_USE_BUSYBOX). But then we should also turn things round: in that case, we should always look whether we can pass a command to BusyBox, and not only when the command isn't in the PATH.

git-for-windows-ci pushed a commit that referenced this issue Apr 14, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
derrickstolee pushed a commit to microsoft/git that referenced this issue Apr 14, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes git-for-windows#2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 14, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit that referenced this issue Apr 17, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
dscho pushed a commit that referenced this issue Apr 18, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 20, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 20, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Apr 24, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue May 1, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue May 9, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue May 15, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue May 29, 2020
Microsoft introduced a new "Universal C Runtime Library" (UCRT) with
Visual Studio 2015. The UCRT comes with a new strftime() implementation that
supports more date formats. We link git against the older "Microsoft Visual C
Runtime Library" (MSVCRT), so to use the UCRT strftime() we need to load it from
ucrtbase.dll using DECLARE_PROC_ADDR()/INIT_PROC_ADDR().

Most supported Windows systems should have recieved the UCRT via Windows update,
but in some cases only MSVCRT might be available. In that case we fall back to
using that implementation.

This fixes #2495

Signed-off-by: Matthias Aßhauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants