From df90ae4d19f395beaba730265a445cf0fe87d0e0 Mon Sep 17 00:00:00 2001 From: Josh Heinrichs Date: Tue, 8 Oct 2024 08:29:20 -0600 Subject: [PATCH 1/6] git-config.1: remove value from positional args in unset usage The synopsis for `git config unset` mentions two positional arguments: `` and ``. While the first argument is correct, the second is not. Users are expected to provide the value via `--value=`. Remove the positional argument. The `--value=` option is already documented correctly, so this is all we need to do to fix the documentation. Signed-off-by: Josh Heinrichs Reviewed-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/git-config.txt | 2 +- builtin/config.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7f81fbbea83a6e..3e420177c1599d 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git config list' [] [] [--includes] 'git config get' [] [] [--includes] [--all] [--regexp] [--value=] [--fixed-value] [--default=] 'git config set' [] [--type=] [--all] [--value=] [--fixed-value] -'git config unset' [] [--all] [--value=] [--fixed-value] +'git config unset' [] [--all] [--value=] [--fixed-value] 'git config rename-section' [] 'git config remove-section' [] 'git config edit' [] diff --git a/builtin/config.c b/builtin/config.c index d8fd3def0efdae..cba702210815b7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -19,7 +19,7 @@ static const char *const builtin_config_usage[] = { N_("git config list [] [] [--includes]"), N_("git config get [] [] [--includes] [--all] [--regexp] [--value=] [--fixed-value] [--default=] "), N_("git config set [] [--type=] [--all] [--value=] [--fixed-value] "), - N_("git config unset [] [--all] [--value=] [--fixed-value] "), + N_("git config unset [] [--all] [--value=] [--fixed-value] "), N_("git config rename-section [] "), N_("git config remove-section [] "), N_("git config edit []"), @@ -43,7 +43,7 @@ static const char *const builtin_config_set_usage[] = { }; static const char *const builtin_config_unset_usage[] = { - N_("git config unset [] [--all] [--value=] [--fixed-value] "), + N_("git config unset [] [--all] [--value=] [--fixed-value] "), NULL }; From cd394c927c1f0b8d41ef644b48f52968519beee0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Oct 2024 04:33:47 -0400 Subject: [PATCH 2/6] simple-ipc: split async server initialization and running To start an async ipc server, you call ipc_server_run_async(). That initializes the ipc_server_data object, and starts all of the threads running, which may immediately start serving clients. This can create some awkward timing problems, though. In the fsmonitor daemon (the sole user of the simple-ipc system), we want to create the ipc server early in the process, which means we may start serving clients before the rest of the daemon is fully initialized. To solve this, let's break run_async() into two parts: an initialization which allocates all data and spawns the threads (without letting them run), and a start function which actually lets them begin work. Since we have two simple-ipc implementations, we have to handle this twice: - in ipc-unix-socket.c, we have a central listener thread which hands connections off to worker threads using a work_available mutex. We can hold that mutex after init, and release it when we're ready to start. We do need an extra "started" flag so that we know whether the main thread is holding the mutex or not (e.g., if we prematurely stop the server, we want to make sure all of the worker threads are released to hear about the shutdown). - in ipc-win32.c, we don't have a central mutex. So we'll introduce a new startup_barrier mutex, which we'll similarly hold until we're ready to let the threads proceed. We again need a "started" flag here to make sure that we release the barrier mutex when shutting down, so that the sub-threads can proceed to the finish. I've renamed the run_async() function to init_async() to make sure we catch all callers, since they'll now need to call the matching start_async(). We could leave run_async() as a wrapper that does both, but there's not much point. There are only two callers, one of which is fsmonitor, which will want to actually do work between the two calls. And the other is just a test-tool wrapper. For now I've added the start_async() calls in fsmonitor where they would otherwise have happened, so there should be no behavior change with this patch. Signed-off-by: Jeff King Acked-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- builtin/fsmonitor--daemon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index f3f6bd330b9ec9..4a077810d0e112 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -1215,6 +1215,8 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) _("could not start IPC thread pool on '%s'"), state->path_ipc.buf); + ipc_server_start_async(&state->ipc_server_data); + /* * Start the fsmonitor listener thread to collect filesystem * events. From 6d2408dad23335eafdc7acba971826dffd242dff Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 7 Oct 2024 21:49:24 +0000 Subject: [PATCH 3/6] docs: fix the `maintain-git` links in `technical/platform-support` These links should point to `.html` files, not to `.txt` ones. Compare also to 4945f046c7f (api docs: link to html version of api-trace2, 2022-09-16). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/technical/platform-support.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/technical/platform-support.txt b/Documentation/technical/platform-support.txt index a227c363d7824c..0a2fb28d6277f1 100644 --- a/Documentation/technical/platform-support.txt +++ b/Documentation/technical/platform-support.txt @@ -49,7 +49,7 @@ will be fixed in a later release: notice problems before they are considered "done with review"; whereas watching `master` means the stable branch could break for your platform, but you have a decent chance of avoiding a tagged release breaking you. See "The - Policy" in link:../howto/maintain-git.txt["How to maintain Git"] for an + Policy" in link:../howto/maintain-git.html["How to maintain Git"] for an overview of which branches are used in the Git project, and how. * The bug report should include information about what platform you are using. @@ -125,7 +125,7 @@ Compatible on `next` To avoid reactive debugging and fixing when changes hit a release or stable, you can aim to ensure `next` always works for your platform. (See "The Policy" in -link:../howto/maintain-git.txt["How to maintain Git"] for an overview of how +link:../howto/maintain-git.html["How to maintain Git"] for an overview of how `next` is used in the Git project.) To do that: * You should add a runner for your platform to the GitHub Actions or GitLab CI From aa83ce5a1d6791288bc7cbe2f96146eec1092151 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Oct 2024 04:36:13 -0400 Subject: [PATCH 4/6] fsmonitor: initialize fs event listener before accepting clients There's a racy hang in fsmonitor on macOS that we sometimes see in CI. When we serve a client, what's supposed to happen is: 1. The client thread calls with_lock__wait_for_cookie() in which we create a cookie file and then wait for a pthread_cond event 2. The filesystem event listener sees the cookie file creation, does some internal book-keeping, and then triggers the pthread_cond. But there's a problem: we start the listener that accepts client threads before we start the fs event thread. So it's possible for us to accept a client which creates the cookie file and starts waiting before the fs event thread is initialized, and we miss those filesystem events entirely. That leaves the client thread hanging forever. In CI, the symptom is that t9210 (which is testing scalar, which always enables fsmonitor under the hood) may hang forever in "scalar clone". It is waiting on "git fetch" which is waiting on the fsmonitor daemon. The race happens more frequently under load, but you can trigger it predictably with a sleep like this, which delays the start of the fs event thread: --- a/compat/fsmonitor/fsm-listen-darwin.c +++ b/compat/fsmonitor/fsm-listen-darwin.c @@ -510,6 +510,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state) FSEventStreamSetDispatchQueue(data->stream, data->dq); data->stream_scheduled = 1; + sleep(1); if (!FSEventStreamStart(data->stream)) { error(_("Failed to start the FSEventStream")); goto force_error_stop_without_loop; One solution might be to reverse the order of initialization: start the fs event thread before we start the thread listening for clients. But the fsmonitor code explicitly does it in the opposite direction. The fs event thread wants to refer to the ipc_server_data struct, so we need it to be initialized first. A further complication is that we need a signal from the fs event thread that it is actually ready and listening. And those details happen within backend-specific fsmonitor code, whereas the initialization is in the shared code. So instead, let's use the ipc_server init/start split added in the previous commit. The generic fsmonitor code will init the ipc_server but _not_ start it, leaving that to the backend specific code, which now needs to call ipc_server_start_async() at the right time. For macOS, that is right after we start the FSEventStream that you can see in the diff above. It's not clear to me if Windows suffers from the same problem (and we simply don't trigger it in CI), or if it is immune. Regardless, the obvious place to start accepting clients there is right after we've established the ReadDirectoryChanges watch. This makes the hangs go away in our macOS CI environment, even when compiled with the sleep() above. Helped-by: Koji Nakamaru Signed-off-by: Jeff King Acked-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- builtin/fsmonitor--daemon.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index 4a077810d0e112..f3f6bd330b9ec9 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -1215,8 +1215,6 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) _("could not start IPC thread pool on '%s'"), state->path_ipc.buf); - ipc_server_start_async(&state->ipc_server_data); - /* * Start the fsmonitor listener thread to collect filesystem * events. From 38430a4366d2e4fdbe194a7e61a684711055bb8f Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Fri, 11 Oct 2024 17:29:47 +0000 Subject: [PATCH 5/6] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the generated 'clar.suite', but this dependency is not taken into account by our Makefile, so that it is possible for a parallel build to fail if Make tries to build 'clar.o' before 'clar.suite' is generated. Correctly specify the dependency. Signed-off-by: Philippe Blain Signed-off-by: Junio C Hamano --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index aea9b14eb8b079..161cb2b6f978f1 100644 --- a/Makefile +++ b/Makefile @@ -3975,6 +3975,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI done >$@ $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS From b484e603644e6833f6c58cd328af90110a3f1c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Krecker?= Date: Thu, 17 Oct 2024 19:18:20 +0200 Subject: [PATCH 6/6] mingw.c: Fix complier warnings for a 64 bit msvc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove some complier warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit integers. Compiling compat/mingw.c under a 64 bit version of msvc produces warnings. An "int" is 32 bit, and ssize_t or size_t should be 64 bit long. Prepare compat/vcbuild/include/unistd.h to have a 64 bit type _ssize_t, when _WIN64 is defined and 32 bit otherwise. Further down in this include file, as before, ssize_t is defined as _ssize_t, if needed. Use size_t instead of int for all variables that hold the result of strlen() or wcslen() (which cannot be negative). Use ssize_t to hold the return value of read(). Signed-off-by: Sören Krecker Signed-off-by: Taylor Blau --- compat/compiler.h | 4 ++-- compat/mingw.c | 23 ++++++++++++++--------- compat/vcbuild/include/unistd.h | 4 ++++ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/compat/compiler.h b/compat/compiler.h index e9ad9db84f2279..e12e426404ab0c 100644 --- a/compat/compiler.h +++ b/compat/compiler.h @@ -9,7 +9,7 @@ static inline void get_compiler_info(struct strbuf *info) { - int len = info->len; + size_t len = info->len; #ifdef __clang__ strbuf_addf(info, "clang: %s\n", __clang_version__); #elif defined(__GNUC__) @@ -27,7 +27,7 @@ static inline void get_compiler_info(struct strbuf *info) static inline void get_libc_info(struct strbuf *info) { - int len = info->len; + size_t len = info->len; #ifdef __GLIBC__ strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); diff --git a/compat/mingw.c b/compat/mingw.c index c1030e40f227d2..be0c7a7a24621e 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1049,7 +1049,7 @@ int mingw_chmod(const char *filename, int mode) */ static int has_valid_directory_prefix(wchar_t *wfilename) { - int n = wcslen(wfilename); + size_t n = wcslen(wfilename); while (n > 0) { wchar_t c = wfilename[--n]; @@ -1628,7 +1628,8 @@ static const char *parse_interpreter(const char *cmd) { static char buf[MAX_PATH]; char *p, *opt; - int n, fd; + ssize_t n; /* read() can return negative values */ + int fd; /* don't even try a .exe */ n = strlen(cmd); @@ -1752,7 +1753,7 @@ static char *path_lookup(const char *cmd, int exe_only) { const char *path; char *prog = NULL; - int len = strlen(cmd); + size_t len = strlen(cmd); int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe"); if (strpbrk(cmd, "/\\")) @@ -2389,7 +2390,7 @@ char *mingw_getenv(const char *name) #define GETENV_MAX_RETAIN 64 static char *values[GETENV_MAX_RETAIN]; static int value_counter; - int len_key, len_value; + size_t len_key, len_value; wchar_t *w_key; char *value; wchar_t w_value[32768]; @@ -2401,7 +2402,8 @@ char *mingw_getenv(const char *name) /* We cannot use xcalloc() here because that uses getenv() itself */ w_key = calloc(len_key, sizeof(wchar_t)); if (!w_key) - die("Out of memory, (tried to allocate %u wchar_t's)", len_key); + die("Out of memory, (tried to allocate %"PRIuMAX" wchar_t's)", + (uintmax_t)len_key); xutftowcs(w_key, name, len_key); /* GetEnvironmentVariableW() only sets the last error upon failure */ SetLastError(ERROR_SUCCESS); @@ -2416,7 +2418,8 @@ char *mingw_getenv(const char *name) /* We cannot use xcalloc() here because that uses getenv() itself */ value = calloc(len_value, sizeof(char)); if (!value) - die("Out of memory, (tried to allocate %u bytes)", len_value); + die("Out of memory, (tried to allocate %"PRIuMAX" bytes)", + (uintmax_t)len_value); xwcstoutf(value, w_value, len_value); /* @@ -2434,7 +2437,7 @@ char *mingw_getenv(const char *name) int mingw_putenv(const char *namevalue) { - int size; + size_t size; wchar_t *wide, *equal; BOOL result; @@ -2444,7 +2447,8 @@ int mingw_putenv(const char *namevalue) size = strlen(namevalue) * 2 + 1; wide = calloc(size, sizeof(wchar_t)); if (!wide) - die("Out of memory, (tried to allocate %u wchar_t's)", size); + die("Out of memory, (tried to allocate %" PRIuMAX " wchar_t's)", + (uintmax_t)size); xutftowcs(wide, namevalue, size); equal = wcschr(wide, L'='); if (!equal) @@ -4072,7 +4076,8 @@ static BOOL WINAPI handle_ctrl_c(DWORD ctrl_type) */ int wmain(int argc, const wchar_t **wargv) { - int i, maxlen, exit_status; + int i, exit_status; + size_t maxlen; char *buffer, **save; const char **argv; diff --git a/compat/vcbuild/include/unistd.h b/compat/vcbuild/include/unistd.h index 3a959d124ca794..a261a925b7f850 100644 --- a/compat/vcbuild/include/unistd.h +++ b/compat/vcbuild/include/unistd.h @@ -14,7 +14,11 @@ typedef _mode_t mode_t; #ifndef _SSIZE_T_ #define _SSIZE_T_ +#ifdef _WIN64 +typedef __int64 _ssize_t; +#else typedef long _ssize_t; +#endif /* _WIN64 */ #ifndef _OFF_T_ #define _OFF_T_