{"thread":{"id":"53615","subject":"git-fast-import out of memory","startedAt":"2020-06-05T05:15:21Z","lastAt":"2020-06-08T22:58:57Z","messageCount":9,"participants":["Billes Tibor","brian m. carlson","Tibor Billes","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"399164","messageId":"c53bb69b-682d-3b47-4ed0-5f4559e69e37@gmx.com","threadId":"53615","inReplyTo":null,"subject":"git-fast-import out of memory","fromName":"Billes Tibor","fromEmail":"tbilles@gmx.com","sentAt":"2020-06-05T05:15:04Z","receivedAt":"2020-06-05T05:15:21Z","isPatch":false,"sender":{"key":"tbilles@gmx.com","avatar":null},"body":"Hi,\n\nI recently upgraded my git to version 2.27.0-1~ppa0~ubuntu18.04.1 and\nnoticed\nthat git-fast-import uses so much memory it gets killed. I'm fetching from a\nMercurial repo using an importer from\nhttps://github.com/mnauw/git-remote-hg.git which uses git-fast-import to\nfetch\ncommits from Mercurial.\n\nHere is an output of a git fetch showing is used 14Gb of RAM (on a 16Gb\nmachine)\n# time git fetch\nerror: git-fast-import died of signal 9\nfatal: error while running fast-import\nCommand exited with non-zero status 128\n2.02user 3.82system 0:08.00elapsed 73%CPU (0avgtext+0avgdata\n14744800maxresident)k\n104920inputs+0outputs (414major+3688606minor)pagefaults 0swaps\n\nstrace shows that git-fast-import is reading the marks from a file, then\nallocate some memory, reads more marks, allocates more memory, and so on:\n\n11191 06:19:08.180572 read(7<.../.git/hg/origin/marks-git>, \"79798\n8ea080f15ab22807608aae4696dd23edefd8febe\\n:220396\n919079de10d43caf3fcde56bb1a17994b47a6214\\n:75683\n928813193a1535dc1274ed9da2f54f5de2caf2f4\\n:155297\n9108211d7ba318076fb53b2bd3d291102b376dbf\\n:162042\n9458fe329e9be30ad2b61e75197595889d80144b\\n:305834\n93485ce7991b4330a1114136b5d8e08d8bd1505b\\n:223654\n9750bdef7d22a885d2522bdd9e0a0e882979098e\\n...\", 4096) = 4096 <0.000027>\n11191 06:19:08.182162 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be38ef000 <0.000024>\n11191 06:19:08.183403 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be36ee000 <0.000127>\n11191 06:19:08.184775 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be34ed000 <0.000059>\n11191 06:19:08.186036 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be32ec000 <0.000121>\n11191 06:19:08.187412 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be30eb000 <0.000110>\n11191 06:19:08.188743 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be2eea000 <0.000022>\n11191 06:19:08.189929 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be2ce9000 <0.000039>\n11191 06:19:08.191150 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be2ae8000 <0.000019>\n11191 06:19:08.192329 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be28e7000 <0.000023>\n11191 06:19:08.193536 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be26e6000 <0.000038>\n11191 06:19:08.194523 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be24e5000 <0.000019>\n11191 06:19:08.195474 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be22e4000 <0.000212>\n11191 06:19:08.196677 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be20e3000 <0.000027>\n11191 06:19:08.197729 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be1ee2000 <0.000128>\n11191 06:19:08.198883 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be1ce1000 <0.000043>\n11191 06:19:08.199881 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be1ae0000 <0.000124>\n11191 06:19:08.200959 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be18df000 <0.000020>\n11191 06:19:08.201943 mmap(NULL, 2101248, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f5be16de000 <0.000021>\n\nThe following shows that memory allocation seems to be linear with\nrespect to\nthe number of marks, but with a very high constant factor:\n# cut -d' ' -f 3 /tmp/gitfetch.strace | cut -d '(' -f 1 | uniq -c\n\n         [ ... cut (this is not the start of the allocations) ... ]\n       1 read\n      47 mmap\n       1 read\n      79 mmap\n       1 read\n      36 mmap\n         [ ... removed some other syscalls ... ]\n      73 mmap\n       1 read\n     141 mmap\n       1 read\n     173 mmap\n       1 read\n     204 mmap\n       1 read\n     235 mmap\n       1 read\n     267 mmap\n       1 read\n     297 mmap\n       1 read\n     329 mmap\n       1 read\n     361 mmap\n       1 read\n     392 mmap\n       1 read\n     424 mmap\n       1 read\n     454 mmap\n       1 read\n     493 mmap\n\nMy marks file contains 91k entries, git fetch reads only 1400 before killed.\n\nI bisected the problem, below is my bisect log:\ngit bisect start\n# good: [af6b65d45ef179ed52087e80cb089f6b2349f4ec] Git 2.26.2\ngit bisect good af6b65d45ef179ed52087e80cb089f6b2349f4ec\n# bad: [b3d7a52fac39193503a0b6728771d1bf6a161464] Git 2.27\ngit bisect bad b3d7a52fac39193503a0b6728771d1bf6a161464\n# bad: [af986863c1ae2e306d5627f4e42cc6d2cf2a057f] Merge branch\n'dd/ci-musl-libc'\ngit bisect bad af986863c1ae2e306d5627f4e42cc6d2cf2a057f\n# bad: [7a8bb6db7cc04add05484c4fc907e34f76b12fb9] Merge branch\n'jm/gitweb-fastcgi-utf8'\ngit bisect bad 7a8bb6db7cc04add05484c4fc907e34f76b12fb9\n# bad: [4e4baee3f44da26a5eaab27c76d597b04fef5259] Merge branch\n'bc/filter-process'\ngit bisect bad 4e4baee3f44da26a5eaab27c76d597b04fef5259\n# good: [883e23820ed21b4ae65463f2a87152285bf77937] Merge branch\n'en/oidset-uninclude-hashmap'\ngit bisect good 883e23820ed21b4ae65463f2a87152285bf77937\n# bad: [1bdca816412910e1206c15ef47f2a8a6b369b831] fast-import: add\noptions for rewriting submodules\ngit bisect bad 1bdca816412910e1206c15ef47f2a8a6b369b831\n# good: [bf154a878281b6a971ece0fb6d917938298be60d] t/helper: make\nrepository tests hash independent\ngit bisect good bf154a878281b6a971ece0fb6d917938298be60d\n# good: [e02a7141f83326f7098800fed764061ecf1f0eff] worktree: allow\nrepository version 1\ngit bisect good e02a7141f83326f7098800fed764061ecf1f0eff\n# bad: [abe0cc536414f2b9cfa37f208b36df5126e6356a] fast-import: add\nhelper function for inserting mark object entries\ngit bisect bad abe0cc536414f2b9cfa37f208b36df5126e6356a\n# bad: [ddddf8d7e254f4af6297d0ed62ea6a5d7eabdb64] fast-import: permit\nreading multiple marks files\ngit bisect bad ddddf8d7e254f4af6297d0ed62ea6a5d7eabdb64\n# good: [42d4e1d1128fa1cb56032ac58f65ea3dd1296a9a] commit: use expected\nsignature header for SHA-256\ngit bisect good 42d4e1d1128fa1cb56032ac58f65ea3dd1296a9a\n# first bad commit: [ddddf8d7e254f4af6297d0ed62ea6a5d7eabdb64]\nfast-import: permit reading multiple marks files\n\nAccording to the bisect the first bad commit is:\n\ncommit ddddf8d7e254f4af6297d0ed62ea6a5d7eabdb64 (refs/bisect/bad)\nAuthor: brian m. carlson <sandals@crustytoothpaste.net>\nDate:   Sat Feb 22 20:17:45 2020 +0000\n\n     fast-import: permit reading multiple marks files\n\n     In the future, we'll want to read marks files for submodules as well.\n     Refactor the existing code to make it possible to read multiple marks\n     files, each into their own marks set.\n\n     Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n     Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nWhen doing the bisect it was easier for me to use git from the Ubuntu\npackage and only replace the git-fast-import binary with the one I was\ntesting.\nI hope it doesn't falsify the bisect results. The behavior seemed to be\nconsistent: it either produced the issue above, or it worked perfectly fine.\n\nCan you help me fix this issue? I hope the information I gathered is\nenough to\nhelp you find the cause of this behavior. I'd be happy to provide more\ninformation if needed or test patches.  Unfortunately the source code I was\nfetching is proprietary, I cannot post it.\n\nBest Regards,\nTibor Billes\n"},{"id":"399217","messageId":"20200605224342.GF6569@camp.crustytoothpaste.net","threadId":"53615","inReplyTo":"c53bb69b-682d-3b47-4ed0-5f4559e69e37@gmx.com","subject":"Re: git-fast-import out of memory","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-05T22:43:42Z","receivedAt":"2020-06-05T22:43:50Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-06-05 at 05:15:04, Billes Tibor wrote:\n> Can you help me fix this issue? I hope the information I gathered is\n> enough to\n> help you find the cause of this behavior. I'd be happy to provide more\n> information if needed or test patches.  Unfortunately the source code I was\n> fetching is proprietary, I cannot post it.\n\nHey, thanks for this report.  I can confirm this with git.git and the\nfollowing commands:\n\n  git fast-export --all --reencode=yes --signed-tags=verbatim --tag-of-filtered-object=drop >$TMP/git.fe\n  cd $TMP && rm -fr test-repo && git init test-repo &&\n    (cd test-repo && /usr/bin/time git fast-import --export-marks=$TMP/git.marks < $TMP/git.fe &&\n    /usr/bin/time git fast-import --import-marks=$TMP/git.marks < $TMP/git.fe)\n\nThe latter fast-import command gets killed with SIGKILL due to using 16\nGiB of memory, which I would agree is unreasonable.\n\nIt looks like with Junio's patch at\n<xmqqeeqto4x1.fsf@gitster.c.googlers.com>, the memory usage is down to\n1.4 GiB, which considering that a pack must be written, is much more\nreasonable.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"399224","messageId":"20200606002241.1578150-1-sandals@crustytoothpaste.net","threadId":"53615","inReplyTo":"c53bb69b-682d-3b47-4ed0-5f4559e69e37@gmx.com","subject":"[PATCH] fast-import: fix incomplete conversion with multiple mark files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-06T00:22:39Z","receivedAt":"2020-06-06T00:23:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When ddddf8d7e2 (\"fast-import: permit reading multiple marks files\",\n2020-02-22) converted fast-import to handle multiple marks files in\npreparation for submodule support, the conversion was incomplete.  With\na large number of marks, we would actually modify the marks variable\neven though we had passed in a different variable to operate on.  In\naddition, we didn't consider the fact that the code can replace the mark\nset passed in, so when we did so we happened to leak quite a bit of\nmemory, since we never reused the structure we created, instead\nreallocating a new one each time.\n\nIt doesn't appear from some testing that we actually produce incorrect\nresults in this case, only that we leak a substantial amount of memory.\nTo make things work properly and avoid leaking, pass a pointer to\npointer to struct mark_set, which allows us to modify the set of marks\nwhen the number of marks is large.\n\nWith this patch, importing a dump of git.git with a set of exported\nmarks goes from taking in excess of 15 GiB of memory (and being killed\nby the Linux OOM killer) to using a maximum of 1.4 GiB of memory.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n fast-import.c | 28 +++++++++++++++-------------\n 1 file changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 0dfa14dc8c..ed87d6e380 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -150,7 +150,7 @@ struct recent_command {\n \tchar *buf;\n };\n \n-typedef void (*mark_set_inserter_t)(struct mark_set *s, struct object_id *oid, uintmax_t mark);\n+typedef void (*mark_set_inserter_t)(struct mark_set **s, struct object_id *oid, uintmax_t mark);\n typedef void (*each_mark_fn_t)(uintmax_t mark, void *obj, void *cbp);\n \n /* Configured limits on output */\n@@ -534,13 +534,15 @@ static char *pool_strdup(const char *s)\n \treturn r;\n }\n \n-static void insert_mark(struct mark_set *s, uintmax_t idnum, struct object_entry *oe)\n+static void insert_mark(struct mark_set **sp, uintmax_t idnum, struct object_entry *oe)\n {\n+\tstruct mark_set *s = *sp;\n+\n \twhile ((idnum >> s->shift) >= 1024) {\n \t\ts = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));\n-\t\ts->shift = marks->shift + 10;\n-\t\ts->data.sets[0] = marks;\n-\t\tmarks = s;\n+\t\ts->shift = (*sp)->shift + 10;\n+\t\ts->data.sets[0] = (*sp);\n+\t\t(*sp) = s;\n \t}\n \twhile (s->shift) {\n \t\tuintmax_t i = idnum >> s->shift;\n@@ -958,7 +960,7 @@ static int store_object(\n \n \te = insert_object(&oid);\n \tif (mark)\n-\t\tinsert_mark(marks, mark, e);\n+\t\tinsert_mark(&marks, mark, e);\n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n@@ -1156,7 +1158,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \te = insert_object(&oid);\n \n \tif (mark)\n-\t\tinsert_mark(marks, mark, e);\n+\t\tinsert_mark(&marks, mark, e);\n \n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[OBJ_BLOB]++;\n@@ -1731,7 +1733,7 @@ static void dump_marks(void)\n \t}\n }\n \n-static void insert_object_entry(struct mark_set *s, struct object_id *oid, uintmax_t mark)\n+static void insert_object_entry(struct mark_set **s, struct object_id *oid, uintmax_t mark)\n {\n \tstruct object_entry *e;\n \te = find_object(oid);\n@@ -1748,12 +1750,12 @@ static void insert_object_entry(struct mark_set *s, struct object_id *oid, uintm\n \tinsert_mark(s, mark, e);\n }\n \n-static void insert_oid_entry(struct mark_set *s, struct object_id *oid, uintmax_t mark)\n+static void insert_oid_entry(struct mark_set **s, struct object_id *oid, uintmax_t mark)\n {\n \tinsert_mark(s, mark, xmemdupz(oid, sizeof(*oid)));\n }\n \n-static void read_mark_file(struct mark_set *s, FILE *f, mark_set_inserter_t inserter)\n+static void read_mark_file(struct mark_set **s, FILE *f, mark_set_inserter_t inserter)\n {\n \tchar line[512];\n \twhile (fgets(line, sizeof(line), f)) {\n@@ -1786,7 +1788,7 @@ static void read_marks(void)\n \t\tgoto done; /* Marks file does not exist */\n \telse\n \t\tdie_errno(\"cannot read '%s'\", import_marks_file);\n-\tread_mark_file(marks, f, insert_object_entry);\n+\tread_mark_file(&marks, f, insert_object_entry);\n \tfclose(f);\n done:\n \timport_marks_file_done = 1;\n@@ -3242,7 +3244,7 @@ static void parse_alias(void)\n \t\tdie(_(\"Expected 'to' command, got %s\"), command_buf.buf);\n \te = find_object(&b.oid);\n \tassert(e);\n-\tinsert_mark(marks, next_mark, e);\n+\tinsert_mark(&marks, next_mark, e);\n }\n \n static char* make_fast_import_path(const char *path)\n@@ -3340,7 +3342,7 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)\n \tfp = fopen(f, \"r\");\n \tif (!fp)\n \t\tdie_errno(\"cannot read '%s'\", f);\n-\tread_mark_file(ms, fp, insert_oid_entry);\n+\tread_mark_file(&ms, fp, insert_oid_entry);\n \tfclose(fp);\n }\n \n"},{"id":"399225","messageId":"20200606002241.1578150-3-sandals@crustytoothpaste.net","threadId":"53615","inReplyTo":"20200606002241.1578150-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] exec: run final pipeline command in a subshell in sh mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-06T00:22:41Z","receivedAt":"2020-06-06T00:23:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"zsh typically runs the final command in a pipeline in the main shell\ninstead of a subshell.  However, POSIX requires that all commands in a\npipeline run in a subshell, but permits zsh's behavior as an extension.\n\nSince zsh may be used as /bin/sh in some cases (such as macOS Catalina),\nit makes sense to have the POSIX behavior when emulating sh, so do that\nby checking for being the final item of a multi-item pipeline and\ncreating a subshell in that case.\n\nFrom the comment above execpline(), we know the following:\n\n  last1 is a flag that this command is the last command in a shell that\n  is about to exit, so we can exec instead of forking.  It gets passed\n  all the way down to execcmd() which actually makes the decision.  A 0\n  is always passed if the command is not the last in the pipeline. […]\n  If last1 is zero but the command is at the end of a pipeline, we pass\n  2 down to execcmd().\n\nSo there are three cases to consider in this code:\n\n• last1 is 0, which means we are not at the end of a pipeline, in which\n  case we should not change behavior.\n• last1 is 1, which means we are effectively running in a subshell,\n  because nothing that happens due to the exec is going to affect the\n  actual shell, since it will have been replaced.  So there is nothing\n  to do here.\n• last1 is 2, which means our command is at the end of the pipeline, so\n  in sh mode we should create a subshell by forking.\n\ninput is nonzero if the input to this process is a pipe that we've\nopened.  At the end of a multi-stage pipeline, it will necessarily be\nnonzero.\n\nNote that several of the tests may appear bizarre, since most developers\ndo not place useless variable assignments directly at the end of a\npipeline.  However, as the function tests demonstrate, there are cases\nwhere assignments may occur when a shell function is used at the end of\na command.  The remaining assignment tests simply test additional cases,\nsuch as the use of local, that would otherwise be untested.\n---\n Src/exec.c           | 10 ++++++----\n Test/B07emulate.ztst | 22 ++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/Src/exec.c b/Src/exec.c\nindex 29f4fc5ca..f2650f311 100644\n--- a/Src/exec.c\n+++ b/Src/exec.c\n@@ -2866,11 +2866,13 @@ execcmd_exec(Estate state, Execcmd_params eparams,\n \t    pushnode(args, dupstring(\"fg\"));\n     }\n \n-    if ((how & Z_ASYNC) || output) {\n+    if ((how & Z_ASYNC) || output ||\n+\t(last1 == 2 && input && EMULATION(EMULATE_SH))) {\n \t/*\n-\t * If running in the background, or not the last command in a\n-\t * pipeline, we don't need any of the rest of this function to\n-\t * affect the state in the main shell, so fork immediately.\n+\t * If running in the background, not the last command in a\n+\t * pipeline, or the last command in a multi-stage pipeline\n+\t * in sh mode, we don't need any of the rest of this function\n+\t * to affect the state in the main shell, so fork immediately.\n \t *\n \t * In other cases we may need to process the command line\n \t * a bit further before we make the decision.\ndiff --git a/Test/B07emulate.ztst b/Test/B07emulate.ztst\nindex 7b1592fa9..45c39b51d 100644\n--- a/Test/B07emulate.ztst\n+++ b/Test/B07emulate.ztst\n@@ -276,3 +276,25 @@ F:Some reserved tokens are handled in alias expansion\n 0:--emulate followed by other options\n >yes\n >no\n+\n+  emulate sh -c '\n+  foo () {\n+    VAR=foo &&\n+    echo $VAR | bar &&\n+    echo \"$VAR\"\n+  }\n+  bar () {\n+    tr f b &&\n+    VAR=\"$(echo bar | tr r z)\" &&\n+    echo \"$VAR\"\n+  }\n+  foo\n+  '\n+  emulate sh -c 'func() { echo | local def=\"abc\"; echo $def;}; func'\n+  emulate sh -c 'abc=\"def\"; echo | abc=\"ghi\"; echo $abc'\n+0:emulate sh uses subshell for last pipe entry\n+>boo\n+>baz\n+>foo\n+>\n+>def\n"},{"id":"399226","messageId":"20200606002241.1578150-2-sandals@crustytoothpaste.net","threadId":"53615","inReplyTo":"20200606002241.1578150-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 0/1] Run pipeline command in subshell in sh mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-06T00:22:40Z","receivedAt":"2020-06-06T00:23:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"POSIX sh implementations run each command in a pipeline in a subshell,\nalthough zsh (and AT&T ksh) do not: instead, they run the final command\nin the main shell.  This leads to very different behavior when the final\ncommand is a shell function which modifies variables.\n\nzsh is starting to be used in some cases as /bin/sh, such as on macOS\nCatalina.  Consequently, it makes sense to emulate the POSIX behavior as\nmuch as possible when emulating sh, since that's the least surprising\nbehavior.  This patch does exactly that.\n\nWith this patch, using \"zsh --emulate sh\" passes the Git testsuite.  I\nexpect that it will also be fully functional as /bin/sh on Debian,\nalthough I have not tested.\n\nThis patch was sent before, but didn't get picked up.  In hopes of\naiding reviewers, I've resent it with a significantly expanded commit\nmessage so that it is easier to reason about.\n\nI'm not subscribed to the list, so please CC me if you have questions or\ncomments.\n\nbrian m. carlson (1):\n  exec: run final pipeline command in a subshell in sh mode\n\n Src/exec.c           | 10 ++++++----\n Test/B07emulate.ztst | 22 ++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\n"},{"id":"399227","messageId":"20200606003134.GH6569@camp.crustytoothpaste.net","threadId":"53615","inReplyTo":"20200606002241.1578150-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/1] Run pipeline command in subshell in sh mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-06T00:31:34Z","receivedAt":"2020-06-06T00:32:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-06-06 at 00:22:40, brian m. carlson wrote:\n> POSIX sh implementations run each command in a pipeline in a subshell,\n> although zsh (and AT&T ksh) do not: instead, they run the final command\n> in the main shell.  This leads to very different behavior when the final\n> command is a shell function which modifies variables.\n\nSorry about this and the following patch.  I forgot to clean my patches\ndirectory before sending, and accidentally sent some patches for zsh.\nMy apologies.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"399299","messageId":"alpine.DEB.2.21.2006081739520.9949@serpens","threadId":"53615","inReplyTo":"20200606002241.1578150-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] fast-import: fix incomplete conversion with multiple mark files","fromName":"Tibor Billes","fromEmail":"tbilles@gmx.com","sentAt":"2020-06-08T15:52:16Z","receivedAt":"2020-06-08T15:52:38Z","isPatch":true,"sender":{"key":"tbilles@gmx.com","avatar":null},"body":"Hi,\n\nOn Sat, 6 Jun 2020, brian m. carlson wrote:\n\n> When ddddf8d7e2 (\"fast-import: permit reading multiple marks files\",\n> 2020-02-22) converted fast-import to handle multiple marks files in\n> preparation for submodule support, the conversion was incomplete.  With\n> a large number of marks, we would actually modify the marks variable\n> even though we had passed in a different variable to operate on.  In\n> addition, we didn't consider the fact that the code can replace the mark\n> set passed in, so when we did so we happened to leak quite a bit of\n> memory, since we never reused the structure we created, instead\n> reallocating a new one each time.\n>\n> It doesn't appear from some testing that we actually produce incorrect\n> results in this case, only that we leak a substantial amount of memory.\n> To make things work properly and avoid leaking, pass a pointer to\n> pointer to struct mark_set, which allows us to modify the set of marks\n> when the number of marks is large.\n>\n> With this patch, importing a dump of git.git with a set of exported\n> marks goes from taking in excess of 15 GiB of memory (and being killed\n> by the Linux OOM killer) to using a maximum of 1.4 GiB of memory.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n\nThanks for the quickly patching it! I tested the patch and I can confirm this\nsolves the memory leak for me.\n\nThanks,\nTibor Billes\n"},{"id":"399304","messageId":"xmqqtuzlld1d.fsf@gitster.c.googlers.com","threadId":"53615","inReplyTo":"alpine.DEB.2.21.2006081739520.9949@serpens","subject":"Re: [PATCH] fast-import: fix incomplete conversion with multiple mark files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T16:47:58Z","receivedAt":"2020-06-08T16:48:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tibor Billes <tbilles@gmx.com> writes:\n\n> On Sat, 6 Jun 2020, brian m. carlson wrote:\n>\n>> When ddddf8d7e2 (\"fast-import: permit reading multiple marks files\",\n>> 2020-02-22) converted fast-import to handle multiple marks files in\n>> preparation for submodule support, the conversion was incomplete.\n>...\n>\n> Thanks for the quickly patching it! I tested the patch and I can confirm this\n> solves the memory leak for me.\n\nTibor, thanks for testing.\n\nBrian, I notice that the singleton global \"marks_set_count\", even\nthough the number could be counted per instance of the mark_set\nstructure, is still singleton.  I didn't bother thinking about it\nwhen I wrote my \"perhaps along this line\" patch, but now I read it\nagain, I think it is OK to leave it (and other stats counters) as\nis, primarily because we don't have a need (yet) to show stats per\nmark_set.  Did you leave it as a global for the same reason?  Just\nsanity-checking.\n\nThanks.\n"},{"id":"399330","messageId":"20200608225820.GM6569@camp.crustytoothpaste.net","threadId":"53615","inReplyTo":"xmqqtuzlld1d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix incomplete conversion with multiple mark files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-08T22:58:20Z","receivedAt":"2020-06-08T22:58:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-06-08 at 16:47:58, Junio C Hamano wrote:\n> Brian, I notice that the singleton global \"marks_set_count\", even\n> though the number could be counted per instance of the mark_set\n> structure, is still singleton.  I didn't bother thinking about it\n> when I wrote my \"perhaps along this line\" patch, but now I read it\n> again, I think it is OK to leave it (and other stats counters) as\n> is, primarily because we don't have a need (yet) to show stats per\n> mark_set.  Did you leave it as a global for the same reason?  Just\n> sanity-checking.\n\nYes, I did; sorry for not mentioning that in the commit message.  I\nthink it's fine to count the total number of marks processed as a\nstatistic, and that would be independent of how we processed them.  It's\nan interesting metric, but not so interesting that folks will have a\nneed to see a detailed breakdown, I think.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}