summaryrefslogtreecommitdiff
path: root/compat
AgeCommit message (Collapse)AuthorFilesLines
2021-02-03MacOS: precompose_argv_prefix()Libravatar Torsten Bögershausen2-20/+34
The following sequence leads to a "BUG" assertion running under MacOS: DIR=git-test-restore-p Adiarnfd=$(printf 'A\314\210') DIRNAME=xx${Adiarnfd}yy mkdir $DIR && cd $DIR && git init && mkdir $DIRNAME && cd $DIRNAME && echo "Initial" >file && git add file && echo "One more line" >>file && echo y | git restore -p . Initialized empty Git repository in /tmp/git-test-restore-p/.git/ BUG: pathspec.c:495: error initializing pathspec_item Cannot close git diff-index --cached --numstat [snip] The command `git restore` is run from a directory inside a Git repo. Git needs to split the $CWD into 2 parts: The path to the repo and "the rest", if any. "The rest" becomes a "prefix" later used inside the pathspec code. As an example, "/path/to/repo/dir-inside-repå" would determine "/path/to/repo" as the root of the repo, the place where the configuration file .git/config is found. The rest becomes the prefix ("dir-inside-repå"), from where the pathspec machinery expands the ".", more about this later. If there is a decomposed form, (making the decomposing visible like this), "dir-inside-rep°a" doesn't match "dir-inside-repå". Git commands need to: (a) read the configuration variable "core.precomposeunicode" (b) precocompose argv[] (c) precompose the prefix, if there was any The first commit, 76759c7dff53 "git on Mac OS and precomposed unicode" addressed (a) and (b). The call to precompose_argv() was added into parse-options.c, because that seemed to be a good place when the patch was written. Commands that don't use parse-options need to do (a) and (b) themselfs. The commands `diff-files`, `diff-index`, `diff-tree` and `diff` learned (a) and (b) in commit 90a78b83e0b8 "diff: run arguments through precompose_argv" Branch names (or refs in general) using decomposed code points resulting in decomposed file names had been fixed in commit 8e712ef6fc97 "Honor core.precomposeUnicode in more places" The bug report from above shows 2 things: - more commands need to handle precomposed unicode - (c) should be implemented for all commands using pathspecs Solution: precompose_argv() now handles the prefix (if needed), and is renamed into precompose_argv_prefix(). Inside this function the config variable core.precomposeunicode is read into the global variable precomposed_unicode, as before. This reading is skipped if precomposed_unicode had been read before. The original patch for preocomposed unicode, 76759c7dff53, placed precompose_argv() into parse-options.c Now add it into git.c::run_builtin() as well. Existing precompose calls in diff-files.c and others may become redundant, and if we audit the callflows that reach these places to make sure that they can never be reached without going through the new call added to run_builtin(), we might be able to remove these existing ones. But in this commit, we do not bother to do so and leave these precompose callsites as they are. Because precompose() is idempotent and can be called on an already precomposed string safely, this is safer than removing existing calls without fully vetting the callflows. There is certainly room for cleanups - this change intends to be a bug fix. Cleanups needs more tests in e.g. t/t3910-mac-os-precompose.sh, and should be done in future commits. [1] git-bugreport-2021-01-06-1209.txt (git can't deal with special characters) [2] https://lore.kernel.org/git/A102844A-9501-4A86-854D-E3B387D378AA@icloud.com/ Reported-by: Daniel Troger <random_n0body@icloud.com> Helped-By: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-12-14Merge branch 'da/vs-build-iconv-fix'Libravatar Junio C Hamano1-1/+1
Build update. * da/vs-build-iconv-fix: ci(vs-build): stop passing the iconv library location explicitly
2020-12-04ci(vs-build): stop passing the iconv library location explicitlyLibravatar Dennis Ameling1-1/+1
Something changed in `vcpkg` (which we use in our Visual C++ build to provide the dependencies such as libcurl) and our `vs-build` job started failing in CI. The reason is that we had a work-around in place to help CMake find iconv, and this work-around is neither needed nor does it work anymore. For the full discussion with the vcpkg project, see this comment: https://github.com/microsoft/vcpkg/issues/14780#issuecomment-735368280 Signed-off-by: Dennis Ameling <dennis@dennisameling.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-11compat/bswap.h: don't assume MSVC is little-endianLibravatar Daniel Gurney1-1/+1
In 1af265f0 (compat/bswap.h: simplify MSVC endianness detection, 2020-11-08) we attempted to simplify code by assuming MSVC builds will be for little-endian machines, since only unusably old versions of MSVC supported big-endian MIPS and m68k architectures. However, it's possible that MSVC could be ported to build for a big-endian architecture again, so the simplification wasn't as future-proof as hoped. So let's go back to the old way of detecting MSVC, and then checking architecture from a list of little-endian architecture macros. Note that MSVC does not treat ARM64 as bi-endian, so we can safely treat it as little-endian. Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Helped-by: Jeff King <peff@peff.net> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Daniel Gurney <dgurney99@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-09compat/bswap.h: simplify MSVC endianness detectionLibravatar Daniel Gurney1-1/+1
Modern MSVC or Windows versions don't support big-endian, so it's unnecessary to consider architectures when using it. This also makes ARM64 MSVC builds succeed. Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Daniel Gurney <dgurney99@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-07compat/mingw.h: drop extern from function declarationLibravatar Denton Liu1-1/+1
In 554544276a (*.[ch]: remove extern from function declarations using spatch, 2019-04-29), `extern` on function declarations were declared to be redundant and thus removed from the codebase. An `extern` was accidentally reintroduced in 08809c09aa (mingw: add a helper function to attach GDB to the current process, 2020-02-13). Remove this spurious `extern`. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-04Merge branch 'jk/drop-unaligned-loads'Libravatar Junio C Hamano1-24/+0
Compilation fix around type punning. * jk/drop-unaligned-loads: Revert "fast-export: use local array to store anonymized oid" bswap.h: drop unaligned loads
2020-09-24bswap.h: drop unaligned loadsLibravatar Jeff King1-24/+0
Our put_be32() routine and its variants (get_be32(), put_be64(), etc) has two implementations: on some platforms we cast memory in place and use nothl()/htonl(), which can cause unaligned memory access. And on others, we pick out the individual bytes using bitshifts. This introduces extra complexity, and sometimes causes compilers to generate warnings about type-punning. And it's not clear there's any performance advantage. This split goes back to 660231aa97 (block-sha1: support for architectures with memory alignment restrictions, 2009-08-12). The unaligned versions were part of the original block-sha1 code in d7c208a92e (Add new optimized C 'block-sha1' routines, 2009-08-05), which says it is: Based on the mozilla SHA1 routine, but doing the input data accesses a word at a time and with 'htonl()' instead of loading bytes and shifting. Back then, Linus provided timings versus the mozilla code which showed a 27% improvement: https://lore.kernel.org/git/alpine.LFD.2.01.0908051545000.3390@localhost.localdomain/ However, the unaligned loads were either not the useful part of that speedup, or perhaps compilers and processors have changed since then. Here are times for computing the sha1 of 4GB of random data, with and without -DNO_UNALIGNED_LOADS (and BLK_SHA1=1, of course). This is with gcc 10, -O2, and the processor is a Core i9-9880H. [stock] Benchmark #1: t/helper/test-tool sha1 <foo.rand Time (mean ± σ): 6.638 s ± 0.081 s [User: 6.269 s, System: 0.368 s] Range (min … max): 6.550 s … 6.841 s 10 runs [-DNO_UNALIGNED_LOADS] Benchmark #1: t/helper/test-tool sha1 <foo.rand Time (mean ± σ): 6.418 s ± 0.015 s [User: 6.058 s, System: 0.360 s] Range (min … max): 6.394 s … 6.447 s 10 runs And here's the same test run on an AMD A8-7600, using gcc 8. [stock] Benchmark #1: t/helper/test-tool sha1 <foo.rand Time (mean ± σ): 11.721 s ± 0.113 s [User: 10.761 s, System: 0.951 s] Range (min … max): 11.509 s … 11.861 s 10 runs [-DNO_UNALIGNED_LOADS] Benchmark #1: t/helper/test-tool sha1 <foo.rand Time (mean ± σ): 11.744 s ± 0.066 s [User: 10.807 s, System: 0.928 s] Range (min … max): 11.637 s … 11.863 s 10 runs So the unaligned loads don't seem to help much, and actually make things worse. It's possible there are platforms where they provide more benefit, but: - the non-x86 platforms for which we use this code are old and obscure (powerpc and s390). - the main caller that cares about performance is block-sha1. But these days it is rarely used anyway, in favor of sha1dc (which is already much slower, and nobody seems to have cared that much). Let's just drop unaligned versions entirely in the name of simplicity. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-09-03vcbuild: fix batch file name in READMELibravatar Orgad Shaneh1-2/+2
Signed-off-by: Orgad Shaneh <orgads@gmail.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-09-03vcbuild: fix library name for expat with make MSVC=1Libravatar Orgad Shaneh1-1/+1
Signed-off-by: Orgad Shaneh <orgads@gmail.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-19Merge branch 'jh/mingw-unlink'Libravatar Junio C Hamano1-0/+3
"unlink" emulation on MinGW has been optimized. * jh/mingw-unlink: mingw: improve performance of mingw_unlink()
2020-08-17mingw: improve performance of mingw_unlink()Libravatar Jeff Hostetler1-0/+3
Update mingw_unlink() to first try to delete the file with existing permissions before trying to force it. Windows throws an error when trying to delete a read-only file. The mingw_unlink() compatibility wrapper always tries to _wchmod(666) the file before calling _wunlink() to avoid that error. However, since most files in the worktree are already writable, this is usually wasted effort. Update mingw_unlink() to just call DeleteFileW() directly and if that succeeds return. If that fails, fall back into the existing code path to update the permissions and use _wunlink() to get the existing error code mapping. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-07-28strvec: convert more callers away from argv_array nameLibravatar Jeff King2-11/+11
We eventually want to drop the argv_array name and just use strvec consistently. There's no particular reason we have to do it all at once, or care about interactions between converted and unconverted bits. Because of our preprocessor compat layer, the names are interchangeable to the compiler (so even a definition and declaration using different names is OK). This patch converts remaining files from the first half of the alphabet, to keep the diff to a manageable size. The conversion was done purely mechanically with: git ls-files '*.c' '*.h' | xargs perl -i -pe ' s/ARGV_ARRAY/STRVEC/g; s/argv_array/strvec/g; ' and then selectively staging files with "git add '[abcdefghjkl]*'". We'll deal with any indentation/style fallouts separately. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-06-17Merge branch 'js/msvc-build-fix'Libravatar Junio C Hamano1-1/+3
Workaround breakage in MSVC build, where "curl-config --cflags" gives settings appropriate for GCC build. * js/msvc-build-fix: msvc: fix "REG_STARTEND" issue
2020-06-04msvc: fix "REG_STARTEND" issueLibravatar Johannes Schindelin1-1/+3
In 897d68e7af82 (Makefile: use curl-config --cflags, 2020-03-26), we taught the build process to use `curl-config --cflags` to make sure that it can find cURL's headers. In the MSVC build, this is completely bogus because we're running in a Git for Windows SDK whose `curl-config` supports the _GCC_ build. Let's just ignore each and every `-I<path>` option where `<path>` points to GCC/Clang specific headers. Reported by Jeff Hostetler in https://github.com/microsoft/git/issues/275. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-05-20Merge branch 'cb/no-more-gmtime'Libravatar Junio C Hamano1-29/+0
Code clean-up by removing a compatibility implementation of a function we no longer use. * cb/no-more-gmtime: compat: remove gmtime
2020-05-14compat: remove gmtimeLibravatar Carlo Marcelo Arenas Belón1-29/+0
ccd469450a (date.c: switch to reentrant {gm,local}time_r, 2019-11-28) removes the only gmtime() call we had and moves to gmtime_r() which doesn't have the same portability problems. Remove the compat gmtime code since it is no longer needed, and confirm by successfull running t4212 in FreeBSD 9.3 amd64 (the oldest I could get a hold off). Further work might be needed to ensure 32bit time_t systems (like FreeBSD i386) will handle correctly the overflows tested in t4212, but that is orthogonal to this change, and it doesn't change the current behaviour as neither gmtime() or gmtime_r() will ever return NULL on those systems because time_t is unsigned. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-05-01Merge branch 'es/bugreport'Libravatar Junio C Hamano2-2/+43
The "bugreport" tool. * es/bugreport: bugreport: drop extraneous includes bugreport: add compiler info bugreport: add uname info bugreport: gather git version and build info bugreport: add tool to generate debugging info help: move list_config_help to builtin/help
2020-04-27compat/regex: move stdlib.h up in inclusion chainLibravatar Đoàn Trần Công Danh2-1/+1
In Linux with musl libc, we have this inclusion chain: compat/regex/regex.c:69 `-> compat/regex/regex_internal.h `-> /usr/include/stdlib.h `-> /usr/include/features.h `-> /usr/include/alloca.h In that inclusion chain, `<features.h>` claims it's _BSD_SOURCE compatible when it's NOT asked to be either {_POSIX,_GNU,_XOPEN,_BSD}_SOURCE, or __STRICT_ANSI__. And, `<stdlib.h>` will include `<alloca.h>` to be compatible with software written for GNU and BSD. Thus, redefine `alloca` macro, which was defined before at compat/regex/regex.c:66. Considering this is only compat code, we've taken from other project, it's not our business to decide which source should we adhere to. Include `<stdlib.h>` early to prevent the redefinition of alloca. This also remove a potential warning about alloca not defined on: #undef alloca Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-22Merge branch 'js/mingw-isilon-nfs'Libravatar Junio C Hamano1-2/+15
* js/mingw-isilon-nfs: mingw: cope with the Isilon network file system
2020-04-22Merge branch 'js/mingw-fixes'Libravatar Junio C Hamano1-4/+41
Misc fixes for Windows. * js/mingw-fixes: mingw: help debugging by optionally executing bash with strace mingw: do not treat `COM0` as a reserved file name mingw: use modern strftime implementation if possible
2020-04-16bugreport: add compiler infoLibravatar Emily Shaffer1-0/+41
To help pinpoint the source of a regression, it is useful to know some info about the compiler which the user's Git client was built with. By adding a generic get_compiler_info() in 'compat/' we can choose which relevant information to share per compiler; to get started, let's demonstrate the version of glibc if the user built with 'gcc'. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-16help: move list_config_help to builtin/helpLibravatar Emily Shaffer1-2/+2
Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o, builtin/clean.o, and builtin/config.o. This meant that help.o was unusable outside of the context of the main Git executable. To make help.o usable by other commands again, move list_config_help() into builtin/help.c (where it makes sense to assume other builtin libraries are present). When command-list.h is included but a member is not used, we start to hear a compiler warning. Since the config list is generated in a fairly different way than the command list, and since commands and config options are semantically different, move the config list into its own header and move the generator into its own script and build rule. For reasons explained in 976aaedc (msvc: add a Makefile target to pre-generate the Visual Studio solution, 2019-07-29), some build artifacts we consider non-source files cannot be generated in the Visual Studio environment, and we already have some Makefile tweaks to help Visual Studio to use generated command-list.h header file. Do the same to a new generated file, config-list.h, introduced by this change. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
2020-04-10mingw: cope with the Isilon network file systemLibravatar Nathan Sanders1-2/+15
On certain network filesystems (currently encountered with Isilon, but in theory more network storage solutions could be causing the same issue), when the directory in question is missing, `raceproof_create_file()` fails with an `ERROR_INVALID_PARAMETER` instead of an `ERROR_PATH_NOT_FOUND`. Since it is highly unlikely that we produce such an error by mistake (the parameters we pass are fairly benign), we can be relatively certain that the directory is missing in this instance. So let's just translate that error automagically. This fixes https://github.com/git-for-windows/git/issues/1345. Signed-off-by: Nathan Sanders <spekbukkem@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10mingw: help debugging by optionally executing bash with straceLibravatar Johannes Schindelin1-0/+26
MSYS2's strace facility is very useful for debugging... With this patch, the bash will be executed through strace if the environment variable GIT_STRACE_COMMANDS is set, which comes in real handy when investigating issues in the test suite. Also support passing a path to a log file via GIT_STRACE_COMMANDS to force Git to call strace.exe with the `-o <path>` argument, i.e. to log into a file rather than print the log directly. That comes in handy when the output would otherwise misinterpreted by a calling process as part of Git's output. Note: the values "1", "yes" or "true" are *not* specifying paths, but tell Git to let strace.exe log directly to the console. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-08mingw: do not treat `COM0` as a reserved file nameLibravatar Johannes Schindelin1-3/+5
In 4dc42c6c186 (mingw: refuse paths containing reserved names, 2019-12-21), we started disallowing file names that are reserved, e.g. `NUL`, `CONOUT$`, etc. This included `COM<n>` where `<n>` is a digit. Unfortunately, this includes `COM0` but only `COM1`, ..., `COM9` are reserved, according to the official documentation, `COM0` is mentioned in the "NT Namespaces" section but it is explicitly _omitted_ from the list of reserved names: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions Tests corroborate this: it is totally possible to write a file called `com0.c` on Windows 10, but not `com1.c`. So let's tighten the code to disallow only the reserved `COM<n>` file names, but to allow `COM0` again. This fixes https://github.com/git-for-windows/git/issues/2470. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-08mingw: use modern strftime implementation if possibleLibravatar Matthias Aßhauer1-1/+10
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 https://github.com/git-for-windows/git/issues/2495 Signed-off-by: Matthias Aßhauer <mha1993@live.de> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-27run-command: trigger PATH lookup properly on CygwinLibravatar Andras Kucsma1-0/+11
On Cygwin, the codepath for POSIX-like systems is taken in run-command.c::start_command(). The prepare_cmd() helper function is called to decide if the command needs to be looked up in the PATH. The logic there is to do the PATH-lookup if and only if it does not have any slash '/' in it. If this test passes we end up attempting to run the command by appending the string after each colon-separated component of PATH. The Cygwin environment supports both Windows and POSIX style paths, so both forwardslahes '/' and back slashes '\' can be used as directory separators for any external program the user supplies. Examples for path strings which are being incorrectly searched for in the PATH instead of being executed as is: - "C:\Program Files\some-program.exe" - "a\b\c.exe" To handle these, the PATH lookup detection logic in prepare_cmd() is taught to know about this Cygwin quirk, by introducing has_dir_sep(path) helper function to abstract away the difference between true POSIX and Cygwin systems. Signed-off-by: Andras Kucsma <r0maikx02b@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-17Merge branch 'js/mingw-open-in-gdb' into maintLibravatar Junio C Hamano2-0/+23
Dev support. * js/mingw-open-in-gdb: mingw: add a helper function to attach GDB to the current process
2020-03-17Merge branch 'am/mingw-poll-fix' into maintLibravatar Junio C Hamano1-28/+3
MinGW's poll() emulation has been improved. * am/mingw-poll-fix: mingw: workaround for hangs when sending STDIN
2020-03-17Merge branch 'jk/clang-sanitizer-fixes' into maintLibravatar Junio C Hamano1-2/+4
C pedantry ;-) fix. * jk/clang-sanitizer-fixes: obstack: avoid computing offsets from NULL pointer xdiff: avoid computing non-zero offset from NULL pointer avoid computing zero offsets from NULL pointer merge-recursive: use subtraction to flip stage merge-recursive: silence -Wxor-used-as-pow warning
2020-03-09Merge branch 'am/mingw-poll-fix'Libravatar Junio C Hamano1-28/+3
MinGW's poll() emulation has been improved. * am/mingw-poll-fix: mingw: workaround for hangs when sending STDIN
2020-03-02Merge branch 'rs/micro-cleanups'Libravatar Junio C Hamano1-1/+1
Code cleanup. * rs/micro-cleanups: use strpbrk(3) to search for characters from a given set quote: use isalnum() to check for alphanumeric characters
2020-02-27mingw: workaround for hangs when sending STDINLibravatar Alexandr Miloslavskiy1-28/+3
Explanation ----------- The problem here is flawed `poll()` implementation. When it tries to see if pipe can be written without blocking, it eventually calls `NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However, the meaning of quota was misunderstood. The value of quota is reduced when either some data was written to a pipe, *or* there is a pending read on the pipe. Therefore, if there is a pending read of size >= than the pipe's buffer size, poll() will think that pipe is not writable and will hang forever, usually that means deadlocking both pipe users. I have studied the problem and found that Windows pipes track two values: `QuotaUsed` and `BytesInQueue`. The code in `poll()` apparently wants to know `BytesInQueue` instead of quota. Unfortunately, `BytesInQueue` can only be requested from read end of the pipe, while `poll()` receives write end. The git's implementation of `poll()` was copied from gnulib, which also contains a flawed implementation up to today. I also had a look at implementation in cygwin, which is also broken in a subtle way. It uses this code in `pipe_data_available()`: fpli.WriteQuotaAvailable = (fpli.OutboundQuota - fpli.ReadDataAvailable) However, `ReadDataAvailable` always returns 0 for the write end of the pipe, turning the code into an obfuscated version of returning pipe's total buffer size, which I guess will in turn have `poll()` always say that pipe is writable. The commit that introduced the code doesn't say anything about this change, so it could be some debugging code that slipped in. These are the typical sizes used in git: 0x2000 - default read size in `strbuf_read()` 0x1000 - default read size in CRT, used by `strbuf_getwholeline()` 0x2000 - pipe buffer size in compat\mingw.c As a consequence, as soon as child process uses `strbuf_read()`, `poll()` in parent process will hang forever, deadlocking both processes. This results in two observable behaviors: 1) If parent process begins sending STDIN quickly (and usually that's the case), then first `poll()` will succeed and first block will go through. MAX_IO_SIZE_DEFAULT is 8MB, so if STDIN exceeds 8MB, then it will deadlock. 2) If parent process waits a little bit for any reason (including OS scheduler) and child is first to issue `strbuf_read()`, then it will deadlock immediately even on small STDINs. The problem is illustrated by `git stash push`, which will currently read the entire patch into memory and then send it to `git apply` via STDIN. If patch exceeds 8MB, git hangs on Windows. Possible solutions ------------------ 1) Somehow obtain `BytesInQueue` instead of `QuotaUsed` I did a pretty thorough search and didn't find any ways to obtain the value from write end of the pipe. 2) Also give read end of the pipe to `poll()` That can be done, but it will probably invite some dirty code, because `poll()` * can accept multiple pipes at once * can accept things that are not pipes * is expected to have a well known signature. 3) Make `poll()` always reply "writable" for write end of the pipe Afterall it seems that cygwin (accidentally?) does that for years. Also, it should be noted that `pump_io_round()` writes 8MB blocks, completely ignoring the fact that pipe's buffer size is only 8KB, which means that pipe gets clogged many times during that single write. This may invite a deadlock, if child's STDERR/STDOUT gets clogged while it's trying to deal with 8MB of STDIN. Such deadlocks could be defeated with writing less than pipe's buffer size per round, and always reading everything from STDOUT/STDERR before starting next round. Therefore, making `poll()` always reply "writable" shouldn't cause any new issues or block any future solutions. 4) Increase the size of the pipe's buffer The difference between `BytesInQueue` and `QuotaUsed` is the size of pending reads. Therefore, if buffer is bigger than size of reads, `poll()` won't hang so easily. However, I found that for example `strbuf_read()` will get more and more hungry as it reads large inputs, eventually surpassing any reasonable pipe buffer size. Chosen solution --------------- Make `poll()` always reply "writable" for write end of the pipe. Hopefully one day someone will find a way to implement it properly. Reproduction ------------ printf "%8388608s" X >large_file.txt git stash push --include-untracked -- large_file.txt I have decided not to include this as test to avoid slowing down the test suite. I don't expect the specific problem to come back, and chances are that `git stash push` will be reworked to avoid sending the entire patch via STDIN. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24use strpbrk(3) to search for characters from a given setLibravatar René Scharfe1-1/+1
We can check if certain characters are present in a string by calling strchr(3) on each of them, or we can pass them all to a single strpbrk(3) call. The latter is shorter, less repetitive and slightly more efficient, so let's do that instead. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-17Merge branch 'js/mingw-open-in-gdb'Libravatar Junio C Hamano2-0/+23
Dev support. * js/mingw-open-in-gdb: mingw: add a helper function to attach GDB to the current process
2020-02-14Merge branch 'jk/asan-build-fix' into maintLibravatar Junio C Hamano1-0/+5
Work around test breakages caused by custom regex engine used in libasan, when address sanitizer is used with more recent versions of gcc and clang. * jk/asan-build-fix: Makefile: use compat regex with SANITIZE=address
2020-02-14mingw: add a helper function to attach GDB to the current processLibravatar Johannes Schindelin2-0/+23
When debugging Git, the criss-cross spawning of processes can make things quite a bit difficult, especially when a Unix shell script is thrown in the mix that calls a `git.exe` that then segfaults. To help debugging such things, we introduce the `open_in_gdb()` function which can be called at a code location where the segfault happens (or as close as one can get); This will open a new MinTTY window with a GDB that already attached to the current process. Inspired by Derrick Stolee. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-12Merge branch 'jk/clang-sanitizer-fixes'Libravatar Junio C Hamano1-2/+4
C pedantry ;-) fix. * jk/clang-sanitizer-fixes: obstack: avoid computing offsets from NULL pointer xdiff: avoid computing non-zero offset from NULL pointer avoid computing zero offsets from NULL pointer merge-recursive: use subtraction to flip stage merge-recursive: silence -Wxor-used-as-pow warning
2020-02-05Merge branch 'js/add-p-leftover-bits'Libravatar Junio C Hamano2-4/+248
The final leg of rewriting "add -i/-p" in C. * js/add-p-leftover-bits: ci: include the built-in `git add -i` in the `linux-gcc` job built-in add -p: handle Escape sequences more efficiently built-in add -p: handle Escape sequences in interactive.singlekey mode built-in add -p: respect the `interactive.singlekey` config setting terminal: add a new function to read a single keystroke terminal: accommodate Git for Windows' default terminal terminal: make the code of disable_echo() reusable built-in add -p: handle diff.algorithm built-in add -p: support interactive.diffFilter t3701: adjust difffilter test
2020-01-30Merge branch 'jk/asan-build-fix'Libravatar Junio C Hamano1-0/+5
Work around test breakages caused by custom regex engine used in libasan, when address sanitizer is used with more recent versions of gcc and clang. * jk/asan-build-fix: Makefile: use compat regex with SANITIZE=address
2020-01-28obstack: avoid computing offsets from NULL pointerLibravatar Jeff King1-2/+4
As with the previous two commits, UBSan with clang-11 complains about computing offsets from a NULL pointer. The failures in t4013 (and elsewhere) look like this: kwset.c:102:23: runtime error: applying non-zero offset 107820859019600 to null pointer ... not ok 79 - git log -SF master # magic is (not used) That line is not enlightening: ... = obstack_alloc(&kwset->obstack, sizeof (struct trie)); because obstack is implemented almost entirely in macros, and the actual problem is five macros deep (I temporarily converted them to inline functions to get better compiler errors, which was tedious but worked reasonably well). The actual problem is in these pointer-alignment macros: /* If B is the base of an object addressed by P, return the result of aligning P to the next multiple of A + 1. B and P must be of type char *. A + 1 must be a power of 2. */ #define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A))) /* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case where pointers can be converted to integers, aligned as integers, and converted back again. If PTR_INT_TYPE is narrower than a pointer (e.g., the AS/400), play it safe and compute the alignment relative to B. Otherwise, use the faster strategy of computing the alignment relative to 0. */ #define __PTR_ALIGN(B, P, A) \ __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \ P, A) If we have a sufficiently-large integer pointer type, then we do the computation using a NULL pointer constant. That turns __BPTR_ALIGN() into something like: NULL + (P - NULL + A) & ~A and UBSan is complaining about adding the full value of P to that initial NULL. We can fix this by doing our math as an integer type, and then casting the result back to a pointer. The problem case only happens when we know that the integer type is large enough, so there should be no issue with truncation. Another option would be just simplify out all the 0's from __BPTR_ALIGN() for the NULL-pointer case. That probably wouldn't work for a platform where the NULL pointer isn't all-zeroes, but Git already wouldn't work on such a platform (due to our use of memset to set pointers in structs to NULL). But I tried here to keep as close to the original as possible. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-16Sync with maintLibravatar Junio C Hamano1-2/+2
* maint: msvc: accommodate for vcpkg's upgrade to OpenSSL v1.1.x
2020-01-16Makefile: use compat regex with SANITIZE=addressLibravatar Jeff King1-0/+5
Recent versions of the gcc and clang Address Sanitizer produce test failures related to regexec(). This triggers with gcc-10 and clang-8 (but not gcc-9 nor clang-7). Running: make CC=gcc-10 SANITIZE=address test results in failures in t4018, t3206, and t4062. The cause seems to be that when built with ASan, we use a different version of regexec() than normal. And this version doesn't understand the REG_STARTEND flag. Here's my evidence supporting that. The failure in t4062 is an ASan warning: expecting success of 4062.2 '-G matches': git diff --name-only -G "^(0{64}){64}$" HEAD^ >out && test 4096-zeroes.txt = "$(cat out)" ================================================================= ==672994==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fa76f672000 at pc 0x7fa7726f75b6 bp 0x7ffe41bdda70 sp 0x7ffe41bdd220 READ of size 4097 at 0x7fa76f672000 thread T0 #0 0x7fa7726f75b5 (/lib/x86_64-linux-gnu/libasan.so.6+0x4f5b5) #1 0x562ae0c9c40e in regexec_buf /home/peff/compile/git/git-compat-util.h:1117 #2 0x562ae0c9c40e in diff_grep /home/peff/compile/git/diffcore-pickaxe.c:52 #3 0x562ae0c9cc28 in pickaxe_match /home/peff/compile/git/diffcore-pickaxe.c:166 [...] In this case we're looking in a buffer which was mmap'd via reuse_worktree_file(), and whose size is 4096 bytes. But libasan's regex tries to look at byte 4097 anyway! If we tweak Git like this: diff --git a/diff.c b/diff.c index 8e2914c031..cfae60c120 100644 --- a/diff.c +++ b/diff.c @@ -3880,7 +3880,7 @@ static int reuse_worktree_file(struct index_state *istate, */ if (ce_uptodate(ce) || (!lstat(name, &st) && !ie_match_stat(istate, ce, &st, 0))) - return 1; + return 0; return 0; } to use a regular buffer (with a trailing NUL) instead of an mmap, then the complaint goes away. The other failures are actually diff output with an incorrect funcname header. If I instrument xdiff to show the funcname matching like so: diff --git a/xdiff-interface.c b/xdiff-interface.c index 8509f9ea22..f6c3dc1986 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -197,6 +197,7 @@ struct ff_regs { struct ff_reg { regex_t re; int negate; + char *printable; } *array; }; @@ -218,7 +219,12 @@ static long ff_regexp(const char *line, long len, for (i = 0; i < regs->nr; i++) { struct ff_reg *reg = regs->array + i; - if (!regexec_buf(&reg->re, line, len, 2, pmatch, 0)) { + int ret = regexec_buf(&reg->re, line, len, 2, pmatch, 0); + warning("regexec %s:\n regex: %s\n buf: %.*s", + ret == 0 ? "matched" : "did not match", + reg->printable, + (int)len, line); + if (!ret) { if (reg->negate) return -1; break; @@ -264,6 +270,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *value, int cflags) expression = value; if (regcomp(&reg->re, expression, cflags)) die("Invalid regexp to look for hunk header: %s", expression); + reg->printable = xstrdup(expression); free(buffer); value = ep + 1; } then when compiling with ASan and gcc-10, running the diff from t4018.66 produces this: $ git diff -U1 cpp-skip-access-specifiers warning: regexec did not match: regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*]) buf: private: warning: regexec matched: regex: ^((::[[:space:]]*)?[A-Za-z_].*)$ buf: private: diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers index 4d4a9db..ebd6f42 100644 --- a/cpp-skip-access-specifiers +++ b/cpp-skip-access-specifiers @@ -6,3 +6,3 @@ private: void DoSomething(); int ChangeMe; }; void DoSomething(); - int ChangeMe; + int IWasChanged; }; That first regex should match (and is negated, so it should be telling us _not_ to match "private:"). But it wouldn't if regexec() is looking at the whole buffer, and not just the length-limited line we've fed to regexec_buf(). So this is consistent again with REG_STARTEND being ignored. The correct output (compiling without ASan, or gcc-9 with Asan) looks like this: warning: regexec matched: regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*]) buf: private: [...more lines that we end up not using...] warning: regexec matched: regex: ^((::[[:space:]]*)?[A-Za-z_].*)$ buf: class RIGHT : public Baseclass diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers index 4d4a9db..ebd6f42 100644 --- a/cpp-skip-access-specifiers +++ b/cpp-skip-access-specifiers @@ -6,3 +6,3 @@ class RIGHT : public Baseclass void DoSomething(); - int ChangeMe; + int IWasChanged; }; So it really does seem like libasan's regex engine is ignoring REG_STARTEND. We should be able to work around it by compiling with NO_REGEX, which would use our local regexec(). But to make matters even more interesting, this isn't enough by itself. Because ASan has support from the compiler, it doesn't seem to intercept our call to regexec() at the dynamic library level. It actually recognizes when we are compiling a call to regexec() and replaces it with ASan-specific code at that point. And unlike most of our other compat code, where we might have git_mmap() or similar, the actual symbol name in the compiled compat/regex code is regexec(). So just compiling with NO_REGEX isn't enough; we still end up in libasan! We can work around that by having the preprocessor replace regexec with git_regexec (both in the callers and in the actual implementation), and we truly end up with a call to our custom regex code, even when compiling with ASan. That's probably a good thing to do anyway, as it means anybody looking at the symbols later (e.g., in a debugger) would have a better indication of which function is which. So we'll do the same for the other common regex functions (even though just regexec() is enough to fix this ASan problem). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-16msvc: accommodate for vcpkg's upgrade to OpenSSL v1.1.xLibravatar Johannes Schindelin1-2/+2
With the upgrade, the library names changed from libeay32/ssleay32 to libcrypto/libssl. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-15built-in add -p: handle Escape sequences more efficientlyLibravatar Johannes Schindelin1-1/+72
When `interactive.singlekey = true`, we react immediately to keystrokes, even to Escape sequences (e.g. when pressing a cursor key). The problem with Escape sequences is that we do not really know when they are done, and as a heuristic we poll standard input for half a second to make sure that we got all of it. While waiting half a second is not asking for a whole lot, it can become quite annoying over time, therefore with this patch, we read the terminal capabilities (if available) and extract known Escape sequences from there, then stop polling immediately when we detected that the user pressed a key that generated such a known sequence. This recapitulates the remaining part of b5cc003253c8 (add -i: ignore terminal escape sequences, 2011-05-17). Note: We do *not* query the terminal capabilities directly. That would either require a lot of platform-specific code, or it would require linking to a library such as ncurses. Linking to a library in the built-ins is something we try very hard to avoid (we even kicked the libcurl dependency to a non-built-in remote helper, just to shave off a tiny fraction of a second from Git's startup time). And the platform-specific code would be a maintenance nightmare. Even worse: in Git for Windows' case, we would need to query MSYS2 pseudo terminals, which `git.exe` simply cannot do (because it is intentionally *not* an MSYS2 program). To address this, we simply spawn `infocmp -L -1` and parse its output (which works even in Git for Windows, because that helper is included in the end-user facing installations). This is done only once, as in the Perl version, but it is done only when the first Escape sequence is encountered, not upon startup of `git add -i`; This saves on startup time, yet makes reacting to the first Escape sequence slightly more sluggish. But it allows us to keep the terminal-related code encapsulated in the `compat/terminal.c` file. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-15built-in add -p: handle Escape sequences in interactive.singlekey modeLibravatar Johannes Schindelin1-1/+55
This recapitulates part of b5cc003253c8 (add -i: ignore terminal escape sequences, 2011-05-17): add -i: ignore terminal escape sequences On the author's terminal, the up-arrow input sequence is ^[[A, and thus fat-fingering an up-arrow into 'git checkout -p' is quite dangerous: git-add--interactive.perl will ignore the ^[ and [ characters and happily treat A as "discard everything". As a band-aid fix, use Term::Cap to get all terminal capabilities. Then use the heuristic that any capability value that starts with ^[ (i.e., \e in perl) must be a key input sequence. Finally, given an input that starts with ^[, read more characters until we have read a full escape sequence, then return that to the caller. We use a timeout of 0.5 seconds on the subsequent reads to avoid getting stuck if the user actually input a lone ^[. Since none of the currently recognized keys start with ^[, the net result is that the sequence as a whole will be ignored and the help displayed. Note that we leave part for later which uses "Term::Cap to get all terminal capabilities", for several reasons: 1. it is actually not really necessary, as the timeout of 0.5 seconds should be plenty sufficient to catch Escape sequences, 2. it is cleaner to keep the change to special-case Escape sequences separate from the change that reads all terminal capabilities to speed things up, and 3. in practice, relying on the terminal capabilities is a bit overrated, as the information could be incomplete, or plain wrong. For example, in this developer's tmux sessions, the terminal capabilities claim that the "cursor up" sequence is ^[M, but the actual sequence produced by the "cursor up" key is ^[[A. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-15terminal: add a new function to read a single keystrokeLibravatar Johannes Schindelin2-0/+58
Typically, input on the command-line is line-based. It is actually not really easy to get single characters (or better put: keystrokes). We provide two implementations here: - One that handles `/dev/tty` based systems as well as native Windows. The former uses the `tcsetattr()` function to put the terminal into "raw mode", which allows us to read individual keystrokes, one by one. The latter uses `stty.exe` to do the same, falling back to direct Win32 Console access. Thanks to the refactoring leading up to this commit, this is a single function, with the platform-specific details hidden away in conditionally-compiled code blocks. - A fall-back which simply punts and reads back an entire line. Note that the function writes the keystroke into an `strbuf` rather than a `char`, in preparation for reading Escape sequences (e.g. when the user hit an arrow key). This is also required for UTF-8 sequences in case the keystroke corresponds to a non-ASCII letter. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-15terminal: accommodate Git for Windows' default terminalLibravatar Johannes Schindelin1-0/+50
Git for Windows' Git Bash runs in MinTTY by default, which does not have a Win32 Console instance, but uses MSYS2 pseudo terminals instead. This is a problem, as Git for Windows does not want to use the MSYS2 emulation layer for Git itself, and therefore has no direct way to interact with that pseudo terminal. As a workaround, use the `stty` utility (which is included in Git for Windows, and which *is* an MSYS2 program, so it knows how to deal with the pseudo terminal). Note: If Git runs in a regular CMD or PowerShell window, there *is* a regular Win32 Console to work with. This is not a problem for the MSYS2 `stty`: it copes with this scenario just fine. Also note that we introduce support for more bits than would be necessary for a mere `disable_echo()` here, in preparation for the upcoming `enable_non_canonical()` function. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-15terminal: make the code of disable_echo() reusableLibravatar Johannes Schindelin1-4/+15
We are about to introduce the function `enable_non_canonical()`, which shares almost the complete code with `disable_echo()`. Let's prepare for that, by refactoring out that shared code. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>