{"thread":{"id":"54413","subject":"git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","startedAt":"2020-10-14T09:23:58Z","lastAt":"2020-10-16T20:25:23Z","messageCount":20,"participants":["Dipl. Ing. Sergey Brester","Jeff King","René Scharfe","Junio C Hamano","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"407496","messageId":"1eeb49305cb7c712e141dcae2c434d96@sebres.de","threadId":"54413","inReplyTo":null,"subject":"git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","fromName":"Dipl. Ing. Sergey Brester","fromEmail":"serg.brester@sebres.de","sentAt":"2020-10-14T09:22:03Z","receivedAt":"2020-10-14T09:23:58Z","isPatch":false,"sender":{"key":"serg.brester@sebres.de","avatar":null},"body":"Steps to reproduce the issue:\n\n  1. export from fossil and import the dump to the git:\n```\nfossil export --git --import-marks .git/.fossil2git-fssl --export-marks \n.git/.fossil2git-fssl.tmp ^\n  | git fast-import --import-marks=.git/.fossil2git-git \n--export-marks=.git/.fossil2git-git.tmp\n```\n  during the import git-fast-import.exe is growing on memory (more than I \nhave physically, e.g. noticed over 20GB),\n  SO FINALLY IT IS CRASHING WITH:\n```\nfatal: Out of memory, malloc failed (tried to allocate 2097152 bytes)\nfast-import: dumping crash report to .git/fast_import_crash_1800\n```\n  the crash report contains:\n```\nfast-import crash report:\n  fast-import process: 1800\n  parent process : 1\n  at 2020-10-13 18:55:19 +0000\n\nfatal: Out of memory, malloc failed (tried to allocate 2097152 bytes)\n\nMost Recent Commands Before Crash\n---------------------------------\n\nActive Branch LRU\n-----------------\n  active_branches = 0 cur, 5 max\n\n  pos clock name\n  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nInactive Branches\n-----------------\n\nMarks\n-----\n  exported to .git/.fossil2git-git.tmp\n\n-------------------\nEND OF CRASH REPORT\n```\n\n  2. if I do the export firstly (redirect to file), fossil creates 22MB \nlarge export dump-file,\n  if I import it via git hereafter, it crashes in the same way.\n```\nfossil export --git --import-marks .git/.fossil2git-fssl --export-marks \n.git/.fossil2git-fssl.tmp > tmp-dump-out-of-mem.txt\n\ndir tmp-dump-out-of-mem.txt\n13.10.2020 20:37 22.916.280 tmp-dump-out-of-mem.txt\n\ntype tmp-dump-out-of-mem.txt | git fast-import \n--import-marks=.git/.fossil2git-git \n--export-marks=.git/.fossil2git-git.tmp\n\nfatal: Out of memory, malloc failed (tried to allocate 2097152 bytes)\nfast-import: dumping crash report to .git/fast_import_crash_1800\n```\n\nI did not see any issues with (even much larger) imports, before I \nupgraded git to 2.28.0 (from 2.25.1, I guess).\n\n[System Info]\ngit version:\ngit version 2.28.0.windows.1\ncpu: x86_64\nbuilt from commit: 77982caf269b7ee713a76da2bcf260c34d3bf7a7\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Windows 10.0 18363\ncompiler info: gnuc: 10.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): <unset>\n\nAn attempt to repeat this with 2.27.0 (portable) changes nothing \n(crashed with the same issue).\n\nBUT TRYING THAT WITH 2.14.4 (MINGIT-2.14.4.WINDOWS.7-64-BIT) WORKS WELL:\n```\nC:SoftDevGit-2.14mingw64bingit-fast-import.exe statistics:\n---------------------------------------------------------------------\nAlloc'd objects: 130000\nTotal objects: 591 ( 80 duplicates )\n  blobs : 224 ( 0 duplicates 151 deltas of 224 attempts)\n  trees : 260 ( 80 duplicates 191 deltas of 260 attempts)\n  commits: 107 ( 0 duplicates 0 deltas of 0 attempts)\n  tags : 0 ( 0 duplicates 0 deltas of 0 attempts)\nTotal branches: 201 ( 46 loads )\n  marks: 1048576 ( 129437 unique )\n  atoms: 1503\nMemory total: 10439 KiB\n  pools: 4346 KiB\n  objects: 6093 KiB\n---------------------------------------------------------------------\npack_report: getpagesize() = 65536\npack_report: core.packedGitWindowSize = 1073741824\npack_report: core.packedGitLimit = 35184372088832\npack_report: pack_used_ctr = 131659\npack_report: pack_mmap_calls = 119\npack_report: pack_open_windows = 66 / 66\npack_report: pack_mapped = 488903419 / 488903419\n---------------------------------------------------------------------\n```\n\nThe crash with newer versions is pretty well reproducible, so I have \nfrozen this state to be able to test it later (or check whether it gets \nfixed).\nDon't hesitate to ping me if you need some data or tests.\n\n-- \n\nRegards,\nSergey Brester\n"},{"id":"407556","messageId":"20201015012636.GA387901@coredump.intra.peff.net","threadId":"54413","inReplyTo":"1eeb49305cb7c712e141dcae2c434d96@sebres.de","subject":"Re: git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T01:26:36Z","receivedAt":"2020-10-15T01:26:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2020 at 11:22:03AM +0200, Dipl. Ing. Sergey Brester wrote:\n\n> I did not see any issues with (even much larger) imports, before I upgraded\n> git to 2.28.0 (from 2.25.1, I guess).\n\nI wasn't able to reproduce, but it sounds like there's something special\nabout this repo (since the dump is not that big, and you said much\nlarger dumps have succeeded).\n\nIs it possible to make either the fossil repo, or the fast-import input\nfile available?\n\nIf not, since you said it works with 2.14.4, is it possible to bisect in\nGit to see when it stopped working? You'll need to be able to build Git\nfrom source.\n\n-Peff\n"},{"id":"407596","messageId":"72a4d4d8dff95351122bd192976dd6b1@sebres.de","threadId":"54413","inReplyTo":"20201015012636.GA387901@coredump.intra.peff.net","subject":"Re: git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","fromName":"Dipl. Ing. Sergey Brester","fromEmail":"serg.brester@sebres.de","sentAt":"2020-10-15T11:50:30Z","receivedAt":"2020-10-15T11:55:08Z","isPatch":false,"sender":{"key":"serg.brester@sebres.de","avatar":null},"body":"Hi Jeff,\n\nwell, I don't know how you were trying to reproduce it.\n\nMy first attempt with a git-repository (cloned from \nhttps://github.com/git/git.git) showed that immediately to me.\nHere you go (I used git bash here):\n\n```\n# clone or copy git repository (we'll use it for export and import):\ngit clone https://github.com/git/git.git\ncd git\n\n# make 1st fast-export in order to generate marks (don't need the dump, \nonly marks are needed):\ngit fast-export --reencode=yes --export-marks=.tmp.exp-1.tmp \ne83c5163316f89bfbde7d9ab23ca2e25604af290..dc04167d378fb29d30e1647ff6ff51dd182bc9a3 \n > /dev/null\n\n# make 2nd fast-export in order to generate partial export dump (file \n\".tmp.dump\" will be ca. 87MB):\ngit fast-export --reencode=yes --import-marks=.tmp.exp-1.tmp \n--export-marks=.tmp.exp-2.tmp \n61addb841f2a6d74a1737a01e03df1f773e04944..master > .tmp.dump\n\n# now try to import this dump, using first marks as import marks (we \nhave all revs in git-repo):\ngit fast-import --import-marks=.tmp.exp-1.tmp \n--export-marks=.tmp.imp.tmp < .tmp.dump\n\n```\n\nAnd see how git-fast-import eating your whole memory and enjoy the crash \n:)\n\n```\nfatal: Out of memory, malloc failed (tried to allocate 2097152 bytes)\nfast-import: dumping crash report to .git/fast_import_crash_6684\n```\n\nRegards,\nSergey\n\n15.10.2020 03:26, Jeff King wrote:\n\n> On Wed, Oct 14, 2020 at 11:22:03AM +0200, Dipl. Ing. Sergey Brester \n> wrote:\n> \n>> I did not see any issues with (even much larger) imports, before I \n>> upgraded git to 2.28.0 (from 2.25.1, I guess).\n> \n> I wasn't able to reproduce, but it sounds like there's something \n> special\n> about this repo (since the dump is not that big, and you said much\n> larger dumps have succeeded).\n> \n> Is it possible to make either the fossil repo, or the fast-import input\n> file available?\n> \n> If not, since you said it works with 2.14.4, is it possible to bisect \n> in\n> Git to see when it stopped working? You'll need to be able to build Git\n> from source.\n> \n> -Peff\n"},{"id":"407612","messageId":"20201015153849.GA551964@coredump.intra.peff.net","threadId":"54413","inReplyTo":"72a4d4d8dff95351122bd192976dd6b1@sebres.de","subject":"[PATCH] fast-import: fix over-allocation of marks storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T15:38:49Z","receivedAt":"2020-10-15T15:38:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 01:50:30PM +0200, Dipl. Ing. Sergey Brester wrote:\n\n> well, I don't know how you were trying to reproduce it.\n> \n> My first attempt with a git-repository (cloned from\n> https://github.com/git/git.git) showed that immediately to me.\n> Here you go (I used git bash here):\n> [...]\n\nThanks for this recipe. The key thing I was missing was having a\nreasonably large number of marks to be imported.\n\nThe problem bisects to ddddf8d7e2 (fast-import: permit reading multiple\nmarks files, 2020-02-22), which is in v2.27.0. The fix is below. Since\nwe're entering the -rc2 period for v2.29 today and this isn't a\nregression since v2.28, it probably won't make it into v2.29. But it's a\npretty serious bug (I'm actually surprised it took this long for anyone\nto notice, as mark importing of any decent size is basically broken),\nso I hope it will make it onto the maint branch soon after release.\n\n-- >8 --\nSubject: [PATCH] fast-import: fix over-allocation of marks storage\n\nFast-import stores its marks in a trie-like structure made of mark_set\nstructs. Each struct has a fixed size (1024). If our id number is too\nlarge to fit in the struct, then we allocate a new struct which shifts\nthe id number by 10 bits. Our original struct becomes a child node\nof this new layer, and the new struct becomes the top level of the trie.\n\nThis scheme was broken by ddddf8d7e2 (fast-import: permit reading\nmultiple marks files, 2020-02-22). Before then, we had a top-level\n\"marks\" pointer, and the push-down worked by assigning the new top-level\nstruct to \"marks\". But after that commit, insert_mark() takes a pointer\nto the mark_set, rather than using the global \"marks\". It continued to\nassign to the global \"marks\" variable during the push down, which was\nwrong for two reasons:\n\n  - we added a call in option_rewrite_submodules() which uses a separate\n    mark set; pushing down on \"marks\" is outright wrong here. We'd\n    corrupt the \"marks\" set, and we'd fail to correctly store any\n    submodule mappings with an id over 1024.\n\n  - the other callers passed \"marks\", but the push-down was still wrong.\n    In read_mark_file(), we take the pointer to the mark_set as a\n    parameter. So even though insert_mark() was updating the global\n    \"marks\", the local pointer we had in read_mark_file() was not\n    updated. As a result, we'd add a new level when needed, but then the\n    next call to insert_mark() wouldn't see it! It would then allocate a\n    new layer, which would also not be seen, and so on. Lookups for the\n    lost layers obviously wouldn't work, but before we even hit any\n    lookup stage, we'd generally run out of memory and die.\n\nOur tests didn't notice either of these cases because they didn't have\nenough marks to trigger the push-down behavior. The new tests in t9304\ncover both cases (and fail without this patch).\n\nWe can solve the problem by having insert_mark() take a pointer-to-pointer\nof the top-level of the set. Then our push down can assign to it in a\nway that the caller actually sees. Note the subtle reordering in\noption_rewrite_submodules(). Our call to read_mark_file() may modify our\ntop-level set pointer, so we have to wait until after it returns to\nassign its value into the string_list.\n\nReported-by: Sergey Brester <serg.brester@sebres.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTwo additional notes:\n\n  - we could rename the global to \"marks_toplevel\" or something to make\n    sure we got all references to it. But it makes the lookup code much\n    uglier (it has to use the new name, and otherwise doesn't need\n    touched by this patch). I actually did that temporarily to make sure\n    there weren't any other lingering references, but it was too ugly to\n    keep.\n\n  - there's another global in insert_mark(), which is marks_set_count.\n    We increment it once for each mark. We use the same counter whether\n    we're adding a real mark, or a submodule-rewrite mark. Since it's\n    not used for anything except reporting statistics at the end of the\n    program, I think it's fine (it's not clear whether somebody would\n    want the set of actual marks, or to know how often we had to call\n    into the mark-insertion code).\n\n builtin/fast-import.c        | 31 ++++++++++++----------\n t/t9304-fast-import-marks.sh | 51 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 14 deletions(-)\n create mode 100755 t/t9304-fast-import-marks.sh\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 1bf50a73dc..70d7d25eed 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/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@@ -526,13 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)\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 **top, uintmax_t idnum, struct object_entry *oe)\n {\n+\tstruct mark_set *s = *top;\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 = (*top)->shift + 10;\n+\t\ts->data.sets[0] = *top;\n+\t\t*top = s;\n \t}\n \twhile (s->shift) {\n \t\tuintmax_t i = idnum >> s->shift;\n@@ -944,7 +946,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@@ -1142,7 +1144,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@@ -1717,7 +1719,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@@ -1734,12 +1736,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@@ -1772,7 +1774,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@@ -3228,7 +3230,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@@ -3321,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)\n \t*f = '\\0';\n \tf++;\n \tms = xcalloc(1, sizeof(*ms));\n-\tstring_list_insert(list, s)->util = ms;\n \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+\tstring_list_insert(list, s)->util = ms;\n }\n \n static int parse_one_option(const char *option)\ndiff --git a/t/t9304-fast-import-marks.sh b/t/t9304-fast-import-marks.sh\nnew file mode 100755\nindex 0000000000..d4359dba21\n--- /dev/null\n+++ b/t/t9304-fast-import-marks.sh\n@@ -0,0 +1,51 @@\n+#!/bin/sh\n+\n+test_description='test exotic situations with marks'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup dump of basic history' '\n+\ttest_commit one &&\n+\tgit fast-export --export-marks=marks HEAD >dump\n+'\n+\n+test_expect_success 'setup large marks file' '\n+\t# normally a marks file would have a lot of useful, unique\n+\t# marks. But for our purposes, just having a lot of nonsense\n+\t# ones is fine. Start at 1024 to avoid clashing with marks\n+\t# legitimately used in our tiny dump.\n+\tblob=$(git rev-parse HEAD:one.t) &&\n+\tfor i in $(test_seq 1024 16384)\n+\tdo\n+\t\techo \":$i $blob\"\n+\tdone >>marks\n+'\n+\n+test_expect_success 'import with large marks file' '\n+\tgit fast-import --import-marks=marks <dump\n+'\n+\n+test_expect_success 'setup dump with submodule' '\n+\tgit submodule add \"$PWD\" sub &&\n+\tgit commit -m \"add submodule\" &&\n+\tgit fast-export HEAD >dump\n+'\n+\n+test_expect_success 'setup submodule mapping with large id' '\n+\told=$(git rev-parse HEAD:sub) &&\n+\tnew=$(echo $old | sed s/./a/g) &&\n+\techo \":12345 $old\" >from &&\n+\techo \":12345 $new\" >to\n+'\n+\n+test_expect_success 'import with submodule mapping' '\n+\tgit init dst &&\n+\tgit -C dst fast-import \\\n+\t\t--rewrite-submodules-from=sub:../from \\\n+\t\t--rewrite-submodules-to=sub:../to \\\n+\t\t<dump &&\n+\tgit -C dst rev-parse HEAD:sub >actual &&\n+\techo \"$new\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.29.0.rc1.562.g7bd9bc8902\n\n"},{"id":"407613","messageId":"79cbeb4c-5840-d5b7-a2b9-d72cf47968df@web.de","threadId":"54413","inReplyTo":"72a4d4d8dff95351122bd192976dd6b1@sebres.de","subject":"Re: git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-10-15T15:52:27Z","receivedAt":"2020-10-15T15:53:05Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.10.20 um 13:50 schrieb Dipl. Ing. Sergey Brester:\n> ```\n> # clone or copy git repository (we'll use it for export and import):\n> git clone https://github.com/git/git.git\n> cd git\n>\n> # make 1st fast-export in order to generate marks (don't need the dump, only marks are needed):\n> git fast-export --reencode=yes --export-marks=.tmp.exp-1.tmp e83c5163316f89bfbde7d9ab23ca2e25604af290..dc04167d378fb29d30e1647ff6ff51dd182bc9a3 > /dev/null\n>\n> # make 2nd fast-export in order to generate partial export dump (file \".tmp.dump\" will be ca. 87MB):\n> git fast-export --reencode=yes --import-marks=.tmp.exp-1.tmp --export-marks=.tmp.exp-2.tmp 61addb841f2a6d74a1737a01e03df1f773e04944..master > .tmp.dump\n\nIn case someone else is wondering about the meaning of those hashes,\nhere are their reference strings:\n\ne83c516331 (Initial revision of \"git\", the information manager from hell, 2005-04-07)\ndc04167d37 (Fourth batch, 2020-08-04)\n61addb841f (Merge branch 'jk/strvec' into next, 2020-08-04)\n\n> # now try to import this dump, using first marks as import marks (we have all revs in git-repo):\n> git fast-import --import-marks=.tmp.exp-1.tmp --export-marks=.tmp.imp.tmp < .tmp.dump\n>\n> ```\n\nSo you use the marks generated by the first export to import the second\nexport.  I wonder if that's relevant to trigger the memory allocation\nissue.\n\n> And see how git-fast-import eating your whole memory and enjoy the crash :)\n>\n> ```\n> fatal: Out of memory, malloc failed (tried to allocate 2097152 bytes)\n> fast-import: dumping crash report to .git/fast_import_crash_6684\n> ```\n\nI can reproduce this on Debian -- but don't get a crash report, just a\nnotice from the OOM killer.  It bisects to ddddf8d7e2 (fast-import:\npermit reading multiple marks files, 2020-02-22).\n\nRené\n"},{"id":"407619","messageId":"181f35ca5dff05d8d2288f020354a860@sebres.de","threadId":"54413","inReplyTo":"79cbeb4c-5840-d5b7-a2b9-d72cf47968df@web.de","subject":"Re: git fast-import leaks memory drastically, so crashes with out of memory by attempt to import 22MB export dump","fromName":"Dipl. Ing. Sergey Brester","fromEmail":"serg.brester@sebres.de","sentAt":"2020-10-15T16:19:34Z","receivedAt":"2020-10-15T16:19:39Z","isPatch":false,"sender":{"key":"serg.brester@sebres.de","avatar":null},"body":"15.10.2020 17:52, René Scharfe wrote:\n\n> In case someone else is wondering about the meaning of those hashes,\n> here are their reference strings:\n> e83c516331 (Initial revision of \"git\", the information manager from \n> hell, 2005-04-07)\n> dc04167d37 (Fourth batch, 2020-08-04)\n> 61addb841f (Merge branch 'jk/strvec' into next, 2020-08-04)\n> \n\nThese are just revisions of master branch, and the process was splittet \ninto two parts to generate a large marks file with a small partial dump, \nthat can be imported over first marks.\nSo quasi to simulate normal situation (large repo, small partial export \ndump), for which import/export with marks is basically used.\nIt doesn't matter here, just helpful to export/import single branch only \n(to bypass signed tags, avoid installing gpg keys, etc).\n\n> So you use the marks generated by the first export to import the second\n> export.\n\nIt also doesn't matter, because you could import whole first dump \n(several gigabytes, I guess) in order to generate new import marks...\nWhich will then be the same after all :) just because it is the same \nrepository used for export (and all the revisions are already there, \nmoreover having the same hash values, let alone internal marks index and \nthe export sequence).\n\n> I wonder if that's relevant to trigger the memory allocation issue.\n\nNo, it is not relevant.\nAlso note my initial report where it is affected by real situation (over \nimport marks generated previosly with git fast-import).\n\n> I can reproduce this on Debian -- but don't get a crash report, just a\n> notice from the OOM killer. It bisects to ddddf8d7e2 (fast-import:\n> permit reading multiple marks files, 2020-02-22).\n\nSure, OOM manager kills this process (before it is able to crash e. g. \ncreate a dump on failed malloc)...\nIf you'll disable OOMK (or try to run it on windows), you'll see the \ncrash and dump is there. :)\n\nRegards,\nSergey.\n"},{"id":"407642","messageId":"xmqqzh4nfldw.fsf@gitster.c.googlers.com","threadId":"54413","inReplyTo":"20201015153849.GA551964@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T17:29:31Z","receivedAt":"2020-10-15T17:29:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] fast-import: fix over-allocation of marks storage\n>\n> Fast-import stores its marks in a trie-like structure made of mark_set\n> structs. Each struct has a fixed size (1024). If our id number is too\n> large to fit in the struct, then we allocate a new struct which shifts\n> the id number by 10 bits. Our original struct becomes a child node\n> of this new layer, and the new struct becomes the top level of the trie.\n>\n> This scheme was broken by ddddf8d7e2 (fast-import: permit reading\n> multiple marks files, 2020-02-22). Before then, we had a top-level\n> \"marks\" pointer, and the push-down worked by assigning the new top-level\n> struct to \"marks\". But after that commit, insert_mark() takes a pointer\n> to the mark_set, rather than using the global \"marks\". It continued to\n> assign to the global \"marks\" variable during the push down, which was\n> wrong for two reasons:\n>\n>   - we added a call in option_rewrite_submodules() which uses a separate\n>     mark set; pushing down on \"marks\" is outright wrong here. We'd\n>     corrupt the \"marks\" set, and we'd fail to correctly store any\n>     submodule mappings with an id over 1024.\n>\n>   - the other callers passed \"marks\", but the push-down was still wrong.\n>     In read_mark_file(), we take the pointer to the mark_set as a\n>     parameter. So even though insert_mark() was updating the global\n>     \"marks\", the local pointer we had in read_mark_file() was not\n>     updated. As a result, we'd add a new level when needed, but then the\n>     next call to insert_mark() wouldn't see it! It would then allocate a\n>     new layer, which would also not be seen, and so on. Lookups for the\n>     lost layers obviously wouldn't work, but before we even hit any\n>     lookup stage, we'd generally run out of memory and die.\n>\n> Our tests didn't notice either of these cases because they didn't have\n> enough marks to trigger the push-down behavior. The new tests in t9304\n> cover both cases (and fail without this patch).\n>\n> We can solve the problem by having insert_mark() take a pointer-to-pointer\n> of the top-level of the set. Then our push down can assign to it in a\n> way that the caller actually sees. Note the subtle reordering in\n> option_rewrite_submodules(). Our call to read_mark_file() may modify our\n> top-level set pointer, so we have to wait until after it returns to\n> assign its value into the string_list.\n\nNice.\n\nWhy does this vaguely look familiar, I wonder.  I can swear that we\nsaw a breakage due to a similar pattern that attempts to convert a\nglobal to an arg that is passed down to the callchain but not doing\nso fully.\n\nAnyway, the diagnoses above look correct and cleanly described.\n\nWill queue.  Thanks.\n\n>\n> Reported-by: Sergey Brester <serg.brester@sebres.de>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Two additional notes:\n>\n>   - we could rename the global to \"marks_toplevel\" or something to make\n>     sure we got all references to it. But it makes the lookup code much\n>     uglier (it has to use the new name, and otherwise doesn't need\n>     touched by this patch). I actually did that temporarily to make sure\n>     there weren't any other lingering references, but it was too ugly to\n>     keep.\n>\n>   - there's another global in insert_mark(), which is marks_set_count.\n>     We increment it once for each mark. We use the same counter whether\n>     we're adding a real mark, or a submodule-rewrite mark. Since it's\n>     not used for anything except reporting statistics at the end of the\n>     program, I think it's fine (it's not clear whether somebody would\n>     want the set of actual marks, or to know how often we had to call\n>     into the mark-insertion code).\n>\n>  builtin/fast-import.c        | 31 ++++++++++++----------\n>  t/t9304-fast-import-marks.sh | 51 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 68 insertions(+), 14 deletions(-)\n>  create mode 100755 t/t9304-fast-import-marks.sh\n>\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 1bf50a73dc..70d7d25eed 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/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> @@ -526,13 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)\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 **top, uintmax_t idnum, struct object_entry *oe)\n>  {\n> +\tstruct mark_set *s = *top;\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 = (*top)->shift + 10;\n> +\t\ts->data.sets[0] = *top;\n> +\t\t*top = s;\n>  \t}\n>  \twhile (s->shift) {\n>  \t\tuintmax_t i = idnum >> s->shift;\n> @@ -944,7 +946,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> @@ -1142,7 +1144,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> @@ -1717,7 +1719,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> @@ -1734,12 +1736,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> @@ -1772,7 +1774,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> @@ -3228,7 +3230,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> @@ -3321,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)\n>  \t*f = '\\0';\n>  \tf++;\n>  \tms = xcalloc(1, sizeof(*ms));\n> -\tstring_list_insert(list, s)->util = ms;\n>  \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> +\tstring_list_insert(list, s)->util = ms;\n>  }\n>  \n>  static int parse_one_option(const char *option)\n> diff --git a/t/t9304-fast-import-marks.sh b/t/t9304-fast-import-marks.sh\n> new file mode 100755\n> index 0000000000..d4359dba21\n> --- /dev/null\n> +++ b/t/t9304-fast-import-marks.sh\n> @@ -0,0 +1,51 @@\n> +#!/bin/sh\n> +\n> +test_description='test exotic situations with marks'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup dump of basic history' '\n> +\ttest_commit one &&\n> +\tgit fast-export --export-marks=marks HEAD >dump\n> +'\n> +\n> +test_expect_success 'setup large marks file' '\n> +\t# normally a marks file would have a lot of useful, unique\n> +\t# marks. But for our purposes, just having a lot of nonsense\n> +\t# ones is fine. Start at 1024 to avoid clashing with marks\n> +\t# legitimately used in our tiny dump.\n> +\tblob=$(git rev-parse HEAD:one.t) &&\n> +\tfor i in $(test_seq 1024 16384)\n> +\tdo\n> +\t\techo \":$i $blob\"\n> +\tdone >>marks\n> +'\n> +\n> +test_expect_success 'import with large marks file' '\n> +\tgit fast-import --import-marks=marks <dump\n> +'\n> +\n> +test_expect_success 'setup dump with submodule' '\n> +\tgit submodule add \"$PWD\" sub &&\n> +\tgit commit -m \"add submodule\" &&\n> +\tgit fast-export HEAD >dump\n> +'\n> +\n> +test_expect_success 'setup submodule mapping with large id' '\n> +\told=$(git rev-parse HEAD:sub) &&\n> +\tnew=$(echo $old | sed s/./a/g) &&\n> +\techo \":12345 $old\" >from &&\n> +\techo \":12345 $new\" >to\n> +'\n> +\n> +test_expect_success 'import with submodule mapping' '\n> +\tgit init dst &&\n> +\tgit -C dst fast-import \\\n> +\t\t--rewrite-submodules-from=sub:../from \\\n> +\t\t--rewrite-submodules-to=sub:../to \\\n> +\t\t<dump &&\n> +\tgit -C dst rev-parse HEAD:sub >actual &&\n> +\techo \"$new\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n"},{"id":"407643","messageId":"xmqqv9fbfl68.fsf@gitster.c.googlers.com","threadId":"54413","inReplyTo":"xmqqzh4nfldw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T17:34:07Z","receivedAt":"2020-10-15T17:34:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Why does this vaguely look familiar, I wonder.  I can swear that we\n> saw a breakage due to a similar pattern that attempts to convert a\n> global to an arg that is passed down to the callchain but not doing\n> so fully.\n\nAre we revisiting this thread?\n\nhttps://lore.kernel.org/git/xmqqtuzlld1d.fsf@gitster.c.googlers.com/\n\nI wonder what happend to the thread at the end ...\n\n> Anyway, the diagnoses above look correct and cleanly described.\n>\n> Will queue.  Thanks.\n"},{"id":"407645","messageId":"6606f7fc-763a-d81c-2e14-4c2699f81f88@web.de","threadId":"54413","inReplyTo":"20201015153849.GA551964@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-10-15T17:57:45Z","receivedAt":"2020-10-15T17:58:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.10.20 um 17:38 schrieb Jeff King:\n> On Thu, Oct 15, 2020 at 01:50:30PM +0200, Dipl. Ing. Sergey Brester wrote:\n>\n>> well, I don't know how you were trying to reproduce it.\n>>\n>> My first attempt with a git-repository (cloned from\n>> https://github.com/git/git.git) showed that immediately to me.\n>> Here you go (I used git bash here):\n>> [...]\n>\n> Thanks for this recipe. The key thing I was missing was having a\n> reasonably large number of marks to be imported.\n>\n> The problem bisects to ddddf8d7e2 (fast-import: permit reading multiple\n> marks files, 2020-02-22), which is in v2.27.0. The fix is below. Since\n> we're entering the -rc2 period for v2.29 today and this isn't a\n> regression since v2.28, it probably won't make it into v2.29. But it's a\n> pretty serious bug (I'm actually surprised it took this long for anyone\n> to notice, as mark importing of any decent size is basically broken),\n> so I hope it will make it onto the maint branch soon after release.\n>\n> -- >8 --\n> Subject: [PATCH] fast-import: fix over-allocation of marks storage\n>\n> Fast-import stores its marks in a trie-like structure made of mark_set\n> structs. Each struct has a fixed size (1024). If our id number is too\n> large to fit in the struct, then we allocate a new struct which shifts\n> the id number by 10 bits. Our original struct becomes a child node\n> of this new layer, and the new struct becomes the top level of the trie.\n>\n> This scheme was broken by ddddf8d7e2 (fast-import: permit reading\n> multiple marks files, 2020-02-22). Before then, we had a top-level\n> \"marks\" pointer, and the push-down worked by assigning the new top-level\n> struct to \"marks\". But after that commit, insert_mark() takes a pointer\n> to the mark_set, rather than using the global \"marks\". It continued to\n> assign to the global \"marks\" variable during the push down, which was\n> wrong for two reasons:\n>\n>   - we added a call in option_rewrite_submodules() which uses a separate\n>     mark set; pushing down on \"marks\" is outright wrong here. We'd\n>     corrupt the \"marks\" set, and we'd fail to correctly store any\n>     submodule mappings with an id over 1024.\n>\n>   - the other callers passed \"marks\", but the push-down was still wrong.\n>     In read_mark_file(), we take the pointer to the mark_set as a\n>     parameter. So even though insert_mark() was updating the global\n>     \"marks\", the local pointer we had in read_mark_file() was not\n>     updated. As a result, we'd add a new level when needed, but then the\n>     next call to insert_mark() wouldn't see it! It would then allocate a\n>     new layer, which would also not be seen, and so on. Lookups for the\n>     lost layers obviously wouldn't work, but before we even hit any\n>     lookup stage, we'd generally run out of memory and die.\n>\n> Our tests didn't notice either of these cases because they didn't have\n> enough marks to trigger the push-down behavior. The new tests in t9304\n> cover both cases (and fail without this patch).\n>\n> We can solve the problem by having insert_mark() take a pointer-to-pointer\n> of the top-level of the set. Then our push down can assign to it in a\n> way that the caller actually sees. Note the subtle reordering in\n> option_rewrite_submodules(). Our call to read_mark_file() may modify our\n> top-level set pointer, so we have to wait until after it returns to\n> assign its value into the string_list.\n\nLooks good to me, and FWIW it fixes Sergey's test case for me as well.\n(I wish I'd seen it before I hit send on my other reply..)\n\n>\n> Reported-by: Sergey Brester <serg.brester@sebres.de>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Two additional notes:\n>\n>   - we could rename the global to \"marks_toplevel\" or something to make\n>     sure we got all references to it. But it makes the lookup code much\n>     uglier (it has to use the new name, and otherwise doesn't need\n>     touched by this patch). I actually did that temporarily to make sure\n>     there weren't any other lingering references, but it was too ugly to\n>     keep.\n>\n>   - there's another global in insert_mark(), which is marks_set_count.\n>     We increment it once for each mark. We use the same counter whether\n>     we're adding a real mark, or a submodule-rewrite mark. Since it's\n>     not used for anything except reporting statistics at the end of the\n>     program, I think it's fine (it's not clear whether somebody would\n>     want the set of actual marks, or to know how often we had to call\n>     into the mark-insertion code).\n\nSemi-related: We should turn the struct object_entry pointers in\ninsert_mark() and find_mark() into void pointers, since they are\nused \"generically\".  Perhaps struct mark_set would benefit from a\ntype member to implement our own type checks.\n\n>\n>  builtin/fast-import.c        | 31 ++++++++++++----------\n>  t/t9304-fast-import-marks.sh | 51 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 68 insertions(+), 14 deletions(-)\n>  create mode 100755 t/t9304-fast-import-marks.sh\n>\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 1bf50a73dc..70d7d25eed 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/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> @@ -526,13 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)\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 **top, uintmax_t idnum, struct object_entry *oe)\n>  {\n> +\tstruct mark_set *s = *top;\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 = (*top)->shift + 10;\n> +\t\ts->data.sets[0] = *top;\n> +\t\t*top = s;\n>  \t}\n>  \twhile (s->shift) {\n>  \t\tuintmax_t i = idnum >> s->shift;\n> @@ -944,7 +946,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> @@ -1142,7 +1144,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> @@ -1717,7 +1719,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> @@ -1734,12 +1736,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> @@ -1772,7 +1774,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> @@ -3228,7 +3230,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> @@ -3321,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)\n>  \t*f = '\\0';\n>  \tf++;\n>  \tms = xcalloc(1, sizeof(*ms));\n> -\tstring_list_insert(list, s)->util = ms;\n>\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> +\tstring_list_insert(list, s)->util = ms;\n>  }\n>\n>  static int parse_one_option(const char *option)\n> diff --git a/t/t9304-fast-import-marks.sh b/t/t9304-fast-import-marks.sh\n> new file mode 100755\n> index 0000000000..d4359dba21\n> --- /dev/null\n> +++ b/t/t9304-fast-import-marks.sh\n> @@ -0,0 +1,51 @@\n> +#!/bin/sh\n> +\n> +test_description='test exotic situations with marks'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup dump of basic history' '\n> +\ttest_commit one &&\n> +\tgit fast-export --export-marks=marks HEAD >dump\n> +'\n> +\n> +test_expect_success 'setup large marks file' '\n> +\t# normally a marks file would have a lot of useful, unique\n> +\t# marks. But for our purposes, just having a lot of nonsense\n> +\t# ones is fine. Start at 1024 to avoid clashing with marks\n> +\t# legitimately used in our tiny dump.\n> +\tblob=$(git rev-parse HEAD:one.t) &&\n> +\tfor i in $(test_seq 1024 16384)\n> +\tdo\n> +\t\techo \":$i $blob\"\n> +\tdone >>marks\n> +'\n> +\n> +test_expect_success 'import with large marks file' '\n> +\tgit fast-import --import-marks=marks <dump\n> +'\n> +\n> +test_expect_success 'setup dump with submodule' '\n> +\tgit submodule add \"$PWD\" sub &&\n> +\tgit commit -m \"add submodule\" &&\n> +\tgit fast-export HEAD >dump\n> +'\n> +\n> +test_expect_success 'setup submodule mapping with large id' '\n> +\told=$(git rev-parse HEAD:sub) &&\n> +\tnew=$(echo $old | sed s/./a/g) &&\n> +\techo \":12345 $old\" >from &&\n> +\techo \":12345 $new\" >to\n> +'\n> +\n> +test_expect_success 'import with submodule mapping' '\n> +\tgit init dst &&\n> +\tgit -C dst fast-import \\\n> +\t\t--rewrite-submodules-from=sub:../from \\\n> +\t\t--rewrite-submodules-to=sub:../to \\\n> +\t\t<dump &&\n> +\tgit -C dst rev-parse HEAD:sub >actual &&\n> +\techo \"$new\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n>\n\n"},{"id":"407651","messageId":"f3e4f7ccc36dc214201c1838acce4aff@sebres.de","threadId":"54413","inReplyTo":"xmqqv9fbfl68.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Dipl. Ing. Sergey Brester","fromEmail":"serg.brester@sebres.de","sentAt":"2020-10-15T18:09:47Z","receivedAt":"2020-10-15T18:09:57Z","isPatch":true,"sender":{"key":"serg.brester@sebres.de","avatar":null},"body":"Nice.\n\nBrian's patch looks indeed really similar.\n\nMay be this is a sign to introduce real issue tracker finally? :)\nNo offence, but I was always wondering how a team is able to hold all \nthe issue related stuff in form\nof a mailing list, without to experience such an \"embarrassments\".\nEspecially on such large projects and communities.\n\nRegards,\nSergey\n\n15.10.2020 19:34, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Why does this vaguely look familiar, I wonder. I can swear that we saw \n>> a breakage due to a similar pattern that attempts to convert a global \n>> to an arg that is passed down to the callchain but not doing so fully.\n> \n> Are we revisiting this thread?\n> \n> https://lore.kernel.org/git/xmqqtuzlld1d.fsf@gitster.c.googlers.com/ \n> [1]\n> \n> I wonder what happend to the thread at the end ...\n> \n>> Anyway, the diagnoses above look correct and cleanly described. Will \n>> queue. Thanks.\n\n\nLinks:\n------\n[1] https://lore.kernel.org/git/xmqqtuzlld1d.fsf@gitster.c.googlers.com/\n"},{"id":"407652","messageId":"xmqqo8l3fibz.fsf@gitster.c.googlers.com","threadId":"54413","inReplyTo":"f3e4f7ccc36dc214201c1838acce4aff@sebres.de","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T18:35:28Z","receivedAt":"2020-10-15T18:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dipl. Ing. Sergey Brester\" <serg.brester@sebres.de> writes:\n\n> May be this is a sign to introduce real issue tracker finally? :)\n> No offence, but I was always wondering how a team is able to hold all\n> the issue related stuff in form\n> of a mailing list, without to experience such an \"embarrassments\".\n> Especially on such large projects and communities.\n\nI do not know if an issue-tracker would have helped, though.  The\nissue was discovered and discussed there the day before:\n\n  https://lore.kernel.org/git/xmqqimg5o5fq.fsf@gitster.c.googlers.com/\n\nand then was discussed in other thread the next day.  Somehow the\ndiscussion petered out without producing the final patch to be\napplied.  For this particular case, what we need is a functioning\npatch tracker *and* people who pay attention to patches in the \"came\nvery close to conclusion but no final patch in the tree\" state.  We\nneed people who can volunteer their eyeballs and attention to nudge,\nprod and help patches to perfection, and that won't be me.\n\nBy the way, now I know why it looked familiar---the fix largely was\nmy code.  And the diff between Brian's from June and Peff's in this\nthread is indeed quite small (shown below), which actually worries\nme.  Was there something in the old attempt that was incomplete that\nmade us wait for the final finishing touches?  If so, is the current\nround missing the same thing?  Or perhaps the test was what was\nmissing in the old attempt, in which case it's perfect (in the\nattached diff, I excluded t/ directroy as the old fix didn't have\ntests).\n\nThanks.\n\ndiff --git w/builtin/fast-import.c c/builtin/fast-import.c\nindex 71289b21e3..70d7d25eed 100644\n--- w/builtin/fast-import.c\n+++ c/builtin/fast-import.c\n@@ -526,15 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)\n \treturn r;\n }\n \n-static void insert_mark(struct mark_set **sp, uintmax_t idnum, struct object_entry *oe)\n+static void insert_mark(struct mark_set **top, uintmax_t idnum, struct object_entry *oe)\n {\n-\tstruct mark_set *s = *sp;\n+\tstruct mark_set *s = *top;\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 = (*sp)->shift + 10;\n-\t\ts->data.sets[0] = (*sp);\n-\t\t(*sp) = s;\n+\t\ts->shift = (*top)->shift + 10;\n+\t\ts->data.sets[0] = *top;\n+\t\t*top = s;\n \t}\n \twhile (s->shift) {\n \t\tuintmax_t i = idnum >> s->shift;\n@@ -3323,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)\n \t*f = '\\0';\n \tf++;\n \tms = xcalloc(1, sizeof(*ms));\n-\tstring_list_insert(list, s)->util = ms;\n \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 \tfclose(fp);\n+\n+\tstring_list_insert(list, s)->util = ms;\n }\n \n static int parse_one_option(const char *option)\n"},{"id":"407656","messageId":"20201015185853.GA1108210@coredump.intra.peff.net","threadId":"54413","inReplyTo":"xmqqo8l3fibz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T18:58:53Z","receivedAt":"2020-10-15T18:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 11:35:28AM -0700, Junio C Hamano wrote:\n\n> \"Dipl. Ing. Sergey Brester\" <serg.brester@sebres.de> writes:\n> \n> > May be this is a sign to introduce real issue tracker finally? :)\n> > No offence, but I was always wondering how a team is able to hold all\n> > the issue related stuff in form\n> > of a mailing list, without to experience such an \"embarrassments\".\n> > Especially on such large projects and communities.\n> \n> I do not know if an issue-tracker would have helped, though.  The\n> issue was discovered and discussed there the day before:\n> \n>   https://lore.kernel.org/git/xmqqimg5o5fq.fsf@gitster.c.googlers.com/\n\nDoh, and I was so proud of myself for diagnosing and fixing it. ;)\n\nI hadn't read either of the threads you linked before today (I found\nthem in my \"catch up on list reading\" queue, though likely I would have\ndeclared bankruptcy before reading them anyway).\n\nAt least that explains my surprise that the issue was not reported\nearlier. It was. :)\n\nIMHO an issue tracker wouldn't really change things here. The original\ncan be found in the first page of results of:\n\n  https://lore.kernel.org/git/?q=fast-import+leak\n\n(though if you add \"-cooking -announce\" there is even less noise). I\ndon't know that searching an issue tracker would do much better.\n\n> By the way, now I know why it looked familiar---the fix largely was\n> my code.  And the diff between Brian's from June and Peff's in this\n> thread is indeed quite small (shown below), which actually worries\n> me.  Was there something in the old attempt that was incomplete that\n> made us wait for the final finishing touches?  If so, is the current\n> round missing the same thing?  Or perhaps the test was what was\n> missing in the old attempt, in which case it's perfect (in the\n> attached diff, I excluded t/ directroy as the old fix didn't have\n> tests).\n\nLooking over the thread, I don't see any problems pointed out (though\nas your diff below shows, the original patch missed the re-ordering\nrequired for the submodule mapping call).\n\nSo I'd prefer my patch because of that fix and because of the tests.\n\n-Peff\n"},{"id":"407657","messageId":"20201015190532.GB1108210@coredump.intra.peff.net","threadId":"54413","inReplyTo":"xmqqo8l3fibz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:05:32Z","receivedAt":"2020-10-15T19:05:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 11:35:28AM -0700, Junio C Hamano wrote:\n\n> For this particular case, what we need is a functioning\n> patch tracker *and* people who pay attention to patches in the \"came\n> very close to conclusion but no final patch in the tree\" state.  We\n> need people who can volunteer their eyeballs and attention to nudge,\n> prod and help patches to perfection, and that won't be me.\n\nUsually I'd expect this to be the responsibility of the patch submitter\nto make sure their stuff gets merged (and if not, to figure out why).\n\nPersonally I make a branch for almost every patch/series I submit, no\nmatter how trivial[1]. And then part of my daily ritual is seeing which\nones have been merged, and dropping them. You can use git-cherry for\nthat, though it's not 100% perfect (sometimes patches are munged as they\nare applied). I use a combination of that and aggressively rebasing\npatches forward (and eventually they rebase down into nothing when\nthey've been fully merged).\n\nGitHub PRs can also serve as an open bookmark if people use them, but\nthey're similarly not good at auto-closing because of our patch workflow\n(I don't know if GGG has any intelligence there, but it would presumably\nbe subject to the same git-cherry issues).\n\n-Peff\n"},{"id":"407658","messageId":"20201015190638.GC1108210@coredump.intra.peff.net","threadId":"54413","inReplyTo":"20201015190532.GB1108210@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:06:38Z","receivedAt":"2020-10-15T19:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 03:05:32PM -0400, Jeff King wrote:\n\n> Personally I make a branch for almost every patch/series I submit, no\n> matter how trivial[1]. And then part of my daily ritual is seeing which\n\nI forgot my footnote. It wasn't very interesting, but to avoid\nconfusion, it was:\n\n  I very occasionally will not bother to create a branch, but my mental\n  checklist for that is always: will I be sad if this patch gets lost in\n  the discussion and never applied? And the answer is almost always\n  \"yes\", so I create a branch.\n\n-Peff\n"},{"id":"407659","messageId":"xmqqft6ffgjy.fsf@gitster.c.googlers.com","threadId":"54413","inReplyTo":"20201015185853.GA1108210@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T19:13:53Z","receivedAt":"2020-10-15T19:16:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Looking over the thread, I don't see any problems pointed out (though\n> as your diff below shows, the original patch missed the re-ordering\n> required for the submodule mapping call).\n>\n> So I'd prefer my patch because of that fix and because of the tests.\n\nOh, no question about that.  I would take a fresh fix that was done\non a more recent codebase than the one that was 4 months old and had\nto be rebased.  Having a test too is a huge plus.\n\nThanks.  And by the way, thanks for the \"-cooking -announce\" tip ;-)\n\n"},{"id":"407661","messageId":"65f46be7283239e86bb345e94bc5dbda@sebres.de","threadId":"54413","inReplyTo":"xmqqo8l3fibz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Dipl. Ing. Sergey Brester","fromEmail":"serg.brester@sebres.de","sentAt":"2020-10-15T19:17:58Z","receivedAt":"2020-10-15T19:18:17Z","isPatch":true,"sender":{"key":"serg.brester@sebres.de","avatar":null},"body":"\nWell, a bug-tracker had some clear benefits:\n\n1. it has controlling and statistic mechanisms like a road-map, \nmilestones etc;\n\n2. the issues get a priority (so one is able to select bugs with high \nprecedence);\n\n3. they can be labeled or even characterized with other metrics to \nsignal seriousness of the issue or\nto emphasize them in order to be flashy in the road-map.\n\n4. the issues are bound to the participating contributors (reporter, \ndevs, tester, etc), so for example there are reports like \"open issues\nbelonging to me\", which could also help to organize work a bit \n(\"remember\" them).\n\n5. Transparency of the representation of issue or some lists is not to \ncompare with a thread in a mailing list at all.\n\nI could continue yet, but unsure the arguments will be heard or welcome \n(I know it is your work, and everyone\norganizes his/her workplace how one need or want). I was just wondering \nhow it could work, even for years.\nIn my opinion all pros of a bug-tracker are obvious for everyone once \nusing them.\n\nAnd an automatic export/import from/to tracker into mailing lists (to \nretain it for conservative part of\nold school participants) is mostly trivial.\n\nThanks,\nSerg.\n\n15.10.2020 20:35, Junio C Hamano wrote:\n\n> \"Dipl. Ing. Sergey Brester\" <serg.brester@sebres.de> writes:\n> \n>> May be this is a sign to introduce real issue tracker finally? :) No \n>> offence, but I was always wondering how a team is able to hold all the \n>> issue related stuff in form of a mailing list, without to experience \n>> such an \"embarrassments\". Especially on such large projects and \n>> communities.\n> \n> I do not know if an issue-tracker would have helped, though.\n"},{"id":"407674","messageId":"xmqq362ffdom.fsf@gitster.c.googlers.com","threadId":"54413","inReplyTo":"65f46be7283239e86bb345e94bc5dbda@sebres.de","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T20:15:53Z","receivedAt":"2020-10-15T20:16:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dipl. Ing. Sergey Brester\" <serg.brester@sebres.de> writes:\n\n> Well, a bug-tracker had some clear benefits:\n>\n> 1. it has controlling and statistic mechanisms like a road-map,\n> milestones etc;\n>\n> 2. the issues get a priority (so one is able to select bugs with high\n> precedence);\n>\n> 3. they can be labeled or even characterized with other metrics to\n> signal seriousness of the issue or\n> to emphasize them in order to be flashy in the road-map.\n>\n> 4. the issues are bound to the participating contributors (reporter,\n> devs, tester, etc), so for example there are reports like \"open issues\n> belonging to me\", which could also help to organize work a bit\n> (\"remember\" them).\n>\n> 5. Transparency of the representation of issue or some lists is not to\n> compare with a thread in a mailing list at all.\n\nWhat is curious in the above list is that their benefits are heavily\ndependent on people actually curating them.  No issue tracker\nautomatically makes random issues \"prioritized\", no issue tracker\nautomatically closes a stale and/or irrelevant issue.  And without\ncuration, it tends to become pile of useless reports.\n\n> I could continue yet, but unsure the arguments will be heard or\n> welcome ...\n\nI actually think people already _know_ the benefit of having well\ncurated tracker, and suspect that what needs to be stressed is *not*\nthe \"arguments\" for having one.  It just is that those who bring it\nup on this list never seem to be interested in maintaining the\ntracker in useful state, and assumes that issue curation magically\nhappens for free, and that is probably it never materializes.\n\nIt would be great if you are volunteering to be one of the issue\nmanagers, of course.  FWIW, I think somebody (jrnieder?) already\nruns a tracker for us.  I do not know how much community\nparticipation the instance gets to to keep it relevant.\n"},{"id":"407703","messageId":"20201016023735.GD490427@camp.crustytoothpaste.net","threadId":"54413","inReplyTo":"20201015185853.GA1108210@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-16T02:37:35Z","receivedAt":"2020-10-16T02:38:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-15 at 18:58:53, Jeff King wrote:\n> On Thu, Oct 15, 2020 at 11:35:28AM -0700, Junio C Hamano wrote:\n> > I do not know if an issue-tracker would have helped, though.  The\n> > issue was discovered and discussed there the day before:\n> > \n> >   https://lore.kernel.org/git/xmqqimg5o5fq.fsf@gitster.c.googlers.com/\n> \n> Doh, and I was so proud of myself for diagnosing and fixing it. ;)\n\nWell, you did write a great commit message and patch with functional\ntests.\n\n> Looking over the thread, I don't see any problems pointed out (though\n> as your diff below shows, the original patch missed the re-ordering\n> required for the submodule mapping call).\n> \n> So I'd prefer my patch because of that fix and because of the tests.\n\nYeah, I'm fine with taking your patch as well.  Thanks for the tests,\nwhich I think help us avoid future regressions here.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"407705","messageId":"20201016031834.GE490427@camp.crustytoothpaste.net","threadId":"54413","inReplyTo":"20201015190532.GB1108210@coredump.intra.peff.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-16T03:18:34Z","receivedAt":"2020-10-16T03:18:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-15 at 19:05:32, Jeff King wrote:\n> On Thu, Oct 15, 2020 at 11:35:28AM -0700, Junio C Hamano wrote:\n> \n> > For this particular case, what we need is a functioning\n> > patch tracker *and* people who pay attention to patches in the \"came\n> > very close to conclusion but no final patch in the tree\" state.  We\n> > need people who can volunteer their eyeballs and attention to nudge,\n> > prod and help patches to perfection, and that won't be me.\n> \n> Usually I'd expect this to be the responsibility of the patch submitter\n> to make sure their stuff gets merged (and if not, to figure out why).\n\nNormally I try to keep up with what's cooking emails, but I remember the\noriginal bug report came in on a day when I had some other event and I\nprobably got distracted with whatever else I was doing later and forgot\nabout keeping up with the patch.\n\nIt would be very convenient if we did have a functioning patch tracker\nwhich could be looked up by user, because then it'd be easier to monitor\none's own series.\n\n> Personally I make a branch for almost every patch/series I submit, no\n> matter how trivial[1]. And then part of my daily ritual is seeing which\n> ones have been merged, and dropping them. You can use git-cherry for\n> that, though it's not 100% perfect (sometimes patches are munged as they\n> are applied). I use a combination of that and aggressively rebasing\n> patches forward (and eventually they rebase down into nothing when\n> they've been fully merged).\n\nI'm really terrible at deleting data, so I have (exactly) 256 branches\nin my local repository.  Some of them are merged, and some are not.\nThis would be a viable approach if I were better about deleting old\nseries (and completing and sending in the prototypes I've built), or if\nI sent in series that were smaller so rebasing them were not so big of a\ntime commitment.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"407754","messageId":"20201016202520.GB3356073@coredump.intra.peff.net","threadId":"54413","inReplyTo":"20201016031834.GE490427@camp.crustytoothpaste.net","subject":"Re: [PATCH] fast-import: fix over-allocation of marks storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-16T20:25:20Z","receivedAt":"2020-10-16T20:25:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 16, 2020 at 03:18:34AM +0000, brian m. carlson wrote:\n\n> > Personally I make a branch for almost every patch/series I submit, no\n> > matter how trivial[1]. And then part of my daily ritual is seeing which\n> > ones have been merged, and dropping them. You can use git-cherry for\n> > that, though it's not 100% perfect (sometimes patches are munged as they\n> > are applied). I use a combination of that and aggressively rebasing\n> > patches forward (and eventually they rebase down into nothing when\n> > they've been fully merged).\n> \n> I'm really terrible at deleting data, so I have (exactly) 256 branches\n> in my local repository.  Some of them are merged, and some are not.\n> This would be a viable approach if I were better about deleting old\n> series (and completing and sending in the prototypes I've built), or if\n> I sent in series that were smaller so rebasing them were not so big of a\n> time commitment.\n\nThat's pretty good. I only have about 80. :)\n\nI hesitate to point anybody at my mass of it-works-for-me scripts for\nfear of making their lives worse, but certainly you (or anybody) is\nwelcome to adopt them if you want to do the aggressive-rebase thing. You\ncan see them at:\n\n  https://github.com/peff/git/tree/meta\n\nI generally do:\n\n  git clone --single-branch -b meta https://github.com/peff/git Meta\n\nin my git.git checkout. And then:\n\n  Meta/merged\n\nshows what has been merged to master or next. Running:\n\n  Meta/rebase\n\nwill try to rebase everything on its upstream (I almost always point\nbranch upstreams to origin/master, though the script also handles the\ncase that branch X's upstream is a local branch Y). That catches cases\nwhere the patch-ids changed enough that git-cherry can't recognize them.\n\nIdeally that shrinks merged series down to nothing through a combination\nof automatic and manual skipping.  Of course that will often yield\nconflicts if later already-merged patches in the series touched the same\nlines of code. You can either \"git rebase --skip\" past such commits, or\nif you realize the whole giant series is merged, just \"rebase --abort\"\nand delete the branch manually.\n\nPerhaps either those scripts or at least the techniques within them\nmight help with your cleanup.\n\nSort-of orthogonal, I also use:\n\n  Meta/private\n\nto create a branch \"private\" that is a merge of all of my topics on top\nof next (minus ones that have \"-wip\" in the name). That's what I use for\nmy daily-driver version of Git.\n\nThat actually helps somewhat with the rebase process because rerere\nlearns about conflicts early during this step, and then later the rebase\ncan reuse the results. It's not perfect though (it merges in\nalphabetical order, but the order in which topics graduate might be\ndifferent, meaning we see a conflict in reverse order. Junio's Meta\nscripts take a specific order from a topics file; but he has the luxury\nof knowing that the same topics file is what will be used to actually\ngraduate the topics for real).\n\nAnyway. I hope that might perhaps help a little, but don't feel\ncompelled to jump down the rabbit hole if you're happy with your own\nworkflows. ;)\n\n-Peff\n"}]}