user/sven/git.git/compat, branch v2.26.0-rc2

Merge branch 'am/mingw-poll-fix'

2020-03-09T18:21:20Z

MinGW's poll() emulation has been improved. * am/mingw-poll-fix: mingw: workaround for hangs when sending STDIN

Merge branch 'rs/micro-cleanups'

2020-03-02T23:07:20Z

Code cleanup. * rs/micro-cleanups: use strpbrk(3) to search for characters from a given set quote: use isalnum() to check for alphanumeric characters

mingw: workaround for hangs when sending STDIN

2020-02-27T22:23:29Z

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 Signed-off-by: Junio C Hamano

use strpbrk(3) to search for characters from a given set

2020-02-24T17:30:31Z

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 Signed-off-by: Junio C Hamano

Merge branch 'js/mingw-open-in-gdb'

2020-02-17T21:22:18Z

Dev support. * js/mingw-open-in-gdb: mingw: add a helper function to attach GDB to the current process

mingw: add a helper function to attach GDB to the current process

2020-02-14T18:02:07Z

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 Signed-off-by: Junio C Hamano

Merge branch 'jk/clang-sanitizer-fixes'

2020-02-12T20:41:36Z

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

Merge branch 'js/add-p-leftover-bits'

2020-02-05T22:34:58Z

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

Merge branch 'jk/asan-build-fix'

2020-01-30T22:17:09Z

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

obstack: avoid computing offsets from NULL pointer

2020-01-29T07:13:25Z

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 Signed-off-by: Junio C Hamano