{"thread":{"id":"42814","subject":"[RFC] Long running Git clean/smudge filter","startedAt":"2016-07-10T11:35:21Z","lastAt":"2016-07-12T09:30:18Z","messageCount":3,"participants":["larsxschneider@gmail.com","Joey Hess","Lars Schneider"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291133","messageId":"1468150507-40928-1-git-send-email-larsxschneider@gmail.com","threadId":"42814","inReplyTo":null,"subject":"[RFC] Long running Git clean/smudge filter","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-10T11:35:07Z","receivedAt":"2016-07-10T11:35:21Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nGit's clean/smudge mechanism invokes an external filter process for every\nsingle file that is affected by the filter. If you have *a lot* of files to\nfilter then the startup time of the external filter process becomes a problem\nas discussed here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/298293\n\nThis patch might be a solution to this problem and I am looking for feedback.\nPlease note that this is a RFC/proof-of-concept patch. If the list agrees with\nthis approach then I will post a proper patch series with more tests, error\nhandling, and documentation.\n\n\n## How does it work?\nGit's \"convert.c\" got a hashmap that maps a filter command to a process. If\nwe need to filter a file, then we look into the hashmap. If the filter command\nis not there then we create and start a new filter process and setup in/out\npipes. If the filter command is there then we just take the existing and running\nprocess.\n\nIn case of a clean filter we send the filename of the file to clean via pipe\nto the filter process. The filter process reads the file, generates the cleaned\nversion and sends it back to Git via pipe.\n\nIn case of a smudge filter we send the destination filename of the file to\nsmudge via pipe as well as its content size and content. The filter process\nreads all this info, generates the smudged version and stores the smudged\nversion at the location of the destination filename. Finally, the filter\nprocess sends an \"OK\" to tell Git that everything worked as expected.\n\nThis patch is based on Joey Hess' great \"clean-smudge-annex\" topic, idea,\nand implementation:\nhttps://github.com/gitster/git/tree/jh/clean-smudge-annex\n\nv1: http://thread.gmane.org/gmane.comp.version-control.git/297475\nv4 (latest): http://thread.gmane.org/gmane.comp.version-control.git/297994\n\nYou can find a branch with with my patch on top of Joey's work here:\nhttps://github.com/larsxschneider/git/commits/clean-smudge-file-long-running/v1\n\n\n## Questions\n\n(1) Do you see a general problem with this kind of \"pipe\" protocol and long\nrunning Git filter processes? Other than the test cases I haven't implemented\na real world filter, yet (on my todo list for tomorrow).\n\n(2) Joey's topic, which is the base for my patch, looks stalled for more than\n2 weeks:\nhttp://thread.gmane.org/gmane.comp.version-control.git/297994/focus=298006\nI would be happy to address Junio's comments and post a reroll. However,\nI don't want to interfere with Joey's plans. How does the list usually\naddress this kind of situation?\n\n@Joey (in case you are reading this):\nMy patch changes your initial idea quite a bit. However, I believe it is an\nimprovement that could be beneficial for git-annex, too. Would you prefer to\nwork with me on the combination of our ideas (file clean/smudge + long running\nclean/smudge processes) or would you prefer to keep your interface?\n\n(3) Clean/smudge filter use Git's async command API to create a separate thread\nand start the filter process. In my implementation I start the filter process\ndirectly as this eases the pipe communication. I assume I am missing something\nobvious here, but what is the advantage of the async command API? Git needs to\nwait for the result of the filter anyways and therefore async is not really\nnecessary, or?\n\n(4) I am not really experienced with process pipes. Is it OK to use the\n\"strbuf_read_once\" function to read from a pipe? If I use \"strbuf_read\" then\nthe process waits indefinitely. Is there a way to tell Git to read X bytes from\na pipe (I just found the size \"hint\")?\n\n(5) As mentioned above \"convert.c\" got a hashmap that maps a filter command to\na process. At the end of the Git process I would like to close the pipes and\nstop the filter processes. What would be the best place in the code to do that?\n\nThanks,\nLars\n\n---\n convert.c             | 143 +++++++++++++++++++++++++++++++++++++++++++-------\n t/t0021-conversion.sh |  80 +++++++++++++++++++++-------\n 2 files changed, 185 insertions(+), 38 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex e4db007..590b85a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -373,9 +373,120 @@ struct filter_params {\n \tint fd;\n \tconst char *cmd;\n \tconst char *path; /* Path within the git repository */\n-\tconst char *fspath; /* Path to file on disk */\n };\n\n+static int cmd_process_map_init = 0;\n+static struct hashmap cmd_process_map;\n+\n+struct cmd2process {\n+\tstruct hashmap_entry ent; /* must be the first member! */\n+\tconst char *cmd;\n+\tstruct child_process process;\n+};\n+\n+static int cmd2process_cmp(const struct cmd2process *e1,\n+\t\t\t\t\t\t\tconst struct cmd2process *e2,\n+\t\t\t\t\t\t\tconst void *unused)\n+{\n+\treturn strcmp(e1->cmd, e2->cmd);\n+}\n+\n+static struct cmd2process *find_entry(const char *cmd)\n+{\n+\tstruct cmd2process k;\n+\thashmap_entry_init(&k, memhash(&cmd, sizeof(char *)));\n+\tk.cmd = cmd;\n+\treturn hashmap_get(&cmd_process_map, &k, NULL);\n+}\n+\n+static int apply_file_filter(const char *path, const char *src, size_t len,\n+\t\t\t\t\t\t\tstruct strbuf *dst, const char *cmd)\n+{\n+\tint ret = 1;\n+\tstruct strbuf nbuf = STRBUF_INIT;\n+\tstruct cmd2process *entry = NULL;\n+\tstruct child_process *process = NULL;\n+\tconst char *argv[] = { NULL, NULL };\n+\n+\tif (!cmd || !*cmd)\n+\t\treturn 0;\n+\n+\tif (!dst)\n+\t\treturn 1;\n+\n+\tif (!cmd_process_map_init) {\n+\t\tcmd_process_map_init = 1;\n+\t\thashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);\n+\t} else {\n+\t\tentry = find_entry(cmd);\n+\t}\n+\n+\tif (entry) {\n+\t\tprocess = &entry->process;\n+\t} else {\n+\t\tentry = malloc(sizeof(struct cmd2process));\n+\t\thashmap_entry_init(entry, memhash(&cmd, sizeof(char *)));\n+\t\tentry->cmd = cmd;\n+\t\thashmap_add(&cmd_process_map, entry);\n+\t\tprocess = &entry->process;\n+\n+\t\tchild_process_init(process);\n+\t\targv[0] = cmd;\n+\t\tprocess->argv = argv;\n+\t\tprocess->use_shell = 1;\n+\t\tprocess->in = -1;\n+\t\tprocess->out = -1;\n+\n+\t\tif (start_command(process)) {\n+\t\t\terror(\"cannot fork to run external filter %s\", cmd);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tfflush(NULL);\n+\n+\t// TODO: Is the signchain_push OK here?\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\twrite_str_in_full(process->in, path);\n+\twrite_str_in_full(process->in, \"\\n\");\n+\tif (src && len > 0) {\n+\t\t// smudge filter processing\n+\t\tstruct strbuf lenstr = STRBUF_INIT;\n+\t\tstrbuf_reset(&lenstr);\n+\t\tstrbuf_addf(&lenstr, \"%zu\", len);\n+\t\twrite_str_in_full(process->in, lenstr.buf);\n+\t\twrite_str_in_full(process->in, \"\\n\");\n+\t\twrite_in_full(process->in, src, len);\n+\t\tstrbuf_reset(&nbuf);\n+\t\tif (strbuf_read_once(&nbuf, process->out, 2) < 0 ||\n+\t\t\tstrcmp(nbuf.buf, \"OK\") != 0) {\n+\t\t\terror(\"read from external file filter %s failed\", cmd);\n+\t\t\tret = 0;\n+\t\t}\n+\t} else {\n+\t\t// clean filter processing\n+\t\tstrbuf_reset(&nbuf);\n+\t\t// TODO: Should we read the expected size here first?\n+\t\tif (strbuf_read_once(&nbuf, process->out, 0) < 0) {\n+\t\t\terror(\"read from external file filter %s failed\", cmd);\n+\t\t\tret = 0;\n+\t\t} else {\n+\t\t\tstrbuf_swap(dst, &nbuf);\n+\t\t}\n+\t\t// TODO: Should we ask for an OK from the filter here, too?\n+\t}\n+\tsigchain_pop(SIGPIPE);\n+\n+\t// TODO: We probably should close the pipes and finish all processes\n+\t// in the hashmap. What would be a good place to do this?\n+\t// close(process->in)\n+\t// close(process->out)\n+\t// finish_command(process);\n+\n+\tstrbuf_release(&nbuf);\n+\treturn ret;\n+}\n+\n static int filter_buffer_or_fd(int in, int out, void *data)\n {\n \t/*\n@@ -402,15 +513,6 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tstrbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);\n \tstrbuf_release(&path);\n\n-\t/* append fspath to the command if it's set, separated with a space */\n-\tif (params->fspath) {\n-\t\tstruct strbuf fspath = STRBUF_INIT;\n-\t\tsq_quote_buf(&fspath, params->fspath);\n-\t\tstrbuf_addstr(&cmd, \" \");\n-\t\tstrbuf_addbuf(&cmd, &fspath);\n-\t\tstrbuf_release(&fspath);\n-\t}\n-\n \targv[0] = cmd.buf;\n\n \tchild_process.argv = argv;\n@@ -449,9 +551,8 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \treturn (write_err || status);\n }\n\n-static int apply_filter(const char *path, const char *fspath,\n-\t\t\tconst char *src, size_t len, int fd,\n-                        struct strbuf *dst, const char *cmd)\n+static int apply_filter(const char *path, const char *src, size_t len,\n+\t\t\t\t\t\tint fd, struct strbuf *dst, const char *cmd)\n {\n \t/*\n \t * Create a pipeline to have the command filter the buffer's\n@@ -479,7 +580,6 @@ static int apply_filter(const char *path, const char *fspath,\n \tparams.fd = fd;\n \tparams.cmd = cmd;\n \tparams.path = path;\n-\tparams.fspath = fspath;\n\n \tfflush(NULL);\n \tif (start_async(&async))\n@@ -610,7 +710,7 @@ static int count_ident(const char *cp, unsigned long size)\n }\n\n static int ident_to_git(const char *path, const char *src, size_t len,\n-                        struct strbuf *buf, int ident)\n+\t\t\t\t\t\tstruct strbuf *buf, int ident)\n {\n \tchar *dst, *dollar;\n\n@@ -860,7 +960,7 @@ int would_convert_to_git_filter_fd(const char *path)\n \tif (!ca.drv->required)\n \t\treturn 0;\n\n-\treturn apply_filter(path, NULL, NULL, 0, -1, NULL, ca.drv->clean);\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);\n }\n\n static int can_filter_file(const char *filefilter, const char *filefiltername,\n@@ -950,7 +1050,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\trequired = ca.drv->required;\n \t}\n\n-\tret |= apply_filter(path, NULL, src, len, -1, dst, filter);\n+\tret |= apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret && required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n\n@@ -976,7 +1076,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n \tassert(ca.drv);\n \tassert(ca.drv->clean);\n\n-\tif (!apply_filter(path, NULL, NULL, 0, fd, dst, ca.drv->clean))\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n\n \tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action,\n@@ -995,7 +1095,7 @@ void convert_to_git_filter_from_file(const char *path, struct strbuf *dst,\n \tassert(ca.drv->clean);\n \tassert(ca.drv->clean_from_file);\n\n-\tif (!apply_filter(path, path, \"\", 0, -1, dst, ca.drv->clean_from_file))\n+\tif (!apply_file_filter(path, \"\", 0, dst, ca.drv->clean_from_file))\n \t\tdie(\"%s: cleanFromFile filter '%s' failed\", path, ca.drv->name);\n\n \tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action,\n@@ -1040,7 +1140,10 @@ static int convert_to_working_tree_internal(const char *path,\n \t\t}\n \t}\n\n-\tret_filter = apply_filter(path, destpath, src, len, -1, dst, filter);\n+\tif (destpath)\n+\t\tret_filter = apply_file_filter(destpath, src, len, dst, filter);\n+\telse\n+\t\tret_filter = apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret_filter && required)\n \t\tdie(\"%s: %s filter %s failed\", path, destpath ? \"smudgeToFile\" : \"smudge\", ca.drv->name);\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 2722013..1eafe67 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -14,19 +14,39 @@ chmod +x rot13.sh\n\n cat <<EOF >rot13-from-file.sh\n #!$SHELL_PATH\n-srcfile=\"\\$1\"\n-touch rot13-from-file.ran\n-cat \"\\$srcfile\" | ./rot13.sh\n+while read LINE; do\n+   srcfile=\"\\$LINE\"\n+   echo \"CLEAN \\$srcfile\" >> rot13-from-file.ran\n+   cat \"\\$srcfile\" | ./rot13.sh\n+done\n+\n EOF\n chmod +x rot13-from-file.sh\n\n-cat <<EOF >rot13-to-file.sh\n-#!$SHELL_PATH\n-destfile=\"\\$1\"\n-touch rot13-to-file.ran\n-./rot13.sh > \"\\$destfile\"\n+# TODO: Is there a way to read X bytes from a pipe via shell?\n+# I implemented the filter in Python as workaround. If Python\n+# is an undesired dependency then I could reimplement it in Perl, too.\n+cat <<EOF >rot13-to-file.py\n+#!/usr/bin/env python\n+import sys\n+from subprocess import Popen, PIPE, STDOUT\n+\n+for data in iter(sys.stdin.readline, ''):\n+\tfilename = data[:-1]\n+\tcontent_size = sys.stdin.readline()[:-1]\n+\tcontent = sys.stdin.read(int(content_size))\n+\n+\tp = Popen(['./rot13.sh'], stdout=PIPE, stdin=PIPE, stderr=PIPE)\n+\twith open(filename, 'w') as f:\n+\t\tf.write(p.communicate(input=content)[0])\n+\n+\twith open('rot13-to-file.ran', 'a') as f:\n+\t\tf.write('SMUDGE {} {}\\n'.format(filename, content_size))\n+\n+\tsys.stdout.write('OK')\n+\tsys.stdout.flush()\n EOF\n-chmod +x rot13-to-file.sh\n+chmod +x rot13-to-file.py\n\n cat <<EOF >delete-file-and-fail.sh\n #!$SHELL_PATH\n@@ -293,28 +313,52 @@ test_expect_success 'disable filter with empty override' '\n '\n\n test_expect_success 'cleanFromFile filter is used when adding a file' '\n+\t{\n+\t    echo a\n+\t    echo bb\n+\t    echo ccc\n+\t} >test2 &&\n+\n+\n \ttest_config filter.rot13.cleanFromFile ./rot13-from-file.sh &&\n+\ttest_config filter.rot13.required true &&\n\n \techo \"*.t filter=rot13\" >.gitattributes &&\n\n \tcat test >fstest.t &&\n-\tgit add fstest.t &&\n-\ttest -e rot13-from-file.ran &&\n+\tcat test2 >fstest2.t &&\n+\tgit add fstest.t fstest2.t &&\n+\n+\ttest_line_count = 2 rot13-from-file.ran &&\n+\tgrep \"CLEAN fstest.t\" rot13-from-file.ran &&\n+\tgrep \"CLEAN fstest2.t\" rot13-from-file.ran &&\n \trm -f rot13-from-file.ran &&\n\n \trm -f fstest.t &&\n-\tgit checkout -- fstest.t &&\n-\tcmp test fstest.t\n+\trm -f fstest2.t &&\n+\n+\tgit checkout . &&\n+\tcmp test fstest.t &&\n+\tcmp test2 fstest2.t\n '\n\n test_expect_success 'smudgeToFile filter is used when checking out a file' '\n-\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&\n+\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.py &&\n+\ttest_config filter.rot13.required true &&\n\n \trm -f fstest.t &&\n-\tgit checkout -- fstest.t &&\n+\trm -f fstest2.t &&\n+\tgit checkout . &&\n+\n+\t# TODO: Why is this necessary?\n+\tsleep 0.1 &&\n+\n \tcmp test fstest.t &&\n+\tcmp test2 fstest2.t &&\n\n-\ttest -e rot13-to-file.ran &&\n+\ttest_line_count = 2 rot13-to-file.ran &&\n+\tgrep \"SMUDGE fstest.t 57\" rot13-to-file.ran &&\n+\tgrep \"SMUDGE fstest2.t 9\" rot13-to-file.ran &&\n \trm -f rot13-to-file.ran\n '\n\n@@ -335,7 +379,7 @@ test_expect_success 'recovery from failure of smudgeToFile filter that deletes t\n '\n\n test_expect_success 'smudgeToFile filter is used in merge' '\n-\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&\n+\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.py &&\n\n \tgit commit -m \"added fstest.t\" fstest.t &&\n \tgit checkout -b old &&\n@@ -350,7 +394,7 @@ test_expect_success 'smudgeToFile filter is used in merge' '\n '\n\n test_expect_success 'smudgeToFile filter is used by git am' '\n-\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&\n+\ttest_config filter.rot13.smudgeToFile ./rot13-to-file.py &&\n\n \tgit format-patch HEAD^ --stdout > fstest.patch &&\n \tgit reset --hard HEAD^ &&\n--\n2.5.1\n\n"},{"id":"291140","messageId":"20160710151046.GA7306@kitenet.net","threadId":"42814","inReplyTo":"1468150507-40928-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [RFC] Long running Git clean/smudge filter","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2016-07-10T15:10:46Z","receivedAt":"2016-07-10T15:11:01Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"larsxschneider@gmail.com wrote:\n> (2) Joey's topic, which is the base for my patch, looks stalled for more than\n> 2 weeks:\n> http://thread.gmane.org/gmane.comp.version-control.git/297994/focus=298006\n> I would be happy to address Junio's comments and post a reroll. However,\n> I don't want to interfere with Joey's plans.\n\nI've been on vacation and plan to get back to that in the upcoming week.\n\n> @Joey (in case you are reading this):\n> My patch changes your initial idea quite a bit. However, I believe it is an\n> improvement that could be beneficial for git-annex, too. Would you prefer to\n> work with me on the combination of our ideas (file clean/smudge + long running\n> clean/smudge processes) or would you prefer to keep your interface?\n\nLong running filters mean that you need a more complicated protocol to\nspeak over the pipes. Seems that such a protocol could be designed to work\nwith the original smudge/clean filters as well as with my\nsmudgeToFile/cleanFromFile filters. Assuming that there's a way to\ntell whether the filters support being long-running or not.\n\nNote that the interface we arrived at for smudgeToFile/cleanFromFile is as\nsimilar as possible to smudge/clean, so the filter developer only has to\nchange one thing. That's a big plus, and so I don't like diverging the\ntwo interfaces.\n\nSo, I don't want to entangle these two ideas very much.\n\n-- \nsee shy jo\n"},{"id":"291245","messageId":"4947BBA2-6260-4546-9641-E639A8C70174@gmail.com","threadId":"42814","inReplyTo":"20160710151046.GA7306@kitenet.net","subject":"Re: [RFC] Long running Git clean/smudge filter","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-12T09:30:06Z","receivedAt":"2016-07-12T09:30:18Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Jul 2016, at 17:10, Joey Hess <id@joeyh.name> wrote:\n> \n> larsxschneider@gmail.com wrote:\n>> (2) Joey's topic, which is the base for my patch, looks stalled for more than\n>> 2 weeks:\n>> http://thread.gmane.org/gmane.comp.version-control.git/297994/focus=298006\n>> I would be happy to address Junio's comments and post a reroll. However,\n>> I don't want to interfere with Joey's plans.\n> \n> I've been on vacation and plan to get back to that in the upcoming week.\n\nGood to hear :-) ! I hope you had a great vacation!\n\n\n\n>> @Joey (in case you are reading this):\n>> My patch changes your initial idea quite a bit. However, I believe it is an\n>> improvement that could be beneficial for git-annex, too. Would you prefer to\n>> work with me on the combination of our ideas (file clean/smudge + long running\n>> clean/smudge processes) or would you prefer to keep your interface?\n> \n> Long running filters mean that you need a more complicated protocol to\n> speak over the pipes. Seems that such a protocol could be designed to work\n> with the original smudge/clean filters as well as with my\n> smudgeToFile/cleanFromFile filters. Assuming that there's a way to\n> tell whether the filters support being long-running or not.\n> \n> Note that the interface we arrived at for smudgeToFile/cleanFromFile is as\n> similar as possible to smudge/clean, so the filter developer only has to\n> change one thing. That's a big plus, and so I don't like diverging the\n> two interfaces.\nI understand, thanks for the clarification. My plan is to implement long running \nfilters only for smudgeToFile/cleanFromFile (at least initially) as this should\nsolve the major pain point already and is relatively straight forward.\n\nWhat do you think about this kind of config? Any idea for a better config name?\n\n[filter \"bar\"]\n    clean = foo1 %f\n    smudge = foo2 %f\n    cleanFromFile foo3\n    smudgeToFile foo4\n    longRunning = true\n\nI think it would be easy to support both of our interfaces in parallel. Do\nyou understand why the \"async\" API is necessary?\nhttps://github.com/larsxschneider/git/blob/74e22bd4e0b505785fa8ffc2ef15721909635d1c/convert.c#L585-L599\n\nWould you be OK if I implement smudgeToFile/cleanFromFile as separate\n\"apply_filter\" function as I have prototyped here:\nhttps://github.com/larsxschneider/git/blob/74e22bd4e0b505785fa8ffc2ef15721909635d1c/convert.c#L1143-L1146\nhttps://github.com/larsxschneider/git/blob/74e22bd4e0b505785fa8ffc2ef15721909635d1c/convert.c#L402-L488\n\n\nThanks,\nLars\n"}]}