Re: [PATCH v3 00/26] inotify support

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 10, 2014, 10:37 UTC
Message-ID: <CACsJy8A9JxDWhEWpdUUL_6tTJZvmf93Ga_nPt09yUzG44mc-Qg@mail.gmail.com>
In-Reply-To: <52F7E2BC.5030905@web.de>

On Mon, Feb 10, 2014 at 3:19 AM, Torsten Bögershausen <tboegi@web.de> wrote:

Show 13 quoted lines

> 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.

Show 18 quoted lines

> ----------------
> 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.

Show 10 quoted lines

> -------------------
> 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.

Show 10 quoted lines

> --------------------------
> +++ 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

Show 17 quoted lines

> 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