Re: [PATCH v3 00/26] inotify support

["Torsten Bögershausen" <tboegi@web.de>]

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