Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 13, 2016, 13:43 UTC
Message-ID: <CACsJy8DE40dMiiqkTb=Pz8uidPk-q1-kuX514s7mO55ChFLXhg@mail.gmail.com>
In-Reply-To: <1460507589-25525-4-git-send-email-dturner@twopensource.com>

On Wed, Apr 13, 2016 at 7:32 AM, David Turner <dturner@twopensource.com> wrote:

> +NOTES
> +-----
> +
> +$GIT_DIR/index-helper.path is a symlink

In multiple worktree context, this file will be per-worktree. So we have one daemon per worktree. I think that's fine.

> to a directory in $TMPDIR
> +containing a Unix domain socket called 's' that the daemon reads
> +commands from.

Oops. I stand corrected, now it's one daemon per repository... Probably good to hide the socket path in $GIT_DIR though, people may protect it with dir permission of one of ancestor directories.

Show 6 quoted lines

> The directory will also contain files named
> +"git-index-<SHA1>".  These are used as backing stores for shared
> +memory.  Normally the daemon will clean up these files when it exits
> +or when they are no longer relevant.  But if it crashes, some objects
> +could remain there and they can be safely deleted with "rm"
> +command.

Alternatively, we could store all these in $GIT_DIR/helper or something and clean up automatically when index-helper starts. But I guess at least with TMPDIR we have a chance to put them on tmpfs.

Show 25 quoted lines

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c07e0c1..8b878fe 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1045,4 +1046,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
>  #define getc_unlocked(fh) getc(fh)
>  #endif
>
> +#ifdef __linux__
> +#define UNIX_PATH_MAX 108
> +#elif defined(__APPLE__) || defined(BSD)
> +#define UNIX_PATH_MAX 104
> +#else
> +/*
> + * Quoth POSIX: The size of sun_path has intentionally been left
> + * undefined. This is because different implementations use different
> + * sizes. For example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a
> + * size of 104. Since most implementations originate from BSD
> + * versions, the size is typically in the range 92 to 108.
> + *
> + * Thanks, POSIX!  Super-helpful!  Hope we don't overflow any buffers!
> + */
> +#define UNIX_PATH_MAX 92
> +#endif
> +

This looks like dead code (or at least not used in this patch).

Show 18 quoted lines

> +static int poke_daemon(struct index_state *istate,
> +                      const struct stat *st, int refresh_cache)
> +{
> +       int fd;
> +       int ret = 0;
> +       const char *socket_path;
> +
> +       /* if this is from index-helper, do not poke itself (recursively) */
> +       if (istate->to_shm)
> +               return 0;
> +
> +       socket_path = index_helper_path("s");
> +       if (!socket_path)
> +               return -1;
> +
> +       fd = unix_stream_connect(socket_path);
> +       if (refresh_cache) {
> +               ret = write_in_full(fd, "refresh", 8) != 8;

Since we've moved to unix socket and had bidirectional communication, it's probably a good idea to read an "ok" back, giving index-helper time to prepare the cache. As I recall the last discussion with Johannes, missing a cache here when the index is around 300MB could hurt more than wait patiently once and have it ready next time.

Show 7 quoted lines

> +       } else {
> +               ret = write_in_full(fd, "poke", 5) != 5;
> +       }
> +
> +       close(fd);
> +       return ret;
> +}

-- 
Duy