[RFC] Simple IPC mechanism#717
[RFC] Simple IPC mechanism#717jeffhostetler wants to merge 850 commits intogitgitgadget:jk/strvecfrom
Conversation
|
@jeffhostetler would you mind integrating dscho@889e38b and dscho@889e38b into your branch ( |
dscho
left a comment
There was a problem hiding this comment.
Phew. Lots to read, and I cannot even say that I (re-)read the backend code in detail...
unix-socket.c
Outdated
| * portion in `sockaddr_un.path`. This is problematic | ||
| * if called from threaded context. | ||
| */ | ||
| int size = strlen(path) + 1; |
There was a problem hiding this comment.
Or inline strlen(path) + 1? Or avoid the arithmetic? Like so:
if (opts->disallow_chdir &&
strlen(path) >= sizeof(sa.sun_path))
return ENAMETOOLONG;There was a problem hiding this comment.
BTW there will also be the question, I am sure, why this is not folded into struct unix_sockaddr_context.
There was a problem hiding this comment.
WRT the arithmetic, I was using the same spelling as in unix_sockaddr_init()
which does it that way.
There was a problem hiding this comment.
WRT unix_sockaddr_context, that is passed uninitialized to unix_sockaddr_init() and populated by it, if a CD was necessary.
So changing the initialization details felt wrong. However, _context is
private and all the callers are static, so maybe it wouldn't be so bad.
Let me take a look at this.
There was a problem hiding this comment.
I overhauled the disallow-chdir commit. Let me know if this looks better.
There was a problem hiding this comment.
I would still recommend to put the if (ctx->disallow_chdir) { errno = ENAMETOOLONG; return -1; } inside the existing if (size > sizeof(sa->sun_path)) { ... } block, but that's a minor thing.
There was a problem hiding this comment.
i thought about that but it means I have to touch the code inside the next block -- they init a variable as they declare it and it looks like an expensive function call, and I'd have to undo that to get my new if statement wedged in there. and that seems a bit too much. but i'm open to suggestion here.
| return 0; | ||
| } | ||
|
|
||
| /* A randomly chosen value. */ |
| trace2_data_string("ipc-server", NULL, "try-listen-force", path); | ||
|
|
||
| opts.force_unlink_before_bind = 1; | ||
| fd_listen = unix_stream_listen_gently(path, &opts); |
There was a problem hiding this comment.
There is a race here, right? Between unix_stream_connect() and unix_stream_listen_gently(), another service process could have just started listening. Maybe clarify that in an inline comment? (To me, the comment above the function does not really clarify this point.)
There was a problem hiding this comment.
i've completely reworded the comments above and in-line in that function. let me know if that helps.
ae3b033 to
6624f47
Compare
|
I just saw your note about about fixing the CI builds and put those 2 commits on top for now. Thanks!!! |
6f3929c to
eef62bc
Compare
|
@dscho Hey, big thanks for major review and suggestions. I think I have addressed most all of them. |
eef62bc to
040fd49
Compare
|
There is an issue in commit 7b9c519: |
7b9c519 to
af408d2
Compare
af408d2 to
84b54bd
Compare
|
Rebased my simple-ipc branch from "git.git master v2.28 + 11th batch" back to "v2.28 + jk/strvec". |
"git worktree add" learns that the "-d" is a synonym to "--detach" option to create a new worktree without being on a branch. * es/wt-add-detach: git-worktree.txt: discuss branch-based vs. throwaway worktrees worktree: teach `add` to recognize -d as shorthand for --detach git-checkout.txt: document -d short option for --detach
Optimization around submodule handling. * os/collect-changed-submodules-optim: submodule: suppress checking for file name and ref ambiguity for object ids
"git status --short" quoted a path with SP in it when tracked, but not those that are untracked, ignored or unmerged. They are all shown quoted consistently. * jc/quote-path-cleanup: quote: turn 'nodq' parameter into a set of flags quote: rename misnamed sq_lookup[] to cq_lookup[] wt-status: consistently quote paths in "status --short" output quote_path: code clarification quote_path: optionally allow quoting a path with SP in it quote_path: give flags parameter to quote_path() quote_path: rename quote_path_relative() to quote_path()
"git diff/show" on a change that involves a submodule used to read the information on commits in the submodule from a wrong repository and gave a wrong information when the commit-graph is involved. * mf/submodule-summary-with-correct-repository: submodule: use submodule repository when preparing summary revision: use repository from rev_info when parsing commits
Allow maintainers to tweak $(TAR) invocations done while making distribution tarballs. * jc/dist-tarball-tweak: Makefile: allow extra tweaking of distribution tarball
Unlike "git config --local", "git config --worktree" did not fail early and cleanly when started outside a git repository. * mt/config-fail-nongit-early: config: complain about --worktree outside of a git repo
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Call hashwrite_be32() instead of open-coding it. This shortens the code a bit and makes it easier to read. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-p4 unshelve uses HEAD^$n to find the parent commit, which fails if there is an additional commit. Signed-off-by: Luke Diamand <luke@diamand.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* 'master' of github.com:alshopov/git-po: l10n: bg.po: Updated Bulgarian translation (5013t)
I am excited. Because I like a lot languages, and because I believe this is the way to contribute to a large number of Portuguese speaking person. Jiang Xin and last Portuguese team gave me the lead. Thank you very much. Honored to be a part of such a project. Signed-off-by: Daniel Santos <hello@brighterdan.com>
Pull from the language Coordenator repository and `make` done at the top-level directory. Signed-off-by: Daniel Santos <hello@brighterdan.com>
Signed-off-by: Tran Ngoc Quan <vnwildman@gmail.com>
* 'pt-PT' of github.com:git-l10n-pt-PT/git-po: l10n: pt_PT: make on po/pt_PT.po l10n: Portuguese translation team has changed. Wohoo!
Reviewed-by: Ralf Thielow <ralf.thielow@gmail.com> Reviewed-by: Phillip Szelat <phillip.szelat@gmail.com> Signed-off-by: Matthias Rüster <matthias.ruester@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Mac, tooltips are not automatically removed when a window loses
focus. Furthermore, mouse-move events are only dispatched to the active
window, which means that if we Command-tab to another application while
a tool tip is showing, the tool tip will stay there forever (in front of
other applications). So we must hide it manually when we lose focus.
Do this unconditionally here (i.e. without if {[is_MacOSX]}); it
shouldn't hurt on other platforms, even though they don't seem to have
this problem.
Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
Make sure `git gui blame` tooltips are destroyed once the window loses focus on MacOS. * sh/blame-tooltip: git-gui: blame: prevent tool tips from sticking around after Command-Tab
* https://github.com/prati0100/git-gui: git-gui: blame: prevent tool tips from sticking around after Command-Tab git-gui: improve dark mode support git-gui: fix mixed tabs and spaces; prefer tabs
Translate 124 new messages (5013t0f0u) for git 2.29.0. Reviewed-by: 依云 <lilydjwg@gmail.com> Reviewed-by: Fangyi Zhou <me@fangyi.io> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
* 'master' of github.com:Softcatala/git-po: l10n: Update Catalan translation
l10n for Git 2.29.0 round 2 * tag 'l10n-2.29.0-rnd2' of git://github.com/git-l10n/git-po: l10n: zh_CN: for git v2.29.0 l10n round 1 and 2 l10n: de.po: Update German translation for Git 2.29.0 l10n: vi(5013t): Updated translation for v2.29.0 rd2 l10n: pt_PT: make on po/pt_PT.po l10n: Portuguese translation team has changed. Wohoo! l10n: bg.po: Updated Bulgarian translation (5013t) l10n: sv.po: Update Swedish translation (5013t0f0u) l10n: it.po: update the Italian translation l10n: tr: v2.29.0 round 2 l10n: zh_TW.po: v2.29.0 round 2 (2 untranslated) l10n: fr: v2.29.0 rnd 2 l10n: git.pot: v2.29.0 round 2 (1 new, 1 removed) l10n: fr: v2.29.0 rnd 1 l10n: it.po: update the Italian translation for Git 2.29.0 round 1 l10n: tr: v2.29.0 round 1 l10n: Update Catalan translation l10n: git.pot: v2.29.0 round 1 (124 new, 42 removed)
Teach packet_write_gently() to use a stack buffer rather than a static buffer when composing the packet line message. This helps get us ready for threaded operations. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
So far, the (possibly indirect) callers of `get_packet_data()` can ask that function to return an error instead of `die()`ing upon end-of-file. However, random read errors will still cause the process to die. So let's introduce an explicit option to tell the packet reader machinery to please be nice and only return an error. This change prepares pkt-line for use by long-running daemon processes. Such processes should be able to serve multiple concurrent clients and and survive random IO errors. If there is an error on one connection, a daemon should be able to drop that connection and continue serving existing and future connections. This ability will be used by a Git-aware "Internal FSMonitor" feature in a later patch series. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…uf() This function currently has only one caller: `apply_multi_file_filter()` in `convert.c`. That caller wants a flush packet to be written after writing the payload. However, we are about to introduce a user that wants to write many packets before a final flush packet, so let's extend this function to prepare for that scenario. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `read_packetized_to_strbuf()` function reads packets into a strbuf until a flush packet has been received. So far, it has only one caller: `apply_multi_file_filter()` in `convert.c`. This caller really only needs the `PACKET_READ_GENTLE_ON_EOF` option to be passed to `packet_read()` (which makes sense in the scenario where packets should be read until a flush packet is received). We are about to introduce a caller that wants to pass other options through to `packet_read()`, so let's extend the function signature accordingly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Brief design documentation for new IPC mechanism allowing foreground Git client to talk with an existing daemon process at a known location using a named pipe or unix domain socket. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create Windows implementation of "simple-ipc" using named pipes. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create a gentle version of `unix_stream_listen()`. This version does not call `die()` if a socket-fd cannot be created and does not assume that it is safe to `unlink()` an existing socket-inode. `unix_stream_listen()` uses `unix_stream_socket()` helper function to create the socket-fd. Avoid that helper because it calls `die()` on errors. `unix_stream_listen()` always tries to `unlink()` the socket-path before calling `bind()`. If there is an existing server/daemon already bound and listening on that socket-path, our `unlink()` would have the effect of disassociating the existing server's bound-socket-fd from the socket-path without notifying the existing server. The existing server could continue to service existing connections (accepted-socket-fd's), but would not receive any futher new connections (since clients rendezvous via the socket-path). The existing server would effectively be offline but yet appear to be active. Furthermore, `unix_stream_listen()` creates an opportunity for a brief race condition for connecting clients if they try to connect in the interval between the forced `unlink()` and the subsequent `bind()` (which recreates the socket-path that is bound to a new socket-fd in the current process). Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Calls to `chdir()` are dangerous in a multi-threaded context. If `unix_stream_listen()` is given a socket pathname that is too big to fit in a `sockaddr_un` structure, it will `chdir()` to the parent directory of the requested socket pathname, create the socket using a relative pathname, and then `chdir()` back. This is not thread-safe. Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change all callers to pass an initialized context structure. Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag is set. Extend the public interface to `unix_stream_listen_gently()` to also expose this new flag. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create unit tests for "simple-ipc". These are currently only enabled on Windows. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create Unix domain socket based implementation of "simple-ipc". Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
84b54bd to
03cfed4
Compare
|
The pull request has 850 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
|
Rebased simple-ipc onto v2.29.0 in my branch. |
|
See #766 |
This series introduces a multi-threaded IPC mechanism called
simple-ipc.It allows a foreground Git process to communicate with an existing Git daemon
process using named pipes (on Windows) or unix domain sockets (on other
platforms).
This will serve as a basis for a future Git-aware FSMonitor service daemon
feature.