* [PATCH v3 01/26] pkt-line.c: rename global variable buffer[] to something less generic
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 02/26] pkt-line.c: add packet_write_timeout() Nguyễn Thái Ngọc Duy
` (25 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
"buffer" is a local variable in some other functions. Rename the
global one to make it less confusing.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
pkt-line.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index bc63b3b..eac45ad 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -64,14 +64,15 @@ void packet_buf_flush(struct strbuf *buf)
}
#define hex(a) (hexchar[(a) & 15])
-static char buffer[1000];
+static char write_buffer[1000];
static unsigned format_packet(const char *fmt, va_list args)
{
static char hexchar[] = "0123456789abcdef";
+ char *buffer = write_buffer;
unsigned n;
- n = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);
- if (n >= sizeof(buffer)-4)
+ n = vsnprintf(buffer + 4, sizeof(write_buffer) - 4, fmt, args);
+ if (n >= sizeof(write_buffer)-4)
die("protocol error: impossibly long line");
n += 4;
buffer[0] = hex(n >> 12);
@@ -90,7 +91,7 @@ void packet_write(int fd, const char *fmt, ...)
va_start(args, fmt);
n = format_packet(fmt, args);
va_end(args);
- write_or_die(fd, buffer, n);
+ write_or_die(fd, write_buffer, n);
}
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
@@ -101,7 +102,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
va_start(args, fmt);
n = format_packet(fmt, args);
va_end(args);
- strbuf_add(buf, buffer, n);
+ strbuf_add(buf, write_buffer, n);
}
static int get_packet_data(int fd, char **src_buf, size_t *src_size,
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 02/26] pkt-line.c: add packet_write_timeout()
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 01/26] pkt-line.c: rename global variable buffer[] to something less generic Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 03/26] pkt-line.c: add packet_read_line_timeout() Nguyễn Thái Ngọc Duy
` (24 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
pkt-line.c | 15 +++++++++++++++
pkt-line.h | 1 +
wrapper.c | 26 ++++++++++++++++++++++++++
4 files changed, 43 insertions(+)
diff --git a/cache.h b/cache.h
index dc040fb..718e32b 100644
--- a/cache.h
+++ b/cache.h
@@ -1231,6 +1231,7 @@ extern void fsync_or_die(int fd, const char *);
extern ssize_t read_in_full(int fd, void *buf, size_t count);
extern ssize_t write_in_full(int fd, const void *buf, size_t count);
+extern ssize_t write_in_full_timeout(int fd, const void *buf, size_t count, int timeout);
static inline ssize_t write_str_in_full(int fd, const char *str)
{
return write_in_full(fd, str, strlen(str));
diff --git a/pkt-line.c b/pkt-line.c
index eac45ad..cf681e9 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -94,6 +94,21 @@ void packet_write(int fd, const char *fmt, ...)
write_or_die(fd, write_buffer, n);
}
+int packet_write_timeout(int fd, int timeout, const char *fmt, ...)
+{
+ static struct strbuf sb = STRBUF_INIT;
+ va_list args;
+ unsigned n;
+
+ if (fd == -1)
+ return -1;
+ va_start(args, fmt);
+ strbuf_reset(&sb);
+ n = format_packet(fmt, args);
+ va_end(args);
+ return write_in_full_timeout(fd, write_buffer, n, timeout);
+}
+
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
{
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 0a838d1..4b93a0c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -21,6 +21,7 @@
*/
void packet_flush(int fd);
void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+int packet_write_timeout(int fd, int timeout, const char *fmt, ...) __attribute__((format (printf, 3, 4)));
void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
diff --git a/wrapper.c b/wrapper.c
index 0cc5636..9a0e289 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -214,6 +214,32 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
return total;
}
+ssize_t write_in_full_timeout(int fd, const void *buf,
+ size_t count, int timeout)
+{
+ struct pollfd pfd;
+ const char *p = buf;
+ ssize_t total = 0;
+
+ pfd.fd = fd;
+ pfd.events = POLLOUT;
+ while (count > 0 && poll(&pfd, 1, timeout) > 0 &&
+ (pfd.revents & POLLOUT)) {
+ ssize_t written = xwrite(fd, p, count);
+ if (written < 0)
+ return -1;
+ if (!written) {
+ errno = ENOSPC;
+ return -1;
+ }
+ count -= written;
+ p += written;
+ total += written;
+ }
+
+ return count ? -1 : total;
+}
+
int xdup(int fd)
{
int ret = dup(fd);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 03/26] pkt-line.c: add packet_read_line_timeout()
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 01/26] pkt-line.c: rename global variable buffer[] to something less generic Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 02/26] pkt-line.c: add packet_write_timeout() Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 04/26] unix-socket: make unlink() optional in unix_stream_listen() Nguyễn Thái Ngọc Duy
` (23 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This version is also gentler than its friend packet_read_line()
because it's designed for side channel I/O that should not abort the
program even if the channel is broken.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
pkt-line.c | 35 +++++++++++++++++++++++++++++++++++
pkt-line.h | 1 +
wrapper.c | 21 +++++++++++++++++++++
4 files changed, 58 insertions(+)
diff --git a/cache.h b/cache.h
index 718e32b..939db46 100644
--- a/cache.h
+++ b/cache.h
@@ -1230,6 +1230,7 @@ extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
extern void fsync_or_die(int fd, const char *);
extern ssize_t read_in_full(int fd, void *buf, size_t count);
+extern ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout);
extern ssize_t write_in_full(int fd, const void *buf, size_t count);
extern ssize_t write_in_full_timeout(int fd, const void *buf, size_t count, int timeout);
static inline ssize_t write_str_in_full(int fd, const char *str)
diff --git a/pkt-line.c b/pkt-line.c
index cf681e9..5a07e97 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -229,3 +229,38 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
{
return packet_read_line_generic(-1, src, src_len, dst_len);
}
+
+char *packet_read_line_timeout(int fd, int timeout, int *len_p)
+{
+ char *buf = packet_buffer;
+ int ret, len, buf_len = sizeof(packet_buffer);
+ char linelen[4];
+
+ if (fd == -1)
+ return NULL;
+ if ((ret = read_in_full_timeout(fd, linelen, 4, timeout)) < 0)
+ return NULL;
+ len = packet_length(linelen);
+ if (len < 0) {
+ error("protocol error: bad line length character: %.4s", linelen);
+ return NULL;
+ }
+ if (!len) {
+ packet_trace("0000", 4, 0);
+ if (len_p)
+ *len_p = 0;
+ return "";
+ }
+ len -= 4;
+ if (len >= buf_len) {
+ error("protocol error: bad line length %d", len);
+ return NULL;
+ }
+ if ((ret = read_in_full_timeout(fd, buf, len, timeout)) < 0)
+ return NULL;
+ buf[len] = '\0';
+ if (len_p)
+ *len_p = len;
+ packet_trace(buf, len, 0);
+ return buf;
+}
diff --git a/pkt-line.h b/pkt-line.h
index 4b93a0c..d47dca5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -69,6 +69,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
* packet is written to it.
*/
char *packet_read_line(int fd, int *size);
+char *packet_read_line_timeout(int fd, int timeout, int *size);
/*
* Same as packet_read_line, but read from a buf rather than a descriptor;
diff --git a/wrapper.c b/wrapper.c
index 9a0e289..9cf10b2 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -193,6 +193,27 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
return total;
}
+ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout)
+{
+ char *p = buf;
+ ssize_t total = 0;
+ struct pollfd pfd;
+
+ pfd.fd = fd;
+ pfd.events = POLLIN;
+ while (count > 0 && poll(&pfd, 1, timeout) > 0 &&
+ (pfd.revents & POLLIN)) {
+ ssize_t loaded = xread(fd, p, count);
+ if (loaded <= 0)
+ return -1;
+ count -= loaded;
+ p += loaded;
+ total += loaded;
+ }
+
+ return count ? -1 : total;
+}
+
ssize_t write_in_full(int fd, const void *buf, size_t count)
{
const char *p = buf;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 04/26] unix-socket: make unlink() optional in unix_stream_listen()
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (2 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 03/26] pkt-line.c: add packet_read_line_timeout() Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 05/26] Add git-file-watcher and basic connection handling logic Nguyễn Thái Ngọc Duy
` (22 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
credential-cache--daemon.c | 2 +-
unix-socket.c | 5 +++--
unix-socket.h | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..1b995a9 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -207,7 +207,7 @@ static void serve_cache(const char *socket_path)
{
int fd;
- fd = unix_stream_listen(socket_path);
+ fd = unix_stream_listen(socket_path, 1);
if (fd < 0)
die_errno("unable to bind to '%s'", socket_path);
diff --git a/unix-socket.c b/unix-socket.c
index 01f119f..2be1af6 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -93,7 +93,7 @@ fail:
return -1;
}
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path, int replace)
{
int fd, saved_errno;
struct sockaddr_un sa;
@@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
return -1;
fd = unix_stream_socket();
- unlink(path);
+ if (replace)
+ unlink(path);
if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
goto fail;
diff --git a/unix-socket.h b/unix-socket.h
index e271aee..18ee290 100644
--- a/unix-socket.h
+++ b/unix-socket.h
@@ -2,6 +2,6 @@
#define UNIX_SOCKET_H
int unix_stream_connect(const char *path);
-int unix_stream_listen(const char *path);
+int unix_stream_listen(const char *path, int replace);
#endif /* UNIX_SOCKET_H */
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 05/26] Add git-file-watcher and basic connection handling logic
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (3 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 04/26] unix-socket: make unlink() optional in unix_stream_listen() Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 06/26] file-watcher: check socket directory permission Nguyễn Thái Ngọc Duy
` (21 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
git-file-watcher is a daemon (*) that watches file changes and tells
git about them. The intent is to avoid lstat() calls at index refresh
time, which could be a lot on big working directory.
The actual monitoring needs support from OS (inotify, FSEvents,
FindFirstChangeNotification or kqueue) and is not part of this patch
or the next few yet. This patch only provides a UNIX socket server.
(*) it will be a a daemon in this end, but in this patch it runs in
the foreground.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
.gitignore | 1 +
Documentation/git-file-watcher.txt (new) | 29 +++++++
Makefile | 1 +
file-watcher.c (new) | 140 +++++++++++++++++++++++++++++++
4 files changed, 171 insertions(+)
create mode 100644 Documentation/git-file-watcher.txt
create mode 100644 file-watcher.c
diff --git a/.gitignore b/.gitignore
index b5f9def..12c78f0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -56,6 +56,7 @@
/git-fast-import
/git-fetch
/git-fetch-pack
+/git-file-watcher
/git-filter-branch
/git-fmt-merge-msg
/git-for-each-ref
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
new file mode 100644
index 0000000..625a389
--- /dev/null
+++ b/Documentation/git-file-watcher.txt
@@ -0,0 +1,29 @@
+git-file-watcher(1)
+===================
+
+NAME
+----
+git-file-watcher - File update notification daemon
+
+SYNOPSIS
+--------
+[verse]
+'git file-watcher' [options] <socket directory>
+
+DESCRIPTION
+-----------
+This program watches file changes in a git working directory and let
+Git now what files have been changed so that Git does not have to call
+lstat(2) to detect that itself.
+
+OPTIONS
+-------
+
+SEE ALSO
+--------
+linkgit:git-update-index[1],
+linkgit:git-config[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index dddaf4f..8eef0d6 100644
--- a/Makefile
+++ b/Makefile
@@ -536,6 +536,7 @@ PROGRAMS += $(EXTRA_PROGRAMS)
PROGRAM_OBJS += credential-store.o
PROGRAM_OBJS += daemon.o
PROGRAM_OBJS += fast-import.o
+PROGRAM_OBJS += file-watcher.o
PROGRAM_OBJS += http-backend.o
PROGRAM_OBJS += imap-send.o
PROGRAM_OBJS += sh-i18n--envsubst.o
diff --git a/file-watcher.c b/file-watcher.c
new file mode 100644
index 0000000..a6d7f17
--- /dev/null
+++ b/file-watcher.c
@@ -0,0 +1,140 @@
+#include "cache.h"
+#include "sigchain.h"
+#include "parse-options.h"
+#include "exec_cmd.h"
+#include "unix-socket.h"
+
+static const char *const file_watcher_usage[] = {
+ N_("git file-watcher [options] <socket directory>"),
+ NULL
+};
+
+struct connection {
+ int sock;
+};
+
+static struct connection **conns;
+static struct pollfd *pfd;
+static int conns_alloc, pfd_nr, pfd_alloc;
+
+static int shutdown_connection(int id)
+{
+ struct connection *conn = conns[id];
+ conns[id] = NULL;
+ pfd[id].fd = -1;
+ if (!conn)
+ return 0;
+ close(conn->sock);
+ free(conn);
+ return 0;
+}
+
+static int handle_command(int conn_id)
+{
+ return 0;
+}
+
+static void accept_connection(int fd)
+{
+ struct connection *conn;
+ int client = accept(fd, NULL, NULL);
+ if (client < 0) {
+ warning(_("accept failed: %s"), strerror(errno));
+ return;
+ }
+
+ ALLOC_GROW(pfd, pfd_nr + 1, pfd_alloc);
+ pfd[pfd_nr].fd = client;
+ pfd[pfd_nr].events = POLLIN;
+ pfd[pfd_nr].revents = 0;
+
+ ALLOC_GROW(conns, pfd_nr + 1, conns_alloc);
+ conn = xmalloc(sizeof(*conn));
+ memset(conn, 0, sizeof(*conn));
+ conn->sock = client;
+ conns[pfd_nr] = conn;
+ pfd_nr++;
+}
+
+int main(int argc, const char **argv)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int i, new_nr, fd, quit = 0, nr_common;
+ const char *socket_path = NULL;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ git_extract_argv0_path(argv[0]);
+ git_setup_gettext();
+ argc = parse_options(argc, argv, NULL, options,
+ file_watcher_usage, 0);
+ if (argc < 1)
+ die(_("socket path missing"));
+ else if (argc > 1)
+ die(_("too many arguments"));
+
+ socket_path = argv[0];
+ strbuf_addf(&sb, "%s/socket", socket_path);
+ fd = unix_stream_listen(sb.buf, 0);
+ if (fd == -1)
+ die_errno(_("unable to listen at %s"), sb.buf);
+ strbuf_reset(&sb);
+
+ nr_common = 1;
+ pfd_alloc = pfd_nr = nr_common;
+ pfd = xmalloc(sizeof(*pfd) * pfd_alloc);
+ pfd[0].fd = fd;
+ pfd[0].events = POLLIN;
+
+ while (!quit) {
+ if (poll(pfd, pfd_nr, -1) < 0) {
+ if (errno != EINTR) {
+ error("Poll failed, resuming: %s",
+ strerror(errno));
+ sleep(1);
+ }
+ continue;
+ }
+
+ for (new_nr = i = nr_common; i < pfd_nr; i++) {
+ if (pfd[i].revents & (POLLERR | POLLNVAL))
+ shutdown_connection(i);
+ else if ((pfd[i].revents & POLLIN) && conns[i]) {
+ unsigned int avail = 1;
+ /*
+ * pkt-line is not gentle with eof, at
+ * least not with
+ * packet_read_line(). Avoid feeding
+ * eof to it.
+ */
+ if ((pfd[i].revents & POLLHUP) &&
+ ioctl(pfd[i].fd, FIONREAD, &avail))
+ die_errno("unable to FIONREAD inotify handle");
+ /*
+ * We better have a way to handle all
+ * packets in one go...
+ */
+ if (avail)
+ handle_command(i);
+ else
+ shutdown_connection(i);
+ } else if (pfd[i].revents & POLLHUP)
+ shutdown_connection(i);
+ if (!conns[i])
+ continue;
+ if (i != new_nr) { /* pfd[] is shrunk, move pfd[i] up */
+ conns[new_nr] = conns[i];
+ pfd[new_nr] = pfd[i];
+ }
+ new_nr++; /* keep the good socket */
+ }
+ pfd_nr = new_nr;
+
+ if (pfd[0].revents & POLLIN)
+ accept_connection(pfd[0].fd);
+ if (pfd[0].revents & (POLLHUP | POLLERR | POLLNVAL))
+ die(_("error on listening socket"));
+ }
+ return 0;
+}
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 06/26] file-watcher: check socket directory permission
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (4 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 05/26] Add git-file-watcher and basic connection handling logic Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 07/26] file-watcher: remove socket on exit Nguyễn Thái Ngọc Duy
` (20 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Access to the unix socket $WATCHER/socket is covered by $WATCHER's
permission. While the file watcher does not carry much information,
repo file listing is sometimes not something you want other users to
see. Make sure $WATCHER has 0700 permission to stop unwanted access.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/file-watcher.c b/file-watcher.c
index a6d7f17..91f4cfe 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -56,6 +56,37 @@ static void accept_connection(int fd)
pfd_nr++;
}
+static const char permissions_advice[] =
+N_("The permissions on your socket directory are too loose; other\n"
+ "processes may be able to read your file listing. Consider running:\n"
+ "\n"
+ " chmod 0700 %s");
+static void check_socket_directory(const char *path)
+{
+ struct stat st;
+ char *path_copy = xstrdup(path);
+ char *dir = dirname(path_copy);
+
+ if (!stat(dir, &st)) {
+ if (st.st_mode & 077)
+ die(_(permissions_advice), dir);
+ free(path_copy);
+ return;
+ }
+
+ /*
+ * We must be sure to create the directory with the correct mode,
+ * not just chmod it after the fact; otherwise, there is a race
+ * condition in which somebody can chdir to it, sleep, then try to open
+ * our protected socket.
+ */
+ if (safe_create_leading_directories_const(dir) < 0)
+ die_errno(_("unable to create directories for '%s'"), dir);
+ if (mkdir(dir, 0700) < 0)
+ die_errno(_("unable to mkdir '%s'"), dir);
+ free(path_copy);
+}
+
int main(int argc, const char **argv)
{
struct strbuf sb = STRBUF_INIT;
@@ -76,6 +107,7 @@ int main(int argc, const char **argv)
socket_path = argv[0];
strbuf_addf(&sb, "%s/socket", socket_path);
+ check_socket_directory(sb.buf);
fd = unix_stream_listen(sb.buf, 0);
if (fd == -1)
die_errno(_("unable to listen at %s"), sb.buf);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 07/26] file-watcher: remove socket on exit
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (5 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 06/26] file-watcher: check socket directory permission Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 08/26] file-watcher: add --detach Nguyễn Thái Ngọc Duy
` (19 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/file-watcher.c b/file-watcher.c
index 91f4cfe..9c639ef 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -56,6 +56,26 @@ static void accept_connection(int fd)
pfd_nr++;
}
+static const char *socket_path;
+static int do_not_clean_up;
+
+static void cleanup(void)
+{
+ struct strbuf sb = STRBUF_INIT;
+ if (do_not_clean_up)
+ return;
+ strbuf_addf(&sb, "%s/socket", socket_path);
+ unlink(sb.buf);
+ strbuf_release(&sb);
+}
+
+static void cleanup_on_signal(int signo)
+{
+ cleanup();
+ sigchain_pop(signo);
+ raise(signo);
+}
+
static const char permissions_advice[] =
N_("The permissions on your socket directory are too loose; other\n"
"processes may be able to read your file listing. Consider running:\n"
@@ -91,7 +111,6 @@ int main(int argc, const char **argv)
{
struct strbuf sb = STRBUF_INIT;
int i, new_nr, fd, quit = 0, nr_common;
- const char *socket_path = NULL;
struct option options[] = {
OPT_END()
};
@@ -113,6 +132,9 @@ int main(int argc, const char **argv)
die_errno(_("unable to listen at %s"), sb.buf);
strbuf_reset(&sb);
+ atexit(cleanup);
+ sigchain_push_common(cleanup_on_signal);
+
nr_common = 1;
pfd_alloc = pfd_nr = nr_common;
pfd = xmalloc(sizeof(*pfd) * pfd_alloc);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 08/26] file-watcher: add --detach
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (6 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 07/26] file-watcher: remove socket on exit Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 09/26] read-cache: save trailing sha-1 Nguyễn Thái Ngọc Duy
` (18 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
In daemon mode, stdout and stderr are saved in $WATCHER/log.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-file-watcher.txt | 2 ++
cache.h | 1 +
daemon.c | 30 ++++--------------------------
file-watcher.c | 17 +++++++++++++++++
setup.c | 25 +++++++++++++++++++++++++
5 files changed, 49 insertions(+), 26 deletions(-)
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index 625a389..ec81f18 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -18,6 +18,8 @@ lstat(2) to detect that itself.
OPTIONS
-------
+--detach::
+ Run in background.
SEE ALSO
--------
diff --git a/cache.h b/cache.h
index 939db46..7a836b1 100644
--- a/cache.h
+++ b/cache.h
@@ -434,6 +434,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
extern int init_db(const char *template_dir, unsigned int flags);
extern void sanitize_stdfds(void);
+extern int daemonize(int *);
#define alloc_nr(x) (((x)+16)*3/2)
diff --git a/daemon.c b/daemon.c
index 503e039..2650504 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1056,11 +1056,6 @@ static void drop_privileges(struct credentials *cred)
/* nothing */
}
-static void daemonize(void)
-{
- die("--detach not supported on this platform");
-}
-
static struct credentials *prepare_credentials(const char *user_name,
const char *group_name)
{
@@ -1102,24 +1097,6 @@ static struct credentials *prepare_credentials(const char *user_name,
return &c;
}
-
-static void daemonize(void)
-{
- switch (fork()) {
- case 0:
- break;
- case -1:
- die_errno("fork failed");
- default:
- exit(0);
- }
- if (setsid() == -1)
- die_errno("setsid failed");
- close(0);
- close(1);
- close(2);
- sanitize_stdfds();
-}
#endif
static void store_pid(const char *path)
@@ -1333,9 +1310,10 @@ int main(int argc, char **argv)
if (inetd_mode || serve_mode)
return execute();
- if (detach)
- daemonize();
- else
+ if (detach) {
+ if (daemonize(NULL))
+ die("--detach not supported on this platform");
+ } else
sanitize_stdfds();
if (pid_file)
diff --git a/file-watcher.c b/file-watcher.c
index 9c639ef..1e1ccad 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -111,7 +111,10 @@ int main(int argc, const char **argv)
{
struct strbuf sb = STRBUF_INIT;
int i, new_nr, fd, quit = 0, nr_common;
+ int daemon = 0;
struct option options[] = {
+ OPT_BOOL(0, "detach", &daemon,
+ N_("run in background")),
OPT_END()
};
@@ -135,6 +138,20 @@ int main(int argc, const char **argv)
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
+ if (daemon) {
+ int err;
+ strbuf_addf(&sb, "%s/log", socket_path);
+ err = open(sb.buf, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+ adjust_shared_perm(sb.buf);
+ if (err == -1)
+ die_errno(_("unable to create %s"), sb.buf);
+ if (daemonize(&do_not_clean_up))
+ die(_("--detach not supported on this platform"));
+ dup2(err, 1);
+ dup2(err, 2);
+ close(err);
+ }
+
nr_common = 1;
pfd_alloc = pfd_nr = nr_common;
pfd = xmalloc(sizeof(*pfd) * pfd_alloc);
diff --git a/setup.c b/setup.c
index 6c3f85f..757c45f 100644
--- a/setup.c
+++ b/setup.c
@@ -787,3 +787,28 @@ void sanitize_stdfds(void)
if (fd > 2)
close(fd);
}
+
+int daemonize(int *flag)
+{
+#ifndef NO_POSIX_GOODIES
+ switch (fork()) {
+ case 0:
+ break;
+ case -1:
+ die_errno("fork failed");
+ default:
+ if (flag)
+ *flag = 1;
+ exit(0);
+ }
+ if (setsid() == -1)
+ die_errno("setsid failed");
+ close(0);
+ close(1);
+ close(2);
+ sanitize_stdfds();
+ return 0;
+#else
+ return -1;
+#endif
+}
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 09/26] read-cache: save trailing sha-1
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (7 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 08/26] file-watcher: add --detach Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 10/26] read-cache: new flag CE_WATCHED to mark what file is watched Nguyễn Thái Ngọc Duy
` (17 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This will be used as signature to know if the index has changed.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
read-cache.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 7a836b1..f14d535 100644
--- a/cache.h
+++ b/cache.h
@@ -279,6 +279,7 @@ struct index_state {
initialized : 1;
struct hash_table name_hash;
struct hash_table dir_hash;
+ unsigned char sha1[20];
};
extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 33dd676..3b6daf1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1269,10 +1269,11 @@ struct ondisk_cache_entry_extended {
ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
ondisk_cache_entry_size(ce_namelen(ce)))
-static int verify_hdr(struct cache_header *hdr, unsigned long size)
+static int verify_hdr(struct cache_header *hdr,
+ unsigned long size,
+ unsigned char *sha1)
{
git_SHA_CTX c;
- unsigned char sha1[20];
int hdr_version;
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
@@ -1461,7 +1462,7 @@ int read_index_from(struct index_state *istate, const char *path)
close(fd);
hdr = mmap;
- if (verify_hdr(hdr, mmap_size) < 0)
+ if (verify_hdr(hdr, mmap_size, istate->sha1) < 0)
goto unmap;
istate->version = ntohl(hdr->hdr_version);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 10/26] read-cache: new flag CE_WATCHED to mark what file is watched
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (8 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 09/26] read-cache: save trailing sha-1 Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:28 ` [PATCH v3 11/26] Clear CE_WATCHED when set CE_VALID alone Nguyễn Thái Ngọc Duy
` (16 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This bit is basically "dynamic CE_VALID". It marks entries that are
being watched by the incoming file-watcher. When an index is loaded,
file watcher is contacted and the list of updated paths is retrieved.
These paths will have CE_WATCHED cleared and lstat() will be called on
them. Those that have CE_WATCHED and not in the list will have
CE_VALID turn on to skip lstat(). The setting is temporarily, CE_VALID
is not saved to disk if CE_WATCHED is also set.
We keep the CE_WATCHED in a new extension, separated from the entries
to save some space because extended ce_flags adds 2 bytes per entry
and this flag would be present in the majority of entries. When stored
as bitmap, this extension could compress very well with ewah algorithm.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/technical/index-format.txt | 6 +++++
cache.h | 3 +++
read-cache.c | 41 +++++++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index f352a9b..24fd0ae 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -198,3 +198,9 @@ Git index format
- At most three 160-bit object names of the entry in stages from 1 to 3
(nothing is written for a missing stage).
+=== File watcher
+
+ The signature of this extension is { 'W', 'A', 'T', 'C' }.
+
+ - A bit map of all entries in the index, n-th bit of m-th byte
+ corresponds to CE_WATCHED of the <m * 8+ n>-th index entry.
diff --git a/cache.h b/cache.h
index f14d535..a0af2a5 100644
--- a/cache.h
+++ b/cache.h
@@ -169,6 +169,9 @@ struct cache_entry {
/* used to temporarily mark paths matched by pathspecs */
#define CE_MATCHED (1 << 26)
+/* set CE_VALID at runtime if the entry is guaranteed not updated */
+#define CE_WATCHED (1 << 27)
+
/*
* Extended on-disk flags
*/
diff --git a/read-cache.c b/read-cache.c
index 3b6daf1..098d3b6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -33,6 +33,7 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall
#define CACHE_EXT(s) ( (s[0]<<24)|(s[1]<<16)|(s[2]<<8)|(s[3]) )
#define CACHE_EXT_TREE 0x54524545 /* "TREE" */
#define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
+#define CACHE_EXT_WATCH 0x57415443 /* "WATC" */
struct index_state the_index;
@@ -1289,6 +1290,19 @@ static int verify_hdr(struct cache_header *hdr,
return 0;
}
+static void read_watch_extension(struct index_state *istate, uint8_t *data,
+ unsigned long sz)
+{
+ int i;
+ if ((istate->cache_nr + 7) / 8 != sz) {
+ error("invalid 'WATC' extension");
+ return;
+ }
+ for (i = 0; i < istate->cache_nr; i++)
+ if (data[i / 8] & (1 << (i % 8)))
+ istate->cache[i]->ce_flags |= CE_WATCHED;
+}
+
static int read_index_extension(struct index_state *istate,
const char *ext, void *data, unsigned long sz)
{
@@ -1299,6 +1313,9 @@ static int read_index_extension(struct index_state *istate,
case CACHE_EXT_RESOLVE_UNDO:
istate->resolve_undo = resolve_undo_read(data, sz);
break;
+ case CACHE_EXT_WATCH:
+ read_watch_extension(istate, data, sz);
+ break;
default:
if (*ext < 'A' || 'Z' < *ext)
return error("index uses %.4s extension, which we do not understand",
@@ -1777,7 +1794,7 @@ int write_index(struct index_state *istate, int newfd)
{
git_SHA_CTX c;
struct cache_header hdr;
- int i, err, removed, extended, hdr_version;
+ int i, err, removed, extended, hdr_version, has_watches = 0;
struct cache_entry **cache = istate->cache;
int entries = istate->cache_nr;
struct stat st;
@@ -1786,6 +1803,8 @@ int write_index(struct index_state *istate, int newfd)
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
removed++;
+ else if (cache[i]->ce_flags & CE_WATCHED)
+ has_watches++;
/* reduce extended entries if possible */
cache[i]->ce_flags &= ~CE_EXTENDED;
@@ -1857,6 +1876,26 @@ int write_index(struct index_state *istate, int newfd)
if (err)
return -1;
}
+ if (has_watches) {
+ int id, sz = (entries - removed + 7) / 8;
+ uint8_t *data = xmalloc(sz);
+ memset(data, 0, sz);
+ for (i = 0, id = 0; i < entries && has_watches; i++) {
+ struct cache_entry *ce = cache[i];
+ if (ce->ce_flags & CE_REMOVE)
+ continue;
+ if (ce->ce_flags & CE_WATCHED) {
+ data[id / 8] |= 1 << (id % 8);
+ has_watches--;
+ }
+ id++;
+ }
+ err = write_index_ext_header(&c, newfd, CACHE_EXT_WATCH, sz) < 0
+ || ce_write(&c, newfd, data, sz) < 0;
+ free(data);
+ if (err)
+ return -1;
+ }
if (ce_flush(&c, newfd) || fstat(newfd, &st))
return -1;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 11/26] Clear CE_WATCHED when set CE_VALID alone
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (9 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 10/26] read-cache: new flag CE_WATCHED to mark what file is watched Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:28 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 12/26] read-cache: basic hand shaking to the file watcher Nguyễn Thái Ngọc Duy
` (15 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
If CE_WATCHED is set, CE_VALID is controlled by CE_WATCHED and will be
cleared bfore writing to disk. Users of --assume-unchanged therefore
need to clear CE_WATCHED to avoid this side effect.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/update-index.c | 12 ++++++++----
read-cache.c | 2 +-
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index e3a10d7..9283fd6 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -50,9 +50,13 @@ static int mark_ce_flags(const char *path, int flag, int mark)
int namelen = strlen(path);
int pos = cache_name_pos(path, namelen);
if (0 <= pos) {
- if (mark)
- active_cache[pos]->ce_flags |= flag;
- else
+ if (mark) {
+ struct cache_entry *ce = active_cache[pos];
+ if (flag == CE_VALID)
+ ce->ce_flags = (ce->ce_flags & ~CE_WATCHED) | CE_VALID;
+ else
+ ce->ce_flags |= flag;
+ } else
active_cache[pos]->ce_flags &= ~flag;
cache_tree_invalidate_path(active_cache_tree, path);
active_cache_changed = 1;
@@ -235,7 +239,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
if (assume_unchanged)
- ce->ce_flags |= CE_VALID;
+ ce->ce_flags = (ce->ce_flags & ~CE_WATCHED) | CE_VALID;
option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;
option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;
if (add_cache_entry(ce, option))
diff --git a/read-cache.c b/read-cache.c
index 098d3b6..8961864 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -133,7 +133,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
fill_stat_data(&ce->ce_stat_data, st);
if (assume_unchanged)
- ce->ce_flags |= CE_VALID;
+ ce->ce_flags = (ce->ce_flags & ~CE_WATCHED) | CE_VALID;
if (S_ISREG(st->st_mode))
ce_mark_uptodate(ce);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 12/26] read-cache: basic hand shaking to the file watcher
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (10 preceding siblings ...)
2014-02-03 4:28 ` [PATCH v3 11/26] Clear CE_WATCHED when set CE_VALID alone Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 13/26] read-cache: ask file watcher to watch files Nguyễn Thái Ngọc Duy
` (14 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
read_cache() connects to the file watcher, specified by
filewatcher.path config, and performs basic hand shaking. CE_WATCHED
is cleared if git and file watcher have different views on the index
state.
All send/receive calls must be complete within a limited time to avoid
a buggy file-watcher hang "git status" forever. And the whole point of
doing this is speed. If file watcher can't respond fast enough, for
whatever reason, then it should not be used.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 10 +++
Documentation/git-file-watcher.txt | 4 +-
Makefile | 1 +
cache.h | 1 +
file-watcher-lib.c (new) | 91 ++++++++++++++++++++++
file-watcher-lib.h (new) | 6 ++
file-watcher.c | 152 ++++++++++++++++++++++++++++++++++++-
read-cache.c | 6 ++
8 files changed, 269 insertions(+), 2 deletions(-)
create mode 100644 file-watcher-lib.c
create mode 100644 file-watcher-lib.h
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..6ad653a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1042,6 +1042,16 @@ difftool.<tool>.cmd::
difftool.prompt::
Prompt before each invocation of the diff tool.
+filewatcher.path::
+ The directory that contains the socket of `git file-watcher`.
+ If it's not an absolute path, it's relative to $GIT_DIR. An
+ empty path means no connection to file watcher.
+
+filewatcher.timeout::
+ This is the maximum time in milliseconds that Git waits for
+ the file watcher to respond before giving up. Default value is
+ 50. Setting to -1 makes Git wait forever.
+
fetch.recurseSubmodules::
This option can be either set to a boolean value or to 'on-demand'.
Setting it to a boolean changes the behavior of fetch and pull to
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index ec81f18..d91caf3 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -14,7 +14,9 @@ DESCRIPTION
-----------
This program watches file changes in a git working directory and let
Git now what files have been changed so that Git does not have to call
-lstat(2) to detect that itself.
+lstat(2) to detect that itself. Config key filewatcher.path needs to
+be set to `<socket directory>` so Git knows how to contact to the file
+watcher.
OPTIONS
-------
diff --git a/Makefile b/Makefile
index 8eef0d6..1c4d659 100644
--- a/Makefile
+++ b/Makefile
@@ -798,6 +798,7 @@ LIB_OBJS += entry.o
LIB_OBJS += environment.o
LIB_OBJS += exec_cmd.o
LIB_OBJS += fetch-pack.o
+LIB_OBJS += file-watcher-lib.o
LIB_OBJS += fsck.o
LIB_OBJS += gettext.o
LIB_OBJS += gpg-interface.o
diff --git a/cache.h b/cache.h
index a0af2a5..b3ea574 100644
--- a/cache.h
+++ b/cache.h
@@ -283,6 +283,7 @@ struct index_state {
struct hash_table name_hash;
struct hash_table dir_hash;
unsigned char sha1[20];
+ int watcher;
};
extern struct index_state the_index;
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
new file mode 100644
index 0000000..d0636cc
--- /dev/null
+++ b/file-watcher-lib.c
@@ -0,0 +1,91 @@
+#include "cache.h"
+#include "file-watcher-lib.h"
+#include "pkt-line.h"
+#include "unix-socket.h"
+
+static char *watcher_path;
+static int WAIT_TIME = 50; /* in ms */
+
+static int connect_watcher(const char *path)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int fd;
+
+ if (!path || !*path)
+ return -1;
+
+ strbuf_addf(&sb, "%s/socket", path);
+ fd = unix_stream_connect(sb.buf);
+ strbuf_release(&sb);
+ return fd;
+}
+
+static void reset_watches(struct index_state *istate, int disconnect)
+{
+ int i;
+ for (i = 0; i < istate->cache_nr; i++)
+ if (istate->cache[i]->ce_flags & CE_WATCHED) {
+ istate->cache[i]->ce_flags &= ~(CE_WATCHED | CE_VALID);
+ istate->cache_changed = 1;
+ }
+ if (disconnect && istate->watcher > 0) {
+ close(istate->watcher);
+ istate->watcher = -1;
+ }
+}
+
+static int watcher_config(const char *var, const char *value, void *data)
+{
+ if (!strcmp(var, "filewatcher.path")) {
+ if (is_absolute_path(value))
+ watcher_path = xstrdup(value);
+ else if (*value == '~')
+ watcher_path = expand_user_path(value);
+ else
+ watcher_path = git_pathdup("%s", value);
+ return 0;
+ }
+ if (!strcmp(var, "filewatcher.timeout")) {
+ WAIT_TIME = git_config_int(var, value);
+ return 0;
+ }
+ return 0;
+}
+
+void open_watcher(struct index_state *istate)
+{
+ static int read_config = 0;
+ char *msg;
+
+ if (!get_git_work_tree()) {
+ reset_watches(istate, 1);
+ return;
+ }
+
+ if (!read_config) {
+ /*
+ * can't hook into git_default_config because
+ * read_cache() may be called even before git_config()
+ * call.
+ */
+ git_config(watcher_config, NULL);
+ read_config = 1;
+ }
+
+ istate->watcher = connect_watcher(watcher_path);
+ if (packet_write_timeout(istate->watcher, WAIT_TIME, "hello") <= 0 ||
+ (msg = packet_read_line_timeout(istate->watcher, WAIT_TIME, NULL)) == NULL ||
+ strcmp(msg, "hello")) {
+ reset_watches(istate, 1);
+ return;
+ }
+
+ if (packet_write_timeout(istate->watcher, WAIT_TIME, "index %s %s",
+ sha1_to_hex(istate->sha1),
+ get_git_work_tree()) <= 0 ||
+ (msg = packet_read_line_timeout(istate->watcher, WAIT_TIME, NULL)) == NULL ||
+ strcmp(msg, "ok")) {
+ reset_watches(istate, 0);
+ return;
+ }
+}
diff --git a/file-watcher-lib.h b/file-watcher-lib.h
new file mode 100644
index 0000000..eb6edf5
--- /dev/null
+++ b/file-watcher-lib.h
@@ -0,0 +1,6 @@
+#ifndef __FILE_WATCHER_LIB__
+#define __FILE_WATCHER_LIB__
+
+void open_watcher(struct index_state *istate);
+
+#endif
diff --git a/file-watcher.c b/file-watcher.c
index 1e1ccad..6df3a48 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -3,20 +3,78 @@
#include "parse-options.h"
#include "exec_cmd.h"
#include "unix-socket.h"
+#include "pkt-line.h"
static const char *const file_watcher_usage[] = {
N_("git file-watcher [options] <socket directory>"),
NULL
};
+struct repository {
+ char *work_tree;
+ char index_signature[41];
+ /*
+ * At least with inotify we don't keep track down to "/". So
+ * if worktree is /abc/def and someone moves /abc to /ghi, and
+ * /jlk to /abc (and /jlk/def exists before the move), we
+ * cant' detect that /abc/def is totally new. Checking inode
+ * is probably enough for this case.
+ */
+ ino_t inode;
+};
+
+const char *invalid_signature = "0000000000000000000000000000000000000000";
+
+static struct repository **repos;
+static int nr_repos;
+
struct connection {
- int sock;
+ int sock, polite;
+ struct repository *repo;
};
static struct connection **conns;
static struct pollfd *pfd;
static int conns_alloc, pfd_nr, pfd_alloc;
+static struct repository *get_repo(const char *work_tree)
+{
+ int first, last;
+ struct repository *repo;
+
+ first = 0;
+ last = nr_repos;
+ while (last > first) {
+ int next = (last + first) >> 1;
+ int cmp = strcmp(work_tree, repos[next]->work_tree);
+ if (!cmp)
+ return repos[next];
+ if (cmp < 0) {
+ last = next;
+ continue;
+ }
+ first = next+1;
+ }
+
+ nr_repos++;
+ repos = xrealloc(repos, sizeof(*repos) * nr_repos);
+ if (nr_repos > first + 1)
+ memmove(repos + first + 1, repos + first,
+ (nr_repos - first - 1) * sizeof(*repos));
+ repo = xmalloc(sizeof(*repo));
+ memset(repo, 0, sizeof(*repo));
+ repo->work_tree = xstrdup(work_tree);
+ memset(repo->index_signature, '0', 40);
+ repos[first] = repo;
+ return repo;
+}
+
+static void reset_repo(struct repository *repo, ino_t inode)
+{
+ memcpy(repo->index_signature, invalid_signature, 40);
+ repo->inode = inode;
+}
+
static int shutdown_connection(int id)
{
struct connection *conn = conns[id];
@@ -31,6 +89,98 @@ static int shutdown_connection(int id)
static int handle_command(int conn_id)
{
+ int fd = conns[conn_id]->sock;
+ int len;
+ const char *arg;
+ char *msg;
+
+ /*
+ * ">" denotes an incoming packet, "<" outgoing. The lack of
+ * "<" means no reply expected.
+ *
+ * < "error" SP ERROR-STRING
+ *
+ * This can be sent whenever the client violates the protocol.
+ */
+
+ msg = packet_read_line(fd, &len);
+ if (!msg) {
+ packet_write(fd, "error invalid pkt-line");
+ return shutdown_connection(conn_id);
+ }
+
+ /*
+ * > "hello" [SP CAP [SP CAP..]]
+ * < "hello" [SP CAP [SP CAP..]]
+ *
+ * Advertise capabilities of both sides. File watcher may
+ * disconnect if the client does not advertise the required
+ * capabilities. Capabilities in uppercase MUST be
+ * supported. If any side does not understand any of the
+ * advertised uppercase capabilities, it must disconnect.
+ */
+ if ((arg = skip_prefix(msg, "hello"))) {
+ if (*arg) { /* no capabilities supported yet */
+ packet_write(fd, "error capabilities not supported");
+ return shutdown_connection(conn_id);
+ }
+ packet_write(fd, "hello");
+ conns[conn_id]->polite = 1;
+ }
+
+ /*
+ * > "index" SP INDEX-SIGNATURE SP WORK-TREE-PATH
+ * < "ok" | "inconsistent"
+ *
+ * INDEX-SIGNATURE consists of 40 hexadecimal letters
+ * WORK-TREE-PATH must be absolute and normalized path
+ *
+ * Watch file changes in index. The client sends the index and
+ * work tree info. File watcher validates that it holds the
+ * same info. If so it sends "ok" back indicating both sides
+ * are on the same page and CE_WATCHED bits can be ketpt.
+ *
+ * Otherwise it sends "inconsistent" and both sides must reset
+ * back to initial state. File watcher keeps its index
+ * signature all-zero until the client has updated the index
+ * ondisk and request to update index signature.
+ *
+ * "hello" must be exchanged first. After this command the
+ * connection is associated with a worktree/index. Many
+ * commands may require this to proceed.
+ */
+ else if (starts_with(msg, "index ")) {
+ struct repository *repo;
+ struct stat st;
+ if (!conns[conn_id]->polite) {
+ packet_write(fd, "error why did you not greet me? go away");
+ return shutdown_connection(conn_id);
+ }
+ if (len < 47 || msg[46] != ' ' || !is_absolute_path(msg + 47)) {
+ packet_write(fd, "error invalid index line %s", msg);
+ return shutdown_connection(conn_id);
+ }
+
+ if (lstat(msg + 47, &st) || !S_ISDIR(st.st_mode)) {
+ packet_write(fd, "error work tree does not exist: %s",
+ strerror(errno));
+ return shutdown_connection(conn_id);
+ }
+ repo = get_repo(msg + 47);
+ conns[conn_id]->repo = repo;
+ if (memcmp(msg + 6, repo->index_signature, 40) ||
+ !memcmp(msg + 6, invalid_signature, 40) ||
+ repo->inode != st.st_ino) {
+ packet_write(fd, "inconsistent");
+ reset_repo(repo, st.st_ino);
+ return 0;
+ }
+ packet_write(fd, "ok");
+ }
+ else {
+ packet_write(fd, "error unrecognized command %s", msg);
+ return shutdown_connection(conn_id);
+ }
return 0;
}
diff --git a/read-cache.c b/read-cache.c
index 8961864..a7e5735 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "strbuf.h"
#include "varint.h"
+#include "file-watcher-lib.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
@@ -1528,6 +1529,7 @@ int read_index_from(struct index_state *istate, const char *path)
src_offset += extsize;
}
munmap(mmap, mmap_size);
+ open_watcher(istate);
return istate->cache_nr;
unmap:
@@ -1553,6 +1555,10 @@ int discard_index(struct index_state *istate)
istate->timestamp.nsec = 0;
free_name_hash(istate);
cache_tree_free(&(istate->cache_tree));
+ if (istate->watcher > 0) {
+ close(istate->watcher);
+ istate->watcher = -1;
+ }
istate->initialized = 0;
free(istate->cache);
istate->cache = NULL;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 13/26] read-cache: ask file watcher to watch files
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (11 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 12/26] read-cache: basic hand shaking to the file watcher Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 14/26] read-cache: put some limits on file watching Nguyễn Thái Ngọc Duy
` (13 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
We want to watch files that are never changed because lstat() on those
files is a wasted effort. So we sort unwatched files by date and start
adding them to the file watcher until it barfs (e.g. hits inotify
limit).
Note that we still do lstat() on these new watched files because they
could have changed before the file watcher could watch them. Watched
files may only skip lstat() at the next run.
Also, at this early in the index loading process, we don't know what
files are dirty and thus can skip watching (we do clear CE_WATCHED on
entries that are not verified clean before writing index). So the
watches are set, but git ignores its results. Maybe in future we could
store the list of dirty files in WATC extension and use it as a hint
to skip watching.
In the future, file watcher should figure out what paths are
watchable, what not (e.g. network filesystems) and reject them. For
now it's the user resposibility to set (or unset) filewatcher.path
properly.
The previous attempt sends paths in batch, 64k per pkt-line, then wait
for response. It's designed to stop short in case file watcher is out
of resources. But that's a rare case, and send/wait cycles increase
latency.
Instead we now send everything in one packet, and not in pkt-line to
avoid the 64k limit. Then we wait for the response. On webkit.git,
normal "status -uno" takes 0m1.138s. The sending 14M (of 182k paths)
takes 52ms extra. Previous approach takes 213ms extra. Of course in
the end, extra time is longer because file watcher is basically no-op
so far.
There is not much room for improvement. If we compress paths to reduce
payload, zlib time costs about 300ms (so how small the end result is
no longer matters). Even simple prefix compressing (index v4 style)
would cost 76ms on processing time alone (reducing payload to 3M).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher-lib.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
file-watcher-lib.h | 1 +
file-watcher.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
read-cache.c | 18 +++++++++--
4 files changed, 191 insertions(+), 2 deletions(-)
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
index d0636cc..791faae 100644
--- a/file-watcher-lib.c
+++ b/file-watcher-lib.c
@@ -89,3 +89,86 @@ void open_watcher(struct index_state *istate)
return;
}
}
+
+static int sort_by_date(const void *a_, const void *b_)
+{
+ const struct cache_entry *a = *(const struct cache_entry **)a_;
+ const struct cache_entry *b = *(const struct cache_entry **)b_;
+ uint32_t seca = a->ce_stat_data.sd_mtime.sec;
+ uint32_t secb = b->ce_stat_data.sd_mtime.sec;
+ return seca - secb;
+}
+
+static inline int ce_watchable(struct cache_entry *ce)
+{
+ return
+ !(ce->ce_flags & CE_WATCHED) &&
+ !(ce->ce_flags & CE_VALID) &&
+ /*
+ * S_IFGITLINK should not be watched
+ * obviously. S_IFLNK could be problematic because
+ * inotify may follow symlinks without IN_DONT_FOLLOW
+ */
+ S_ISREG(ce->ce_mode);
+}
+
+static void send_watches(struct index_state *istate,
+ struct cache_entry **sorted, int nr)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int i, len = 0;
+
+ for (i = 0; i < nr; i++)
+ len += ce_namelen(sorted[i]) + 1;
+
+ if (packet_write_timeout(istate->watcher, WAIT_TIME, "watch %d", len) <= 0)
+ return;
+
+ strbuf_grow(&sb, len);
+ for (i = 0; i < nr; i++)
+ strbuf_add(&sb, sorted[i]->name, ce_namelen(sorted[i]) + 1);
+
+ if (write_in_full_timeout(istate->watcher, sb.buf,
+ sb.len, WAIT_TIME) != sb.len) {
+ strbuf_release(&sb);
+ return;
+ }
+ strbuf_release(&sb);
+
+ for (;;) {
+ char *line, *end;
+ unsigned long n;
+
+ if (!(line = packet_read_line_timeout(istate->watcher,
+ WAIT_TIME, &len)))
+ return;
+ if (starts_with(line, "watching "))
+ continue;
+ if (!starts_with(line, "watched "))
+ return;
+ n = strtoul(line + 8, &end, 10);
+ for (i = 0; i < n; i++)
+ sorted[i]->ce_flags |= CE_WATCHED;
+ istate->cache_changed = 1;
+ break;
+ }
+}
+
+void watch_entries(struct index_state *istate)
+{
+ int i, nr;
+ struct cache_entry **sorted;
+
+ if (istate->watcher <= 0)
+ return;
+ for (i = nr = 0; i < istate->cache_nr; i++)
+ if (ce_watchable(istate->cache[i]))
+ nr++;
+ sorted = xmalloc(sizeof(*sorted) * nr);
+ for (i = nr = 0; i < istate->cache_nr; i++)
+ if (ce_watchable(istate->cache[i]))
+ sorted[nr++] = istate->cache[i];
+ qsort(sorted, nr, sizeof(*sorted), sort_by_date);
+ send_watches(istate, sorted, nr);
+ free(sorted);
+}
diff --git a/file-watcher-lib.h b/file-watcher-lib.h
index eb6edf5..1641024 100644
--- a/file-watcher-lib.h
+++ b/file-watcher-lib.h
@@ -2,5 +2,6 @@
#define __FILE_WATCHER_LIB__
void open_watcher(struct index_state *istate);
+void watch_entries(struct index_state *istate);
#endif
diff --git a/file-watcher.c b/file-watcher.c
index 6df3a48..c257414 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -37,6 +37,70 @@ static struct connection **conns;
static struct pollfd *pfd;
static int conns_alloc, pfd_nr, pfd_alloc;
+static int watch_path(struct repository *repo, char *path)
+{
+ return -1;
+}
+
+static inline uint64_t stamp(void)
+{
+ struct timeval tv;
+ gettimeofday(&tv, NULL);
+ return (uint64_t)tv.tv_sec * 1000000 + tv.tv_usec;
+}
+
+static int shutdown_connection(int id);
+static void watch_paths(int conn_id, char *buf, int maxlen)
+{
+ int ret, len, n;
+ uint64_t start, now;
+ char *end;
+
+ n = strtol(buf, &end, 10);
+ if (end != buf + maxlen) {
+ packet_write(conns[conn_id]->sock,
+ "error invalid watch number %s", buf);
+ shutdown_connection(conn_id);
+ return;
+ }
+
+ buf = xmallocz(n);
+ end = buf + n;
+ /*
+ * Careful if this takes longer than 50ms, it'll upset other
+ * connections
+ */
+ if (read_in_full(conns[conn_id]->sock, buf, n) != n) {
+ shutdown_connection(conn_id);
+ return;
+ }
+ if (chdir(conns[conn_id]->repo->work_tree)) {
+ packet_write(conns[conn_id]->sock,
+ "error chdir %s", strerror(errno));
+ return;
+ }
+ start = stamp();
+ for (n = ret = 0; buf < end; buf += len + 1) {
+ len = strlen(buf);
+ if (watch_path(conns[conn_id]->repo, buf))
+ break;
+ n++;
+ if (n & 0x3ff)
+ continue;
+ now = stamp();
+ /*
+ * If we process for too long, the client may timeout
+ * and give up. Let the client know we're not dead
+ * yet, every 30ms.
+ */
+ if (start + 30000 < now) {
+ packet_write(conns[conn_id]->sock, "watching %d", n);
+ start = now;
+ }
+ }
+ packet_write(conns[conn_id]->sock, "watched %u", n);
+}
+
static struct repository *get_repo(const char *work_tree)
{
int first, last;
@@ -177,6 +241,33 @@ static int handle_command(int conn_id)
}
packet_write(fd, "ok");
}
+
+ /*
+ * > "watch" SP LENGTH
+ * > PATH-LIST
+ * < "watching" SP NUM
+ * < "watched" SP NUM
+ *
+ * PATH-LIST is the list of paths, each terminated with
+ * NUL. PATH-LIST is not wrapped in pkt-line format. LENGTH is
+ * the size of PATH-LIST in bytes.
+ *
+ * The client asks file watcher to watcher a number of
+ * paths. File watcher starts to process from path by path in
+ * received order. File watcher returns the actual number of
+ * watched paths with "watched" command.
+ *
+ * File watcher may send any number of "watching" messages
+ * before "watched". This packet is to keep the connection
+ * alive and has no other values.
+ */
+ else if (starts_with(msg, "watch ")) {
+ if (!conns[conn_id]->repo) {
+ packet_write(fd, "error have not received index command");
+ return shutdown_connection(conn_id);
+ }
+ watch_paths(conn_id, msg + 6, len - 6);
+ }
else {
packet_write(fd, "error unrecognized command %s", msg);
return shutdown_connection(conn_id);
diff --git a/read-cache.c b/read-cache.c
index a7e5735..cb2188f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1530,6 +1530,7 @@ int read_index_from(struct index_state *istate, const char *path)
}
munmap(mmap, mmap_size);
open_watcher(istate);
+ watch_entries(istate);
return istate->cache_nr;
unmap:
@@ -1809,8 +1810,21 @@ int write_index(struct index_state *istate, int newfd)
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
removed++;
- else if (cache[i]->ce_flags & CE_WATCHED)
- has_watches++;
+ else if (cache[i]->ce_flags & CE_WATCHED) {
+ /*
+ * We may set CE_WATCHED (but not CE_VALID)
+ * early when refresh has not been done
+ * yet. At that time we had no idea if the
+ * entry may have been updated. If it has
+ * been, remove CE_WATCHED so CE_VALID won't
+ * incorrectly be set next time if the file
+ * watcher reports no changes.
+ */
+ if (!ce_uptodate(cache[i]))
+ cache[i]->ce_flags &= ~CE_WATCHED;
+ else
+ has_watches++;
+ }
/* reduce extended entries if possible */
cache[i]->ce_flags &= ~CE_EXTENDED;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 14/26] read-cache: put some limits on file watching
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (12 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 13/26] read-cache: ask file watcher to watch files Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 15/26] read-cache: get changed file list from file watcher Nguyễn Thái Ngọc Duy
` (12 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
watch_entries() is a lot of computation and could trigger a lot more
lookups in file-watcher. Normally after the first set of watches are
in place, we do not need to update often. Moreover if the number of
entries is small, the overhead of file watcher may actually slow git
down.
This patch only allows to update watches if the number of watchable
files is over a limit (and there are new files added if this is not
the first time). Measurements on Core i5-2520M and Linux 3.7.6, about
920 lstat() take 1ms. Somewhere between 2^16 and 2^17 lstat calls that
it starts to take longer than 100ms. 2^16 is chosen at the minimum
limit to start using file watcher.
Of course this is only sensible default for single-repo use
case. Lower it when you need to work with many small repos.
Recently updated files are not considered watchable because they are
likely to be updated again soon, not worth the ping-pong game with
file watcher. The default limit 10min is just a random value. Recent
limit is ignored if there are no watched files (e.g. a fresh clone, or
after a bad hand shake with file watcher).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 9 +++++++
Documentation/technical/index-format.txt | 3 +++
cache.h | 1 +
file-watcher-lib.c | 42 ++++++++++++++++++++++++++------
read-cache.c | 11 ++++++---
5 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6ad653a..451c100 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1052,6 +1052,15 @@ filewatcher.timeout::
the file watcher to respond before giving up. Default value is
50. Setting to -1 makes Git wait forever.
+filewatcher.minfiles::
+ Start watching files if the number of watchable files are
+ above this limit. Default value is 65536.
+
+filewatcher.recentlimit::
+ Files that are last updated within filewatcher.recentlimit
+ seconds from now are not considered watchable. Default value
+ is 600 (5 minutes).
+
fetch.recurseSubmodules::
This option can be either set to a boolean value or to 'on-demand'.
Setting it to a boolean changes the behavior of fetch and pull to
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index 24fd0ae..7081e55 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -204,3 +204,6 @@ Git index format
- A bit map of all entries in the index, n-th bit of m-th byte
corresponds to CE_WATCHED of the <m * 8+ n>-th index entry.
+
+ - 1-byte, non-zero indicates the index should be scanned for new
+ watched entries.
diff --git a/cache.h b/cache.h
index b3ea574..10ff33e 100644
--- a/cache.h
+++ b/cache.h
@@ -279,6 +279,7 @@ struct index_state {
struct cache_tree *cache_tree;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+ update_watches : 1,
initialized : 1;
struct hash_table name_hash;
struct hash_table dir_hash;
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
index 791faae..d4949a5 100644
--- a/file-watcher-lib.c
+++ b/file-watcher-lib.c
@@ -5,6 +5,8 @@
static char *watcher_path;
static int WAIT_TIME = 50; /* in ms */
+static int watch_lowerlimit = 65536;
+static int recent_limit = 600;
static int connect_watcher(const char *path)
{
@@ -22,12 +24,17 @@ static int connect_watcher(const char *path)
static void reset_watches(struct index_state *istate, int disconnect)
{
- int i;
+ int i, changed = 0;
for (i = 0; i < istate->cache_nr; i++)
if (istate->cache[i]->ce_flags & CE_WATCHED) {
istate->cache[i]->ce_flags &= ~(CE_WATCHED | CE_VALID);
- istate->cache_changed = 1;
+ changed = 1;
}
+ recent_limit = 0;
+ if (changed) {
+ istate->update_watches = 1;
+ istate->cache_changed = 1;
+ }
if (disconnect && istate->watcher > 0) {
close(istate->watcher);
istate->watcher = -1;
@@ -49,6 +56,14 @@ static int watcher_config(const char *var, const char *value, void *data)
WAIT_TIME = git_config_int(var, value);
return 0;
}
+ if (!strcmp(var, "filewatcher.minfiles")) {
+ watch_lowerlimit = git_config_int(var, value);
+ return 0;
+ }
+ if (!strcmp(var, "filewatcher.recentlimit")) {
+ recent_limit = git_config_int(var, value);
+ return 0;
+ }
return 0;
}
@@ -63,12 +78,18 @@ void open_watcher(struct index_state *istate)
}
if (!read_config) {
+ int i;
/*
* can't hook into git_default_config because
* read_cache() may be called even before git_config()
* call.
*/
git_config(watcher_config, NULL);
+ for (i = 0; i < istate->cache_nr; i++)
+ if (istate->cache[i]->ce_flags & CE_WATCHED)
+ break;
+ if (i == istate->cache_nr)
+ recent_limit = 0;
read_config = 1;
}
@@ -86,6 +107,7 @@ void open_watcher(struct index_state *istate)
(msg = packet_read_line_timeout(istate->watcher, WAIT_TIME, NULL)) == NULL ||
strcmp(msg, "ok")) {
reset_watches(istate, 0);
+ istate->update_watches = 1;
return;
}
}
@@ -99,7 +121,7 @@ static int sort_by_date(const void *a_, const void *b_)
return seca - secb;
}
-static inline int ce_watchable(struct cache_entry *ce)
+static inline int ce_watchable(struct cache_entry *ce, time_t now)
{
return
!(ce->ce_flags & CE_WATCHED) &&
@@ -109,7 +131,8 @@ static inline int ce_watchable(struct cache_entry *ce)
* obviously. S_IFLNK could be problematic because
* inotify may follow symlinks without IN_DONT_FOLLOW
*/
- S_ISREG(ce->ce_mode);
+ S_ISREG(ce->ce_mode) &&
+ (ce->ce_stat_data.sd_mtime.sec + recent_limit <= now);
}
static void send_watches(struct index_state *istate,
@@ -158,15 +181,20 @@ void watch_entries(struct index_state *istate)
{
int i, nr;
struct cache_entry **sorted;
+ time_t now = time(NULL);
- if (istate->watcher <= 0)
+ if (istate->watcher <= 0 || !istate->update_watches)
return;
+ istate->update_watches = 0;
+ istate->cache_changed = 1;
for (i = nr = 0; i < istate->cache_nr; i++)
- if (ce_watchable(istate->cache[i]))
+ if (ce_watchable(istate->cache[i], now))
nr++;
+ if (nr < watch_lowerlimit)
+ return;
sorted = xmalloc(sizeof(*sorted) * nr);
for (i = nr = 0; i < istate->cache_nr; i++)
- if (ce_watchable(istate->cache[i]))
+ if (ce_watchable(istate->cache[i], now))
sorted[nr++] = istate->cache[i];
qsort(sorted, nr, sizeof(*sorted), sort_by_date);
send_watches(istate, sorted, nr);
diff --git a/read-cache.c b/read-cache.c
index cb2188f..dc49858 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1009,6 +1009,7 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
(istate->cache_nr - pos - 1) * sizeof(ce));
set_index_entry(istate, pos, ce);
istate->cache_changed = 1;
+ istate->update_watches = 1;
return 0;
}
@@ -1295,13 +1296,14 @@ static void read_watch_extension(struct index_state *istate, uint8_t *data,
unsigned long sz)
{
int i;
- if ((istate->cache_nr + 7) / 8 != sz) {
+ if ((istate->cache_nr + 7) / 8 + 1 != sz) {
error("invalid 'WATC' extension");
return;
}
for (i = 0; i < istate->cache_nr; i++)
if (data[i / 8] & (1 << (i % 8)))
istate->cache[i]->ce_flags |= CE_WATCHED;
+ istate->update_watches = data[sz - 1];
}
static int read_index_extension(struct index_state *istate,
@@ -1488,6 +1490,7 @@ int read_index_from(struct index_state *istate, const char *path)
istate->cache_alloc = alloc_nr(istate->cache_nr);
istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
istate->initialized = 1;
+ istate->update_watches = 1;
if (istate->version == 4)
previous_name = &previous_name_buf;
@@ -1896,8 +1899,9 @@ int write_index(struct index_state *istate, int newfd)
if (err)
return -1;
}
- if (has_watches) {
- int id, sz = (entries - removed + 7) / 8;
+ if (has_watches ||
+ (istate->watcher != -1 && !istate->update_watches)) {
+ int id, sz = (entries - removed + 7) / 8 + 1;
uint8_t *data = xmalloc(sz);
memset(data, 0, sz);
for (i = 0, id = 0; i < entries && has_watches; i++) {
@@ -1910,6 +1914,7 @@ int write_index(struct index_state *istate, int newfd)
}
id++;
}
+ data[sz - 1] = istate->update_watches;
err = write_index_ext_header(&c, newfd, CACHE_EXT_WATCH, sz) < 0
|| ce_write(&c, newfd, data, sz) < 0;
free(data);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 15/26] read-cache: get changed file list from file watcher
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (13 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 14/26] read-cache: put some limits on file watching Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 16/26] git-compat-util.h: add inotify stubs on non-Linux platforms Nguyễn Thái Ngọc Duy
` (11 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
When some paths are watched, they are added to the "watched" list in
file watcher. When a path in this list is updated, the path is moved
to "changed" list and no longer watched.
With this patch we have a complete path exchanging picture between git
and file-watcher:
1) Hand shake
2) Get the list of changed paths, clear CE_WATCHED on these paths. Set
CE_VALID on the remaining CE_WATCHED paths
3) (Optionally) Ask to watch more paths. Set CE_WATCHED on
them. CE_VALID is not set so these are still lstat'd
4) Refresh as usual. lstat is skipped on CE_VALID paths. If one of
those paths at step 3 are found modified, CE_WATCHED is removed.
5) Write index to disk. Notify file-watcher about new index
signature. Ask file watcher to remove the "changed paths".
A few points:
- Changed list remains until step 5. If git crashes or do not write
index down, next time it starts, it'll fed the same changed list.
- If git writes index down without telling file-watcher about it,
next time it starts, hand shake should fail and git should clear
all CE_WATCHED.
- There's a buffer between starting watch at #3 and saving watch at
#5. We do verify paths are clean at #4. But that time all watches
should have been active for a while. No chance for race conditions.
- #5 is sort of atomic. If git crashes half way through step 5, file
watcher should not update its index signature. Which means next
time git starts, hand shake fails (because new index's written) so
we'll start over.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
file-watcher-lib.c | 99 ++++++++++++++++++++++++++++++++++++++
file-watcher-lib.h | 1 +
file-watcher.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++++++++
read-cache.c | 21 +++++++-
5 files changed, 258 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 10ff33e..9f7d952 100644
--- a/cache.h
+++ b/cache.h
@@ -285,6 +285,7 @@ struct index_state {
struct hash_table dir_hash;
unsigned char sha1[20];
int watcher;
+ struct string_list *updated_entries;
};
extern struct index_state the_index;
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
index d4949a5..b6b0848 100644
--- a/file-watcher-lib.c
+++ b/file-watcher-lib.c
@@ -2,6 +2,7 @@
#include "file-watcher-lib.h"
#include "pkt-line.h"
#include "unix-socket.h"
+#include "string-list.h"
static char *watcher_path;
static int WAIT_TIME = 50; /* in ms */
@@ -25,6 +26,11 @@ static int connect_watcher(const char *path)
static void reset_watches(struct index_state *istate, int disconnect)
{
int i, changed = 0;
+ if (istate->updated_entries) {
+ string_list_clear(istate->updated_entries, 0);
+ free(istate->updated_entries);
+ istate->updated_entries = NULL;
+ }
for (i = 0; i < istate->cache_nr; i++)
if (istate->cache[i]->ce_flags & CE_WATCHED) {
istate->cache[i]->ce_flags &= ~(CE_WATCHED | CE_VALID);
@@ -41,6 +47,58 @@ static void reset_watches(struct index_state *istate, int disconnect)
}
}
+static void mark_ce_valid(struct index_state *istate)
+{
+ struct strbuf sb = STRBUF_INIT;
+ char *line, *end;
+ int i, len;
+ unsigned long n;
+ if (packet_write_timeout(istate->watcher, WAIT_TIME, "get-changed") <= 0 ||
+ !(line = packet_read_line_timeout(istate->watcher, WAIT_TIME, &len)) ||
+ !starts_with(line, "changed ")) {
+ reset_watches(istate, 1);
+ return;
+ }
+ n = strtoul(line + 8, &end, 10);
+ if (end != line + len) {
+ reset_watches(istate, 1);
+ return;
+ }
+ if (!n)
+ goto done;
+ strbuf_grow(&sb, n);
+ if (read_in_full_timeout(istate->watcher, sb.buf, n, WAIT_TIME) != n) {
+ strbuf_release(&sb);
+ reset_watches(istate, 1);
+ return;
+ }
+ line = sb.buf;
+ end = line + n;
+ for (; line < end; line += len + 1) {
+ len = strlen(line);
+ i = index_name_pos(istate, line, len);
+ if (i < 0)
+ continue;
+ if (istate->cache[i]->ce_flags & CE_WATCHED) {
+ istate->cache[i]->ce_flags &= ~CE_WATCHED;
+ istate->cache_changed = 1;
+ }
+ if (!istate->updated_entries) {
+ struct string_list *sl;
+ sl = xmalloc(sizeof(*sl));
+ memset(sl, 0, sizeof(*sl));
+ sl->strdup_strings = 1;
+ istate->updated_entries = sl;
+ }
+ string_list_append(istate->updated_entries, line);
+ }
+ strbuf_release(&sb);
+done:
+ for (i = 0; i < istate->cache_nr; i++)
+ if (istate->cache[i]->ce_flags & CE_WATCHED)
+ istate->cache[i]->ce_flags |= CE_VALID;
+}
+
static int watcher_config(const char *var, const char *value, void *data)
{
if (!strcmp(var, "filewatcher.path")) {
@@ -110,6 +168,8 @@ void open_watcher(struct index_state *istate)
istate->update_watches = 1;
return;
}
+
+ mark_ce_valid(istate);
}
static int sort_by_date(const void *a_, const void *b_)
@@ -200,3 +260,42 @@ void watch_entries(struct index_state *istate)
send_watches(istate, sorted, nr);
free(sorted);
}
+
+void close_watcher(struct index_state *istate, const unsigned char *sha1)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int len, i, nr;
+ if (istate->watcher <= 0)
+ return;
+ if (packet_write_timeout(istate->watcher, WAIT_TIME,
+ "new-index %s", sha1_to_hex(sha1)) <= 0)
+ goto done;
+ nr = istate->updated_entries ? istate->updated_entries->nr : 0;
+ if (!nr) {
+ packet_write_timeout(istate->watcher, WAIT_TIME, "unchange 0");
+ goto done;
+ }
+ for (i = len = 0; i < nr; i++) {
+ const char *s = istate->updated_entries->items[i].string;
+ len += strlen(s) + 1;
+ }
+ if (packet_write_timeout(istate->watcher, WAIT_TIME,
+ "unchange %d", len) <= 0)
+ goto done;
+ strbuf_grow(&sb, len);
+ for (i = 0; i < nr; i++) {
+ const char *s = istate->updated_entries->items[i].string;
+ int len = strlen(s);
+ strbuf_add(&sb, s, len + 1);
+ }
+ /*
+ * it does not matter if it fails anymore, we're closing
+ * down. If it only gets through partially, file watcher
+ * should ignore it.
+ */
+ write_in_full_timeout(istate->watcher, sb.buf, sb.len, WAIT_TIME);
+ strbuf_release(&sb);
+done:
+ close(istate->watcher);
+ istate->watcher = -1;
+}
diff --git a/file-watcher-lib.h b/file-watcher-lib.h
index 1641024..df68a73 100644
--- a/file-watcher-lib.h
+++ b/file-watcher-lib.h
@@ -3,5 +3,6 @@
void open_watcher(struct index_state *istate);
void watch_entries(struct index_state *istate);
+void close_watcher(struct index_state *istate, const unsigned char *sha1);
#endif
diff --git a/file-watcher.c b/file-watcher.c
index c257414..aa2daf6 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -3,6 +3,7 @@
#include "parse-options.h"
#include "exec_cmd.h"
#include "unix-socket.h"
+#include "string-list.h"
#include "pkt-line.h"
static const char *const file_watcher_usage[] = {
@@ -21,6 +22,9 @@ struct repository {
* is probably enough for this case.
*/
ino_t inode;
+ struct string_list updated;
+ int updated_sorted;
+ int updating;
};
const char *invalid_signature = "0000000000000000000000000000000000000000";
@@ -31,6 +35,8 @@ static int nr_repos;
struct connection {
int sock, polite;
struct repository *repo;
+
+ char new_index[41];
};
static struct connection **conns;
@@ -42,6 +48,24 @@ static int watch_path(struct repository *repo, char *path)
return -1;
}
+static void get_changed_list(int conn_id)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int i, size, fd = conns[conn_id]->sock;
+ struct repository *repo = conns[conn_id]->repo;
+
+ for (i = size = 0; i < repo->updated.nr; i++)
+ size += strlen(repo->updated.items[i].string) + 1;
+ packet_write(fd, "changed %d", size);
+ if (!size)
+ return;
+ strbuf_grow(&sb, size);
+ for (i = 0; i < repo->updated.nr; i++)
+ strbuf_add(&sb, repo->updated.items[i].string,
+ strlen(repo->updated.items[i].string) + 1);
+ write_in_full(fd, sb.buf, sb.len);
+}
+
static inline uint64_t stamp(void)
{
struct timeval tv;
@@ -101,6 +125,43 @@ static void watch_paths(int conn_id, char *buf, int maxlen)
packet_write(conns[conn_id]->sock, "watched %u", n);
}
+static void unchange(int conn_id, unsigned long size)
+{
+ struct connection *conn = conns[conn_id];
+ struct repository *repo = conn->repo;
+ if (size) {
+ struct strbuf sb = STRBUF_INIT;
+ char *p;
+ int len;
+ strbuf_grow(&sb, size);
+ if (read_in_full(conn->sock, sb.buf, size) <= 0)
+ return;
+ if (!repo->updated_sorted) {
+ sort_string_list(&repo->updated);
+ repo->updated_sorted = 1;
+ }
+ for (p = sb.buf; p - sb.buf < size; p += len + 1) {
+ struct string_list_item *item;
+ len = strlen(p);
+ item = string_list_lookup(&repo->updated, p);
+ if (!item)
+ continue;
+ unsorted_string_list_delete_item(&repo->updated,
+ item - repo->updated.items, 0);
+ }
+ strbuf_release(&sb);
+ }
+ memcpy(repo->index_signature, conn->new_index, 40);
+ /*
+ * If other connections on this repo are in some sort of
+ * session that depend on the previous repository state, we
+ * may need to disconnect them to be safe.
+ */
+
+ /* pfd[0] is the listening socket, can't be a connection */
+ repo->updating = 0;
+}
+
static struct repository *get_repo(const char *work_tree)
{
int first, last;
@@ -129,12 +190,14 @@ static struct repository *get_repo(const char *work_tree)
memset(repo, 0, sizeof(*repo));
repo->work_tree = xstrdup(work_tree);
memset(repo->index_signature, '0', 40);
+ repo->updated.strdup_strings = 1;
repos[first] = repo;
return repo;
}
static void reset_repo(struct repository *repo, ino_t inode)
{
+ string_list_clear(&repo->updated, 0);
memcpy(repo->index_signature, invalid_signature, 40);
repo->inode = inode;
}
@@ -147,6 +210,8 @@ static int shutdown_connection(int id)
if (!conn)
return 0;
close(conn->sock);
+ if (conn->repo && conn->repo->updating == id)
+ conn->repo->updating = 0;
free(conn);
return 0;
}
@@ -268,6 +333,77 @@ static int handle_command(int conn_id)
}
watch_paths(conn_id, msg + 6, len - 6);
}
+
+ /*
+ * > "get-changed"
+ * < changed SP LENGTH
+ * < PATH-LIST
+ *
+ * When watched path gets updated, the path is moved from
+ * "watched" list to "changed" list and is no longer watched.
+ * This command get the list of changed paths. PATH-LIST is
+ * also sent if LENGTH is non-zero.
+ */
+ else if (!strcmp(msg, "get-changed")) {
+ if (!conns[conn_id]->repo) {
+ packet_write(fd, "error have not received index command");
+ return shutdown_connection(conn_id);
+ }
+ get_changed_list(conn_id);
+ }
+
+ /*
+ * > "new-index" INDEX-SIGNATURE
+ * > "unchange" SP LENGTH
+ * > PATH-LIST
+ *
+ * "new-index" passes new index signature from the
+ * client. "unchange" sends the list of paths to be removed
+ * from "changed" list.
+ *
+ * "new-index" must be sent before "unchange". File watcher
+ * waits until the last "unchange" line, then update its index
+ * signature as well as "changed" list.
+ */
+ else if (starts_with(msg, "new-index ")) {
+ if (len != 50) {
+ packet_write(fd, "error invalid new-index line %s", msg);
+ return shutdown_connection(conn_id);
+ }
+ if (!conns[conn_id]->repo) {
+ packet_write(fd, "error have not received index command");
+ return shutdown_connection(conn_id);
+ }
+ if (conns[conn_id]->repo->updating == conn_id) {
+ packet_write(fd, "error received new-index command more than once");
+ return shutdown_connection(conn_id);
+ }
+ memcpy(conns[conn_id]->new_index, msg + 10, 40);
+ /*
+ * if updating is non-zero the other client will get
+ * disconnected at the next "unchange" command because
+ * "updating" no longer points to its connection.
+ */
+ conns[conn_id]->repo->updating = conn_id;
+ }
+ else if (skip_prefix(msg, "unchange ")) {
+ unsigned long n;
+ char *end;
+ n = strtoul(msg + 9, &end, 10);
+ if (end != msg + len) {
+ packet_write(fd, "error invalid unchange line %s", msg);
+ return shutdown_connection(conn_id);
+ }
+ if (!conns[conn_id]->repo) {
+ packet_write(fd, "error have not received index command");
+ return shutdown_connection(conn_id);
+ }
+ if (conns[conn_id]->repo->updating != conn_id) {
+ packet_write(fd, "error have not received new-index command");
+ return shutdown_connection(conn_id);
+ }
+ unchange(conn_id, n);
+ }
else {
packet_write(fd, "error unrecognized command %s", msg);
return shutdown_connection(conn_id);
@@ -436,6 +572,8 @@ int main(int argc, const char **argv)
if (!conns[i])
continue;
if (i != new_nr) { /* pfd[] is shrunk, move pfd[i] up */
+ if (conns[i]->repo && conns[i]->repo->updating == i)
+ conns[i]->repo->updating = new_nr;
conns[new_nr] = conns[i];
pfd[new_nr] = pfd[i];
}
diff --git a/read-cache.c b/read-cache.c
index dc49858..5540b06 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1567,6 +1567,11 @@ int discard_index(struct index_state *istate)
free(istate->cache);
istate->cache = NULL;
istate->cache_alloc = 0;
+ if (istate->updated_entries) {
+ string_list_clear(istate->updated_entries, 0);
+ free(istate->updated_entries);
+ istate->updated_entries = NULL;
+ }
return 0;
}
@@ -1627,7 +1632,7 @@ static int write_index_ext_header(git_SHA_CTX *context, int fd,
(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;
}
-static int ce_flush(git_SHA_CTX *context, int fd)
+static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
{
unsigned int left = write_buffer_len;
@@ -1645,6 +1650,8 @@ static int ce_flush(git_SHA_CTX *context, int fd)
/* Append the SHA1 signature at the end */
git_SHA1_Final(write_buffer + left, context);
+ if (sha1)
+ hashcpy(sha1, write_buffer + left);
left += 20;
return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;
}
@@ -1809,12 +1816,21 @@ int write_index(struct index_state *istate, int newfd)
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+ unsigned char sha1[20];
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
removed++;
else if (cache[i]->ce_flags & CE_WATCHED) {
/*
+ * CE_VALID when used with CE_WATCHED is not
+ * supposed to be persistent. Next time git
+ * runs, if this entry is still watched and
+ * nothing has changed, CE_VALID will be
+ * reinstated.
+ */
+ cache[i]->ce_flags &= ~CE_VALID;
+ /*
* We may set CE_WATCHED (but not CE_VALID)
* early when refresh has not been done
* yet. At that time we had no idea if the
@@ -1922,8 +1938,9 @@ int write_index(struct index_state *istate, int newfd)
return -1;
}
- if (ce_flush(&c, newfd) || fstat(newfd, &st))
+ if (ce_flush(&c, newfd, sha1) || fstat(newfd, &st))
return -1;
+ close_watcher(istate, sha1);
istate->timestamp.sec = (unsigned int)st.st_mtime;
istate->timestamp.nsec = ST_MTIME_NSEC(st);
return 0;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 16/26] git-compat-util.h: add inotify stubs on non-Linux platforms
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (14 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 15/26] read-cache: get changed file list from file watcher Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 17/26] file-watcher: inotify support, watching part Nguyễn Thái Ngọc Duy
` (10 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This is to avoid spreading #ifdef HAVE_INOTIFY around and keep most
code compiled even if it's never active.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
config.mak.uname | 1 +
git-compat-util.h | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index 7d31fad..ee548f5 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -33,6 +33,7 @@ ifeq ($(uname_S),Linux)
HAVE_PATHS_H = YesPlease
LIBC_CONTAINS_LIBINTL = YesPlease
HAVE_DEV_TTY = YesPlease
+ BASIC_CFLAGS += -DHAVE_INOTIFY
endif
ifeq ($(uname_S),GNU/kFreeBSD)
NO_STRLCPY = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..8b55dd0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -128,6 +128,9 @@
#else
#include <poll.h>
#endif
+#ifdef HAVE_INOTIFY
+#include <sys/inotify.h>
+#endif
#if defined(__MINGW32__)
/* pull in Windows compatibility stuff */
@@ -721,4 +724,44 @@ void warn_on_inaccessible(const char *path);
/* Get the passwd entry for the UID of the current process. */
struct passwd *xgetpwuid_self(void);
+#ifndef HAVE_INOTIFY
+/* Keep inotify-specific code build, even if it's not used */
+
+#define IN_DELETE_SELF 1
+#define IN_MOVE_SELF 2
+#define IN_ATTRIB 4
+#define IN_DELETE 8
+#define IN_MODIFY 16
+#define IN_MOVED_FROM 32
+#define IN_MOVED_TO 64
+#define IN_Q_OVERFLOW 128
+#define IN_UNMOUNT 256
+#define IN_CREATE 512
+#define IN_ISDIR 1024
+#define IN_IGNORED 2048
+
+struct inotify_event {
+ int event, mask, wd, len;
+ char name[FLEX_ARRAY];
+};
+
+static inline int inotify_init()
+{
+ errno = ENOSYS;
+ return -1;
+}
+
+static inline int inotify_add_watch(int fd, const char *path, int options)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
+static inline int inotify_rm_watch(int fd, int wd)
+{
+ errno = ENOSYS;
+ return -1;
+}
+#endif
+
#endif
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 17/26] file-watcher: inotify support, watching part
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (15 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 16/26] git-compat-util.h: add inotify stubs on non-Linux platforms Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 18/26] file-watcher: inotify support, notification part Nguyễn Thái Ngọc Duy
` (9 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
"struct dir" manages inotify file descriptor and forms a tree. "struct
file" manages a file. When a file is watched, all dirs up to the file
is watched. Any changes on a directory impacts all subdirs and files.
The way data structure is made might be inotify-specific. I haven't
thought of how other file notification mechanisms may be
implemented. So there may be some refactoring later when a new OS is
supported.
Room for improvement: consecutive watched paths likely share the same
directory part (even though they are sorted by mtime, not name). Try
remember the last "dir" sequence and reduce lookups.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-file-watcher.txt | 13 +++
file-watcher.c | 226 ++++++++++++++++++++++++++++++++++++-
2 files changed, 238 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index d91caf3..d694fea 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -18,11 +18,24 @@ lstat(2) to detect that itself. Config key filewatcher.path needs to
be set to `<socket directory>` so Git knows how to contact to the file
watcher.
+This program is only supported under Linux with inotify(7) support.
+
OPTIONS
-------
--detach::
Run in background.
+BUGS
+----
+On Linux, file watcher may fail to detect changes if you move the work
+tree from outside. For example if you have work tree at
+`/tmp/foo/work`, you move `/tmp/foo` to `/tmp/bar`, make some changes
+in `/tmp/bar/work` and move `/tmp/bar` back to `/tmp/foo`, changes
+won't get noticed. Moving `/tmp/foo/work` to something else is fine.
+
+inotify may not work well with network filesystems and a few special
+others. Check inotify documents.
+
SEE ALSO
--------
linkgit:git-update-index[1],
diff --git a/file-watcher.c b/file-watcher.c
index aa2daf6..d0762e6 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -11,6 +11,28 @@ static const char *const file_watcher_usage[] = {
NULL
};
+struct dir;
+struct repository;
+
+struct file {
+ char *name;
+ struct dir *parent;
+ struct repository *repo;
+ struct file *next;
+};
+
+struct dir {
+ char *name;
+ struct dir *parent;
+ struct dir **subdirs;
+ struct file **files;
+ struct repository *repo; /* only for root note */
+ int wd, nr_subdirs, nr_files;
+};
+
+static struct dir **wds;
+static int wds_alloc;
+
struct repository {
char *work_tree;
char index_signature[41];
@@ -25,6 +47,7 @@ struct repository {
struct string_list updated;
int updated_sorted;
int updating;
+ struct dir *root;
};
const char *invalid_signature = "0000000000000000000000000000000000000000";
@@ -42,10 +65,199 @@ struct connection {
static struct connection **conns;
static struct pollfd *pfd;
static int conns_alloc, pfd_nr, pfd_alloc;
+static int inotify_fd;
+
+/*
+ * IN_DONT_FOLLOW does not matter now as we do not monitor
+ * symlinks. See ce_watchable().
+ */
+#define INOTIFY_MASKS (IN_DELETE_SELF | IN_MOVE_SELF | \
+ IN_CREATE | IN_ATTRIB | IN_DELETE | IN_MODIFY | \
+ IN_MOVED_FROM | IN_MOVED_TO)
+static struct dir *create_dir(struct dir *parent, const char *path,
+ const char *basename)
+{
+ struct dir *d;
+ int wd = inotify_add_watch(inotify_fd, path, INOTIFY_MASKS);
+ if (wd < 0)
+ return NULL;
+
+ d = xmalloc(sizeof(*d));
+ memset(d, 0, sizeof(*d));
+ d->wd = wd;
+ d->parent = parent;
+ d->name = xstrdup(basename);
+
+ ALLOC_GROW(wds, wd + 1, wds_alloc);
+ wds[wd] = d;
+ return d;
+}
+
+static int get_dir_pos(struct dir *d, const char *name)
+{
+ int first, last;
+
+ first = 0;
+ last = d->nr_subdirs;
+ while (last > first) {
+ int next = (last + first) >> 1;
+ int cmp = strcmp(name, d->subdirs[next]->name);
+ if (!cmp)
+ return next;
+ if (cmp < 0) {
+ last = next;
+ continue;
+ }
+ first = next+1;
+ }
+
+ return -first-1;
+}
+
+static void free_file(struct dir *d, int pos, int topdown);
+static void free_dir(struct dir *d, int topdown)
+{
+ struct dir *p = d->parent;
+ int pos;
+ if (!topdown && p && (pos = get_dir_pos(p, d->name)) < 0)
+ die("How come this directory is not registered in its parent?");
+ if (d->repo)
+ d->repo->root = NULL;
+ wds[d->wd] = NULL;
+ inotify_rm_watch(inotify_fd, d->wd);
+ if (topdown) {
+ int i;
+ for (i = 0; i < d->nr_subdirs; i++)
+ free_dir(d->subdirs[i], topdown);
+ for (i = 0; i < d->nr_files; i++)
+ free_file(d, i, topdown);
+ }
+ free(d->name);
+ free(d->subdirs);
+ free(d->files);
+ free(d);
+ if (p && !topdown) {
+ p->nr_subdirs--;
+ memmove(p->subdirs + pos, p->subdirs + pos + 1,
+ (p->nr_subdirs - pos) * sizeof(*p->subdirs));
+ if (!p->nr_subdirs && !p->nr_files)
+ free_dir(p, topdown);
+ }
+}
+
+static int get_file_pos(struct dir *d, const char *name)
+{
+ int first, last;
+
+ first = 0;
+ last = d->nr_files;
+ while (last > first) {
+ int next = (last + first) >> 1;
+ int cmp = strcmp(name, d->files[next]->name);
+ if (!cmp)
+ return next;
+ if (cmp < 0) {
+ last = next;
+ continue;
+ }
+ first = next+1;
+ }
+
+ return -first-1;
+}
+
+static void free_file(struct dir *d, int pos, int topdown)
+{
+ struct file *f = d->files[pos];
+ free(f->name);
+ free(f);
+ if (!topdown) {
+ d->nr_files--;
+ memmove(d->files + pos, d->files + pos + 1,
+ (d->nr_files - pos) * sizeof(*d->files));
+ if (!d->nr_subdirs && !d->nr_files)
+ free_dir(d, topdown);
+ }
+}
+
+static struct dir *add_dir(struct dir *d,
+ const char *path, const char *basename)
+{
+ struct dir *new;
+ int pos = get_dir_pos(d, basename);
+ if (pos >= 0)
+ return d->subdirs[pos];
+ pos = -pos-1;
+
+ new = create_dir(d, path, basename);
+ if (!new)
+ return NULL;
+
+ d->nr_subdirs++;
+ d->subdirs = xrealloc(d->subdirs, sizeof(*d->subdirs) * d->nr_subdirs);
+ if (d->nr_subdirs > pos + 1)
+ memmove(d->subdirs + pos + 1, d->subdirs + pos,
+ (d->nr_subdirs - pos - 1) * sizeof(*d->subdirs));
+ d->subdirs[pos] = new;
+ return new;
+}
+
+static struct file *add_file(struct dir *d, const char *name)
+{
+ struct file *new;
+ int pos = get_file_pos(d, name);
+ if (pos >= 0)
+ return d->files[pos];
+ pos = -pos-1;
+
+ new = xmalloc(sizeof(*new));
+ memset(new, 0, sizeof(*new));
+ new->parent = d;
+ new->name = xstrdup(name);
+
+ d->nr_files++;
+ d->files = xrealloc(d->files, sizeof(*d->files) * d->nr_files);
+ if (d->nr_files > pos + 1)
+ memmove(d->files + pos + 1, d->files + pos,
+ (d->nr_files - pos - 1) * sizeof(*d->files));
+ d->files[pos] = new;
+ return new;
+}
static int watch_path(struct repository *repo, char *path)
{
- return -1;
+ struct dir *d = repo->root;
+ char *p = path;
+
+ if (!d) {
+ d = create_dir(NULL, ".", "");
+ if (!d)
+ return -1;
+ repo->root = d;
+ d->repo = repo;
+ }
+
+ for (;;) {
+ char *next, *sep;
+ sep = strchr(p, '/');
+ if (!sep) {
+ struct file *file;
+ file = add_file(d, p);
+ if (!file->repo)
+ file->repo = repo;
+ break;
+ }
+
+ next = sep + 1;
+ *sep = '\0';
+ d = add_dir(d, path, p);
+ if (!d)
+ /* we could free oldest watches and try again */
+ return -1;
+ *sep = '/';
+ p = next;
+ }
+ return 0;
}
static void get_changed_list(int conn_id)
@@ -195,8 +407,15 @@ static struct repository *get_repo(const char *work_tree)
return repo;
}
+static void reset_watches(struct repository *repo)
+{
+ if (repo->root)
+ free_dir(repo->root, 1);
+}
+
static void reset_repo(struct repository *repo, ino_t inode)
{
+ reset_watches(repo);
string_list_clear(&repo->updated, 0);
memcpy(repo->index_signature, invalid_signature, 40);
repo->inode = inode;
@@ -497,6 +716,11 @@ int main(int argc, const char **argv)
git_extract_argv0_path(argv[0]);
git_setup_gettext();
+
+ inotify_fd = inotify_init();
+ if (inotify_fd < 0)
+ die_errno("unable to initialize inotify");
+
argc = parse_options(argc, argv, NULL, options,
file_watcher_usage, 0);
if (argc < 1)
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 18/26] file-watcher: inotify support, notification part
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (16 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 17/26] file-watcher: inotify support, watching part Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 19/26] Wrap CE_VALID test with ce_valid() Nguyễn Thái Ngọc Duy
` (8 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 141 insertions(+), 1 deletion(-)
diff --git a/file-watcher.c b/file-watcher.c
index d0762e6..5867942 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -260,6 +260,131 @@ static int watch_path(struct repository *repo, char *path)
return 0;
}
+static inline void queue_file_changed(struct file *f, struct strbuf *sb)
+{
+ int len = sb->len;
+ strbuf_addf(sb, "%s%s", f->parent->parent ? "/" : "", f->name);
+ string_list_append(&f->repo->updated, sb->buf);
+ f->repo->updated_sorted = 0;
+ strbuf_setlen(sb, len);
+}
+
+static void construct_path(struct dir *d, struct strbuf *sb)
+{
+ if (!d->parent)
+ return;
+ if (!d->parent->parent) {
+ strbuf_addstr(sb, d->name);
+ return;
+ }
+ construct_path(d->parent, sb);
+ strbuf_addf(sb, "/%s", d->name);
+}
+
+static void file_changed(const struct inotify_event *event,
+ struct dir *d, int pos)
+{
+ struct strbuf sb = STRBUF_INIT;
+ construct_path(d, &sb);
+ queue_file_changed(d->files[pos], &sb);
+ strbuf_release(&sb);
+ free_file(d, pos, 0);
+}
+
+static void dir_changed(const struct inotify_event *event, struct dir *d,
+ const char *base)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int i;
+
+ if (!base) /* top call -> base == NULL */
+ construct_path(d, &sb);
+ else {
+ strbuf_addstr(&sb, base);
+ if (sb.len)
+ strbuf_addch(&sb, '/');
+ strbuf_addstr(&sb, d->name);
+ }
+
+ for (i = 0; i < d->nr_files; i++)
+ queue_file_changed(d->files[i], &sb);
+ for (i = 0; i < d->nr_subdirs; i++) {
+ dir_changed(event, d->subdirs[i], sb.buf);
+ if (!base)
+ free_dir(d->subdirs[i], 1);
+ }
+ strbuf_release(&sb);
+ if (!base)
+ free_dir(d, 0);
+}
+
+static void reset_repo(struct repository *repo, ino_t inode);
+static int do_handle_inotify(const struct inotify_event *event)
+{
+ struct dir *d;
+ int pos;
+
+ if (event->mask & (IN_Q_OVERFLOW | IN_UNMOUNT)) {
+ int i;
+ for (i = 0; i < nr_repos; i++)
+ reset_repo(repos[i], 0);
+ return 0;
+ }
+
+ if ((event->mask & IN_IGNORED) ||
+ /*
+ * Perhaps left over events that we have not consumed
+ * before the watch descriptor is removed.
+ */
+ event->wd >= wds_alloc || wds[event->wd] == NULL)
+ return 0;
+
+ d = wds[event->wd];
+
+ /*
+ * If something happened to the watched directory, consider
+ * everything inside modified
+ */
+ if (event->mask & (IN_DELETE_SELF | IN_MOVE_SELF)) {
+ dir_changed(event, d, NULL);
+ return 0;
+ }
+
+ if (!(event->mask & IN_ISDIR)) {
+ pos = get_file_pos(d, event->name);
+ if (pos >= 0)
+ file_changed(event, d, pos);
+ }
+
+ return 0;
+}
+
+static int handle_inotify(int fd)
+{
+ static char *buf;
+ static unsigned int buf_len = 0;
+ unsigned int avail, offset;
+ int ret, len;
+
+ /* drain the event queue */
+ if (ioctl(fd, FIONREAD, &avail))
+ die_errno("unable to FIONREAD inotify handle");
+ if (buf_len < avail) {
+ buf = xrealloc(buf, avail);
+ buf_len = avail;
+ }
+ len = read(fd, buf, avail);
+ if (len <= 0)
+ return -1;
+ ret = offset = 0;
+ while (offset < len) {
+ struct inotify_event *event = (void *)(buf + offset);
+ ret += do_handle_inotify(event);
+ offset += sizeof(struct inotify_event) + event->len;
+ }
+ return ret;
+}
+
static void get_changed_list(int conn_id)
{
struct strbuf sb = STRBUF_INIT;
@@ -466,6 +591,12 @@ static int handle_command(int conn_id)
* capabilities. Capabilities in uppercase MUST be
* supported. If any side does not understand any of the
* advertised uppercase capabilities, it must disconnect.
+ *
+ * The way the main event loop is structured, we should get at
+ * least one handle_inotify() before receiving the next
+ * command. And handle_inotify() should process all events by
+ * this point of time. This guarantees our reports won't miss
+ * anything by the time get-changed is called.
*/
if ((arg = skip_prefix(msg, "hello"))) {
if (*arg) { /* no capabilities supported yet */
@@ -753,11 +884,15 @@ int main(int argc, const char **argv)
close(err);
}
- nr_common = 1;
+ nr_common = 1 + !!inotify_fd;
pfd_alloc = pfd_nr = nr_common;
pfd = xmalloc(sizeof(*pfd) * pfd_alloc);
pfd[0].fd = fd;
pfd[0].events = POLLIN;
+ if (inotify_fd) {
+ pfd[1].fd = inotify_fd;
+ pfd[1].events = POLLIN;
+ }
while (!quit) {
if (poll(pfd, pfd_nr, -1) < 0) {
@@ -769,6 +904,11 @@ int main(int argc, const char **argv)
continue;
}
+ if (inotify_fd && (pfd[1].revents & POLLIN)) {
+ if (handle_inotify(inotify_fd))
+ break;
+ }
+
for (new_nr = i = nr_common; i < pfd_nr; i++) {
if (pfd[i].revents & (POLLERR | POLLNVAL))
shutdown_connection(i);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 19/26] Wrap CE_VALID test with ce_valid()
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (17 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 18/26] file-watcher: inotify support, notification part Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 20/26] read-cache: new variable to verify file-watcher results Nguyễn Thái Ngọc Duy
` (7 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
The next patch wants to ignore CE_VALID under some condition but not
really clears it. Centralizing its access makes such a change easier.
Not all "ce_flags & CE_VALID" is converted though. The tests that
really mean _bit_ CE_VALID remains so. The tests that mean "ignore
worktree" are converted.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/grep.c | 2 +-
cache.h | 4 ++++
diff-lib.c | 4 ++--
diff.c | 2 +-
read-cache.c | 6 +++---
unpack-trees.c | 2 +-
6 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 63f8603..00526a1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
* are identical, even if worktree file has been modified, so use
* cache version instead
*/
- if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
+ if (cached || ce_valid(ce) || ce_skip_worktree(ce)) {
if (ce_stage(ce))
continue;
hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);
diff --git a/cache.h b/cache.h
index 9f7d952..c229bf9 100644
--- a/cache.h
+++ b/cache.h
@@ -222,6 +222,10 @@ static inline unsigned create_ce_flags(unsigned stage)
#define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)
#define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE)
#define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
+static inline int ce_valid(const struct cache_entry *ce)
+{
+ return ce->ce_flags & CE_VALID;
+}
#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
static inline unsigned int create_ce_mode(unsigned int mode)
diff --git a/diff-lib.c b/diff-lib.c
index 346cac6..dcda7f3 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -198,7 +198,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
continue;
/* If CE_VALID is set, don't look at workdir for file removal */
- changed = (ce->ce_flags & CE_VALID) ? 0 : check_removed(ce, &st);
+ changed = ce_valid(ce) ? 0 : check_removed(ce, &st);
if (changed) {
if (changed < 0) {
perror(ce->name);
@@ -369,7 +369,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
- (idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
+ (idx && (ce_valid(idx) || ce_skip_worktree(idx)));
/*
* Backward compatibility wart - "diff-index -m" does
* not mean "do not ignore merges", but "match_missing".
diff --git a/diff.c b/diff.c
index 8e4a6a9..22c73fe 100644
--- a/diff.c
+++ b/diff.c
@@ -2636,7 +2636,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
* If ce is marked as "assume unchanged", there is no
* guarantee that work tree matches what we are looking for.
*/
- if ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce))
+ if (ce_valid(ce) || ce_skip_worktree(ce))
return 0;
/*
diff --git a/read-cache.c b/read-cache.c
index 5540b06..95c9ccb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -289,8 +289,8 @@ int ie_match_stat(const struct index_state *istate,
*/
if (!ignore_skip_worktree && ce_skip_worktree(ce))
return 0;
- if (!ignore_valid && (ce->ce_flags & CE_VALID))
- return 0;
+ if (!ignore_valid && ce_valid(ce))
+ return 0;
/*
* Intent-to-add entries have not been added, so the index entry
@@ -1047,7 +1047,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
ce_mark_uptodate(ce);
return ce;
}
- if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
+ if (!ignore_valid && ce_valid(ce)) {
ce_mark_uptodate(ce);
return ce;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 164354d..61c3f35 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
* if this entry is truly up-to-date because this file may be
* overwritten.
*/
- if ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce))
+ if (ce_valid(ce) || ce_skip_worktree(ce))
; /* keep checking */
else if (o->reset || ce_uptodate(ce))
return 0;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 20/26] read-cache: new variable to verify file-watcher results
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (18 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 19/26] Wrap CE_VALID test with ce_valid() Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 21/26] Support running file watcher with the test suite Nguyễn Thái Ngọc Duy
` (6 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
If GIT_TEST_WATCHED is set to a non-zero value, Git still uses file
watcher if configured. But it does lstat() anyway and notifies the
user if a file is changed but the file watcher said otherwise.
Note that there is a race condition. Changed paths are retrieved at
time X, then refresh and validation at time Y. Even if X and Y are
very close, an update can happen between X and Y, causing a false
report.
If GIT_TEST_WATCHED is set greater than 1, git will abort instead of
just warn and move on.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-file-watcher.txt | 8 ++++++++
cache.h | 5 ++++-
read-cache.c | 17 +++++++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index d694fea..dd09e30 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -25,6 +25,14 @@ OPTIONS
--detach::
Run in background.
+TROUBLESHOOTING
+---------------
+Setting environment variable `GIT_TEST_WATCHED` to a non-zero number
+makes Git communicate with the file watcher, but do lstat anyway to
+verify that the file watcher results. Setting to 1 prints warning when
+file watcher fails to monitor files correctly. Setting to 2 aborts Git
+when it happens.
+
BUGS
----
On Linux, file watcher may fail to detect changes if you move the work
diff --git a/cache.h b/cache.h
index c229bf9..806c886 100644
--- a/cache.h
+++ b/cache.h
@@ -224,7 +224,10 @@ static inline unsigned create_ce_flags(unsigned stage)
#define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
static inline int ce_valid(const struct cache_entry *ce)
{
- return ce->ce_flags & CE_VALID;
+ extern int test_watched;
+ if (!test_watched)
+ return ce->ce_flags & CE_VALID;
+ return (ce->ce_flags & CE_VALID) && !(ce->ce_flags & CE_WATCHED);
}
#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
diff --git a/read-cache.c b/read-cache.c
index 95c9ccb..d5f084a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -37,6 +37,7 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall
#define CACHE_EXT_WATCH 0x57415443 /* "WATC" */
struct index_state the_index;
+int test_watched;
static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)
{
@@ -1117,6 +1118,16 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
printf(fmt, name);
}
+static void report_bad_watcher(struct index_state *istate,
+ struct cache_entry *ce)
+{
+ if (test_watched > 1)
+ die("%s is updated but file-watcher said no",
+ ce->name);
+ warning("%s is updated but file-watcher said no",
+ ce->name);
+}
+
int refresh_index(struct index_state *istate, unsigned int flags,
const struct pathspec *pathspec,
char *seen, const char *header_msg)
@@ -1188,6 +1199,9 @@ int refresh_index(struct index_state *istate, unsigned int flags,
ce->ce_flags &= ~CE_VALID;
istate->cache_changed = 1;
}
+ if (test_watched &&
+ (ce->ce_flags & CE_WATCHED) && (ce->ce_flags & CE_VALID))
+ report_bad_watcher(istate, ce);
if (quiet)
continue;
@@ -1460,6 +1474,9 @@ int read_index_from(struct index_state *istate, const char *path)
if (istate->initialized)
return istate->cache_nr;
+ if (getenv("GIT_TEST_WATCHED"))
+ test_watched = atoi(getenv("GIT_TEST_WATCHED"));
+
istate->timestamp.sec = 0;
istate->timestamp.nsec = 0;
fd = open(path, O_RDONLY);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 21/26] Support running file watcher with the test suite
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (19 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 20/26] read-cache: new variable to verify file-watcher results Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 22/26] file-watcher: quit if $WATCHER/socket is gone Nguyễn Thái Ngọc Duy
` (5 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This is to force running the test suite with file-watcher with
$ mkdir /tmp/watcher
$ chmod 700 /tmp/watcher
$ git-file-watcher /tmp/watcher/
then open another terminal and run
$ export GIT_TEST_WATCHED=2 GIT_TEST_WATCHER=2
$ export GIT_TEST_WATCHER_PATH=/tmp/watcher
$ make test
TIME_WAIT set set to unlimited by GIT_TEST_WATCHER=2 so the test suite
could hang up indefinitely due to a file-watcher bug. Luckily
everything passes.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher-lib.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
index b6b0848..93afb52 100644
--- a/file-watcher-lib.c
+++ b/file-watcher-lib.c
@@ -136,6 +136,7 @@ void open_watcher(struct index_state *istate)
}
if (!read_config) {
+ const char *s;
int i;
/*
* can't hook into git_default_config because
@@ -149,6 +150,18 @@ void open_watcher(struct index_state *istate)
if (i == istate->cache_nr)
recent_limit = 0;
read_config = 1;
+
+ s = getenv("GIT_TEST_WATCHER");
+ if (s) {
+ watch_lowerlimit = 1;
+ recent_limit = 0;
+ WAIT_TIME = -1;
+ if (atoi(s) > 1)
+ istate->update_watches = 1;
+ s = getenv("GIT_TEST_WATCHER_PATH");
+ if (s)
+ watcher_path = xstrdup(s);
+ }
}
istate->watcher = connect_watcher(watcher_path);
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 22/26] file-watcher: quit if $WATCHER/socket is gone
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (20 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 21/26] Support running file watcher with the test suite Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 23/26] file-watcher: tests for the daemon Nguyễn Thái Ngọc Duy
` (4 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This is more of an issue in development than in production. When a
file-watcher related test fails, the daemon may be left hanging. When
you rerun the same test, old $TRASH_DIRECTORY is wiped out and no one
can communicate with the old daemon any more. Make the old daemon quit
after 5 minutes in such cases.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/file-watcher.c b/file-watcher.c
index 5867942..1e45b25 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -834,11 +834,22 @@ static void check_socket_directory(const char *path)
free(path_copy);
}
+static void run_housekeeping(void)
+{
+ struct stat st;
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s/socket", socket_path);
+ if (stat(sb.buf, &st) || !S_ISSOCK(st.st_mode))
+ exit(0);
+ strbuf_release(&sb);
+}
+
int main(int argc, const char **argv)
{
struct strbuf sb = STRBUF_INIT;
int i, new_nr, fd, quit = 0, nr_common;
int daemon = 0;
+ time_t last_checked;
struct option options[] = {
OPT_BOOL(0, "detach", &daemon,
N_("run in background")),
@@ -894,19 +905,25 @@ int main(int argc, const char **argv)
pfd[1].events = POLLIN;
}
+ last_checked = time(NULL);
while (!quit) {
- if (poll(pfd, pfd_nr, -1) < 0) {
+ int ret = poll(pfd, pfd_nr, 300000);
+ int time_for_housekeeping = 0;
+ if (ret < 0) {
if (errno != EINTR) {
error("Poll failed, resuming: %s",
strerror(errno));
sleep(1);
}
continue;
- }
+ } else if (ret == 0)
+ time_for_housekeeping = 1;
if (inotify_fd && (pfd[1].revents & POLLIN)) {
if (handle_inotify(inotify_fd))
break;
+ if (last_checked + 300 < time(NULL))
+ time_for_housekeeping = 1;
}
for (new_nr = i = nr_common; i < pfd_nr; i++) {
@@ -949,6 +966,11 @@ int main(int argc, const char **argv)
accept_connection(pfd[0].fd);
if (pfd[0].revents & (POLLHUP | POLLERR | POLLNVAL))
die(_("error on listening socket"));
+
+ if (time_for_housekeeping) {
+ run_housekeeping();
+ last_checked = time(NULL);
+ }
}
return 0;
}
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 23/26] file-watcher: tests for the daemon
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (21 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 22/26] file-watcher: quit if $WATCHER/socket is gone Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 24/26] ls-files: print CE_WATCHED as W (or "w" with CE_VALID) Nguyễn Thái Ngọc Duy
` (3 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
test-file-watcher is a simple chat program to talk to file
watcher. Typically you would write something like this
cat >expect <<EOF
# send "hello". Oh and this is a comment!
<hello
# Wait for reply and print to stdout.
# test-file-watcher does not care about anything after '>'
>hello
<index foo bar
>ok
EOF
test-file-watcher . <expect >actual
and test-file-watcher will execute the commands and get responses. If
all go well, "actual" should be the same as "expect". '<' and '>'
denote send and receive packets respectively. '<<' and '>>' can be
used to send and receive a list of NUL-terminated paths.
$GIT_TEST_WATCHER enables a few more commands for testing purposes.
The most important one is 'test-mode' where system inotify is taken
out and inotify events could be injected via test-file-watcher.
There are two debug commands in file-watcher that's not used by the
test suite, but would help debugging: setenv and log. They can be used
to turn on GIT_TRACE_PACKET then any "log" command will show, which
functions as barrier between events file watcher.
GIT_TRACE_WATCHER can also be enabled (dynamically or at startup) to
track inotify events.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
.gitignore | 1 +
Makefile | 1 +
file-watcher.c | 181 ++++++++++++++++++-
t/t7513-file-watcher.sh (new +x) | 382 +++++++++++++++++++++++++++++++++++++++
test-file-watcher.c (new) | 96 ++++++++++
5 files changed, 657 insertions(+), 4 deletions(-)
create mode 100755 t/t7513-file-watcher.sh
create mode 100644 test-file-watcher.c
diff --git a/.gitignore b/.gitignore
index 12c78f0..277f929 100644
--- a/.gitignore
+++ b/.gitignore
@@ -181,6 +181,7 @@
/test-date
/test-delta
/test-dump-cache-tree
+/test-file-watcher
/test-scrap-cache-tree
/test-genrandom
/test-index-version
diff --git a/Makefile b/Makefile
index 1c4d659..f0dc2cc 100644
--- a/Makefile
+++ b/Makefile
@@ -555,6 +555,7 @@ TEST_PROGRAMS_NEED_X += test-ctype
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
+TEST_PROGRAMS_NEED_X += test-file-watcher
TEST_PROGRAMS_NEED_X += test-genrandom
TEST_PROGRAMS_NEED_X += test-index-version
TEST_PROGRAMS_NEED_X += test-line-buffer
diff --git a/file-watcher.c b/file-watcher.c
index 1e45b25..3ab0a11 100644
--- a/file-watcher.c
+++ b/file-watcher.c
@@ -65,7 +65,8 @@ struct connection {
static struct connection **conns;
static struct pollfd *pfd;
static int conns_alloc, pfd_nr, pfd_alloc;
-static int inotify_fd;
+static int inotify_fd, test_mode;
+static int wd_counter = 1;
/*
* IN_DONT_FOLLOW does not matter now as we do not monitor
@@ -78,10 +79,19 @@ static struct dir *create_dir(struct dir *parent, const char *path,
const char *basename)
{
struct dir *d;
- int wd = inotify_add_watch(inotify_fd, path, INOTIFY_MASKS);
+ int wd;
+ if (!test_mode)
+ wd = inotify_add_watch(inotify_fd, path, INOTIFY_MASKS);
+ else {
+ wd = wd_counter++;
+ if (wd > 8)
+ wd = -1;
+ }
if (wd < 0)
return NULL;
+ trace_printf_key("GIT_TRACE_WATCHER", "inotify: watch %d %s\n",
+ wd, path);
d = xmalloc(sizeof(*d));
memset(d, 0, sizeof(*d));
d->wd = wd;
@@ -124,7 +134,9 @@ static void free_dir(struct dir *d, int topdown)
if (d->repo)
d->repo->root = NULL;
wds[d->wd] = NULL;
- inotify_rm_watch(inotify_fd, d->wd);
+ if (!test_mode)
+ inotify_rm_watch(inotify_fd, d->wd);
+ trace_printf_key("GIT_TRACE_WATCHER", "inotify: unwatch %d\n", d->wd);
if (topdown) {
int i;
for (i = 0; i < d->nr_subdirs; i++)
@@ -265,6 +277,7 @@ static inline void queue_file_changed(struct file *f, struct strbuf *sb)
int len = sb->len;
strbuf_addf(sb, "%s%s", f->parent->parent ? "/" : "", f->name);
string_list_append(&f->repo->updated, sb->buf);
+ trace_printf_key("GIT_TRACE_WATCHER", "watcher: changed %s\n", sb->buf);
f->repo->updated_sorted = 0;
strbuf_setlen(sb, len);
}
@@ -324,6 +337,10 @@ static int do_handle_inotify(const struct inotify_event *event)
struct dir *d;
int pos;
+ trace_printf_key("GIT_TRACE_WATCHER", "inotify: event %08x wd %d %s\n",
+ event->mask, event->wd,
+ event->len ? event->name : "N/A");
+
if (event->mask & (IN_Q_OVERFLOW | IN_UNMOUNT)) {
int i;
for (i = 0; i < nr_repos; i++)
@@ -385,6 +402,81 @@ static int handle_inotify(int fd)
return ret;
}
+struct constant {
+ const char *name;
+ int value;
+};
+
+#define CONSTANT(x) { #x, x }
+static const struct constant inotify_masks[] = {
+ CONSTANT(IN_DELETE_SELF),
+ CONSTANT(IN_MOVE_SELF),
+ CONSTANT(IN_ATTRIB),
+ CONSTANT(IN_DELETE),
+ CONSTANT(IN_MODIFY),
+ CONSTANT(IN_MOVED_FROM),
+ CONSTANT(IN_MOVED_TO),
+ CONSTANT(IN_Q_OVERFLOW),
+ CONSTANT(IN_UNMOUNT),
+ { NULL, 0 },
+};
+
+static void inject_inotify(const char *msg)
+{
+ char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
+ struct inotify_event *event = (struct inotify_event *)buf;
+ char *end, *p;
+ int i;
+ memset(event, 0, sizeof(*event));
+ event->wd = strtol(msg, &end, 0);
+ if (*end++ != ' ')
+ die("expect a space after watch descriptor");
+ p = end;
+ end = strchrnul(p, ' ');
+ if (*end)
+ strcpy(event->name, end + 1);
+ while (p < end) {
+ char *sep = strchrnul(p, '|');
+ if (sep > end)
+ sep = end;
+ *sep = '\0';
+ for (i = 0; inotify_masks[i].name; i++)
+ if (!strcmp(inotify_masks[i].name, p))
+ break;
+ if (!inotify_masks[i].name)
+ die("unrecognize event mask %s", p);
+ event->mask |= inotify_masks[i].value;
+ p = sep + 1;
+ }
+ do_handle_inotify(event);
+}
+
+static void dump_watches(struct dir *d, struct strbuf *sb, struct strbuf *out)
+{
+ int i, len = sb->len;
+ strbuf_addstr(sb, d->name);
+ strbuf_addf(out, "%s %d%c", sb->buf[0] ? sb->buf : ".", d->wd, '\0');
+ if (d->name[0])
+ strbuf_addch(sb, '/');
+ for (i = 0; i < d->nr_subdirs; i++)
+ dump_watches(d->subdirs[i], sb, out);
+ for (i = 0; i < d->nr_files; i++)
+ strbuf_addf(out, "%s%s%c", sb->buf, d->files[i]->name, '\0');
+ strbuf_setlen(sb, len);
+}
+
+static void dump_changes(struct repository *repo, struct strbuf *sb)
+{
+ int i;
+ if (!repo->updated_sorted) {
+ sort_string_list(&repo->updated);
+ repo->updated_sorted = 1;
+ }
+ for (i = 0; i < repo->updated.nr; i++)
+ strbuf_add(sb, repo->updated.items[i].string,
+ strlen(repo->updated.items[i].string) + 1);
+}
+
static void get_changed_list(int conn_id)
{
struct strbuf sb = STRBUF_INIT;
@@ -483,11 +575,13 @@ static void unchange(int conn_id, unsigned long size)
item = string_list_lookup(&repo->updated, p);
if (!item)
continue;
+ trace_printf_key("GIT_TRACE_WATCHER", "watcher: unchange %s\n", p);
unsorted_string_list_delete_item(&repo->updated,
item - repo->updated.items, 0);
}
strbuf_release(&sb);
}
+ trace_printf_key("GIT_TRACE_WATCHER", "watcher: unchange complete\n");
memcpy(repo->index_signature, conn->new_index, 40);
/*
* If other connections on this repo are in some sort of
@@ -540,6 +634,13 @@ static void reset_watches(struct repository *repo)
static void reset_repo(struct repository *repo, ino_t inode)
{
+ if (test_mode)
+ /*
+ * test-mode is designed for single repo, we can
+ * safely reset wd counter because all wd should be
+ * deleted
+ */
+ wd_counter = 1;
reset_watches(repo);
string_list_clear(&repo->updated, 0);
memcpy(repo->index_signature, invalid_signature, 40);
@@ -560,6 +661,7 @@ static int shutdown_connection(int id)
return 0;
}
+static void cleanup(void);
static int handle_command(int conn_id)
{
int fd = conns[conn_id]->sock;
@@ -754,6 +856,71 @@ static int handle_command(int conn_id)
}
unchange(conn_id, n);
}
+
+ /*
+ * Testing and debugging support
+ */
+ else if (!strcmp(msg, "test-mode") && getenv("GIT_TEST_WATCHER")) {
+ test_mode = 1;
+ packet_write(fd, "test mode on");
+ }
+ else if (starts_with(msg, "setenv ")) {
+ /* useful for setting GIT_TRACE_WATCHER or GIT_TRACE_PACKET */
+ char *sep = strchr(msg + 7, ' ');
+ if (!sep) {
+ packet_write(fd, "error invalid setenv line %s", msg);
+ return shutdown_connection(conn_id);
+ }
+ *sep = '\0';
+ setenv(msg + 7, sep + 1, 1);
+ }
+ else if (starts_with(msg, "log ")) {
+ ; /* do nothing, if GIT_TRACE_PACKET is on, it's already logged */
+ }
+ else if (!strcmp(msg, "die") && getenv("GIT_TEST_WATCHER")) {
+ /*
+ * The client will wait for "see you" before it may
+ * run another daemon with the same path. So there's
+ * no racing on unlink() and listen() on the same
+ * socket path.
+ */
+ cleanup();
+ packet_write(fd, "see you");
+ close(fd);
+ exit(0);
+ }
+ else if (starts_with(msg, "dump ") && getenv("GIT_TEST_WATCHER")) {
+ struct strbuf sb = STRBUF_INIT;
+ struct strbuf out = STRBUF_INIT;
+ const char *reply = NULL;
+ if (!strcmp(msg + 5, "watches")) {
+ if (conns[conn_id]->repo) {
+ if (conns[conn_id]->repo->root)
+ dump_watches(conns[conn_id]->repo->root, &sb, &out);
+ } else {
+ int i;
+ for (i = 0; i < nr_repos; i++) {
+ strbuf_addf(&out, "%s%c", repos[i]->work_tree, '\0');
+ if (repos[i]->root)
+ dump_watches(repos[i]->root, &sb, &out);
+ strbuf_reset(&out);
+ strbuf_reset(&sb);
+ }
+ }
+ reply = "watching";
+ } else if (!strcmp(msg + 5, "changes")) {
+ dump_changes(conns[conn_id]->repo, &out);
+ reply = "changed";
+ }
+ packet_write(fd, "%s %d", reply, (int)out.len);
+ if (out.len)
+ write_in_full(fd, out.buf, out.len);
+ strbuf_release(&out);
+ strbuf_release(&sb);
+ }
+ else if (starts_with(msg, "inotify ") && getenv("GIT_TEST_WATCHER")) {
+ inject_inotify(msg + 8);
+ }
else {
packet_write(fd, "error unrecognized command %s", msg);
return shutdown_connection(conn_id);
@@ -848,11 +1015,13 @@ int main(int argc, const char **argv)
{
struct strbuf sb = STRBUF_INIT;
int i, new_nr, fd, quit = 0, nr_common;
- int daemon = 0;
+ int daemon = 0, check_support = 0;
time_t last_checked;
struct option options[] = {
OPT_BOOL(0, "detach", &daemon,
N_("run in background")),
+ OPT_BOOL(0, "check-support", &check_support,
+ N_("return zero file watcher is available")),
OPT_END()
};
@@ -865,6 +1034,10 @@ int main(int argc, const char **argv)
argc = parse_options(argc, argv, NULL, options,
file_watcher_usage, 0);
+
+ if (check_support)
+ return 0;
+
if (argc < 1)
die(_("socket path missing"));
else if (argc > 1)
diff --git a/t/t7513-file-watcher.sh b/t/t7513-file-watcher.sh
new file mode 100755
index 0000000..bf64fc4
--- /dev/null
+++ b/t/t7513-file-watcher.sh
@@ -0,0 +1,382 @@
+#!/bin/sh
+
+test_description='File watcher daemon tests'
+
+. ./test-lib.sh
+
+if git file-watcher --check-support && test_have_prereq POSIXPERM; then
+ : # good
+else
+ skip_all="file-watcher not supported on this system"
+ test_done
+fi
+
+kill_it() {
+ test-file-watcher "$1" <<EOF >/dev/null
+<die
+>see you
+EOF
+}
+
+GIT_TEST_WATCHER=1
+export GIT_TEST_WATCHER
+
+test_expect_success 'test-file-watcher can kill the daemon' '
+ chmod 700 . &&
+ git file-watcher --detach . &&
+ cat >expect <<EOF &&
+<die
+>see you
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ ! test -S socket
+'
+
+test_expect_success 'exchange hello' '
+ git file-watcher --detach . &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<die
+>see you
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'normal index sequence' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>inconsistent
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ cat >expect2 <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+# inconsistent again because new-index has not been issued yet
+>inconsistent
+<new-index $SIG
+<<unchange
+<<
+EOF
+ test-file-watcher . >actual2 <expect2 &&
+ test_cmp expect2 actual2 &&
+ cat >expect3 <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<die
+>see you
+EOF
+ test-file-watcher . >actual3 <expect3 &&
+ test_cmp expect3 actual3
+'
+
+test_expect_success 'unaccepted index: hello not sent' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<index $SIG $TRASH_DIRECTORY
+>error why did you not greet me? go away
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ kill_it .
+'
+
+test_expect_success 'unaccepted index: signature too short' '
+ git file-watcher --detach . &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index 1234 $TRASH_DIRECTORY
+>error invalid index line index 1234 $TRASH_DIRECTORY
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ kill_it .
+'
+
+test_expect_success 'unaccepted index: worktree unavailable' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY/non-existent
+>error work tree does not exist: No such file or directory
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ kill_it .
+'
+
+test_expect_success 'watch foo and abc/bar' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>inconsistent
+<test-mode
+>test mode on
+<<watch
+<<foo
+<<abc/bar
+<<
+>watched 2
+<dump watches
+>>watching
+>>. 1
+>>abc 2
+>>abc/bar
+>>foo
+<new-index $SIG
+<<unchange
+<<
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'modify abc/bar' '
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<inotify 2 IN_MODIFY bar
+<dump watches
+>>watching
+>>. 1
+>>foo
+<dump changes
+>>changed
+>>abc/bar
+<die
+>see you
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'delete abc makes abc/bar changed' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>inconsistent
+<test-mode
+>test mode on
+<<watch
+<<foo/abc/bar
+<<
+>watched 1
+<dump watches
+>>watching
+>>. 1
+>>foo 2
+>>foo/abc 3
+>>foo/abc/bar
+<inotify 2 IN_DELETE_SELF
+<dump watches
+>>watching
+<dump changes
+>>changed
+>>foo/abc/bar
+<new-index $SIG
+<<unchange
+<<
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'get changed list' '
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<get-changed
+>>changed
+>>foo/abc/bar
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'incomplete new-index request' '
+ SIG=0123456789012345678901234567890123456789 &&
+ SIG2=9123456789012345678901234567890123456780 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<new-index $SIG2
+<dump changes
+>>changed
+>>foo/abc/bar
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'delete abc/bar from changed list' '
+ SIG=0123456789012345678901234567890123456789 &&
+ SIG2=9123456789012345678901234567890123456780 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<new-index $SIG2
+<<unchange
+<<foo/abc/bar
+<<
+<dump changes
+>>changed
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'file-watcher index updated after new-index' '
+ SIG2=9123456789012345678901234567890123456780 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG2 $TRASH_DIRECTORY
+>ok
+<die
+>see you
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+# When test-mode is on, file-watch only accepts 8 directories
+test_expect_success 'watch too many directories' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>inconsistent
+# Do not call inotify_add_watch()
+<test-mode
+>test mode on
+# First batch should be all ok
+<<watch
+<<dir1/foo
+<<dir2/foo
+<<dir3/foo
+<<dir4/foo
+<<
+>watched 4
+# Second batch hits the limit
+<<watch
+<<dir5/foo
+<<dir6/foo
+<<dir7/foo
+<<dir8/foo
+<<dir9/foo
+<<
+>watched 3
+# The third batch is already registered, should accept too
+<<watch
+<<dir5/foo
+<<dir6/foo
+<<dir7/foo
+<<
+>watched 3
+# Try again, see if it still rejects
+<<watch
+<<dir8/foo
+<<dir9/foo
+<<
+>watched 0
+<dump watches
+>>watching
+>>. 1
+>>dir1 2
+>>dir1/foo
+>>dir2 3
+>>dir2/foo
+>>dir3 4
+>>dir3/foo
+>>dir4 5
+>>dir4/foo
+>>dir5 6
+>>dir5/foo
+>>dir6 7
+>>dir6/foo
+>>dir7 8
+>>dir7/foo
+<die
+>see you
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'event overflow' '
+ git file-watcher --detach . &&
+ SIG=0123456789012345678901234567890123456789 &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>inconsistent
+<test-mode
+>test mode on
+<<watch
+<<foo
+<<abc/bar
+<<
+>watched 2
+<inotify 2 IN_MODIFY bar
+<dump watches
+>>watching
+>>. 1
+>>foo
+<dump changes
+>>changed
+>>abc/bar
+<inotify -1 IN_Q_OVERFLOW
+<dump watches
+>>watching
+<dump changes
+>>changed
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual &&
+ cat >expect2 <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+# Must be inconsistent because of IN_Q_OVERFLOW
+>inconsistent
+<die
+>see you
+EOF
+ test-file-watcher . >actual2 <expect2 &&
+ test_cmp expect2 actual2
+'
+
+test_done
diff --git a/test-file-watcher.c b/test-file-watcher.c
new file mode 100644
index 0000000..ffff198
--- /dev/null
+++ b/test-file-watcher.c
@@ -0,0 +1,96 @@
+#include "cache.h"
+#include "unix-socket.h"
+#include "pkt-line.h"
+#include "strbuf.h"
+
+int main(int ac, char **av)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct strbuf packed = STRBUF_INIT;
+ char *packing = NULL;
+ int last_command_is_reply = 0;
+ int fd;
+
+ strbuf_addf(&sb, "%s/socket", av[1]);
+ fd = unix_stream_connect(sb.buf);
+ if (fd < 0)
+ die_errno("connect");
+ strbuf_reset(&sb);
+
+ /*
+ * test-file-watcher crashes sometimes, make sure to flush
+ */
+ setbuf(stdout, NULL);
+
+ while (!strbuf_getline(&sb, stdin, '\n')) {
+ if (sb.buf[0] == '#') {
+ puts(sb.buf);
+ continue;
+ }
+ if (sb.buf[0] == '>') {
+ if (last_command_is_reply)
+ continue;
+ last_command_is_reply = 1;
+ } else
+ last_command_is_reply = 0;
+
+ if (sb.buf[0] == '<' && sb.buf[1] == '<') {
+ puts(sb.buf);
+ if (!packing) {
+ packing = xstrdup(sb.buf + 2);
+ strbuf_reset(&packed);
+ continue;
+ }
+ if (!sb.buf[2]) {
+ packet_write(fd, "%s %d", packing, (int)packed.len);
+ if (packed.len)
+ write_in_full(fd, packed.buf, packed.len);
+ free(packing);
+ packing = NULL;
+ } else
+ strbuf_add(&packed, sb.buf + 2, sb.len - 2 + 1);
+ continue;
+ }
+ if (sb.buf[0] == '<') {
+ packet_write(fd, "%s", sb.buf + 1);
+ puts(sb.buf);
+ continue;
+ }
+ if (sb.buf[0] == '>' && sb.buf[1] == '>') {
+ int len;
+ char *p, *reply = packet_read_line(fd, &len);
+ if (!starts_with(reply, sb.buf + 2) ||
+ reply[sb.len - 2] != ' ') {
+ printf(">%s\n", reply);
+ continue;
+ } else {
+ p = reply + sb.len - 2;
+ printf(">>%.*s\n", (int)(p - reply), reply);
+ len = atoi(p + 1);
+ if (!len)
+ continue;
+ }
+ strbuf_reset(&packed);
+ strbuf_grow(&packed, len);
+ if (read_in_full(fd, packed.buf, len) <= 0)
+ return 1;
+ strbuf_setlen(&packed, len);
+ for (p = packed.buf; p - packed.buf < packed.len; p += len + 1) {
+ len = strlen(p);
+ printf(">>%s\n", p);
+ }
+ continue;
+ }
+ if (sb.buf[0] == '>') {
+ int len;
+ char *reply = packet_read_line(fd, &len);
+ if (!reply)
+ puts(">");
+ else
+ printf(">%s\n", reply);
+ continue;
+ }
+ die("unrecognize command %s", sb.buf);
+ }
+ return 0;
+}
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 24/26] ls-files: print CE_WATCHED as W (or "w" with CE_VALID)
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (22 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 23/26] file-watcher: tests for the daemon Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 25/26] file-watcher: tests for the client side Nguyễn Thái Ngọc Duy
` (2 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-ls-files.txt | 1 +
builtin/ls-files.c | 14 ++++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index c0856a6..bdb17a5 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -123,6 +123,7 @@ a space) at the start of each line:
R:: removed/deleted
C:: modified/changed
K:: to be killed
+ W:: being watched by `git file-watcher`
?:: other
-v::
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e1cf6d8..f1f7c07 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -46,6 +46,7 @@ static const char *tag_killed = "";
static const char *tag_modified = "";
static const char *tag_skip_worktree = "";
static const char *tag_resolve_undo = "";
+static const char *tag_watched = "";
static void write_name(const char *name)
{
@@ -231,6 +232,7 @@ static void show_files(struct dir_struct *dir)
if (show_cached || show_stage) {
for (i = 0; i < active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
+ const char *tag;
if ((dir->flags & DIR_SHOW_IGNORED) &&
!ce_excluded(dir, ce))
continue;
@@ -238,8 +240,15 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce->ce_flags & CE_UPDATE)
continue;
- show_ce_entry(ce_stage(ce) ? tag_unmerged :
- (ce_skip_worktree(ce) ? tag_skip_worktree : tag_cached), ce);
+ if (ce_stage(ce))
+ tag = tag_unmerged;
+ else if (ce_skip_worktree(ce))
+ tag = tag_skip_worktree;
+ else if (ce->ce_flags & CE_WATCHED)
+ tag = tag_watched;
+ else
+ tag = tag_cached;
+ show_ce_entry(tag, ce);
}
}
if (show_deleted || show_modified) {
@@ -530,6 +539,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
tag_killed = "K ";
tag_skip_worktree = "S ";
tag_resolve_undo = "U ";
+ tag_watched = "W ";
}
if (show_modified || show_others || show_deleted || (dir.flags & DIR_SHOW_IGNORED) || show_killed)
require_work_tree = 1;
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 25/26] file-watcher: tests for the client side
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (23 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 24/26] ls-files: print CE_WATCHED as W (or "w" with CE_VALID) Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-03 4:29 ` [PATCH v3 26/26] Disable file-watcher with system inotify on some tests Nguyễn Thái Ngọc Duy
2014-02-08 8:04 ` [PATCH v3 00/26] inotify support Torsten Bögershausen
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
Similar to t7513, system inotify is taken out to give us better
controlled environment.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
file-watcher-lib.c | 21 ++--
t/t7514-file-watcher-lib.sh (new +x) | 190 +++++++++++++++++++++++++++++++++++
test-file-watcher.c | 15 +++
3 files changed, 219 insertions(+), 7 deletions(-)
create mode 100755 t/t7514-file-watcher-lib.sh
diff --git a/file-watcher-lib.c b/file-watcher-lib.c
index 93afb52..8cc3b73 100644
--- a/file-watcher-lib.c
+++ b/file-watcher-lib.c
@@ -23,7 +23,8 @@ static int connect_watcher(const char *path)
return fd;
}
-static void reset_watches(struct index_state *istate, int disconnect)
+static void reset_watches(struct index_state *istate, int disconnect,
+ const char *reason)
{
int i, changed = 0;
if (istate->updated_entries) {
@@ -45,6 +46,8 @@ static void reset_watches(struct index_state *istate, int disconnect)
close(istate->watcher);
istate->watcher = -1;
}
+ trace_printf_key("GIT_TRACE_WATCHER", "reset%s: %s\n",
+ disconnect ? "/disconnect" : "", reason);
}
static void mark_ce_valid(struct index_state *istate)
@@ -53,15 +56,16 @@ static void mark_ce_valid(struct index_state *istate)
char *line, *end;
int i, len;
unsigned long n;
+ trace_printf_key("GIT_TRACE_WATCHER", "mark_ce_valid\n");
if (packet_write_timeout(istate->watcher, WAIT_TIME, "get-changed") <= 0 ||
!(line = packet_read_line_timeout(istate->watcher, WAIT_TIME, &len)) ||
!starts_with(line, "changed ")) {
- reset_watches(istate, 1);
+ reset_watches(istate, 1, "invalid get-changed response");
return;
}
n = strtoul(line + 8, &end, 10);
if (end != line + len) {
- reset_watches(istate, 1);
+ reset_watches(istate, 1, "invalid get-changed response");
return;
}
if (!n)
@@ -69,7 +73,7 @@ static void mark_ce_valid(struct index_state *istate)
strbuf_grow(&sb, n);
if (read_in_full_timeout(istate->watcher, sb.buf, n, WAIT_TIME) != n) {
strbuf_release(&sb);
- reset_watches(istate, 1);
+ reset_watches(istate, 1, "invalid get-changed payload");
return;
}
line = sb.buf;
@@ -131,7 +135,7 @@ void open_watcher(struct index_state *istate)
char *msg;
if (!get_git_work_tree()) {
- reset_watches(istate, 1);
+ reset_watches(istate, 1, "no worktree");
return;
}
@@ -165,10 +169,11 @@ void open_watcher(struct index_state *istate)
}
istate->watcher = connect_watcher(watcher_path);
+ trace_printf_key("GIT_TRACE_WATCHER", "open watcher %d\n", istate->watcher);
if (packet_write_timeout(istate->watcher, WAIT_TIME, "hello") <= 0 ||
(msg = packet_read_line_timeout(istate->watcher, WAIT_TIME, NULL)) == NULL ||
strcmp(msg, "hello")) {
- reset_watches(istate, 1);
+ reset_watches(istate, 1, "invalid hello response");
return;
}
@@ -177,7 +182,7 @@ void open_watcher(struct index_state *istate)
get_git_work_tree()) <= 0 ||
(msg = packet_read_line_timeout(istate->watcher, WAIT_TIME, NULL)) == NULL ||
strcmp(msg, "ok")) {
- reset_watches(istate, 0);
+ reset_watches(istate, 0, "inconsistent");
istate->update_watches = 1;
return;
}
@@ -265,6 +270,7 @@ void watch_entries(struct index_state *istate)
nr++;
if (nr < watch_lowerlimit)
return;
+ trace_printf_key("GIT_TRACE_WATCHER", "watch %d\n", nr);
sorted = xmalloc(sizeof(*sorted) * nr);
for (i = nr = 0; i < istate->cache_nr; i++)
if (ce_watchable(istate->cache[i], now))
@@ -280,6 +286,7 @@ void close_watcher(struct index_state *istate, const unsigned char *sha1)
int len, i, nr;
if (istate->watcher <= 0)
return;
+ trace_printf_key("GIT_TRACE_WATCHER", "close watcher\n");
if (packet_write_timeout(istate->watcher, WAIT_TIME,
"new-index %s", sha1_to_hex(sha1)) <= 0)
goto done;
diff --git a/t/t7514-file-watcher-lib.sh b/t/t7514-file-watcher-lib.sh
new file mode 100755
index 0000000..8dabb13
--- /dev/null
+++ b/t/t7514-file-watcher-lib.sh
@@ -0,0 +1,190 @@
+#!/bin/sh
+
+test_description='File watcher daemon and client tests'
+
+. ./test-lib.sh
+
+if git file-watcher --check-support && test_have_prereq POSIXPERM; then
+ : # good
+else
+ skip_all="file-watcher not supported on this system"
+ test_done
+fi
+
+kill_it() {
+ test-file-watcher "$1" <<EOF >/dev/null
+<die
+>see you
+EOF
+}
+
+GIT_TEST_WATCHER=2
+GIT_TEST_WATCHER_PATH="$TRASH_DIRECTORY"
+export GIT_TEST_WATCHER GIT_TEST_WATCHER_PATH
+
+test_expect_success 'setup' '
+ chmod 700 . &&
+ mkdir foo bar &&
+ touch abc foo/def bar/ghi &&
+ git add . &&
+ git file-watcher --detach . &&
+ cat <<EOF >expect &&
+<test-mode
+>test mode on
+EOF
+ test-file-watcher . <expect >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'initial opening sequence' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git ls-files >/dev/null &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< inconsistent
+packet: git> watch 20
+packet: git< watched 3
+EOF
+ test_cmp expect actual &&
+
+ # second time gives the same result because file-watch has not
+ # received new-index
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual2" git ls-files >/dev/null &&
+ test_cmp expect actual2
+'
+
+test_expect_success 'full sequence' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git status >/dev/null &&
+ SIG2=`test-file-watcher --index-signature .git/index` &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< inconsistent
+packet: git> watch 20
+packet: git< watched 3
+packet: git> new-index $SIG2
+packet: git> unchange 0
+EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'full sequence after file-watcher is active' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git ls-files -v >paths.actual &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< ok
+packet: git> get-changed
+packet: git< changed 0
+EOF
+ test_cmp expect actual &&
+ cat <<EOF >paths.expect &&
+w abc
+w bar/ghi
+w foo/def
+EOF
+ test_cmp paths.expect paths.actual
+'
+
+test_expect_success 'inject a file change' '
+ echo modified >bar/ghi &&
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ cat >expect <<EOF &&
+<hello
+>hello
+<index $SIG $TRASH_DIRECTORY
+>ok
+<inotify 2 IN_MODIFY ghi
+<dump changes
+>>changed
+>>bar/ghi
+EOF
+ test-file-watcher . >actual <expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git obtains the changes' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TEST_WATCHER=1 GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git ls-files -v >paths.actual &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< ok
+packet: git> get-changed
+packet: git< changed 8
+EOF
+ test_cmp expect actual &&
+ cat <<EOF >paths.expect &&
+w abc
+H bar/ghi
+w foo/def
+EOF
+ test_cmp paths.expect paths.actual
+'
+
+test_expect_success 'sync file-watcher after index update' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git status --porcelain | grep -vF "??" >paths.actual &&
+ SIG2=`test-file-watcher --index-signature .git/index` &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< ok
+packet: git> get-changed
+packet: git< changed 8
+packet: git> watch 8
+packet: git< watched 1
+packet: git> new-index $SIG2
+packet: git> unchange 8
+EOF
+ test_cmp expect actual &&
+ cat <<EOF >paths.expect &&
+A abc
+AM bar/ghi
+A foo/def
+EOF
+ test_cmp paths.expect paths.actual
+'
+
+test_expect_success 'make sure file-watcher cleans its changed list' '
+ SIG=`test-file-watcher --index-signature .git/index` &&
+ rm actual &&
+ GIT_TEST_WATCHER=1 GIT_TRACE_PACKET="$TRASH_DIRECTORY/actual" git ls-files -v >paths.actual &&
+ cat <<EOF >expect &&
+packet: git> hello
+packet: git< hello
+packet: git> index $SIG $TRASH_DIRECTORY
+packet: git< ok
+packet: git> get-changed
+packet: git< changed 0
+EOF
+ test_cmp expect actual &&
+ cat <<EOF >paths.expect &&
+w abc
+H bar/ghi
+w foo/def
+EOF
+ test_cmp paths.expect paths.actual
+'
+
+test_expect_success 'closing the daemon' '
+ test-file-watcher . <<EOF >/dev/null
+<die
+>see you
+EOF
+'
+
+test_done
diff --git a/test-file-watcher.c b/test-file-watcher.c
index ffff198..77037e1 100644
--- a/test-file-watcher.c
+++ b/test-file-watcher.c
@@ -11,6 +11,21 @@ int main(int ac, char **av)
int last_command_is_reply = 0;
int fd;
+ if (!strcmp(av[1], "--index-signature")) {
+ struct stat st;
+ void *mmap;
+ if (lstat(av[2], &st))
+ die_errno("lstat");
+ fd = open(av[2], O_RDONLY);
+ if (fd < 0)
+ die_errno("open");
+ mmap = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (mmap == MAP_FAILED)
+ die_errno("mmap");
+ printf("%s\n", sha1_to_hex((unsigned char*)mmap + st.st_size - 20));
+ return 0;
+ }
+
strbuf_addf(&sb, "%s/socket", av[1]);
fd = unix_stream_connect(sb.buf);
if (fd < 0)
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH v3 26/26] Disable file-watcher with system inotify on some tests
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (24 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 25/26] file-watcher: tests for the client side Nguyễn Thái Ngọc Duy
@ 2014-02-03 4:29 ` Nguyễn Thái Ngọc Duy
2014-02-08 8:04 ` [PATCH v3 00/26] inotify support Torsten Bögershausen
26 siblings, 0 replies; 72+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2014-02-03 4:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
When file watcher is active via GIT_TEST_WATCHER_PATH, `ls-files -v`
output may be different (e.g. "H" becomes "W"). Disable file watcher
in those cases.
We could make ls-files turn 'W' back to 'H' for these cases, but not
sure it's worth the effort. The intention of running with
GIT_TEST_WATCHER_PATH is to exercise file watcher in somewhat real
scenarios and the remaining tests are more than enough for that.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t1011-read-tree-sparse-checkout.sh | 2 ++
t/t2104-update-index-skip-worktree.sh | 2 ++
t/t7011-skip-worktree-reading.sh | 2 ++
t/t7012-skip-worktree-writing.sh | 2 ++
4 files changed, 8 insertions(+)
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c74bee..00e92bc 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -14,6 +14,8 @@ test_description='sparse checkout tests
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-read-tree.sh
+unset GIT_TEST_WATCHER_PATH
+
test_expect_success 'setup' '
cat >expected <<-\EOF &&
100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0 init.t
diff --git a/t/t2104-update-index-skip-worktree.sh b/t/t2104-update-index-skip-worktree.sh
index 1d0879b..2b93c58 100755
--- a/t/t2104-update-index-skip-worktree.sh
+++ b/t/t2104-update-index-skip-worktree.sh
@@ -7,6 +7,8 @@ test_description='skip-worktree bit test'
. ./test-lib.sh
+unset GIT_TEST_WATCHER_PATH
+
cat >expect.full <<EOF
H 1
H 2
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 88d60c1..2242718 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -7,6 +7,8 @@ test_description='skip-worktree bit test'
. ./test-lib.sh
+unset GIT_TEST_WATCHER_PATH
+
cat >expect.full <<EOF
H 1
H 2
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9ceaa40..c8e0eb5 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -7,6 +7,8 @@ test_description='test worktree writing operations when skip-worktree is used'
. ./test-lib.sh
+unset GIT_TEST_WATCHER_PATH
+
test_expect_success 'setup' '
test_commit init &&
echo modified >> init.t &&
--
1.8.5.2.240.g8478abd
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH v3 00/26] inotify support
2014-02-03 4:28 ` [PATCH v3 00/26] " Nguyễn Thái Ngọc Duy
` (25 preceding siblings ...)
2014-02-03 4:29 ` [PATCH v3 26/26] Disable file-watcher with system inotify on some tests Nguyễn Thái Ngọc Duy
@ 2014-02-08 8:04 ` Torsten Bögershausen
2014-02-08 8:53 ` Duy Nguyen
26 siblings, 1 reply; 72+ messages in thread
From: Torsten Bögershausen @ 2014-02-08 8:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git,
Torsten Bögershausen
On 03.02.14 05:28, Nguyễn Thái Ngọc Duy wrote:
I managed to review the code 0..12/26, so some parts are missing.
The list below became longer than what I intended,
my comments may be hard to read,
and there is a mixture of minor and major remarks.
I would appreciate if we could have an outline of the protocol
as a seperate "document" somewhere, to be able to have a look at the protocol
first, before looking into the code.
(Am I using wireshark too much to dream about a dissector ?)
All in all I like the concept, thanks for the work.
1)
write_in_full_timeout()
packet_read_line_timeout()
At other places we handle EINTR after calling poll().
Looking at the code, it could be easier to introduce
a new function xpoll() in wrapper.c, and use that instead
of poll().
2)
Similar for the usage of accept().
I like the idea of xread() xwrite() and all the x functions
so it coud make sense to establish xpoll() and xaccept()
before inotify suppport.
3)
> -int unix_stream_listen(const char *path)
> +int unix_stream_listen(const char *path, int replace)
> {
> int fd, saved_errno;
> struct sockaddr_un sa;
> @@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
> return -1;
> fd = unix_stream_socket();
>
> - unlink(path);
> + if (replace)
> + unlink(path);
Minor remark:
As we do not do the replace here:
s/replace/un_link/ may be ?
4)
> +++ b/file-watcher.c
{}
> +static const char *const file_watcher_usage[] = {
> + N_("git file-watcher [options] <socket directory>"),
> + NULL
> +};
Do we already have options here?
I can think about having
-d daemoniye
-u uses Unix doain socket
(And later -t to use a TCP/IP socket, when the repo
is on a mounted NFS (or SAMBA) drive, and the daemon is on a
different machine.
I don't say this patch should include this logic in first round,
But I can see a gain for this kind of setup)
5)
> +++ b/file-watcher.c
[]
> +static int shutdown_connection(int id)
> +{
> + struct connection *conn = conns[id];
> + conns[id] = NULL;
> + pfd[id].fd = -1;
> + if (!conn)
> + return 0;
> + close(conn->sock);
> + free(conn);
> + return 0;
> +}
The function is called shutdown_connection(), but it does a close()
Could it be named close_connection() ?
6)
> +++ b/file-watcher.c
[]
Do we have a sequence chart about the command flow between the watcher
daemon and the client ?
------------
7)
in 03/26:
>This version is also gentler than its friend packet_read_line()
gentler, what does this mean?
>because it's designed for side channel I/O that should not abort the
>program even if the channel is broken.
I'm not so familar with side-channel I/O. How does it fit in here?
Does this make sense:
In opposite to packet_read_line() which can call die()
to abort the program, read_in_full_timeout() will keep the program running.
(or something like this)
>
>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> cache.h | 1 +
> pkt-line.c | 35 +++++++++++++++++++++++++++++++++++
> pkt-line.h | 1 +
> wrapper.c | 21 +++++++++++++++++++++
> 4 files changed, 58 insertions(+)
>
>diff --git a/cache.h b/cache.h
>index 718e32b..939db46 100644
>--- a/cache.h
>+++ b/cache.h
>@@ -1230,6 +1230,7 @@ extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
> extern void fsync_or_die(int fd, const char *);
>
> extern ssize_t read_in_full(int fd, void *buf, size_t count);
>+extern ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout);
> extern ssize_t write_in_full(int fd, const void *buf, size_t count);
> extern ssize_t write_in_full_timeout(int fd, const void *buf, size_t count, int timeout);
> static inline ssize_t write_str_in_full(int fd, const char *str)
>diff --git a/pkt-line.c b/pkt-line.c
>index cf681e9..5a07e97 100644
>--- a/pkt-line.c
>+++ b/pkt-line.c
>@@ -229,3 +229,38 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
> {
> return packet_read_line_generic(-1, src, src_len, dst_len);
> }
>+
In what is the timeout measured ?
seconds, milli years?
As we use poll() I think it is milli seconds.
(I like the idea of naming timeout timeout_ms)
[]
>+ len -= 4;
>+ if (len >= buf_len) {
>+ error("protocol error: bad line length %d", len);
>+ return NULL;
>+ }
>+ if ((ret = read_in_full_timeout(fd, buf, len, timeout)) < 0)
>+ return NULL;
Do we want a packet_trace here?
When a timeout occurs, do we want to close the connection,
marking it as dead?
Or need to look at errno?
>+ buf[len] = '\0';
>+ if (len_p)
>+ *len_p = len;
>+ packet_trace(buf, len, 0);
>+ return buf;
>+}
>diff --git a/pkt-line.h b/pkt-line.h
>index 4b93a0c..d47dca5 100644
>--- a/pkt-line.h
>+++ b/pkt-line.h
>@@ -69,6 +69,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
> * packet is written to it.
> */
> char *packet_read_line(int fd, int *size);
>+char *packet_read_line_timeout(int fd, int timeout, int *size);
>
> /*
> * Same as packet_read_line, but read from a buf rather than a descriptor;
>diff --git a/wrapper.c b/wrapper.c
>index 9a0e289..9cf10b2 100644
>--- a/wrapper.c
>+++ b/wrapper.c
>@@ -193,6 +193,27 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
> return total;
> }
>
>+ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout)
>+{
>+ char *p = buf;
>+ ssize_t total = 0;
>+ struct pollfd pfd;
>+
>+ pfd.fd = fd;
>+ pfd.events = POLLIN;
>+ while (count > 0 && poll(&pfd, 1, timeout) > 0 &&
>+ (pfd.revents & POLLIN)) {
>+ ssize_t loaded = xread(fd, p, count);
>+ if (loaded <= 0)
>+ return -1;
>+ count -= loaded;
>+ p += loaded;
>+ total += loaded;
>+ }
>+
>+ return count ? -1 : total;
Isn't it that
ret < 0 means "error of some kind"
ret == 0 means EOF,
ret > 0 means some data
Why do we turn 0 into -1?
--------------
8)
+++ b/unix-socket.c
@@ -93,7 +93,7 @@ fail:
return -1;
}
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path, int replace)
{
int fd, saved_errno;
struct sockaddr_un sa;
@@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
return -1;
fd = unix_stream_socket();
- unlink(path);
+ if (replace)
+ unlink(path);
if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
Why do we call the parameter replace, when it does an
unlink() ?
s/replace/un_link/ ?
9)
>Access to the unix socket $WATCHER/socket is covered by $WATCHER's
>permission. While the file watcher does not carry much information,
>repo file listing is sometimes not something you want other users to
>see. Make sure $WATCHER has 0700 permission to stop unwanted access.
>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> file-watcher.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
I feel a little bit unsure about the 700.
Most often Git does not care about permissions,
and relies on umask being set appropriatly.
(Please correct me if I'm wrong)
My spontanous feeling is that adjust_shared_perm() could be used.
10)
An other thing:
>strbuf_addf(&sb, "%s/socket", socket_path);
Does it make sense to name the socket "%s/watcher" ?
11)
>In daemon mode, stdout and stderr are saved in $WATCHER/log.
[]
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-file-watcher.txt | 2 ++
cache.h | 1 +
daemon.c | 30 ++++--------------------------
file-watcher.c | 17 +++++++++++++++++
setup.c | 25 +++++++++++++++++++++++++
5 files changed, 49 insertions(+), 26 deletions(-)
diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
index 625a389..ec81f18 100644
--- a/Documentation/git-file-watcher.txt
+++ b/Documentation/git-file-watcher.txt
@@ -18,6 +18,8 @@ lstat(2) to detect that itself.
OPTIONS
-------
+--detach::
+ Run in background.
Shouldn't that be named --daemon ?
12)
>read_cache() connects to the file watcher, specified by
>filewatcher.path config, and performs basic hand shaking. CE_WATCHED
>is cleared if git and file watcher have different views on the index
>state.
>
>All send/receive calls must be complete within a limited time to avoid
>a buggy file-watcher hang "git status" forever. And the whole point of
>doing this is speed. If file watcher can't respond fast enough, for
>whatever reason, then it should not be used.
>
>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>---
> Documentation/config.txt | 10 +++
> Documentation/git-file-watcher.txt | 4 +-
> Makefile | 1 +
> cache.h | 1 +
> file-watcher-lib.c (new) | 91 ++++++++++++++++++++++
> file-watcher-lib.h (new) | 6 ++
> file-watcher.c | 152 ++++++++++++++++++++++++++++++++++++-
> read-cache.c | 6 ++
> 8 files changed, 269 insertions(+), 2 deletions(-)
> create mode 100644 file-watcher-lib.c
> create mode 100644 file-watcher-lib.h
>
>diff --git a/Documentation/config.txt b/Documentation/config.txt
>index 5f4d793..6ad653a 100644
>--- a/Documentation/config.txt
>+++ b/Documentation/config.txt
>@@ -1042,6 +1042,16 @@ difftool.<tool>.cmd::
> difftool.prompt::
> Prompt before each invocation of the diff tool.
>
>+filewatcher.path::
>+ The directory that contains the socket of `git file-watcher`.
>+ If it's not an absolute path, it's relative to $GIT_DIR. An
>+ empty path means no connection to file watcher.
I would really like to be able to have different tranport mechanismen
using the same config.
Today this is only a matter of documentation and naming things:
How about this idea:
filewatcher.url::
The directory that contains the socket of `git file-watcher`.
It can be an absolute path, meaning a unix domain socket in the
file system.
It can be a path relative to $GIT_DIR.
It must start with either
"/" (absolut path) or
"./" (relative path to $GIT_DIR) or
"~/" (relaive path in your $HOME directoy.
An empty url means no connection to file watcher.
(Later we can add url schemes like "tcp://" or "pipe://")
>+
>+filewatcher.timeout::
>+ This is the maximum time in milliseconds that Git waits for
>+ the file watcher to respond before giving up. Default value is
>+ 50. Setting to -1 makes Git wait forever.
50 feels low, especially on "older/slower machines"
200 is probably acceptable even for the very impatient user,
I could think of 500 to make the user aware of it.
[]
>+ /*
>+ * ">" denotes an incoming packet, "<" outgoing. The lack of
>+ * "<" means no reply expected.
>+ *
>+ * < "error" SP ERROR-STRING
>+ *
>+ * This can be sent whenever the client violates the protocol.
>+ */
>+
>+ msg = packet_read_line(fd, &len);
>+ if (!msg) {
>+ packet_write(fd, "error invalid pkt-line");
>+ return shutdown_connection(conn_id);
>+ }
>+
>+ /*
>+ * > "hello" [SP CAP [SP CAP..]]
>+ * < "hello" [SP CAP [SP CAP..]]
>+ *
>+ * Advertise capabilities of both sides. File watcher may
>+ * disconnect if the client does not advertise the required
>+ * capabilities. Capabilities in uppercase MUST be
>+ * supported. If any side does not understand any of the
>+ * advertised uppercase capabilities, it must disconnect.
>+ */
>+ if ((arg = skip_prefix(msg, "hello"))) {
>+ if (*arg) { /* no capabilities supported yet */
>+ packet_write(fd, "error capabilities not supported");
>+ return shutdown_connection(conn_id);
>+ }
>+ packet_write(fd, "hello");
>+ conns[conn_id]->polite = 1;
>+ }
>+
>+ /*
>+ * > "index" SP INDEX-SIGNATURE SP WORK-TREE-PATH
>+ * < "ok" | "inconsistent"
>+ *
>+ * INDEX-SIGNATURE consists of 40 hexadecimal letters
>+ * WORK-TREE-PATH must be absolute and normalized path
>+ *
>+ * Watch file changes in index. The client sends the index and
>+ * work tree info. File watcher validates that it holds the
>+ * same info. If so it sends "ok" back indicating both sides
>+ * are on the same page and CE_WATCHED bits can be ketpt.
>+ *
>+ * Otherwise it sends "inconsistent" and both sides must reset
>+ * back to initial state. File watcher keeps its index
>+ * signature all-zero until the client has updated the index
>+ * ondisk and request to update index signature.
>+ *
>+ * "hello" must be exchanged first. After this command the
>+ * connection is associated with a worktree/index. Many
>+ * commands may require this to proceed.
>+ */
>+ else if (starts_with(msg, "index ")) {
>+ struct repository *repo;
>+ struct stat st;
>+ if (!conns[conn_id]->polite) {
>+ packet_write(fd, "error why did you not greet me? go away");
>+ return shutdown_connection(conn_id);
>+ }
>+ if (len < 47 || msg[46] != ' ' || !is_absolute_path(msg + 47)) {
>+ packet_write(fd, "error invalid index line %s", msg);
>+ return shutdown_connection(conn_id);
>+ }
>+
>+ if (lstat(msg + 47, &st) || !S_ISDIR(st.st_mode)) {
>+ packet_write(fd, "error work tree does not exist: %s",
>+ strerror(errno));
>+ return shutdown_connection(conn_id);
>+ }
>+ repo = get_repo(msg + 47);
>+ conns[conn_id]->repo = repo;
>+ if (memcmp(msg + 6, repo->index_signature, 40) ||
>+ !memcmp(msg + 6, invalid_signature, 40) ||
>+ repo->inode != st.st_ino) {
>+ packet_write(fd, "inconsistent");
>+ reset_repo(repo, st.st_ino);
>+ return 0;
>+ }
>+ packet_write(fd, "ok");
>+ }
>+ else {
>+ packet_write(fd, "error unrecognized command %s", msg);
>+ return shutdown_connection(conn_id);
I feel a little bit unsure about this.
We establish a protocol which is not extendable.
Do we need to call shutdown_connection() ?
(And as I noticed earlier, close_connection() could be a better name)
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH v3 00/26] inotify support
2014-02-08 8:04 ` [PATCH v3 00/26] inotify support Torsten Bögershausen
@ 2014-02-08 8:53 ` Duy Nguyen
2014-02-09 20:19 ` Torsten Bögershausen
0 siblings, 1 reply; 72+ messages in thread
From: Duy Nguyen @ 2014-02-08 8:53 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
Thanks for the comments. I can see I now have some work to do in the
coming weeks :)
On Sat, Feb 8, 2014 at 3:04 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> I would appreciate if we could have an outline of the protocol
> as a seperate "document" somewhere, to be able to have a look at the protocol
> first, before looking into the code.
My fear is document becomes outdated because people don't always
remember to update doc when they change code. Which is why I embed the
protocol as comments in handle_command() function. If the final
version [1] is still not easy to read, I'll split the protocol out
into a separate document.
[1] https://github.com/pclouds/git/blob/file-watcher/file-watcher.c#L672
> (Am I using wireshark too much to dream about a dissector ?)
Try GIT_TRACE_PACKET=2
> 1)
> write_in_full_timeout()
> packet_read_line_timeout()
> At other places we handle EINTR after calling poll().
> Looking at the code, it could be easier to introduce
> a new function xpoll() in wrapper.c, and use that instead
> of poll().
Yeah there are already 4 poll call sites before file-watcher jumps in. Will do.
> 2)
> Similar for the usage of accept().
> I like the idea of xread() xwrite() and all the x functions
> so it coud make sense to establish xpoll() and xaccept()
> before inotify suppport.
OK
> 3)
>> -int unix_stream_listen(const char *path)
>> +int unix_stream_listen(const char *path, int replace)
>> {
>> int fd, saved_errno;
>> struct sockaddr_un sa;
>> @@ -103,7 +103,8 @@ int unix_stream_listen(const char *path)
>> return -1;
>> fd = unix_stream_socket();
>>
>> - unlink(path);
>> + if (replace)
>> + unlink(path);
>
> Minor remark:
> As we do not do the replace here:
> s/replace/un_link/ may be ?
Heh, I thought of using the name "unlink" but it's taken so I chose
"replace" and did not think of underscore.Will do.
> 4)
>> +++ b/file-watcher.c
> {}
>> +static const char *const file_watcher_usage[] = {
>> + N_("git file-watcher [options] <socket directory>"),
>> + NULL
>> +};
> Do we already have options here?
> I can think about having
> -d daemoniye
> -u uses Unix doain socket
> (And later -t to use a TCP/IP socket, when the repo
> is on a mounted NFS (or SAMBA) drive, and the daemon is on a
> different machine.
> I don't say this patch should include this logic in first round,
> But I can see a gain for this kind of setup)
Later on we have two, --detach (i.e. daemonize, I reuse the same name
from git-daemon) and --check-support. Transport settings (like unix vs
tcp/ip...) should be config options though. You don't want to specify
it here, then again when you run "git status". Actually I should add
--default, that will retrieve <socket directory> from config key
filewatcher.path, so the user does not have to specify it..
> 5)
>> +++ b/file-watcher.c
> []
>> +static int shutdown_connection(int id)
>> +{
>> + struct connection *conn = conns[id];
>> + conns[id] = NULL;
>> + pfd[id].fd = -1;
>> + if (!conn)
>> + return 0;
>> + close(conn->sock);
>> + free(conn);
>> + return 0;
>> +}
> The function is called shutdown_connection(), but it does a close()
> Could it be named close_connection() ?
Yes, there was a close_connection() which did something similar, and
then it was killed off. Will rename.
> 6)
>> +++ b/file-watcher.c
> []
> Do we have a sequence chart about the command flow between the watcher
> daemon and the client ?
I suggest you have a look at the file-watcher test. For example, from
[2] we have this sequence
<hello
>hello
<index $SIG $TRASH_DIRECTORY
>inconsistent
# Do not call inotify_add_watch()
<test-mode
>test mode on
# First batch should be all ok
<<watch
<<dir1/foo
<<dir2/foo
<<dir3/foo
<<dir4/foo
...
"<" denotes a packet from the client to file-watcher, ">" the opposite
direction. But you can always obtain a real flow with
GIT_TRACE_PACKET=2 (path lists not available though, so really just a
flow).
[2] https://github.com/pclouds/git/blob/file-watcher/t/t7513-file-watcher.sh#L273
> in 03/26:
>>This version is also gentler than its friend packet_read_line()
> gentler, what does this mean?
No dying for whatever error. packet_read_line is designed for git
protocol. It's the main job so if there's an error, dying is the right
thing to do. file-watcher on the other hand is a side job and should
not stop whatever command from doing. Will update commit message.
>>because it's designed for side channel I/O that should not abort the
>>program even if the channel is broken.
> I'm not so familar with side-channel I/O. How does it fit in here?
To sum up, whatever error in communication with file-watcher should
not stop you from doing whatever you're doing. file-watcher is
contacted whenever $GIT_DIR/index is read, so "whatever you're doing"
is basically all git commands that involve worktree or index.
> Does this make sense:
> In opposite to packet_read_line() which can call die()
> to abort the program, read_in_full_timeout() will keep the program running.
> (or something like this)
Exactly!
>>diff --git a/cache.h b/cache.h
>>index 718e32b..939db46 100644
>>--- a/cache.h
>>+++ b/cache.h
>>@@ -1230,6 +1230,7 @@ extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
>> extern void fsync_or_die(int fd, const char *);
>>
>> extern ssize_t read_in_full(int fd, void *buf, size_t count);
>>+extern ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout);
>> extern ssize_t write_in_full(int fd, const void *buf, size_t count);
>> extern ssize_t write_in_full_timeout(int fd, const void *buf, size_t count, int timeout);
>> static inline ssize_t write_str_in_full(int fd, const char *str)
>>diff --git a/pkt-line.c b/pkt-line.c
>>index cf681e9..5a07e97 100644
>>--- a/pkt-line.c
>>+++ b/pkt-line.c
>>@@ -229,3 +229,38 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
>> {
>> return packet_read_line_generic(-1, src, src_len, dst_len);
>> }
>>+
>
> In what is the timeout measured ?
> seconds, milli years?
> As we use poll() I think it is milli seconds.
> (I like the idea of naming timeout timeout_ms)
Yes in ms. Will rename.
>>+ len -= 4;
>>+ if (len >= buf_len) {
>>+ error("protocol error: bad line length %d", len);
>>+ return NULL;
>>+ }
>>+ if ((ret = read_in_full_timeout(fd, buf, len, timeout)) < 0)
>>+ return NULL;
> Do we want a packet_trace here?
Compared to packet_read_line(), probably not.
> When a timeout occurs, do we want to close the connection,
> marking it as dead?
> Or need to look at errno?
We do want to close the connection, but the caller should do that and
clear the file handle variable at the same time. Or we risk race
condition.
>>diff --git a/wrapper.c b/wrapper.c
>>index 9a0e289..9cf10b2 100644
>>--- a/wrapper.c
>>+++ b/wrapper.c
>>@@ -193,6 +193,27 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
>> return total;
>> }
>>
>>+ssize_t read_in_full_timeout(int fd, void *buf, size_t count, int timeout)
>>+{
>>+ char *p = buf;
>>+ ssize_t total = 0;
>>+ struct pollfd pfd;
>>+
>>+ pfd.fd = fd;
>>+ pfd.events = POLLIN;
>>+ while (count > 0 && poll(&pfd, 1, timeout) > 0 &&
>>+ (pfd.revents & POLLIN)) {
>>+ ssize_t loaded = xread(fd, p, count);
>>+ if (loaded <= 0)
>>+ return -1;
>>+ count -= loaded;
>>+ p += loaded;
>>+ total += loaded;
>>+ }
>>+
>>+ return count ? -1 : total;
> Isn't it that
> ret < 0 means "error of some kind"
> ret == 0 means EOF,
> ret > 0 means some data
> Why do we turn 0 into -1?
Because this is read_in_full. The caller expects either all is read, or errors.
> 9)
>>Access to the unix socket $WATCHER/socket is covered by $WATCHER's
>>permission. While the file watcher does not carry much information,
>>repo file listing is sometimes not something you want other users to
>>see. Make sure $WATCHER has 0700 permission to stop unwanted access.
>
>>Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>---
>> file-watcher.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>
> I feel a little bit unsure about the 700.
> Most often Git does not care about permissions,
> and relies on umask being set appropriatly.
> (Please correct me if I'm wrong)
Git does care. See credential-cache--daemon.c. In fact this function
is a copy of check_socket_directory() from that file.
> My spontanous feeling is that adjust_shared_perm() could be used.
>
> 10)
> An other thing:
>>strbuf_addf(&sb, "%s/socket", socket_path);
> Does it make sense to name the socket "%s/watcher" ?
No strong opinions. Whatever name works for me. Will change unless
somebody else raises their voice.
> 11)
>>In daemon mode, stdout and stderr are saved in $WATCHER/log.
Mostly for debugging purposes.
> diff --git a/Documentation/git-file-watcher.txt b/Documentation/git-file-watcher.txt
> index 625a389..ec81f18 100644
> --- a/Documentation/git-file-watcher.txt
> +++ b/Documentation/git-file-watcher.txt
> @@ -18,6 +18,8 @@ lstat(2) to detect that itself.
>
> OPTIONS
> -------
> +--detach::
> + Run in background.
>
> Shouldn't that be named --daemon ?
I just followed the naming convention in git-daemon. I think detach is
more friendly then daemon anyway.
> 12)
>>+filewatcher.path::
>>+ The directory that contains the socket of `git file-watcher`.
>>+ If it's not an absolute path, it's relative to $GIT_DIR. An
>>+ empty path means no connection to file watcher.
> I would really like to be able to have different tranport mechanismen
> using the same config.
> Today this is only a matter of documentation and naming things:
>
> How about this idea:
> filewatcher.url::
> The directory that contains the socket of `git file-watcher`.
> It can be an absolute path, meaning a unix domain socket in the
> file system.
> It can be a path relative to $GIT_DIR.
>
> It must start with either
> "/" (absolut path) or
> "./" (relative path to $GIT_DIR) or
> "~/" (relaive path in your $HOME directoy.
> An empty url means no connection to file watcher.
> (Later we can add url schemes like "tcp://" or "pipe://")
Makes sense.
>>+filewatcher.timeout::
>>+ This is the maximum time in milliseconds that Git waits for
>>+ the file watcher to respond before giving up. Default value is
>>+ 50. Setting to -1 makes Git wait forever.
> 50 feels low, especially on "older/slower machines"
> 200 is probably acceptable even for the very impatient user,
> I could think of 500 to make the user aware of it.
Note that this timeout should never kick in unless there is an error
in communication. So in practice it should not matter much. OK I lied
a bit because file-watcher hard codes to "ping" the client every 30ms
to avoid this timeout. So it does matter, but very little.
>>+ /*
>>+ * ">" denotes an incoming packet, "<" outgoing. The lack of
>>+ * "<" means no reply expected.
>>+ *
>>+ * < "error" SP ERROR-STRING
>>+ *
>>+ * This can be sent whenever the client violates the protocol.
>>+ */
>>+
>>+ msg = packet_read_line(fd, &len);
>>+ if (!msg) {
>>+ packet_write(fd, "error invalid pkt-line");
>>+ return shutdown_connection(conn_id);
>>+ }
>>+
>>+ /*
>>+ * > "hello" [SP CAP [SP CAP..]]
>>+ * < "hello" [SP CAP [SP CAP..]]
>>+ *
>>+ * Advertise capabilities of both sides. File watcher may
>>+ * disconnect if the client does not advertise the required
>>+ * capabilities. Capabilities in uppercase MUST be
>>+ * supported. If any side does not understand any of the
>>+ * advertised uppercase capabilities, it must disconnect.
>>+ */
>>+ if ((arg = skip_prefix(msg, "hello"))) {
>>+ if (*arg) { /* no capabilities supported yet */
>>+ packet_write(fd, "error capabilities not supported");
>>+ return shutdown_connection(conn_id);
>>+ }
>>+ packet_write(fd, "hello");
>>+ conns[conn_id]->polite = 1;
>>+ }
>>+
>>+ /*
>>+ * > "index" SP INDEX-SIGNATURE SP WORK-TREE-PATH
>>+ * < "ok" | "inconsistent"
>>+ *
>>+ * INDEX-SIGNATURE consists of 40 hexadecimal letters
>>+ * WORK-TREE-PATH must be absolute and normalized path
>>+ *
>>+ * Watch file changes in index. The client sends the index and
>>+ * work tree info. File watcher validates that it holds the
>>+ * same info. If so it sends "ok" back indicating both sides
>>+ * are on the same page and CE_WATCHED bits can be ketpt.
>>+ *
>>+ * Otherwise it sends "inconsistent" and both sides must reset
>>+ * back to initial state. File watcher keeps its index
>>+ * signature all-zero until the client has updated the index
>>+ * ondisk and request to update index signature.
>>+ *
>>+ * "hello" must be exchanged first. After this command the
>>+ * connection is associated with a worktree/index. Many
>>+ * commands may require this to proceed.
>>+ */
>>+ else if (starts_with(msg, "index ")) {
>>+ struct repository *repo;
>>+ struct stat st;
>>+ if (!conns[conn_id]->polite) {
>>+ packet_write(fd, "error why did you not greet me? go away");
>>+ return shutdown_connection(conn_id);
>>+ }
>>+ if (len < 47 || msg[46] != ' ' || !is_absolute_path(msg + 47)) {
>>+ packet_write(fd, "error invalid index line %s", msg);
>>+ return shutdown_connection(conn_id);
>>+ }
>>+
>>+ if (lstat(msg + 47, &st) || !S_ISDIR(st.st_mode)) {
>>+ packet_write(fd, "error work tree does not exist: %s",
>>+ strerror(errno));
>>+ return shutdown_connection(conn_id);
>>+ }
>>+ repo = get_repo(msg + 47);
>>+ conns[conn_id]->repo = repo;
>>+ if (memcmp(msg + 6, repo->index_signature, 40) ||
>>+ !memcmp(msg + 6, invalid_signature, 40) ||
>>+ repo->inode != st.st_ino) {
>>+ packet_write(fd, "inconsistent");
>>+ reset_repo(repo, st.st_ino);
>>+ return 0;
>>+ }
>>+ packet_write(fd, "ok");
>>+ }
>>+ else {
>>+ packet_write(fd, "error unrecognized command %s", msg);
>>+ return shutdown_connection(conn_id);
> I feel a little bit unsure about this.
> We establish a protocol which is not extendable.
It should be extensible via "hello" message, as described in the
comments above it.
> Do we need to call shutdown_connection() ?
If you receive an unrecognized command, there's not much you can do
but shut the connection down.
--
Duy
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH v3 00/26] inotify support
2014-02-08 8:53 ` Duy Nguyen
@ 2014-02-09 20:19 ` Torsten Bögershausen
2014-02-10 10:37 ` Duy Nguyen
2014-02-17 12:36 ` Duy Nguyen
0 siblings, 2 replies; 72+ messages in thread
From: Torsten Bögershausen @ 2014-02-09 20:19 UTC (permalink / raw)
To: Duy Nguyen, Torsten Bögershausen; +Cc: Git Mailing List
On 2014-02-08 09.53, Duy Nguyen wrote:
> Thanks for the comments. I can see I now have some work to do in the
> coming weeks :)
>
--------------------
>>> file-watcher.c | 32 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>
>> I feel a little bit unsure about the 700.
>> Most often Git does not care about permissions,
>> and relies on umask being set appropriatly.
>> (Please correct me if I'm wrong)
>
>Git does care. See credential-cache--daemon.c. In fact this function
>is a copy of check_socket_directory() from that file.
>
I was probably a little bit unclear.
Of course credentials should be protected well and stored with 700.
The rest of the repo could be more loose by using adjust_shared_perm().
Because the whole repo can be shared (or not) and data is visible
to the group or everyone.
(this is a minor issue)
Please see filewatcher.c:
+ if (daemon) {
+ int err;
+ strbuf_addf(&sb, "%s/log", socket_path);
+ err = open(sb.buf, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+ adjust_shared_perm(sb.buf);
(And now we talk about the logfile:
"In daemon mode, stdout and stderr are saved in $WATCHER/log."
It could be nice to make this feature configrable,
either XXX/log or /dev/null.
On the long run we may eat to much disc space on a machine.
The other thing is that we may want to seperate stdout
from stderr, but even this is a low prio comment.
----------------
There is a small issue when I tested on a machine,
where the "data directory" called "daten" is softlinked to another disk:
daten -> /disk3/home2/tb/daten
and the "projects" directory is softlinked to "daten/projects"
projects -> daten/projects/
t7514 fails like this:
--- expect 2014-02-08 14:37:07.000000000 +0000
+++ actual 2014-02-08 14:37:07.000000000 +0000
@@ -1,6 +1,6 @@
packet: git> hello
packet: git< hello
-packet: git> index 6cb9741eee29ca02c5b79e9c0bc647bcf47ce948 /home/tb/projects/git/tb/t/trash directory.t7514-file-watcher-lib
+packet: git> index 6cb9741eee29ca02c5b79e9c0bc647bcf47ce948 /disk3/home2/tb/daten/projects/git/tb/t/trash directory.t7514-file-watcher-lib
Could we use relative path names internally, relative to $GIT_DIR ?
-------------------
Another thing:
Compiling under Mingw gives this:
LINK git-credential-store.exe
libgit.a(file-watcher-lib.o): In function `connect_watcher':
c:\Dokumente und Einstellungen\tb\projects\git\tb/file-watcher-lib.c:21: undefined reference to `unix_stream_connect'
collect2: ld returned 1 exit status
make: *** [git-credential-store.exe] Error 1
We may need a compiler option like HAS_UNIX_SOCKETS or so.
--------------------------
+++ b/file-watcher.c
+#define INOTIFY_MASKS (IN_DELETE_SELF | IN_MOVE_SELF | \
+ IN_CREATE | IN_ATTRIB | IN_DELETE | IN_MODIFY | \
+ IN_MOVED_FROM | IN_MOVED_TO)
This feels confusing:
a) we have inotify_masks with lower case below.
b) how about INOTIFY_NEEDED_BITS ?
---------------
I'm OK with having the protocol having specified in the
test cases.
One thing that I have on the wish list is to make the
commands/responses more unique, to be able to run grep
on the code base.
One idea could be to use a prefix
"fwr" for "file watcher request" or
"fwr" for "file watcher response".
This does not work, hihi, so
"fwq" for "file watcher reQuest" and
"fwe" for "file watcher rEsponse".
Or
"ffw" as "from file watcher" and
"tfw" as "to file watcher" for the people who have problems
with left and right, < and > could work.
This is all for today.
I will have a look at different error scenarios, what happens
when the watcher crashes and needs to be restarted,
or when Git itself dies with a segfault and doesn't tell the
watcher.
The easiest way to simulate this would be in terms of test cases.
So I will try to write some
/Torsten
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH v3 00/26] inotify support
2014-02-09 20:19 ` Torsten Bögershausen
@ 2014-02-10 10:37 ` Duy Nguyen
2014-02-10 16:55 ` Torsten Bögershausen
2014-02-17 12:36 ` Duy Nguyen
1 sibling, 1 reply; 72+ messages in thread
From: Duy Nguyen @ 2014-02-10 10:37 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
On Mon, Feb 10, 2014 at 3:19 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> Please see filewatcher.c:
> + if (daemon) {
> + int err;
> + strbuf_addf(&sb, "%s/log", socket_path);
> + err = open(sb.buf, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> + adjust_shared_perm(sb.buf);
> (And now we talk about the logfile:
> "In daemon mode, stdout and stderr are saved in $WATCHER/log."
> It could be nice to make this feature configrable,
> either XXX/log or /dev/null.
> On the long run we may eat to much disc space on a machine.
> The other thing is that we may want to seperate stdout
> from stderr, but even this is a low prio comment.
I probably should follow git-daemon and put these to syslog.
> ----------------
> There is a small issue when I tested on a machine,
> where the "data directory" called "daten" is softlinked to another disk:
> daten -> /disk3/home2/tb/daten
>
> and the "projects" directory is softlinked to "daten/projects"
> projects -> daten/projects/
>
> t7514 fails like this:
> --- expect 2014-02-08 14:37:07.000000000 +0000
> +++ actual 2014-02-08 14:37:07.000000000 +0000
> @@ -1,6 +1,6 @@
> packet: git> hello
> packet: git< hello
> -packet: git> index 6cb9741eee29ca02c5b79e9c0bc647bcf47ce948 /home/tb/projects/git/tb/t/trash directory.t7514-file-watcher-lib
> +packet: git> index 6cb9741eee29ca02c5b79e9c0bc647bcf47ce948 /disk3/home2/tb/daten/projects/git/tb/t/trash directory.t7514-file-watcher-lib
>
> Could we use relative path names internally, relative to $GIT_DIR ?
No because this is when the client tell the server about $GIT_DIR. I
guess we can use realpath(1) here.
> -------------------
> Another thing:
> Compiling under Mingw gives this:
> LINK git-credential-store.exe
> libgit.a(file-watcher-lib.o): In function `connect_watcher':
> c:\Dokumente und Einstellungen\tb\projects\git\tb/file-watcher-lib.c:21: undefined reference to `unix_stream_connect'
> collect2: ld returned 1 exit status
> make: *** [git-credential-store.exe] Error 1
>
> We may need a compiler option like HAS_UNIX_SOCKETS or so.
I'll make unix-socket.o build unconditionally and return error at runtime.
> --------------------------
> +++ b/file-watcher.c
>
> +#define INOTIFY_MASKS (IN_DELETE_SELF | IN_MOVE_SELF | \
> + IN_CREATE | IN_ATTRIB | IN_DELETE | IN_MODIFY | \
> + IN_MOVED_FROM | IN_MOVED_TO)
> This feels confusing:
> a) we have inotify_masks with lower case below.
> b) how about INOTIFY_NEEDED_BITS ?
> ---------------
OK
> I'm OK with having the protocol having specified in the
> test cases.
> One thing that I have on the wish list is to make the
> commands/responses more unique, to be able to run grep
> on the code base.
>
> One idea could be to use a prefix
> "fwr" for "file watcher request" or
> "fwr" for "file watcher response".
> This does not work, hihi, so
>
> "fwq" for "file watcher reQuest" and
> "fwe" for "file watcher rEsponse".
> Or
> "ffw" as "from file watcher" and
> "tfw" as "to file watcher" for the people who have problems
> with left and right, < and > could work.
If you want I can update test-file-watcher to accept "send<" and
"recv>" instead of "<" and ">", respectively. The only command with
the same name for response and request is "hello". I can make it
"hello" and "helloack" (or "bonjour" as response?).
--
Duy
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH v3 00/26] inotify support
2014-02-10 10:37 ` Duy Nguyen
@ 2014-02-10 16:55 ` Torsten Bögershausen
2014-02-10 23:34 ` Duy Nguyen
0 siblings, 1 reply; 72+ messages in thread
From: Torsten Bögershausen @ 2014-02-10 16:55 UTC (permalink / raw)
To: Duy Nguyen, Torsten Bögershausen; +Cc: Git Mailing List
On 2014-02-10 11.37, Duy Nguyen wrote:
>>
>> Could we use relative path names internally, relative to $GIT_DIR ?
>
> No because this is when the client tell the server about $GIT_DIR. I
> guess we can use realpath(1) here.
Good.
I realized that the watcher can watch several repos at the same time.
However, we could allow relative path names, which will be relative to $SOCKET_DIR,
and loosen the demand for an absolut path name a little bit.
And $SOCKET_DIR can be the same as $GIT_DIR, when we are watching only one repo.
> If you want I can update test-file-watcher to accept "send<" and
> "recv>" instead of "<" and ">", respectively. The only command with
> the same name for response and request is "hello". I can make it
> "hello" and "helloack" (or "bonjour" as response?).
helloack looks good.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 00/26] inotify support
2014-02-10 16:55 ` Torsten Bögershausen
@ 2014-02-10 23:34 ` Duy Nguyen
0 siblings, 0 replies; 72+ messages in thread
From: Duy Nguyen @ 2014-02-10 23:34 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
On Mon, Feb 10, 2014 at 11:55 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2014-02-10 11.37, Duy Nguyen wrote:
>>>
>>> Could we use relative path names internally, relative to $GIT_DIR ?
>>
>> No because this is when the client tell the server about $GIT_DIR. I
>> guess we can use realpath(1) here.
> Good.
>
> I realized that the watcher can watch several repos at the same time.
>
> However, we could allow relative path names, which will be relative to $SOCKET_DIR,
> and loosen the demand for an absolut path name a little bit.
> And $SOCKET_DIR can be the same as $GIT_DIR, when we are watching only one repo.
It does not help much anyway because file-watcher-lib.c sends
get_git_work_tree(), which is absolute/normalized path, to
file-watcher. There's no sources of sending $GIT_DIR relative to
$SOCKET_DIR (and I don't think we want to make get_git_work_tree()
relative before sending, it's more work on both sides we no benefits,
except for tracing).
--
Duy
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 00/26] inotify support
2014-02-09 20:19 ` Torsten Bögershausen
2014-02-10 10:37 ` Duy Nguyen
@ 2014-02-17 12:36 ` Duy Nguyen
1 sibling, 0 replies; 72+ messages in thread
From: Duy Nguyen @ 2014-02-17 12:36 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Git Mailing List
On Mon, Feb 10, 2014 at 3:19 AM, Torsten Bögershausen <tboegi@web.de> wrote:
>
> On 2014-02-08 09.53, Duy Nguyen wrote:
>>>> file-watcher.c | 32 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 32 insertions(+)
>>>
>>> I feel a little bit unsure about the 700.
>>> Most often Git does not care about permissions,
>>> and relies on umask being set appropriatly.
>>> (Please correct me if I'm wrong)
>>
>>Git does care. See credential-cache--daemon.c. In fact this function
>>is a copy of check_socket_directory() from that file.
>>
> I was probably a little bit unclear.
> Of course credentials should be protected well and stored with 700.
> The rest of the repo could be more loose by using adjust_shared_perm().
> Because the whole repo can be shared (or not) and data is visible
> to the group or everyone.
> (this is a minor issue)
So how about a check whenever a worktree is connected to the daemon,
if that worktree has stricter permission, e.g. 0700 vs 0770 of the
daemon socket directory, then the daemon refuses the worktree (maybe
with a warning)?
--
Duy
^ permalink raw reply [flat|nested] 72+ messages in thread