{"thread":{"id":"56744","subject":"[PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","startedAt":"2021-10-21T03:39:48Z","lastAt":"2021-12-30T20:18:43Z","messageCount":63,"participants":["Taylor Blau","Eric Sunshine","Junio C Hamano","Ævar Arnfjörð Bjarmason","Derrick Stolee","Fabian Stelzer","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"439160","messageId":"cover.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":null,"subject":"[PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:44Z","receivedAt":"2021-10-21T03:39:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a topic that Ævar got me interested when he mentioned that t5319 is\nleaky[1].\n\nThis is the result of trying to get t5319 to pass when compiling with\nSANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in\nthe pack-bitmap code, and the remaining 10% are sort of random.\n\nI tried to separate these out based on their respective areas. The last 10% are\nall from leaking memory in the rev_info structure, which I punted on for now by\njust UNLEAK()-ing it. That's all done in the last patch. If we choose to take\nthat last patch, then t5319 passes even under SANITIZE=leak builds. But it's a\nlittle gross, so I'm happy to leave it out if others would prefer.\n\nThis is based on tb/fix-midx-rename-while-mapped, with a back-merge of\njs/windows-ci-path-fix in order to appease CI. It also contains a little bit of\noverlap with a patch[2] from Ævar in another series, which he and I discovered\nindependently. I cherry-picked his patch, since I haven't seen much action on\nthat series lately.\n\n[1]: https://lore.kernel.org/git/87wnmph73b.fsf@evledraar.gmail.com/\n[2]: https://lore.kernel.org/git/patch-v3-08.10-e0a3510dd88-20211013T222329Z-avarab@gmail.com/\n\nTaylor Blau (10):\n midx.c: clean up chunkfile after reading the MIDX\n midx.c: don't leak MIDX from verify_midx_file\n t/helper/test-read-midx.c: free MIDX within read_midx_file()\n builtin/pack-objects.c: don't leak memory via arguments\n builtin/repack.c: avoid leaking child arguments\n builtin/multi-pack-index.c: don't leak concatenated options\n pack-bitmap.c: avoid leaking via midx_bitmap_filename()\n pack-bitmap.c: don't leak type-level bitmaps\n pack-bitmap.c: more aggressively free in free_bitmap_index()\n t5319: UNLEAK() the remaining leaks\n\nÆvar Arnfjörð Bjarmason (1):\n pack-bitmap-write.c: don't return without stop_progress()\n\n builtin/log.c | 1 +\n builtin/multi-pack-index.c | 4 ++++\n builtin/pack-objects.c | 12 ++++++++----\n builtin/repack.c | 35 ++++++++++++++++++++++++-----------\n builtin/rev-list.c | 2 ++\n midx.c | 7 +++++--\n pack-bitmap-write.c | 8 +++++---\n pack-bitmap.c | 18 ++++++++++++++++--\n t/helper/test-read-midx.c | 3 ++-\n t/t5319-multi-pack-index.sh | 1 +\n 10 files changed, 68 insertions(+), 23 deletions(-)\n\n-- \n2.33.0.96.g73915697e6\n"},{"id":"439161","messageId":"30f6f23daf49814f479865eea5f9ee68de209d5f.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:47Z","receivedAt":"2021-10-21T03:39:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In order to read the contents of a MIDX, we initialize a chunkfile\nstructure which can read the table of contents and assign pointers into\ndifferent sections of the file for us.\n\nWe do call free(), since the chunkfile struct is heap allocated, but not\nthe more appropriate free_chunkfile(), which also frees memory that the\nstructure itself owns.\n\nCall that instead to avoid leaking memory in this function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Taylor Blau \n---\nThe patch contents are from Ævar, but the message is mine. I hope that\nhe doesn't mind me forging his sign-off here.\n\n midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8433086ac1..36e4754767 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_packs\", m->num_packs);\n \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_objects\", m->num_objects);\n\n+\tfree_chunkfile(cf);\n \treturn m;\n\n cleanup_fail:\n \tfree(m);\n \tfree(midx_name);\n-\tfree(cf);\n+\tfree_chunkfile(cf);\n \tif (midx_map)\n \t\tmunmap(midx_map, midx_size);\n \tif (0 <= fd)\n--\n2.33.0.96.g73915697e6\n\n"},{"id":"439162","messageId":"b0c79904ab7bdaee7a1bc7a55b0fb26b1f2cf8d3.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:50Z","receivedAt":"2021-10-21T03:39:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function midx.c:verify_midx_file() allocate a MIDX struct by calling\nload_multi_pack_index(). But when cleaning up, it calls free() without\nfreeing any resources associated with the MIDX.\n\nCall the more appropriate close_midx() which does free those resources,\nwhich causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 36e4754767..ad60e48468 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t * Remaining tests assume that we have objects, so we can\n \t\t * return here.\n \t\t */\n-\t\treturn verify_midx_error;\n+\t\tgoto cleanup;\n \t}\n \n \tif (flags & MIDX_PROGRESS)\n@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t}\n \tstop_progress(&progress);\n \n+cleanup:\n \tfree(pairs);\n+\tclose_midx(m);\n \n \treturn verify_midx_error;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439163","messageId":"5157edb41e298c2504ac0bd8b4163fc6851bc1f9.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 03/11] t/helper/test-read-midx.c: free MIDX within read_midx_file()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:53Z","receivedAt":"2021-10-21T03:40:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When calling `read_midx_file()` to show information about a MIDX or list\nthe objects contained within it we fail to call `close_midx()`, leaking\nthe memory allocated to store that MIDX.\n\nFix this by calling `close_midx()` before exiting the function. We can\ndrop the \"early\" return when `show_objects` is non-zero, since the next\ninstruction is also a return.\n\n(We could just as easily put a `cleanup` label here as with previous\npatches. But the only other time we terminate the function early is\nwhen we fail to load a MIDX in the first place. `close_midx()` does\nhandle a NULL argument, but the extra complexity is likely not\nwarranted).\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-read-midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex 9d6fa7a377..27072ba94d 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -55,9 +55,10 @@ static int read_midx_file(const char *object_dir, int show_objects)\n \t\t\tprintf(\"%s %\"PRIu64\"\\t%s\\n\",\n \t\t\t oid_to_hex(&oid), e.offset, e.p->pack_name);\n \t\t}\n-\t\treturn 0;\n \t}\n \n+\tclose_midx(m);\n+\n \treturn 0;\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439164","messageId":"dd3b9a949ea3c8c42801e162ed9df045a1848f05.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 04/11] builtin/pack-objects.c: don't leak memory via arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:55Z","receivedAt":"2021-10-21T03:40:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When constructing arguments to pass to setup_revision(), pack-objects\nonly frees the memory used by that array after calling\nget_object_list().\n\nEnsure that we call strvec_clear() whether or not we use the arguments\narray by cleaning up whenever we exit the function (and rewriting one\nearly return to jump to a label which frees the memory and then\nreturns).\n\nWe could avoid setting this array up altogether unless we are in the\nif-else block that calls get_object_list(), but setting up the argument\narray is intermingled with lots of other side-effects, e.g.:\n\n if (exclude_promisor_objects) {\n use_internal_rev_list = 1;\n fetch_if_missing = 0;\n strvec_push(&rp, \"--exclude-promisor-objects\");\n }\n\nSo it would be awkward to check exclude_promisor_objects twice: first to\nset use_internal_rev_list and fetch_if_missing, and then again above\nget_object_list() to push the relevant argument onto the array.\n\nInstead, leave the array's construction alone and make sure to free it\nunconditionally.\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1a3dd445f8..857be7826f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4148,11 +4148,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_packs_list_from_stdin();\n \t\tif (rev_list_unpacked)\n \t\t\tadd_unreachable_loose_objects();\n-\t} else if (!use_internal_rev_list)\n+\t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\telse {\n+\t} else {\n \t\tget_object_list(rp.nr, rp.v);\n-\t\tstrvec_clear(&rp);\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n@@ -4162,7 +4161,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t the_repository);\n \n \tif (non_empty && !nr_result)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \tif (nr_result) {\n \t\ttrace2_region_enter(\"pack-objects\", \"prepare-pack\",\n \t\t\t\t the_repository);\n@@ -4183,5 +4182,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t \" pack-reused %\"PRIu32),\n \t\t\t written, written_delta, reused, reused_delta,\n \t\t\t reuse_packfile_objects);\n+\n+cleanup:\n+\tstrvec_clear(&rp);\n+\n \treturn 0;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439165","messageId":"a68c77c00638bef7e8ce88929015a587d2d1b6f8.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 05/11] builtin/repack.c: avoid leaking child arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:39:57Z","receivedAt":"2021-10-21T03:40:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`git repack` invokes a handful of child processes: one to write the\nactual pack, and optionally ones to repack promisor objects and update\nthe MIDX.\n\nIn none of these cases do we bother to call child_process_clear(), which\nfrees the memory associated with each child's arguments and environment.\n\nIn order to do so, tweak each function that spawns a child process to\nhave a `cleanup` label that we always visit before returning from each\nfunction. Then, make sure that we call child_process_clear() as a part\nof that label.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0b2d1e5d82..d16bab09a4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tint ret = 0;\n \n \tprepare_pack_objects(&cmd, args);\n \tcmd.in = -1;\n@@ -260,7 +261,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \n \tif (cmd.in == -1)\n \t\t/* No packed objects; cmd was never started */\n-\t\treturn;\n+\t\tgoto cleanup;\n \n \tclose(cmd.in);\n \n@@ -293,7 +294,12 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\tfree(promisor_name);\n \t}\n \tfclose(out);\n-\tif (finish_command(&cmd))\n+\tret = finish_command(&cmd);\n+\n+cleanup:\n+\tchild_process_clear(&cmd);\n+\n+\tif (ret)\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n }\n \n@@ -559,10 +565,10 @@ static int write_midx_included_packs(struct string_list *include,\n \tstruct string_list_item *item;\n \tstruct packed_git *largest = get_largest_active_pack(geometry);\n \tFILE *in;\n-\tint ret;\n+\tint ret = 0;\n \n \tif (!include->nr)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \tcmd.in = -1;\n \tcmd.git_cmd = 1;\n@@ -587,14 +593,19 @@ static int write_midx_included_packs(struct string_list *include,\n \n \tret = start_command(&cmd);\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, include)\n \t\tfprintf(in, \"%s\\n\", item->string);\n \tfclose(in);\n \n-\treturn finish_command(&cmd);\n+\tret = finish_command(&cmd);\n+\n+cleanup:\n+\tchild_process_clear(&cmd);\n+\n+\treturn ret;\n }\n \n int cmd_repack(int argc, const char **argv, const char *prefix)\n@@ -608,7 +619,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct pack_geometry *geometry = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n-\tint i, ext, ret;\n+\tint i, ext, ret = 0;\n \tFILE *out;\n \tint show_progress = isatty(2);\n \n@@ -794,7 +805,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tret = start_command(&cmd);\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \n \tif (geometry) {\n \t\tFILE *in = xfdopen(cmd.in, \"w\");\n@@ -819,7 +830,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfclose(out);\n \tret = finish_command(&cmd);\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \n \tif (!names.nr && !po_args.quiet)\n \t\tprintf_ln(_(\"Nothing new to pack.\"));\n@@ -893,7 +904,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&include, 0);\n \n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto cleanup;\n \t}\n \n \treprepare_packed_git(the_repository);\n@@ -946,12 +957,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n \t}\n \n+cleanup:\n \tstring_list_clear(&names, 0);\n \tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\n \tstrbuf_release(&line);\n+\tchild_process_clear(&cmd);\n \n-\treturn 0;\n+\treturn ret;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439166","messageId":"ffded80c7d20b6e22683f6269208d0a73dee67c6.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 06/11] builtin/multi-pack-index.c: don't leak concatenated options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:00Z","receivedAt":"2021-10-21T03:40:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `multi-pack-index` builtin dynamically allocates an array of\ncommand-line option for each of its separate modes by calling\nadd_common_options() to concatante the common options with sub-command\nspecific ones.\n\nBecause this operation allocates a new array, we have to be careful to\nremember to free it. We already do this in the repack and write\nsub-commands, but verify and expire don't. Rectify this by calling\nFREE_AND_NULL as the other modes do.\n\nSigned-off-by: Taylor Blau \n---\n builtin/multi-pack-index.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 075d15d706..4480ba3982 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -167,6 +167,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n \t\t\t\t options);\n \n+\tFREE_AND_NULL(options);\n+\n \treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n }\n \n@@ -191,6 +193,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n \t\t\t\t options);\n \n+\tFREE_AND_NULL(options);\n+\n \treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439167","messageId":"f3897c3afc0e47933289025c269a4d5f248241ef.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:02Z","receivedAt":"2021-10-21T03:40:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To construct the path for a MIDX bitmap, midx_bitmap_filename() calls\nget_midx_filename(), and uses the result as an argument to xstrfmt().\nBut get_midx_filename() also uses xstrfmt, and it assumes that the\ncaller will free the results.\n\nget_midx_filename() could take a strbuf and write to that, but it would\nmake more work for every other caller. midx_bitmap_filename() could\nstore the result of get_midx_filename() in a temporary variable which is\nfreed before returning.\n\nBut that intermediate allocation feels wasteful. Since this function is\nsimple enough, we can instead \"inline\" its implementation into the\nxstrfmt call itself. That is a little gross, but arguably better than\nallocating memory unnecessarily.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex f47a0a7db4..d292e81af1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -292,8 +292,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \n char *midx_bitmap_filename(struct multi_pack_index *midx)\n {\n-\treturn xstrfmt(\"%s-%s.bitmap\",\n-\t\t get_midx_filename(midx->object_dir),\n+\treturn xstrfmt(\"%s/pack/multi-pack-index-%s.bitmap\",\n+\t\t midx->object_dir,\n \t\t hash_to_hex(get_midx_checksum(midx)));\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439168","messageId":"29920e773527e6e82065205ed98262ce9b01de28.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:05Z","receivedAt":"2021-10-21T03:40:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,\nwhich compares the result of the on-disk bitmaps with ones generated\non-the-fly during a revision walk.\n\nIn fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type\nbitmaps, 2021-08-24), we hardened those tests to also check the four\nspecial type-level bitmaps, but never freed those bitmaps. We should\nhave, since each required an allocation when we EWAH-decompressed them.\n\nFree those, plugging that leak, and also free the base (the scratch-pad\nbitmap), too.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex d292e81af1..0f6656db02 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1721,6 +1721,12 @@ void test_bitmap_walk(struct rev_info *revs)\n \telse\n \t\tdie(\"mismatch in bitmap results\");\n \n+\tbitmap_free(result);\n+\tbitmap_free(tdata.base);\n+\tbitmap_free(tdata.commits);\n+\tbitmap_free(tdata.trees);\n+\tbitmap_free(tdata.blobs);\n+\tbitmap_free(tdata.tags);\n \tfree_bitmap_index(bitmap_git);\n }\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439169","messageId":"e65ac7deb5973b8209e8d94ad9140e005e22c44e.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:07Z","receivedAt":"2021-10-21T03:40:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function free_bitmap_index() is somewhat lax in what it frees. There\nare two notable examples:\n\n - While it does call kh_destroy_oid_map on the \"bitmaps\" map (which\n maps) commit OIDs to their corresponding bitmaps, the bitmaps\n themselves are not freed. Note here that we recycle already-freed\n ewah_bitmaps into a pool, but these are handled correctly by\n ewah_pool_free().\n\n - We never bother to free the extended index's \"positions\" map, which\n we always allocate in load_bitmap().\n\nFix both of these.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 0f6656db02..451ca3512c 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b)\n \tewah_pool_free(b->trees);\n \tewah_pool_free(b->blobs);\n \tewah_pool_free(b->tags);\n+\tif (b->bitmaps) {\n+\t\tstruct stored_bitmap *sb;\n+\t\tkh_foreach_value(b->bitmaps, sb, {\n+\t\t\tewah_pool_free(sb->root);\n+\t\t\tfree(sb);\n+\t\t});\n+\t}\n \tkh_destroy_oid_map(b->bitmaps);\n \tfree(b->ext_index.objects);\n \tfree(b->ext_index.hashes);\n+\tkh_destroy_oid_pos(b->ext_index.positions);\n \tbitmap_free(b->result);\n \tbitmap_free(b->haves);\n \tif (bitmap_is_midx(b)) {\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439170","messageId":"f1bb8b73ffdb78995dc5653791f9a64adf216e21.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 11/11] t5319: UNLEAK() the remaining leaks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:12Z","receivedAt":"2021-10-21T03:40:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This resolves the remaining leaks uncovered by running t5319 when\nbuilding with SANITIZE=leak. All of the leaks below are related to\nallocated memory stored in 'struct rev_info' which is never freed.\n\nWe could alternatively implement a function which frees all allocated\nmemory associated with the rev_info struct, but doing this properly is a\nbigger undertaking than I want to deal with right now.\n\nInstead, let's just UNLEAK() the few ones that are holding up t5319,\nwhich enables us to mark that test as TEST_PASSES_SANITIZE_LEAK.\n\nSigned-off-by: Taylor Blau \n---\n builtin/log.c | 1 +\n builtin/pack-objects.c | 1 +\n builtin/rev-list.c | 2 ++\n pack-bitmap-write.c | 2 ++\n t/t5319-multi-pack-index.sh | 1 +\n 5 files changed, 7 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f75d87e8d7..ad6dfacf77 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -757,6 +757,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \topt.revarg_opt = REVARG_COMMITTISH;\n \topt.tweak = log_setup_revisions_tweak;\n \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n+\tUNLEAK(rev);\n \treturn cmd_log_walk(&rev);\n }\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 857be7826f..6128a2e2a5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3759,6 +3759,7 @@ static void get_object_list(int ac, const char **av)\n \t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(_(\"bad revision '%s'\"), line);\n \t}\n+\tUNLEAK(revs);\n \n \twarn_on_object_refname_ambiguity = save_warning;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 36cb909eba..df3811e763 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \targc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \n+\tUNLEAK(revs);\n+\n \tmemset(&info, 0, sizeof(info));\n \tinfo.revs = &revs;\n \tif (revs.bisect)\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex cab3eaa2ac..742bae4f57 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -234,6 +234,8 @@ static void bitmap_builder_init(struct bitmap_builder *bb,\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \n+\tUNLEAK(revs);\n+\n \twhile ((commit = get_revision(&revs))) {\n \t\tstruct commit_list *p = commit->parents;\n \t\tstruct bb_commit *c_ent;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex a3c72b68f7..9cfc3d6661 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -3,6 +3,7 @@\n test_description='multi-pack-indexes'\n . ./test-lib.sh\n \n+TEST_PASSES_SANITIZE_LEAK=true\n GIT_TEST_MULTI_PACK_INDEX=0\n objdir=.git/objects\n \n-- \n2.33.0.96.g73915697e6\n"},{"id":"439171","messageId":"cb30aa67c0023c435cf472303bbf4894c8b2d7ec.1634787555.git.me@ttaylorr.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"[PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-21T03:40:09Z","receivedAt":"2021-10-21T03:40:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Ævar Arnfjörð Bjarmason \n\nFix a bug that's been here since 7cc8f971085 (pack-objects: implement\nbitmap writing, 2013-12-21), we did not call stop_progress() if we\nreached the early exit in this function.\n\nWe could call stop_progress() before we return, but better yet is to\ndefer calling start_progress() until we need it.\n\nThis will matter in a subsequent commit where we BUG(...) out if this\nhappens, and matters now e.g. because we don't have a corresponding\n\"region_end\" for the progress trace2 event.\n\nSuggested-by: SZEDER Gábor \nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Taylor Blau \n---\n pack-bitmap-write.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 9c55c1531e..cab3eaa2ac 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \n \tQSORT(indexed_commits, indexed_commits_nr, date_compare);\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n-\n \tif (indexed_commits_nr < 100) {\n \t\tfor (i = 0; i < indexed_commits_nr; ++i)\n \t\t\tpush_bitmapped_commit(indexed_commits[i]);\n \t\treturn;\n \t}\n \n+\tif (writer.show_progress)\n+\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n+\n \tfor (;;) {\n \t\tstruct commit *chosen = NULL;\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"439173","messageId":"CAPig+cSnrrT_wELSLbk=Shfu_e_6YM9LpMw2X75-oN0QqOAY3Q@mail.gmail.com","threadId":"56744","inReplyTo":"b0c79904ab7bdaee7a1bc7a55b0fb26b1f2cf8d3.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-21T05:00:45Z","receivedAt":"2021-10-21T05:00:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau wrote:\n> The function midx.c:verify_midx_file() allocate a MIDX struct by calling\n> load_multi_pack_index(). But when cleaning up, it calls free() without\n> freeing any resources associated with the MIDX.\n\ns/allocate/allocates/\n\n> Call the more appropriate close_midx() which does free those resources,\n> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> diff --git a/midx.c b/midx.c\n> @@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n> - return verify_midx_error;\n> + goto cleanup;\n> }\n> @@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n> stop_progress(&progress);\n>\n> +cleanup:\n> free(pairs);\n> + close_midx(m);\n>\n> return verify_midx_error;\n> }\n\nUsing the `goto` idiom to ensure cleanup makes perfect sense. For a\nfew reasons[*], I did spend a some moments wondering if the cognitive\nload might be a bit lower by instead adding two close_midx() calls --\none at the early return and one just before the normal return --\nrather than using a `goto`, but it's so subjective that it's not worth\nworrying about.\n\nFOOTNOTES\n\n[*] First, unlike most cases in which the `goto` jumps over relatively\nshort blocks of code, the distance in this case between `goto` and the\nnew label is significant and it's not easy to see at a glance what is\nbeing skipped and how important it might be. Second, `pairs` is still\nNULL at the point of the `goto`; I spent extra time checking and\ndouble-checking what free(pairs) was doing in this instance. Finally,\nthere are enough start/stop-progress calls in this function that it\nrequires extra mental effort to determine that this `goto` is indeed\nsafe (at least for the present).\n"},{"id":"439174","messageId":"CAPig+cToh_8x+72-g6cdvt559N4GqKwfmBDE9BxTpkXOFrRbsA@mail.gmail.com","threadId":"56744","inReplyTo":"e65ac7deb5973b8209e8d94ad9140e005e22c44e.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-21T05:10:26Z","receivedAt":"2021-10-21T05:10:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau wrote:\n> The function free_bitmap_index() is somewhat lax in what it frees. There\n> are two notable examples:\n>\n> - While it does call kh_destroy_oid_map on the \"bitmaps\" map (which\n> maps) commit OIDs to their corresponding bitmaps, the bitmaps\n> themselves are not freed. Note here that we recycle already-freed\n> ewah_bitmaps into a pool, but these are handled correctly by\n> ewah_pool_free().\n\nThe parentheses placement seems off; it's not clear what the intent\nis. Perhaps either move the closing parenthesis to just before the\ncomma or drop them altogether.\n\n> - We never bother to free the extended index's \"positions\" map, which\n> we always allocate in load_bitmap().\n>\n> Fix both of these.\n>\n> Signed-off-by: Taylor Blau \n"},{"id":"439175","messageId":"CAPig+cRpw=RhojwhyrEjYK0DVMEQ6xNZDCANecLGX=9mhfpQOw@mail.gmail.com","threadId":"56744","inReplyTo":"cb30aa67c0023c435cf472303bbf4894c8b2d7ec.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-21T05:12:36Z","receivedAt":"2021-10-21T05:12:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau wrote:\n> From: Ævar Arnfjörð Bjarmason \n>\n> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement\n> bitmap writing, 2013-12-21), we did not call stop_progress() if we\n> reached the early exit in this function.\n>\n> We could call stop_progress() before we return, but better yet is to\n> defer calling start_progress() until we need it.\n>\n> This will matter in a subsequent commit where we BUG(...) out if this\n> happens, and matters now e.g. because we don't have a corresponding\n> \"region_end\" for the progress trace2 event.\n\nNit: There is no subsequent commit in this series which \"BUGS out\", so\nthis comment is confusing.\n\n> Suggested-by: SZEDER Gábor \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Taylor Blau \n"},{"id":"439177","messageId":"xmqqfssvncyi.fsf@gitster.g","threadId":"56744","inReplyTo":"30f6f23daf49814f479865eea5f9ee68de209d5f.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T05:50:29Z","receivedAt":"2021-10-21T05:50:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> The patch contents are from Ævar, but the message is mine. I hope that\n> he doesn't mind me forging his sign-off here.\n>\n> midx.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 8433086ac1..36e4754767 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_packs\", m->num_packs);\n> \ttrace2_data_intmax(\"midx\", the_repository, \"load/num_objects\", m->num_objects);\n>\n> +\tfree_chunkfile(cf);\n> \treturn m;\n>\n> cleanup_fail:\n> \tfree(m);\n> \tfree(midx_name);\n> -\tfree(cf);\n> +\tfree_chunkfile(cf);\n> \tif (midx_map)\n> \t\tmunmap(midx_map, midx_size);\n> \tif (0 <= fd)\n\nThe former is not something we can mechanically locate, but the\nlatter we should be able to. And indeed this is the only instance\nthe following experiment finds.\n\n\t$ cat >contrib/coccinelle/chunkfile.cocci <<-\\EOF\n\t@@\n\tidentifier f !~ \"^free_chunkfile$\";\n\tstruct chunkfile *cf;\n\t@@\n\t f(...) {<...\n\t- free(cf)\n\t+ free_chunkfile(cf)\n\t ...>}\n\tEOF\n\t$ make contrib/coccinelle/chunkfile.cocci.patch\n\nThanks.\n\n"},{"id":"439178","messageId":"xmqqbl3iorbj.fsf@gitster.g","threadId":"56744","inReplyTo":"CAPig+cSnrrT_wELSLbk=Shfu_e_6YM9LpMw2X75-oN0QqOAY3Q@mail.gmail.com","subject":"Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T05:54:56Z","receivedAt":"2021-10-21T05:55:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau wrote:\n>> The function midx.c:verify_midx_file() allocate a MIDX struct by calling\n>> load_multi_pack_index(). But when cleaning up, it calls free() without\n>> freeing any resources associated with the MIDX.\n>\n> s/allocate/allocates/\n>\n>> Call the more appropriate close_midx() which does free those resources,\n>> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n>>\n>> Signed-off-by: Taylor Blau \n>> ---\n>> diff --git a/midx.c b/midx.c\n>> @@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n>> - return verify_midx_error;\n>> + goto cleanup;\n>> }\n>> @@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n>> stop_progress(&progress);\n>>\n>> +cleanup:\n>> free(pairs);\n>> + close_midx(m);\n>>\n>> return verify_midx_error;\n>> }\n>\n> Using the `goto` idiom to ensure cleanup makes perfect sense. For a\n> few reasons[*], I did spend a some moments wondering if the cognitive\n> load might be a bit lower by instead adding two close_midx() calls --\n> one at the early return and one just before the normal return --\n> rather than using a `goto`, but it's so subjective that it's not worth\n> worrying about.\n>\n> FOOTNOTES\n>\n> [*] First, unlike most cases in which the `goto` jumps over relatively\n> short blocks of code, the distance in this case between `goto` and the\n> new label is significant and it's not easy to see at a glance what is\n> being skipped and how important it might be. Second, `pairs` is still\n> NULL at the point of the `goto`; I spent extra time checking and\n> double-checking what free(pairs) was doing in this instance. Finally,\n> there are enough start/stop-progress calls in this function that it\n> requires extra mental effort to determine that this `goto` is indeed\n> safe (at least for the present).\n\nCarefully reading the patch is very good, but at least in a patch\nlike this, it is immediately obvious that the patch is not making\nany of the things the footnote worries about any worse than the\noriginal, by replacing \"return\" (which by definition will skip all\nthe things the goto did jump over) with a \"goto\" to the bottom, no?\n\nThanks.\n"},{"id":"439182","messageId":"211021.86ee8emx57.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"cb30aa67c0023c435cf472303bbf4894c8b2d7ec.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T11:31:13Z","receivedAt":"2021-10-21T11:32:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 20 2021, Taylor Blau wrote:\n\n> From: Ævar Arnfjörð Bjarmason \n>\n> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement\n> bitmap writing, 2013-12-21), we did not call stop_progress() if we\n> reached the early exit in this function.\n>\n> We could call stop_progress() before we return, but better yet is to\n> defer calling start_progress() until we need it.\n>\n> This will matter in a subsequent commit where we BUG(...) out if this\n> happens, and matters now e.g. because we don't have a corresponding\n> \"region_end\" for the progress trace2 event.\n>\n> Suggested-by: SZEDER Gábor \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Taylor Blau \n> ---\n> pack-bitmap-write.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index 9c55c1531e..cab3eaa2ac 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n> \n> \tQSORT(indexed_commits, indexed_commits_nr, date_compare);\n> \n> -\tif (writer.show_progress)\n> -\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n> -\n> \tif (indexed_commits_nr < 100) {\n> \t\tfor (i = 0; i < indexed_commits_nr; ++i)\n> \t\t\tpush_bitmapped_commit(indexed_commits[i]);\n> \t\treturn;\n> \t}\n> \n> +\tif (writer.show_progress)\n> +\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n> +\n> \tfor (;;) {\n> \t\tstruct commit *chosen = NULL;\n\nLooks good :) Just a note that this is in\n\"ab/only-single-progress-at-once\" already in case you didn't spot it.\n\nThat series is marked for \"next?\" (with a question mark) by Junio in the\nlatest what's cooking, so *hint* *hint* for any last minute review :)\n"},{"id":"439184","messageId":"211021.865ytqmwz8.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"30f6f23daf49814f479865eea5f9ee68de209d5f.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T11:34:30Z","receivedAt":"2021-10-21T11:35:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 20 2021, Taylor Blau wrote:\n\n> The patch contents are from Ævar, but the message is mine. I hope that\n> he doesn't mind me forging his sign-off here.\n\nI don't mind, and (and maybe we should have an in-tree text file for\nthis) like I think Jeff King has noted for his inline-posted patches for\ndiscussion you can assume my SOB for anything I post here on list\n(except noted otherwise) in terms of copyright.\n"},{"id":"439198","messageId":"211021.861r4emv7b.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T11:50:55Z","receivedAt":"2021-10-21T12:14:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 20 2021, Taylor Blau wrote:\n\n> I tried to separate these out based on their respective areas. The last 10% are\n> all from leaking memory in the rev_info structure, which I punted on for now by\n> just UNLEAK()-ing it. That's all done in the last patch. If we choose to take\n> that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a\n> little gross, so I'm happy to leave it out if others would prefer.\n\nI'll defer to you, but I've got a mild preference for keeping it out. An\nupcoming series of patches I'm submitting (I'm waiting on the current\nleak fixes to land) will fix most of those rev_info leaks. So not having\nUNLEAK() markings in-flight makes things a bit easier to manage.\n"},{"id":"439202","messageId":"67b553b7-4e69-9906-b237-9c44cf8aaf44@gmail.com","threadId":"56744","inReplyTo":"a68c77c00638bef7e8ce88929015a587d2d1b6f8.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-21T13:32:16Z","receivedAt":"2021-10-21T13:32:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/20/2021 11:39 PM, Taylor Blau wrote:\n> `git repack` invokes a handful of child processes: one to write the\n> actual pack, and optionally ones to repack promisor objects and update\n> the MIDX.\n> \n> In none of these cases do we bother to call child_process_clear(), which\n> frees the memory associated with each child's arguments and environment.\n> \n> In order to do so, tweak each function that spawns a child process to\n> have a `cleanup` label that we always visit before returning from each\n> function. Then, make sure that we call child_process_clear() as a part\n> of that label.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> builtin/repack.c | 35 ++++++++++++++++++++++++-----------\n> 1 file changed, 24 insertions(+), 11 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 0b2d1e5d82..d16bab09a4 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tFILE *out;\n> \tstruct strbuf line = STRBUF_INIT;\n> +\tint ret = 0;\n\nnit: \"ret\" is short for \"return\" which makes me think \"this will\nbe used as 'return ret;'\" but we don't do that. Instead, we use\nit as the _result_ of an inner call:\n\n> -\tif (finish_command(&cmd))\n> +\tret = finish_command(&cmd);\n> +\n> +cleanup:\n> +\tchild_process_clear(&cmd);\n> +\n> +\tif (ret)\n> \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n\nFor that reason, I would rename this to \"res\" or \"result\".\n\n> -\treturn finish_command(&cmd);\n> +\tret = finish_command(&cmd);\n> +\n> +cleanup:\n> +\tchild_process_clear(&cmd);\n> +\n> +\treturn ret;\n\nHere, you are taking the result of an inner call, but actually\nreturning it. This makes sense to be \"ret\".\n\n> +cleanup:\n> \tstring_list_clear(&names, 0);\n> \tstring_list_clear(&rollback, 0);\n> \tstring_list_clear(&existing_nonkept_packs, 0);\n> \tstring_list_clear(&existing_kept_packs, 0);\n> \tclear_pack_geometry(geometry);\n> \tstrbuf_release(&line);\n> +\tchild_process_clear(&cmd);\n> \n> -\treturn 0;\n> +\treturn ret;\n\nAlso here is good.\n\nThanks,\n-Stolee\n"},{"id":"439203","messageId":"7405ae17-50ab-8efe-8ea4-5a9dea8772de@gmail.com","threadId":"56744","inReplyTo":"cover.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-21T13:37:14Z","receivedAt":"2021-10-21T13:37:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/20/2021 11:39 PM, Taylor Blau wrote:\n> Here is a topic that Ævar got me interested when he mentioned that t5319 is\n> leaky[1].\n> \n> This is the result of trying to get t5319 to pass when compiling with\n> SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in\n> the pack-bitmap code, and the remaining 10% are sort of random.\n> \n> I tried to separate these out based on their respective areas. The last 10% are\n> all from leaking memory in the rev_info structure, which I punted on for now by\n> just UNLEAK()-ing it. That's all done in the last patch. If we choose to take\n> that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a\n> little gross, so I'm happy to leave it out if others would prefer.\n> \n> This is based on tb/fix-midx-rename-while-mapped, with a back-merge of\n> js/windows-ci-path-fix in order to appease CI. It also contains a little bit of\n> overlap with a patch[2] from Ævar in another series, which he and I discovered\n> independently. I cherry-picked his patch, since I haven't seen much action on\n> that series lately.\n\nIn my reading, I only found one nitpick that wasn't already caught by\nother reviewers. Feel free to ignore my comment as it was just something\nthat caused me to pause while reading, but isn't that unusual.\n\nThanks,\n-Stolee\n"},{"id":"439249","messageId":"xmqqy26mmjz8.fsf@gitster.g","threadId":"56744","inReplyTo":"30f6f23daf49814f479865eea5f9ee68de209d5f.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T16:16:27Z","receivedAt":"2021-10-21T16:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> cleanup_fail:\n> \tfree(m);\n> \tfree(midx_name);\n> -\tfree(cf);\n> +\tfree_chunkfile(cf);\n> \tif (midx_map)\n> \t\tmunmap(midx_map, midx_size);\n> \tif (0 <= fd)\n\nNot a fault of this patch, but I think the code is calling close()\non an already closed file descriptor in cleanup_fail codepath, when\n\"goto cleanup_fail\" is reached after xmmap() returned, e.g. when\noid_version() does not match hash_version, when we failed to read\nthe ToC.\n\nAlso, it is not clear why is it a dying offence if we do not find\npacknames chunk, but it is just a \"pretend as if we do not have this\nmidx file and everybody else is happy\" when we failed to read the\nToC.\n\n"},{"id":"439251","messageId":"xmqqr1cemjhl.fsf@gitster.g","threadId":"56744","inReplyTo":"b0c79904ab7bdaee7a1bc7a55b0fb26b1f2cf8d3.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T16:27:02Z","receivedAt":"2021-10-21T16:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> The function midx.c:verify_midx_file() allocate a MIDX struct by calling\n> load_multi_pack_index(). But when cleaning up, it calls free() without\n> freeing any resources associated with the MIDX.\n>\n> Call the more appropriate close_midx() which does free those resources,\n> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.\n\nNice.\n\nBy the way, the function starts like so:\n\n int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags)\n {\n struct pair_pos_vs_id *pairs = NULL;\n uint32_t i;\n struct progress *progress = NULL;\n struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n verify_midx_error = 0;\n\n if (!m) {\n int result = 0;\n struct stat sb;\n char *filename = get_midx_filename(object_dir);\n if (!stat(filename, &sb)) {\n error(_(\"multi-pack-index file exists, but failed to parse\"));\n result = 1;\n }\n free(filename);\n return result;\n }\n\nbut after seeing die() sprinkled in load_multi_pack_index() with\nchecks during parsing, I am not sure if this is a good error\nreporting mechanism we are seeing here.\n\nIt is wonderful to plug leaks here and there, but to be honest, even\nwith only a very little parts I saw in this code, I think there are\nother things that need clean-up here.\n\nAlso, the way the file-scope global verify_midx_error is used is\nbeyond words _ugly_, if the only reason for its use is to make\nmidx_report() look simpler, which is what I think is happening.\n\nNot very impressed, but all of the above is not a new issue\nintroduced by this patch.\n\nThanks.\n"},{"id":"439253","messageId":"xmqqmtn2mj0s.fsf@gitster.g","threadId":"56744","inReplyTo":"a68c77c00638bef7e8ce88929015a587d2d1b6f8.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T16:37:07Z","receivedAt":"2021-10-21T16:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> `git repack` invokes a handful of child processes: one to write the\n> actual pack, and optionally ones to repack promisor objects and update\n> the MIDX.\n>\n> In none of these cases do we bother to call child_process_clear(), which\n> frees the memory associated with each child's arguments and environment.\n>\n> In order to do so, tweak each function that spawns a child process to\n> have a `cleanup` label that we always visit before returning from each\n> function. Then, make sure that we call child_process_clear() as a part\n> of that label.\n\nI have a slight aversion against the fact that this patch makes us\ncall child_process_clear(), when we know we may have called\nfinish_command(). Callers of finish_command() in other codepaths\nrely on the fact that finish_command() calls child_process_clear(),\nwhich means that because of this change, we are now constrained to\nkeep child_process_clear() idempotent?\n"},{"id":"439254","messageId":"xmqqcznymi7k.fsf@gitster.g","threadId":"56744","inReplyTo":"f3897c3afc0e47933289025c269a4d5f248241ef.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T16:54:39Z","receivedAt":"2021-10-21T16:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But that intermediate allocation feels wasteful. Since this function is\n> simple enough, we can instead \"inline\" its implementation into the\n> xstrfmt call itself. That is a little gross, but arguably better than\n> allocating memory unnecessarily.\n\nThe function being \"simple enough\" does not help maintenance burden\nthis change brings in at all, does it? Unless it is accompanied by\na comment \"the template string used here must be kept in sync with\nwhat is used in the get_midx_filename() defined elsewhere\" near the\nlines this patch touches, perhaps in big red letters, that is.\n\nWithout this caller, get_midx_filename() could be file-scope static\nin midx.c and can be eliminated from midx.h, it seems. If there are\nmany other filenames like midx_bitmap that are derrived from\nmidx_filename, it might be worth extending get_midx_filename to take\nan extra format parameter (i.e. \"I want midx-related filename for\nthis object directory, but I am getting this .bitmap kind\"). Another\nremedy may be to move this function to midx.c and make it sit next\nto what this function must be always in sync with.\n\nOr use a C preprocessor macro for \"%s/pack/multi-pack-index\" in\nmidx.h and use it here and there to make sure they will stay in\nsync.\n\nBut after reading all the users of get_midx_filename(), especially\ngiven that it is mostly internal implementation detail inside\nmidx.c, I think rewriting\n\n\tf() {\n\tchar *midx = get_midx_filename(...);\n\n\t... use midx ...\n\n\tcleanup:\n\tfree(midx);\n\t}\n\ninto\n\n\tf() {\n\tstruct strbuf midx;\n\n\tget_midx_filename(&midx, ...);\n\n\t... use midx.buf ...\n\n\tcleanup:\n\tstrbuf_release(&midx);\n\t}\n\nis not too bad a fix for this.\n\n> Signed-off-by: Taylor Blau \n> ---\n> pack-bitmap.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index f47a0a7db4..d292e81af1 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -292,8 +292,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> \n> char *midx_bitmap_filename(struct multi_pack_index *midx)\n> {\n> -\treturn xstrfmt(\"%s-%s.bitmap\",\n> -\t\t get_midx_filename(midx->object_dir),\n> +\treturn xstrfmt(\"%s/pack/multi-pack-index-%s.bitmap\",\n> +\t\t midx->object_dir,\n> \t\t hash_to_hex(get_midx_checksum(midx)));\n> }\n"},{"id":"439255","messageId":"xmqq8rymmhzl.fsf@gitster.g","threadId":"56744","inReplyTo":"29920e773527e6e82065205ed98262ce9b01de28.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T16:59:26Z","receivedAt":"2021-10-21T16:59:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,\n> which compares the result of the on-disk bitmaps with ones generated\n> on-the-fly during a revision walk.\n>\n> In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type\n> bitmaps, 2021-08-24), we hardened those tests to also check the four\n> special type-level bitmaps, but never freed those bitmaps. We should\n> have, since each required an allocation when we EWAH-decompressed them.\n>\n> Free those, plugging that leak, and also free the base (the scratch-pad\n> bitmap), too.\n>\n> Signed-off-by: Taylor Blau \n> ---\n\nI've looked at the patches up to this point. The ones I did not\ncomment (including this one ;-) looked perfect, and all of them\n(including the ones I did comment) were cleanly described to easily\nread the reasoning behind these changes, which I highly appreciated.\n\nEspecially if I did not quite agree with the reasoning, it helped me\nclarify my thought where I found it not agreeable.\n\nThanks.\n"},{"id":"439259","messageId":"xmqqy26mkz4f.fsf@gitster.g","threadId":"56744","inReplyTo":"CAPig+cToh_8x+72-g6cdvt559N4GqKwfmBDE9BxTpkXOFrRbsA@mail.gmail.com","subject":"Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T18:32:16Z","receivedAt":"2021-10-21T18:32:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau wrote:\n>> The function free_bitmap_index() is somewhat lax in what it frees. There\n>> are two notable examples:\n>>\n>> - While it does call kh_destroy_oid_map on the \"bitmaps\" map (which\n>> maps) commit OIDs to their corresponding bitmaps, the bitmaps\n>> themselves are not freed. Note here that we recycle already-freed\n>> ewah_bitmaps into a pool, but these are handled correctly by\n>> ewah_pool_free().\n>\n> The parentheses placement seems off; it's not clear what the intent\n> is. Perhaps either move the closing parenthesis to just before the\n> comma or drop them altogether.\n\nYeah, I think we can do without them and the sentence becomes\nclearer (we can add a comma before \"which\", too).\n\n>> - We never bother to free the extended index's \"positions\" map, which\n>> we always allocate in load_bitmap().\n>>\n>> Fix both of these.\n>>\n>> Signed-off-by: Taylor Blau \n"},{"id":"439263","messageId":"xmqqpmrykys9.fsf@gitster.g","threadId":"56744","inReplyTo":"211021.86ee8emx57.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T18:39:34Z","receivedAt":"2021-10-21T18:39:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Looks good :) Just a note that this is in\n> \"ab/only-single-progress-at-once\" already in case you didn't spot it.\n>\n> That series is marked for \"next?\" (with a question mark) by Junio in the\n> latest what's cooking, so *hint* *hint* for any last minute review :)\n\nI wonder if it would help contributors if we give them a simple rule\nto follow before sending their patches out:\n\n - You develop on an appropriate base (either on maint, or master,\n or a merge of selected topics in flight you absolutely have to\n depend on) as usual.\n\n - You test merge the result to 'seen' and to 'next'. If you do not\n get heavy conflicts and the tests pass, you are OK.\n\n - Otherwise, you study what is conflicting, and should review it\n before you submit your topic.\n\nWithout understanding what others are doing in the same area, it is\nhard to make productive use of your time to develop new things, and\nif you are reading others' work in the same area anyway, it would be\na minimum additional cost to write about what you learned in their\nwork.\n"},{"id":"439264","messageId":"xmqqlf2mkylx.fsf@gitster.g","threadId":"56744","inReplyTo":"e65ac7deb5973b8209e8d94ad9140e005e22c44e.1634787555.git.me@ttaylorr.com","subject":"Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T18:43:22Z","receivedAt":"2021-10-21T18:43:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 0f6656db02..451ca3512c 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b)\n> \tewah_pool_free(b->trees);\n> \tewah_pool_free(b->blobs);\n> \tewah_pool_free(b->tags);\n> +\tif (b->bitmaps) {\n> +\t\tstruct stored_bitmap *sb;\n> +\t\tkh_foreach_value(b->bitmaps, sb, {\n> +\t\t\tewah_pool_free(sb->root);\n> +\t\t\tfree(sb);\n> +\t\t});\n> +\t}\n> \tkh_destroy_oid_map(b->bitmaps);\n> \tfree(b->ext_index.objects);\n> \tfree(b->ext_index.hashes);\n> +\tkh_destroy_oid_pos(b->ext_index.positions);\n> \tbitmap_free(b->result);\n> \tbitmap_free(b->haves);\n> \tif (bitmap_is_midx(b)) {\n\nIt is not a fault of this patch, but I was reading the corresponding\nload_bitmap() to see if we are covering everything here.\n\nOn the error path of that function, where it does \"got failed\", it\ndoes not clean after itself very well, it seems. If we read commits\nbitmap successfully but failed to read trees bitmap, for example,\nthe resource held by commits bitmap is never reclaimed.\n"},{"id":"439265","messageId":"xmqqh7dakyfk.fsf@gitster.g","threadId":"56744","inReplyTo":"67b553b7-4e69-9906-b237-9c44cf8aaf44@gmail.com","subject":"Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T18:47:11Z","receivedAt":"2021-10-21T18:47:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n>> \tFILE *out;\n>> \tstruct strbuf line = STRBUF_INIT;\n>> +\tint ret = 0;\n>\n> nit: \"ret\" is short for \"return\" which makes me think \"this will\n> be used as 'return ret;'\" but we don't do that. Instead, we use\n> it as the _result_ of an inner call:\n\nYup, my reading hiccupped there, too.\n\n>\n>> -\tif (finish_command(&cmd))\n>> +\tret = finish_command(&cmd);\n>> +\n>> +cleanup:\n>> +\tchild_process_clear(&cmd);\n>> +\n>> +\tif (ret)\n>> \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n>\n> For that reason, I would rename this to \"res\" or \"result\".\n\nYeah, if we are introducing a new variable, result is a good name.\n"},{"id":"439324","messageId":"YXIqW1yiaJTS/8in@nand.local","threadId":"56744","inReplyTo":"xmqqy26mmjz8.fsf@gitster.g","subject":"Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T03:04:59Z","receivedAt":"2021-10-22T03:05:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 09:16:27AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > cleanup_fail:\n> > \tfree(m);\n> > \tfree(midx_name);\n> > -\tfree(cf);\n> > +\tfree_chunkfile(cf);\n> > \tif (midx_map)\n> > \t\tmunmap(midx_map, midx_size);\n> > \tif (0 <= fd)\n>\n> Not a fault of this patch, but I think the code is calling close()\n> on an already closed file descriptor in cleanup_fail codepath, when\n> \"goto cleanup_fail\" is reached after xmmap() returned, e.g. when\n> oid_version() does not match hash_version, when we failed to read\n> the ToC.\n>\n> Also, it is not clear why is it a dying offence if we do not find\n> packnames chunk, but it is just a \"pretend as if we do not have this\n> midx file and everybody else is happy\" when we failed to read the\n> ToC.\n\nYep, I agree with you on both of those. I would be happier to see fewer\ndie()s deep within midx.c. I'll start a list for myself of some\npotential future cleanups in this area that don't involve memory leaks.\n\nMy hunch is that there's enough subtlty here that we shouldn't tie the\ntwo (fixing leaks, and other general issues/tidiness in the MIDX code)\ntogether. So I'll keep track of the latter separately and address those\nin future series.\n\nThanks,\nTaylor\n"},{"id":"439327","messageId":"YXIuURFP/FOpae7U@nand.local","threadId":"56744","inReplyTo":"xmqqmtn2mj0s.fsf@gitster.g","subject":"Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T03:21:53Z","receivedAt":"2021-10-22T03:21:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 09:37:07AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > `git repack` invokes a handful of child processes: one to write the\n> > actual pack, and optionally ones to repack promisor objects and update\n> > the MIDX.\n> >\n> > In none of these cases do we bother to call child_process_clear(), which\n> > frees the memory associated with each child's arguments and environment.\n> >\n> > In order to do so, tweak each function that spawns a child process to\n> > have a `cleanup` label that we always visit before returning from each\n> > function. Then, make sure that we call child_process_clear() as a part\n> > of that label.\n>\n> I have a slight aversion against the fact that this patch makes us\n> call child_process_clear(), when we know we may have called\n> finish_command(). Callers of finish_command() in other codepaths\n> rely on the fact that finish_command() calls child_process_clear(),\n> which means that because of this change, we are now constrained to\n> keep child_process_clear() idempotent?\n\nGood point. In functions besides cmd_repack(), it's easy enough to just\ncall child_process_clear() once before returning instead of using a\nlabel. That has the added benefit of side-stepping the variable name\nquestion that you and Stolee raised (but I think whether we call it\n\"result\", \"res\", or \"ret\" is besides the point).\n\nIn cmd_repack(), I'd like to wrap the other cleanup-related calls\nunderneath a 'cleanup' label. So there we want to track whether or not\nwe have called finish_command(), so that we only call\nchild_process_clear() when we *haven't* already called it via\nfinish_command().\n\nThanks,\nTaylor\n"},{"id":"439337","messageId":"YXI9zssVEupYAeWQ@nand.local","threadId":"56744","inReplyTo":"xmqqcznymi7k.fsf@gitster.g","subject":"Re: [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T04:27:58Z","receivedAt":"2021-10-22T04:28:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 09:54:39AM -0700, Junio C Hamano wrote:\n> But after reading all the users of get_midx_filename(), especially\n> given that it is mostly internal implementation detail inside\n> midx.c, I think rewriting\n>\n> \tf() {\n> \tchar *midx = get_midx_filename(...);\n>\n> \t... use midx ...\n>\n> \tcleanup:\n> \tfree(midx);\n> \t}\n>\n> into\n>\n> \tf() {\n> \tstruct strbuf midx;\n>\n> \tget_midx_filename(&midx, ...);\n>\n> \t... use midx.buf ...\n>\n> \tcleanup:\n> \tstrbuf_release(&midx);\n> \t}\n>\n> is not too bad a fix for this.\n\nI was initially worried that this was too heavy-handed an approach. But\nI hadn't actually tried it, and updating the half-dozen callers of\nget_midx_filename() took only a couple of minutes.\n\nBut just because something is easy doesn't necessarily mean that it is a\ngood idea to do ;). Luckily, this approach ended up being clean, and\nallows us to implement midx_bitmap_filename() without copying,\nopen-coding, or leaking.\n\nThanks for the nice suggestion.\n\nTaylor\n"},{"id":"439338","messageId":"YXI+EbnHcA3W1ZPO@nand.local","threadId":"56744","inReplyTo":"xmqqy26mkz4f.fsf@gitster.g","subject":"Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T04:29:05Z","receivedAt":"2021-10-22T04:29:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 11:32:16AM -0700, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n> > On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau wrote:\n> >> The function free_bitmap_index() is somewhat lax in what it frees. There\n> >> are two notable examples:\n> >>\n> >> - While it does call kh_destroy_oid_map on the \"bitmaps\" map (which\n> >> maps) commit OIDs to their corresponding bitmaps, the bitmaps\n> >> themselves are not freed. Note here that we recycle already-freed\n> >> ewah_bitmaps into a pool, but these are handled correctly by\n> >> ewah_pool_free().\n> >\n> > The parentheses placement seems off; it's not clear what the intent\n> > is. Perhaps either move the closing parenthesis to just before the\n> > comma or drop them altogether.\n>\n> Yeah, I think we can do without them and the sentence becomes\n> clearer (we can add a comma before \"which\", too).\n\nYep, thanks both.\n\nThanks,\nTaylor\n"},{"id":"439339","messageId":"YXI+8+vFyfeVenGI@nand.local","threadId":"56744","inReplyTo":"xmqqpmrykys9.fsf@gitster.g","subject":"Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T04:32:51Z","receivedAt":"2021-10-22T04:32:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 11:39:34AM -0700, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n> > Looks good :) Just a note that this is in\n> > \"ab/only-single-progress-at-once\" already in case you didn't spot it.\n> >\n> > That series is marked for \"next?\" (with a question mark) by Junio in the\n> > latest what's cooking, so *hint* *hint* for any last minute review :)\n>\n> I wonder if it would help contributors if we give them a simple rule\n> to follow before sending their patches out:\n>\n> - You develop on an appropriate base (either on maint, or master,\n> or a merge of selected topics in flight you absolutely have to\n> depend on) as usual.\n\nThat seems like sensible advice to me. When I sent the series out last\nnight, I figured that it would be preferable to cherry-pick the commit\nfrom Ævar's series, since it wasn't clear whether or not all or parts of\nthat series were actively moving forward (besides being marked as\n\"next?\" in your What's Cooking email).\n\nBut I'll send the next iteration based on a combined merge of my topic\nand Ævar's, and note your branch names for both of them clearly. To\nencourage both of our topics to move forward, I'll throw Ævar's onto my\nreview queue.\n\nThanks,\nTaylor\n"},{"id":"439340","messageId":"YXJAfICQN8s5Gm7s@nand.local","threadId":"56744","inReplyTo":"211021.861r4emv7b.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-22T04:39:24Z","receivedAt":"2021-10-22T04:39:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Wed, Oct 20 2021, Taylor Blau wrote:\n>\n> > I tried to separate these out based on their respective areas. The last 10% are\n> > all from leaking memory in the rev_info structure, which I punted on for now by\n> > just UNLEAK()-ing it. That's all done in the last patch. If we choose to take\n> > that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a\n> > little gross, so I'm happy to leave it out if others would prefer.\n>\n> I'll defer to you, but I've got a mild preference for keeping it out. An\n> upcoming series of patches I'm submitting (I'm waiting on the current\n> leak fixes to land) will fix most of those rev_info leaks. So not having\n> UNLEAK() markings in-flight makes things a bit easier to manage.\n\nIf you don't mind, I think keeping this in (as a way to verify that we\nreally did make t5319 leak-free) may be good for reviewers. When you\nclean these areas up, you'll have to remember to remove those UNLEAK()s,\nbut hopefully they produce conflicts with your in-flight work that serve\nas not-too-annoying reminders.\n\nI would certainly rather not have to UNLEAK() those at all, so I am very\nexcited to see a series from you which handles freeing these resources\nappropriately. It was just too big a bite for me to chew off when\npreparing this quick series.\n\nThanks,\nTaylor\n"},{"id":"439361","messageId":"211022.86sfwtl6uj.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"YXJAfICQN8s5Gm7s@nand.local","subject":"Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T08:23:26Z","receivedAt":"2021-10-22T09:57:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 22 2021, Taylor Blau wrote:\n\n> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Wed, Oct 20 2021, Taylor Blau wrote:\n>>\n>> > I tried to separate these out based on their respective areas. The last 10% are\n>> > all from leaking memory in the rev_info structure, which I punted on for now by\n>> > just UNLEAK()-ing it. That's all done in the last patch. If we choose to take\n>> > that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a\n>> > little gross, so I'm happy to leave it out if others would prefer.\n>>\n>> I'll defer to you, but I've got a mild preference for keeping it out. An\n>> upcoming series of patches I'm submitting (I'm waiting on the current\n>> leak fixes to land) will fix most of those rev_info leaks. So not having\n>> UNLEAK() markings in-flight makes things a bit easier to manage.\n>\n> If you don't mind, I think keeping this in (as a way to verify that we\n> really did make t5319 leak-free) may be good for reviewers. When you\n> clean these areas up, you'll have to remember to remove those UNLEAK()s,\n> but hopefully they produce conflicts with your in-flight work that serve\n> as not-too-annoying reminders.\n>\n> I would certainly rather not have to UNLEAK() those at all, so I am very\n> excited to see a series from you which handles freeing these resources\n> appropriately. It was just too big a bite for me to chew off when\n> preparing this quick series.\n\nHaving looked at this a bit more carefully I've upgraded that a bit from\n\"I'll defer to you\" to strongly objecting to this specific approach (but\nread to the end, I think you can have your cake and eat it too).\n\nI just glanced at this series in my previous pass-over, and evidently\ndidn't read the CL to the end. I thought based on the commit subject\nthat it was just a change to some midx code impacting t5319.\n\nBut it's a bigger change across the whole test suite than that.\n\nIf you run:\n\n rm .prove; GIT_SKIP_TESTS=t0027 prove -j8 --state=save t[0-9]*.sh :: --immediate\n\nWithout this patch (t0027 will take forever), and then:\n\n GIT_SKIP_TESTS=t0027 prove -j8 --state=failed\n\nYou'll see that (I use GNU screen's logging feature to get the output)\nwe made these tests pass, not just your t5319:\n \n $ grep -w ok screenlog.11 \n t0056-git-C.sh .................................... ok \n t1060-object-corruption.sh ........................ ok \n t4212-log-corrupt.sh .............................. ok \n t4207-log-decoration-colors.sh .................... ok \n t5313-pack-bounds-checks.sh ....................... ok \n t5506-remote-groups.sh ............................ ok \n t5513-fetch-track.sh .............................. ok \n t5319-multi-pack-index.sh ......................... ok \n t5532-fetch-proxy.sh .............................. ok \n t5900-repo-selection.sh ........................... ok \n t6011-rev-list-with-bad-commit.sh ................. ok \n t6100-rev-list-in-order.sh ........................ ok \n t6114-keep-packs.sh ............................... ok \n t6131-pathspec-icase.sh ........................... ok \n t6003-rev-list-topo-order.sh ...................... ok \n t7702-repack-cyclic-alternate.sh .................. ok \n t9108-git-svn-glob.sh ............................. ok \n t9109-git-svn-multi-glob.sh ....................... ok \n t9127-git-svn-partial-rebuild.sh .................. ok \n t9133-git-svn-nested-git-repo.sh .................. ok \n t9168-git-svn-partially-globbed-names.sh .......... ok \n\nI've got subsequent patches on top of what's now in-flight that fix\nthose bigger leaks incrementally, and as part of that add\n\"TEST_PASSES_SANITIZE_LEAK=true\" to /all/ the relevant passing\ntests.\n\nI.e. there isn't a 1=1 correspondance between all current passing tests\nand \"TEST_PASSES_SANITIZE_LEAK=true\" markings (some are still left\nunmarked). But there has been a 1=1 mapping between freshly pasing tests\nas a result of leak fixes and tests that get that\n\"TEST_PASSES_SANITIZE_LEAK=true\" marking.\n\nI think it helps a lot to review those patches & assess their impact if\ncode changes can be paired with relevant test suite changes, which isn't\nthe case if we've got simultaneous UNLEAK() markings for major leaks\nin-flight.\n\nSo the practical effect of that is that I'll need to rebase all that,\nchange my testing setup to test those one-at-at-time with \"git rebase -i\n--exec\" before submission (which I do anyway), but now with some\nselective revert of an earlier UNLEAK() to assert that I'm still fixing\nthe things I expected.\n\nThen either drop the parts of the commit messages that say \"this fixes\nleak XYZ, making tests A, B, C pass\" to omit any mention of \"A, B, C\",\nor reword it for the earlier now-landed UNLEAK() markings.\n\nThese are also just the fully passing tests I ran as a one-off for this\nE-Mail. For those I do more exhaustive runs where I'll e.g. spot if a\ntest goes from N failing to N-X, and note it if that gets us much closer\nto 0.\n\nBut on the \"cake and eat it too\" front I also realize that it sucks to\nnot be able to mark tests you made leak-free as passing just because\nI've got some larger more general leak fixes planned, and even if none\nof your code leaks, it might just be \"git log\" or whatever.\n\nBut we can just use LSAN_OPTIONS with a suppressions file for that. This\ndiff below (assume my SOB yadayada) makes your tests pass if I eject the\n11/11 and replace that with this:\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex a3c72b68f7c..3f6d716f825 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1,8 +1,23 @@\n #!/bin/sh\n \n test_description='multi-pack-indexes'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+test_expect_success SANITIZE_LEAK 'setup LSAN whitelisting' '\n+\tsuppressions=\".git/lsan-suppressions.txt\" &&\t \n+\tcat >\"$suppressions\" <<-\\EOF &&\n+\tleak:add_rev_cmdline\n+\tleak:cmd_log_init_finish\n+\tleak:prep_parse_options\n+\tleak:prepare_revision_walk\n+\tleak:strdup\n+\tEOF\n+\tLSAN_OPTIONS=\"$LSAN_OPTIONS:suppressions=\\\"$PWD/$suppressions\\\"\" &&\n+\texport LSAN_OPTIONS\n+'\n+\n GIT_TEST_MULTI_PACK_INDEX=0\n objdir=.git/objects\n \nI think that would be a much better approach here, and would really\nprefer if we went for some version of that if you really want to test\nthese right away with the linux-leaks job.\n\nI think the better thing to do here is to just leave it, i.e. we've got\ntons of these tests that don't leak themselves, but leak due to \"git\nlog\" or whatever already, but anyway, if what you're doing in\nt5319-multi-pack-index.sh doesn't cause much more work for me as noted\nabove I really don't mind.\n\nIf you want to pick that approach up and run with it I think it would\nprobably make sense to factor that suggested test_expect_success out\ninto a function in test-lib-functions.sh or whatever, and call it as\ne.g.:\n \n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n declare_known_leaks <<-\\EOF\n add_rev_cmdline\n [...]\n EOF\n\nThen pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for\nyou.\n\nDoing it that way would be less boilerplate for each test that wants it,\nand is also more likely to work with other non-LSAN leak appoaches,\ni.e. as long as something can take a list of lines matching stack traces\nwe can feed that to that leak checker's idea of a whitelist.\n"},{"id":"439365","messageId":"patch-1.1-9190f3c128f-20211022T102725Z-avarab@gmail.com","threadId":"56744","inReplyTo":"211022.86sfwtl6uj.gmgdl@evledraar.gmail.com","subject":"[PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T10:32:17Z","receivedAt":"2021-10-22T10:32:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add\na test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally\nbe able to add a \"suppressions\" file to the $LSAN_OPTIONS.\n\nThis allows for marking tests as passing with\n\"TEST_PASSES_SANITIZE_LEAK=true\" when they still have failure due more\ngeneral widespread memory leaks, such as from the \"git log\" family of\ncommands. We can now mark the \"git -C\" tests as passing.\n\nFor getting specific tests to pass this is preferable to using\nUNLEAK() in these codepaths, as I'll have fixes for those leaks soon,\nand being able to atomically mark relevant tests as passing with\n\"TEST_PASSES_SANITIZE_LEAK=true\" helps to explain those changes. See\n[1] for more details.\n\n1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Oct 22 2021, Taylor Blau wrote:\n>\n>> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Wed, Oct 20 2021, Taylor Blau wrote:\n[...]\n> If you want to pick that approach up and run with it I think it would\n> probably make sense to factor that suggested test_expect_success out\n> into a function in test-lib-functions.sh or whatever, and call it as\n> e.g.:\n> \n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> declare_known_leaks <<-\\EOF\n> add_rev_cmdline\n> [...]\n> EOF\n>\n> Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for\n> you.\n>\n> Doing it that way would be less boilerplate for each test that wants it,\n> and is also more likely to work with other non-LSAN leak appoaches,\n> i.e. as long as something can take a list of lines matching stack traces\n> we can feed that to that leak checker's idea of a whitelist.\n\nI just went ahead and hacked that up. If you're OK with that approach\nit would really help reduce the work for leak changes I've got\nplanned, and as noted gives you the end-state of a passing 5319.\n\nI don't know if it makes more sense for you to base on top of this\nif/when Junio picks it up, or to integrate it into your series\netc. Maybe Junio will chime in ...\n\n t/t0056-git-C.sh | 4 ++++\n t/test-lib-functions.sh | 42 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+)\n\ndiff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\nindex 2630e756dab..490aefa81a1 100755\n--- a/t/t0056-git-C.sh\n+++ b/t/t0056-git-C.sh\n@@ -2,7 +2,11 @@\n \n test_description='\"-C \" option and its effects on other path-related options'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n+todo_leaks <<-\\EOF\n+^cmd_log_init_finish$\n+EOF\n \n test_expect_success '\"git -C \" runs git from the directory ' '\n \ttest_create_repo dir1 &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a360..d89bf5da7dc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -243,6 +243,48 @@ debug () {\n \tdone\n }\n \n+# Declare known \"general\" memory leaks, for use with TEST_PASSES_SANITIZE_LEAK=true.\n+#\n+# Matches lines in a stack trace that leaks. Intended for\n+# LSAN_OPTIONS, but the format is intended to be easy to use with\n+# other leak checkers, so the \"leak:\" prefix is omitted (and added for\n+# you).\n+#\n+# Use it immediately after sourcing test-lib.sh (or equivalent), and\n+# after a \"TEST_PASSES_SANITIZE_LEAK=true\" has been set. E.g:\n+#\n+# TEST_PASSES_SANITIZE_LEAK=true\n+# . ./test-lib.sh\n+# todo_leaks <<-\\EOF\n+# ^cmd_log_init_finish$\n+# EOF\n+#\n+# The \"^\" and \"$\" anchors don't suggest full regex syntax support,\n+# that's the only anchoring (or other metacharacter) understood by\n+# LSAN_OPTIONS,.\n+#\n+# See\n+# https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions\n+# for the relevant LSAN_OPTIONS documentation.\n+todo_leaks () {\n+\tif ! test_have_prereq SANITIZE_LEAK\n+\tthen\n+\t\treturn 0\n+\tfi\n+\n+\t# Try not to interfere with any test logic\n+\tsuppressions=.lsan-suppressions.txt\n+\tif test -d .git\n+\tthen\n+\t\tsuppressions=\".git/$suppressions\"\n+\tfi\n+\tsuppressions=\"$PWD/$suppressions\"\n+\n+\tsed 's/^/leak:/' >\"$suppressions\" &&\n+\tLSAN_OPTIONS=\"$LSAN_OPTIONS:suppressions=\\\"$suppressions\\\"\" &&\n+\texport LSAN_OPTIONS\n+}\n+\n # Usage: test_commit [options] [ [ []]]\n # -C :\n #\tRun all git commands in directory \n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439453","messageId":"xmqqilxncwpr.fsf@gitster.g","threadId":"56744","inReplyTo":"xmqqpmrykys9.fsf@gitster.g","subject":"Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-23T20:28:16Z","receivedAt":"2021-10-23T20:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Looks good :) Just a note that this is in\n>> \"ab/only-single-progress-at-once\" already in case you didn't spot it.\n>>\n>> That series is marked for \"next?\" (with a question mark) by Junio in the\n>> latest what's cooking, so *hint* *hint* for any last minute review :)\n>\n> I wonder if it would help contributors if we give them a simple rule\n> to follow before sending their patches out:\n>\n> - You develop on an appropriate base (either on maint, or master,\n> or a merge of selected topics in flight you absolutely have to\n> depend on) as usual.\n>\n> - You test merge the result to 'seen' and to 'next'. If you do not\n> get heavy conflicts and the tests pass, you are OK.\n>\n> - Otherwise, you study what is conflicting, and should review it\n> before you submit your topic.\n\nAddendum to this third step.\n\n ... Also, if you chose to depend on other topics when you\n decided on the base in the first step, giving them your review\n would be a good way to show others that _you_ understand the\n code that you are building on top of, which in turn may give\n others confidence in your topic.\n\nThanks.\n"},{"id":"439454","messageId":"xmqqa6izcwio.fsf_-_@gitster.g","threadId":"56744","inReplyTo":"xmqqilxncwpr.fsf@gitster.g","subject":"SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-23T20:32:31Z","receivedAt":"2021-10-23T20:32:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We encourage identifying what, among many topics on `next`, exact\ntopics a new work depends on, instead of building directly on\n`next`. Let's clarify this in the documentation.\n\nDevelopers should know what they are building on top of, and be\naware of which part of the system is currently being worked on.\nEncouraging them to make trial merges to `next` and `seen`\nthemselves will incentivize them to read others' changes and\nunderstand them, eventually helping the developers to coordinate\namong themselves and reviewing each others' changes.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This, being a guide for those who submit their work, stops short\n of telling them to send reviews on others' patch that interacts\n with their work, but reading and understanding others' work is\n both necessary to make their own work to play well together with\n other topics, and to start reviewing others' work.\n\n Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------\n 1 file changed, 37 insertions(+), 13 deletions(-)\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex e409022d93..2de8f80dc5 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -19,8 +19,11 @@ change is relevant to.\n base your work on the tip of the topic.\n \n * A new feature should be based on `master` in general. If the new\n- feature depends on a topic that is in `seen`, but not in `master`,\n- base your work on the tip of that topic.\n+ feature depends on other topics that are in `next`, but not in\n+ `master`, fork a branch from the tip of `master`, merge these topics\n+ to the branch, and work on that branch. You can remind yourself of\n+ how you prepared the base with `git log --first-parent master..`\n+ easily by doing so.\n \n * Corrections and enhancements to a topic not yet in `master` should\n be based on the tip of that topic. If the topic has not been merged\n@@ -28,10 +31,10 @@ change is relevant to.\n into the series.\n \n * In the exceptional case that a new feature depends on several topics\n- not in `master`, start working on `next` or `seen` privately and send\n- out patches for discussion. Before the final merge, you may have to\n- wait until some of the dependent topics graduate to `master`, and\n- rebase your work.\n+ not in `master`, start working on `next` or `seen` privately and\n+ send out patches only for discussion. Once your new feature starts\n+ to stabilize, you would have to rebase it (see the \"depends on other\n+ topics\" above).\n \n * Some parts of the system have dedicated maintainers with their own\n repositories (see the section \"Subsystems\" below). Changes to\n@@ -71,8 +74,13 @@ Make sure that you have tests for the bug you are fixing. See\n [[tests]]\n When adding a new feature, make sure that you have new tests to show\n the feature triggers the new behavior when it should, and to show the\n-feature does not trigger when it shouldn't. After any code change, make\n-sure that the entire test suite passes.\n+feature does not trigger when it shouldn't. After any code change,\n+make sure that the entire test suite passes. When fixing a bug, make\n+sure you have new tests that breaks if somebody else breaks what you\n+fixed by accident to avoid regression. Also, try merging your work to\n+'next' and 'seen' and make sure the tests still pass; topics by others\n+that are still in flight may have unexpected interactions with what\n+you are trying to do in your topic.\n \n Pushing to a fork of https://github.com/git/git will use their CI\n integration to test your changes on Linux, Mac and Windows. See the\n@@ -144,8 +152,21 @@ without external resources. Instead of giving a URL to a mailing list\n archive, summarize the relevant points of the discussion.\n \n [[commit-reference]]\n-If you want to reference a previous commit in the history of a stable\n-branch, use the format \"abbreviated hash (subject, date)\", like this:\n+\n+There are a few reasons why you may want to refer to another commit in\n+the \"more stable\" part of the history (i.e. on branches like `maint`,\n+`master`, and `next`):\n+\n+. A commit that introduced the root cause of a bug you are fixing.\n+\n+. A commit that introduced a feature that is what you are enhancing.\n+\n+. A commit that conflicts with your work when you made a trial merge\n+ of your work into `next` and `seen` for testing.\n+\n+When you reference a commit on a more stable branch (like `master`,\n+`maint` and `next`), use the format \"abbreviated hash (subject,\n+date)\", like this:\n \n ....\n \tCommit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n@@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch\n is trying to achieve. Make sure to review\n your patch after generating it, to ensure accuracy. Before\n sending out, please make sure it cleanly applies to the `master`\n-branch head. If you are preparing a work based on \"next\" branch,\n-that is fine, but please mark it as such.\n+branch head. If you are preparing a work based on selected topics\n+merged to `master`, please mark your patch as such.\n \n [[send-patches]]\n === Sending your patches.\n@@ -365,7 +386,10 @@ Security mailing list{security-ml-ref}.\n Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n people who are involved in the area you are touching (the `git\n contacts` command in `contrib/contacts/` can help to\n-identify them), to solicit comments and reviews.\n+identify them), to solicit comments and reviews. Also, when you made\n+trial merges of your topic to `next` and `seen`, you may have noticed\n+work by others conflicting with your changes. There is a good possibility\n+that these people may know the area you are touching well.\n \n :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]\n :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n"},{"id":"439459","messageId":"211023.86bl3fih42.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"xmqqa6izcwio.fsf_-_@gitster.g","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-23T20:59:00Z","receivedAt":"2021-10-23T21:08:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 23 2021, Junio C Hamano wrote:\n\n> Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------\n> 1 file changed, 37 insertions(+), 13 deletions(-)\n>\n> diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\n> index e409022d93..2de8f80dc5 100644\n> --- c/Documentation/SubmittingPatches\n> +++ w/Documentation/SubmittingPatches\n> @@ -19,8 +19,11 @@ change is relevant to.\n> base your work on the tip of the topic.\n> \n> * A new feature should be based on `master` in general. If the new\n> - feature depends on a topic that is in `seen`, but not in `master`,\n> - base your work on the tip of that topic.\n> + feature depends on other topics that are in `next`, but not in\n> + `master`, fork a branch from the tip of `master`, merge these topics\n> + to the branch, and work on that branch. You can remind yourself of\n> + how you prepared the base with `git log --first-parent master..`\n> + easily by doing so.\n\nFor what it's worth when I do this I base things on one either your\ngitster/* branches, what I've applied from the ML, or grabbed from the\nauthor's own repo, and then usually rebase it on the latest \"master\" or\n as appropriate so I don't have on old base, particularly in the\ncase of gitster/*, you don't rebase those unless needed.\n\nAnyway, I can see why you suggest the \"base on master and merge\", it has\nits benefits, but it is in apparent conflict with existing advice added\nin 76644e3268b (documentation: add tutorial for first contribution,\n2019-05-17).\n\nI.e. the \"After Review Approval\" section (of all places) discusses what\nto do in that situation. It is narrowly talking about submitting\nsomething on top of your own topic, but the advice should be the same in\neither case I'd think.\n\n> * Corrections and enhancements to a topic not yet in `master` should\n> be based on the tip of that topic. If the topic has not been merged\n> @@ -28,10 +31,10 @@ change is relevant to.\n> into the series.\n> \n> * In the exceptional case that a new feature depends on several topics\n> - not in `master`, start working on `next` or `seen` privately and send\n> - out patches for discussion. Before the final merge, you may have to\n> - wait until some of the dependent topics graduate to `master`, and\n> - rebase your work.\n> + not in `master`, start working on `next` or `seen` privately and\n> + send out patches only for discussion. Once your new feature starts\n> + to stabilize, you would have to rebase it (see the \"depends on other\n> + topics\" above).\n> \n> * Some parts of the system have dedicated maintainers with their own\n> repositories (see the section \"Subsystems\" below). Changes to\n> @@ -71,8 +74,13 @@ Make sure that you have tests for the bug you are fixing. See\n> [[tests]]\n> When adding a new feature, make sure that you have new tests to show\n> the feature triggers the new behavior when it should, and to show the\n> -feature does not trigger when it shouldn't. After any code change, make\n> -sure that the entire test suite passes.\n> +feature does not trigger when it shouldn't. After any code change,\n> +make sure that the entire test suite passes. When fixing a bug, make\n> +sure you have new tests that breaks if somebody else breaks what you\n> +fixed by accident to avoid regression. Also, try merging your work to\n> +'next' and 'seen' and make sure the tests still pass; topics by others\n> +that are still in flight may have unexpected interactions with what\n> +you are trying to do in your topic.\n\nMaybe it would be useful to have a CI target that merged to next/master\nand reported how that went? It would have to be a soft failure, as we\nmight have an easy merge conflict, or someone's pushing a revision to a\ntopic that's already in \"next\" or \"seen\" (and might conflict). But\nhaving it as an FYI might be helpful.\n\n> Pushing to a fork of https://github.com/git/git will use their CI\n> integration to test your changes on Linux, Mac and Windows. See the\n> @@ -144,8 +152,21 @@ without external resources. Instead of giving a URL to a mailing list\n> archive, summarize the relevant points of the discussion.\n> \n> [[commit-reference]]\n> -If you want to reference a previous commit in the history of a stable\n> -branch, use the format \"abbreviated hash (subject, date)\", like this:\n> +\n> +There are a few reasons why you may want to refer to another commit in\n> +the \"more stable\" part of the history (i.e. on branches like `maint`,\n> +`master`, and `next`):\n> +\n> +. A commit that introduced the root cause of a bug you are fixing.\n> +\n> +. A commit that introduced a feature that is what you are enhancing.\n> +\n> +. A commit that conflicts with your work when you made a trial merge\n> + of your work into `next` and `seen` for testing.\n> +\n> +When you reference a commit on a more stable branch (like `master`,\n> +`maint` and `next`), use the format \"abbreviated hash (subject,\n> +date)\", like this:\n\nThis seems like a good clarification, but partially unrelated to the\n$subject, i.e. just the last bullet point is directly relevant, the\nfirst two are new general advice. Perhaps split those out into another\ncommit?\n"},{"id":"439461","messageId":"xmqq7de3bf7v.fsf@gitster.g","threadId":"56744","inReplyTo":"211023.86bl3fih42.gmgdl@evledraar.gmail.com","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-23T21:31:32Z","receivedAt":"2021-10-23T21:37:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Maybe it would be useful to have a CI target that merged to next/master\n> and reported how that went? It would have to be a soft failure, as we\n> might have an easy merge conflict, or someone's pushing a revision to a\n> topic that's already in \"next\" or \"seen\" (and might conflict). But\n> having it as an FYI might be helpful.\n\nSorry, but it would defeat the whole point of this suggestion.\nLetting CI do it to allow contributors not to worry about other\npeople's work is the *last* thing I want to encourage. I want\nto see people get in the habit of making trial merges and ensuring\ntheir work works in the wider context than just \"my patches work in\n'master'---it is not my problem if it does not work when patches\nother people already wrote are present\". \n\n> This seems like a good clarification, but partially unrelated to the\n> $subject, i.e. just the last bullet point is directly relevant, the\n> first two are new general advice. Perhaps split those out into another\n> commit?\n\nNot really. The original didn't say in what situations you would\nwant to refer to other commits. Clarifying that made a three-bullet\nlist, and all three are equally relevant.\n\nThanks.\n"},{"id":"439462","messageId":"xmqq1r4bbetg.fsf@gitster.g","threadId":"56744","inReplyTo":"211023.86bl3fih42.gmgdl@evledraar.gmail.com","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-23T21:40:11Z","receivedAt":"2021-10-23T21:40:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Anyway, I can see why you suggest the \"base on master and merge\", it has\n> its benefits, but it is in apparent conflict with existing advice added\n> in 76644e3268b (documentation: add tutorial for first contribution,\n> 2019-05-17).\n\nI do not mind if you help the effort by updating that document as\nwell to match, but I have a feeling that the first contribution,\nwith a follow-up to fix, is sufficiently different from a more\nadvanced \"I need something more than and not yet in 'master' for my\nnew topic\" use case.\n\n> I.e. the \"After Review Approval\" section (of all places) discusses what\n> to do in that situation. It is narrowly talking about submitting\n> something on top of your own topic, but the advice should be the same in\n> either case I'd think.\n\nNo, the advice should be totally different between these two cases.\n\nThe \"fix or enhance what you already sent\" is a continuation of the\nsame topic, just you, your reviewers and your maintainer were all\nnot competent enough to catch the shortcomings before the topic hits\n'master', and it does make sense to make correction directly on top.\n\nWhat I am describing here is \"I am doing something new, but uses\nwhat is not yet in 'master'. The author of that other thing I am\ndepending on may or may not mean to help this new thing when they\nwrote it, but the important point is that this is not a continuation\nof their topic. It merely depends on them\".\n\nThanks.\n"},{"id":"439520","messageId":"006c186e-c005-28a3-7a5c-27d365ffe026@gigacodes.de","threadId":"56744","inReplyTo":"xmqqa6izcwio.fsf_-_@gitster.g","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-25T08:59:53Z","receivedAt":"2021-10-25T09:00:07Z","isPatch":false,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 23.10.21 22:32, Junio C Hamano wrote:> * A new feature should be\nbased on `master` in general. If the new\n> - feature depends on a topic that is in `seen`, but not in `master`,\n> - base your work on the tip of that topic.\n> + feature depends on other topics that are in `next`, but not in\n> + `master`, fork a branch from the tip of `master`, merge these topics\n> + to the branch, and work on that branch. You can remind yourself of\n> + how you prepared the base with `git log --first-parent master..`\n> + easily by doing so.\n\nUsing the topic branches from gitster/git that were merged? Or by\nselecting the specific commits from the merge into next?\n\n> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch\n> is trying to achieve. Make sure to review\n> your patch after generating it, to ensure accuracy. Before\n> sending out, please make sure it cleanly applies to the `master`\n> -branch head. If you are preparing a work based on \"next\" branch,\n> -that is fine, but please mark it as such.\n> +branch head. If you are preparing a work based on selected topics\n> +merged to `master`, please mark your patch as such.\n\nI think this meant to say 'merged to \"next|maint|seen\"'?\nOr topics selected for being merged into master?\n\nThanks,\nFabian\n"},{"id":"439562","messageId":"xmqqilxl2gpj.fsf@gitster.g","threadId":"56744","inReplyTo":"006c186e-c005-28a3-7a5c-27d365ffe026@gigacodes.de","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T16:48:40Z","receivedAt":"2021-10-25T16:48:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> On 23.10.21 22:32, Junio C Hamano wrote:> * A new feature should be\n> based on `master` in general. If the new\n>> - feature depends on a topic that is in `seen`, but not in `master`,\n>> - base your work on the tip of that topic.\n>> + feature depends on other topics that are in `next`, but not in\n>> + `master`, fork a branch from the tip of `master`, merge these topics\n>> + to the branch, and work on that branch. You can remind yourself of\n>> + how you prepared the base with `git log --first-parent master..`\n>> + easily by doing so.\n>\n> Using the topic branches from gitster/git that were merged? Or by\n> selecting the specific commits from the merge into next?\n\nIf I were doing this, I would find the tip(s) of things I would\ndepend on out of the output from\n\n $ git log --oneline --first-parent origin/master..origin/next\n\nThis lists a series of merge commits and the second parent of each\nmerge commit is the tip of the topic that was merged to 'next'.\n\n>> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch\n>> is trying to achieve. Make sure to review\n>> your patch after generating it, to ensure accuracy. Before\n>> sending out, please make sure it cleanly applies to the `master`\n>> -branch head. If you are preparing a work based on \"next\" branch,\n>> -that is fine, but please mark it as such.\n>> +branch head. If you are preparing a work based on selected topics\n>> +merged to `master`, please mark your patch as such.\n>\n> I think this meant to say 'merged to \"next|maint|seen\"'?\n> Or topics selected for being merged into master?\n\nAh, thanks for catching. I meant \"not merged to 'master'\" (will fix\nlocally).\n\nDepending on stuff that are already in 'master', unless you are\npreparing a fix that would also apply to the maintenance track, is\nrather easy---you can just build on top of 'master'.\n\nAnd in general, I do not want to see a new topic based on another\ntopic that is not yet in 'next'. If a developer has such a topic,\nI'd appreciate if the developer waits and shifts their attention to\nhelp the other topics they are planning to depend on---and one way\nto do so is to review these other topics ;-)\n\nThanks.\n"},{"id":"439563","messageId":"xmqqee892gd1.fsf@gitster.g","threadId":"56744","inReplyTo":"xmqqilxl2gpj.fsf@gitster.g","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T16:56:10Z","receivedAt":"2021-10-25T16:56:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch\n>>> is trying to achieve. Make sure to review\n>>> your patch after generating it, to ensure accuracy. Before\n>>> sending out, please make sure it cleanly applies to the `master`\n>>> -branch head. If you are preparing a work based on \"next\" branch,\n>>> -that is fine, but please mark it as such.\n>>> +branch head. If you are preparing a work based on selected topics\n>>> +merged to `master`, please mark your patch as such.\n>>\n>> I think this meant to say 'merged to \"next|maint|seen\"'?\n>> Or topics selected for being merged into master?\n>\n> Ah, thanks for catching. I meant \"not merged to 'master'\" (will fix\n> locally).\n\nSorry, should have re-read it more carefully. The original text\nis saying what I wanted to say, but I misread it ;-)\n\nWe earlier in the document said that there are three possible bases;\nfixes are usually based on 'maint', new features on 'master', but as\nan exception, if your new feature need to depend on something not\nyet in 'master', then you start a branch from 'master', merge\nselected topics into that branch, and use the resulting branch as\nthe 'base' of your topic. We do not want a new topic based on\n'next', so preparing such a synthetic base by starting from 'next'\nand merging topics that are not yet in 'next' is unwelcome.\n\nPerhaps there needs some rewording to clarify that the sentence is\nreferring to that case.\n\nThanks again.\n"},{"id":"439565","messageId":"xmqqa6ix2g6a.fsf@gitster.g","threadId":"56744","inReplyTo":"xmqqee892gd1.fsf@gitster.g","subject":"Re: SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T17:00:13Z","receivedAt":"2021-10-25T17:00:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps there needs some rewording to clarify that the sentence is\n> referring to that case.\n\nSo, I came up with this incremental clarification. I'd wait for\nother comments before moving the topic further.\n\n Documentation/SubmittingPatches | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex 2de8f80dc5..aea01bf36b 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -280,9 +280,11 @@ Please make sure your patch does not add commented out debugging code,\n or include any extra files which do not relate to what your patch\n is trying to achieve. Make sure to review\n your patch after generating it, to ensure accuracy. Before\n-sending out, please make sure it cleanly applies to the `master`\n-branch head. If you are preparing a work based on selected topics\n-merged to `master`, please mark your patch as such.\n+sending out, please make sure it cleanly applies to the base you\n+have chosen in the \"Decide what to base your work on\" section,\n+and unless it targets the `master` branch (which is the default),\n+mark your patches as such.\n+\n \n [[send-patches]]\n === Sending your patches.\n"},{"id":"439672","messageId":"YXhjstW2XAnixEqh@nand.local","threadId":"56744","inReplyTo":"patch-1.1-9190f3c128f-20211022T102725Z-avarab@gmail.com","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T20:23:14Z","receivedAt":"2021-10-26T20:23:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add\n> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally\n> be able to add a \"suppressions\" file to the $LSAN_OPTIONS.\n>\n> This allows for marking tests as passing with\n> \"TEST_PASSES_SANITIZE_LEAK=true\" when they still have failure due more\n> general widespread memory leaks, such as from the \"git log\" family of\n> commands. We can now mark the \"git -C\" tests as passing.\n>\n> For getting specific tests to pass this is preferable to using\n> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,\n> and being able to atomically mark relevant tests as passing with\n> \"TEST_PASSES_SANITIZE_LEAK=true\" helps to explain those changes. See\n> [1] for more details.\n>\n> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n> > On Fri, Oct 22 2021, Taylor Blau wrote:\n> >\n> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >>>\n> >>> On Wed, Oct 20 2021, Taylor Blau wrote:\n> [...]\n> > If you want to pick that approach up and run with it I think it would\n> > probably make sense to factor that suggested test_expect_success out\n> > into a function in test-lib-functions.sh or whatever, and call it as\n> > e.g.:\n> >\n> > TEST_PASSES_SANITIZE_LEAK=true\n> > . ./test-lib.sh\n> > declare_known_leaks <<-\\EOF\n> > add_rev_cmdline\n> > [...]\n> > EOF\n> >\n> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for\n> > you.\n> >\n> > Doing it that way would be less boilerplate for each test that wants it,\n> > and is also more likely to work with other non-LSAN leak appoaches,\n> > i.e. as long as something can take a list of lines matching stack traces\n> > we can feed that to that leak checker's idea of a whitelist.\n>\n> I just went ahead and hacked that up. If you're OK with that approach\n> it would really help reduce the work for leak changes I've got\n> planned, and as noted gives you the end-state of a passing 5319.\n>\n> I don't know if it makes more sense for you to base on top of this\n> if/when Junio picks it up, or to integrate it into your series\n> etc. Maybe Junio will chime in ...\n\nHmm. This seems neat, but I haven't been able to get it to work without\nencountering a rather annoying bug along the way.\n\nHere is the preamble of my modified t5319 to include all of the leaks I\nfound in the previous round (but decided not to fix):\n\nTEST_PASSES_SANITIZE_LEAK=true\n. ./test-lib.sh\ntodo_leaks <<-\\EOF\n^add_rev_cmdline$\n^cmd_log_init_finish$\n^prepare_revision_walk$\n^prep_parse_options$\nEOF\n\nSo running that when git is compiled with SANITIZE=leak *should* work,\nbut instead produces this rather annoying leak detection after t5319.7:\n\n =================================================================\n ==1785298==ERROR: LeakSanitizer: detected memory leaks\n\n Indirect leak of 41 byte(s) in 3 object(s) allocated from:\n #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42\n\n -----------------------------------------------------\n Suppressions used:\n count bytes template\n 1 576 ^add_rev_cmdline$\n -----------------------------------------------------\n\n SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).\n Aborted\n\nHuh? Looking past __GI___strdup (which I assume is just a\nsymbolification failure on ASan's part), who calls strdup()? That trace\nis annoyingly incomplete, and doesn't really give us anything to go off\nof.\n\nIt does seem to remind me of this:\n\n https://github.com/google/sanitizers/issues/148\n\nthough that issue goes in so many different directions that I'm not sure\nwhether it really is the same issue or not. In any case, that leak\n*still* shows up even when suppressing xmalloc() and xrealloc(), so I\nalmost think that it's a leak within ASan itself.\n\nBut most interesting is that those calls go away when I stop setting\n`suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.\n\nSo this all feels like a bug in ASan to me. I'm curious if it works on\nyour system, but in the meantime I think the best path forward is to\ndrop the last patch of my original series (the one with the three\nUNLEAK() calls) and to avoid relying on this patch for the time being.\n\nThanks,\nTaylor\n"},{"id":"439684","messageId":"YXhvAQgxKpq65qcg@coredump.intra.peff.net","threadId":"56744","inReplyTo":"YXhjstW2XAnixEqh@nand.local","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-26T21:11:29Z","receivedAt":"2021-10-26T21:11:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:\n\n> So this all feels like a bug in ASan to me. I'm curious if it works on\n> your system, but in the meantime I think the best path forward is to\n> drop the last patch of my original series (the one with the three\n> UNLEAK() calls) and to avoid relying on this patch for the time being.\n\nBugs aside, I'd much rather see UNLEAK() annotations than external ones,\nfor all the reasons we introduced UNLEAK() in the first place:\n\n - it keeps the annotations near the code. Yes, that creates conflicts\n when the code is changed (or the leak is actually fixed), but that's\n a feature. It keeps them from going stale.\n\n - leak-checkers only know where things are allocated, not who is\n supposed to own them. So you're often annotating the wrong thing;\n it's not a strdup() call which is buggy and leaking, but the\n function five layers up the stack which was supposed to take\n ownership and didn't.\n\nAnd if we avoid any annotation bugs by doing so, that's icing on the\ncake. :)\n\n-Peff\n"},{"id":"439689","messageId":"YXhzh+pW5KvU7MEA@nand.local","threadId":"56744","inReplyTo":"YXhvAQgxKpq65qcg@coredump.intra.peff.net","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-26T21:30:47Z","receivedAt":"2021-10-26T21:30:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:\n> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:\n>\n> > So this all feels like a bug in ASan to me. I'm curious if it works on\n> > your system, but in the meantime I think the best path forward is to\n> > drop the last patch of my original series (the one with the three\n> > UNLEAK() calls) and to avoid relying on this patch for the time being.\n>\n> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,\n> for all the reasons we introduced UNLEAK() in the first place:\n>\n> - it keeps the annotations near the code. Yes, that creates conflicts\n> when the code is changed (or the leak is actually fixed), but that's\n> a feature. It keeps them from going stale.\n\nI agree completely. I noted as much in my message here:\n\n https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/\n\nbut Ævar made it sound like his work would be made much easier without\nthe conflict. Since I'm not in any kind of rush to make t5319 leak-free,\nI figured that queueing the parts of that series that wouldn't conflict\nwith Ævar's ongoing work would be a net-positive.\n\n> - leak-checkers only know where things are allocated, not who is\n> supposed to own them. So you're often annotating the wrong thing;\n> it's not a strdup() call which is buggy and leaking, but the\n> function five layers up the stack which was supposed to take\n> ownership and didn't.\n\nTBH, I find this much more compelling. Either way, I don't feel strongly\nenough to deviate from v2 much, and I don't want to create more work for\nÆvar along the way.\n\nThanks,\nTaylor\n"},{"id":"439691","messageId":"YXh3v6BPcTAtUMeh@coredump.intra.peff.net","threadId":"56744","inReplyTo":"YXhzh+pW5KvU7MEA@nand.local","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-26T21:48:47Z","receivedAt":"2021-10-26T21:48:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 26, 2021 at 05:30:47PM -0400, Taylor Blau wrote:\n\n> > Bugs aside, I'd much rather see UNLEAK() annotations than external ones,\n> > for all the reasons we introduced UNLEAK() in the first place:\n> >\n> > - it keeps the annotations near the code. Yes, that creates conflicts\n> > when the code is changed (or the leak is actually fixed), but that's\n> > a feature. It keeps them from going stale.\n> \n> I agree completely. I noted as much in my message here:\n> \n> https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/\n> \n> but Ævar made it sound like his work would be made much easier without\n> the conflict. Since I'm not in any kind of rush to make t5319 leak-free,\n> I figured that queueing the parts of that series that wouldn't conflict\n> with Ævar's ongoing work would be a net-positive.\n\nYeah, to be clear, if there's work in progress in an area, then _not_\nannotating it (with either method) is perfectly fine with me in the\nmeantime.\n\n-Peff\n"},{"id":"439718","messageId":"211027.86zgquyk52.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"YXhjstW2XAnixEqh@nand.local","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-27T07:51:38Z","receivedAt":"2021-10-27T07:58:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 26 2021, Taylor Blau wrote:\n\n> On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add\n>> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally\n>> be able to add a \"suppressions\" file to the $LSAN_OPTIONS.\n>>\n>> This allows for marking tests as passing with\n>> \"TEST_PASSES_SANITIZE_LEAK=true\" when they still have failure due more\n>> general widespread memory leaks, such as from the \"git log\" family of\n>> commands. We can now mark the \"git -C\" tests as passing.\n>>\n>> For getting specific tests to pass this is preferable to using\n>> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,\n>> and being able to atomically mark relevant tests as passing with\n>> \"TEST_PASSES_SANITIZE_LEAK=true\" helps to explain those changes. See\n>> [1] for more details.\n>>\n>> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>>\n>> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> > On Fri, Oct 22 2021, Taylor Blau wrote:\n>> >\n>> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> >>>\n>> >>> On Wed, Oct 20 2021, Taylor Blau wrote:\n>> [...]\n>> > If you want to pick that approach up and run with it I think it would\n>> > probably make sense to factor that suggested test_expect_success out\n>> > into a function in test-lib-functions.sh or whatever, and call it as\n>> > e.g.:\n>> >\n>> > TEST_PASSES_SANITIZE_LEAK=true\n>> > . ./test-lib.sh\n>> > declare_known_leaks <<-\\EOF\n>> > add_rev_cmdline\n>> > [...]\n>> > EOF\n>> >\n>> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for\n>> > you.\n>> >\n>> > Doing it that way would be less boilerplate for each test that wants it,\n>> > and is also more likely to work with other non-LSAN leak appoaches,\n>> > i.e. as long as something can take a list of lines matching stack traces\n>> > we can feed that to that leak checker's idea of a whitelist.\n>>\n>> I just went ahead and hacked that up. If you're OK with that approach\n>> it would really help reduce the work for leak changes I've got\n>> planned, and as noted gives you the end-state of a passing 5319.\n>>\n>> I don't know if it makes more sense for you to base on top of this\n>> if/when Junio picks it up, or to integrate it into your series\n>> etc. Maybe Junio will chime in ...\n>\n> Hmm. This seems neat, but I haven't been able to get it to work without\n> encountering a rather annoying bug along the way.\n>\n> Here is the preamble of my modified t5319 to include all of the leaks I\n> found in the previous round (but decided not to fix):\n>\n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> todo_leaks <<-\\EOF\n> ^add_rev_cmdline$\n> ^cmd_log_init_finish$\n> ^prepare_revision_walk$\n> ^prep_parse_options$\n> EOF\n>\n> So running that when git is compiled with SANITIZE=leak *should* work,\n> but instead produces this rather annoying leak detection after t5319.7:\n>\n> =================================================================\n> ==1785298==ERROR: LeakSanitizer: detected memory leaks\n>\n> Indirect leak of 41 byte(s) in 3 object(s) allocated from:\n> #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n> #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42\n>\n> -----------------------------------------------------\n> Suppressions used:\n> count bytes template\n> 1 576 ^add_rev_cmdline$\n> -----------------------------------------------------\n>\n> SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).\n> Aborted\n>\n> Huh? Looking past __GI___strdup (which I assume is just a\n> symbolification failure on ASan's part), who calls strdup()? That trace\n> is annoyingly incomplete, and doesn't really give us anything to go off\n> of.\n>\n> It does seem to remind me of this:\n>\n> https://github.com/google/sanitizers/issues/148\n>\n> though that issue goes in so many different directions that I'm not sure\n> whether it really is the same issue or not. In any case, that leak\n> *still* shows up even when suppressing xmalloc() and xrealloc(), so I\n> almost think that it's a leak within ASan itself.\n>\n> But most interesting is that those calls go away when I stop setting\n> `suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.\n>\n> So this all feels like a bug in ASan to me. I'm curious if it works on\n> your system, but in the meantime I think the best path forward is to\n> drop the last patch of my original series (the one with the three\n> UNLEAK() calls) and to avoid relying on this patch for the time being.\n\nThere are similar cases where LSAN doesn't provide as meaningful of a\nstacktrace as valgrind, sometimes when tracing leaks I get a relatively\nbad stacktrace like that ending in some __GI___*\nfunction. I'll usually compile without SANITIZE=leak and just run a\nslower:\n\n valgrind --leak-check=full --show-leak-kinds=all\n\nIn this case however it's not a bug or bad leak tracing, but an artifact\nof us using these stacktrace exclusions.\n\nIf you run this manually you'll see:\n \n $ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all\n cd0747a9352b58d112f0010134351efc7bbad4a6\n [... snipped output ...]\n \n =================================================================\n ==58023==ERROR: LeakSanitizer: detected memory leaks\n \n Direct leak of 576 byte(s) in 1 object(s) allocated from:\n #0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82\n #1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126\n #2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482\n #3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534\n #4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483\n #5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418\n #6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498\n #7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504\n #8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578\n #9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597\n #10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738\n #11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550\n #12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461\n #13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713\n #14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780\n #15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911\n #16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52\n #17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308\n \n Indirect leak of 41 byte(s) in 3 object(s) allocated from:\n #0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n #1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42\n \n SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s).\n\nWhich is why I put the \"strdup\" there, but forgot to tell you when I\nsubmitted the real PATCH version that that one shouldn't have the ^$\nanchoring.\n\nFWIW this will work on top of your series:\n \n diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n index a3c72b68f7c..891de720b2c 100755\n --- a/t/t5319-multi-pack-index.sh\n +++ b/t/t5319-multi-pack-index.sh\n @@ -2,6 +2,17 @@\n \n test_description='multi-pack-indexes'\n . ./test-lib.sh\n +todo_leaks <<-\\EOF\n +^add_rev_cmdline$\n +^cmd_log_init_finish$\n +^prep_parse_options$\n +^prepare_revision_walk$\n +^start_progress_delay$\n +\n +# An indirect leak reported because we excluded the leaks above\n +strdup$\n +EOF\n +\n \n GIT_TEST_MULTI_PACK_INDEX=0\n objdir=.git/objects\n"},{"id":"439736","messageId":"211027.86v91iyis4.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"YXhvAQgxKpq65qcg@coredump.intra.peff.net","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-27T08:04:17Z","receivedAt":"2021-10-27T08:27:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 26 2021, Jeff King wrote:\n\nIt seems that I've gotten what I wanted from this side-thread, yay!\nThanks both, not to pile on, but just to clarify:\n\n> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:\n>\n>> So this all feels like a bug in ASan to me. I'm curious if it works on\n>> your system, but in the meantime I think the best path forward is to\n>> drop the last patch of my original series (the one with the three\n>> UNLEAK() calls) and to avoid relying on this patch for the time being.\n>\n> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,\n> for all the reasons we introduced UNLEAK() in the first place:\n>\n> - it keeps the annotations near the code. Yes, that creates conflicts\n> when the code is changed (or the leak is actually fixed), but that's\n> a feature. It keeps them from going stale.\n\nI fully agree with you in general for \"good\" uses of UNLEAK(). E.g. consider:\n\n struct strbuf buf = xmalloc(...);\n [...]\n UNLEAK(buf);\n\nIf I was fixing leaks in that sort of code what I pointed out downthread\nin [1] wouldn't apply.\n\nSo to clarify, I'm not asking in [1] that UNLEAK() not be used at all\nwhile we have in-flight leak fixes. I.e. I'd run into a textual\nconflict, but that would be trivial to resolve, and obvious what the\nsemantic & textual conflict was.\n\nRather, it's not marking specific leaks, but UNLEAK() on a container\nstruct that's problematic.\n\nDepending on how they're used those structs may or may not leak. So\ne.g. Taylor's upthread 11/11[2] contained:\n \n diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n index 36cb909eba..df3811e763 100644\n --- a/builtin/rev-list.c\n +++ b/builtin/rev-list.c\n @@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n argc = setup_revisions(argc, argv, &revs, &s_r_opt);\n \n + UNLEAK(revs);\n +\n memset(&info, 0, sizeof(info));\n info.revs = &revs;\n if (revs.bisect)\n\nWhich if you on master replace that with:\n \n diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n index 36cb909ebaa..3cce87e0eb7 100644\n --- a/builtin/rev-list.c\n +++ b/builtin/rev-list.c\n @@ -548,6 +548,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n revs.do_not_die_on_missing_tree = 1;\n \n argc = setup_revisions(argc, argv, &revs, &s_r_opt);\n + BUG(\"using this?\");\n \n memset(&info, 0, sizeof(info));\n info.revs = &revs;\n\nYou'll run into a failure with:\n\n GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0002-gitfile.sh\n\nI.e. our existing leak tests already take that codepath and don't run\ninto a leak from using \"revs\".\n\nSo we wouldn't just be marking a known specific leak, but hiding all\nleaks & non-leaks in container from the top-level, and thus hide\npotential regressions, an addition to attaining the end-goal of marking\nsome specific test as passing.\n\nNow, that specific example isn't very useful right now, since we don't\nhave a release_revisions() at all, but e.g. the first batch of fixes\nI've got for the revisions.[ch] leak fix most common cases (e.g. the\npending array), but not some obscure ones.\n\nBeing able to incrementally mark those leaks as fixed on a test-by-test\nbasis has the advantage over UNLEAK() that we won't have regressions\nwhile we're not at a 100% leak free (at which point we could remove the\nUNLEAK()).\n\n> - leak-checkers only know where things are allocated, not who is\n> supposed to own them. So you're often annotating the wrong thing;\n> it's not a strdup() call which is buggy and leaking, but the\n> function five layers up the stack which was supposed to take\n> ownership and didn't.\n\nAs noted in [3] this case is because the LSAN suppressions list was in\nplay, so we excluded the meaningful part of the stack trace (which is\nshown in that mail).\n\nHrm, now that I think about it I think that the cases where I had to\nresort to valgrind to get around such crappy stacktraces (when that was\nall I got, even without suppressions) were probably all because there\nwas an UNLEAK() in play...\n\n1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/f1bb8b73ffdb78995dc5653791f9a64adf216e21.1634787555.git.me@ttaylorr.com/\n3. https://lore.kernel.org/git/211027.86zgquyk52.gmgdl@evledraar.gmail.com/\n"},{"id":"439747","messageId":"YXkWhFGS/uTAnsI1@coredump.intra.peff.net","threadId":"56744","inReplyTo":"211027.86v91iyis4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-27T09:06:12Z","receivedAt":"2021-10-27T09:06:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > - it keeps the annotations near the code. Yes, that creates conflicts\n> > when the code is changed (or the leak is actually fixed), but that's\n> > a feature. It keeps them from going stale.\n> \n> I fully agree with you in general for \"good\" uses of UNLEAK(). E.g. consider:\n> \n> struct strbuf buf = xmalloc(...);\n> [...]\n> UNLEAK(buf);\n> \n> If I was fixing leaks in that sort of code what I pointed out downthread\n> in [1] wouldn't apply.\n\nI'm OK with such a use of UNLEAK(), though of course you could probably\njust free the buffer in that instance.\n\nBut...\n\n> So to clarify, I'm not asking in [1] that UNLEAK() not be used at all\n> while we have in-flight leak fixes. I.e. I'd run into a textual\n> conflict, but that would be trivial to resolve, and obvious what the\n> semantic & textual conflict was.\n> \n> Rather, it's not marking specific leaks, but UNLEAK() on a container\n> struct that's problematic.\n\n...that's the whole point of UNLEAK(), IMHO. You know that the container\nstruct is leaked, but you don't care because the program is about to\nexit, or there's no easy way to avoid the leak.\n\nSo it's not the \"container\" element, but rather it can be a problem if\npeople annotate too broadly (you will miss some leaks). In the case of\nrev_info, there is no way to _not_ leak right now, because it has no\ncleanup function. In the long run we should probably add one. But in the\nmeantime, annotating them so we can find the _interesting_ leaks is\nworth doing (and that was the whole point of adding UNLEAK in the first\nplace.\n\nAnd I think the UNLEAK() calls in Taylor's patch are fine in that sense.\nWhile yes, _some_ runs of those commands will not leak, the point is to\nsay that when they do leak, they are not interesting. And they are not\ninteresting because that rev_info is in scope until the program exits,\nso anything it points to is only leaked at a moment where we no longer\ncare.\n\n> So we wouldn't just be marking a known specific leak, but hiding all\n> leaks & non-leaks in container from the top-level, and thus hide\n> potential regressions, an addition to attaining the end-goal of marking\n> some specific test as passing.\n\nI'd argue it _is_ a known specific leak. It is rev_info going out of\nscope that causes the leak, not rev_info holding on to memory in things\nlike its pending array.\n\nNow those can be interesting, too (if it no longer needs the memory, can\nit perhaps get rid of it). But IMHO all of that is pretty secondary to\nclearing the noise so you can find \"true\" leaks (ones where some\nsub-function really is allocating and then losing the pointer entirely,\nespecially if it's called an arbitrary number of times).\n\n> > - leak-checkers only know where things are allocated, not who is\n> > supposed to own them. So you're often annotating the wrong thing;\n> > it's not a strdup() call which is buggy and leaking, but the\n> > function five layers up the stack which was supposed to take\n> > ownership and didn't.\n> \n> As noted in [3] this case is because the LSAN suppressions list was in\n> play, so we excluded the meaningful part of the stack trace (which is\n> shown in that mail).\n\nI don't think that's true at all. The annotations you showed in that\nmessage, for example, are pointing at add_rev_cmdline(). But that is\n_not_ the source of the leak, nor where it should be fixed. It is a\nnecessary part of how rev_info works. The leak is when the rev_info goes\nout of scope without anybody cleaning it up.\n\n> Hrm, now that I think about it I think that the cases where I had to\n> resort to valgrind to get around such crappy stacktraces (when that was\n> all I got, even without suppressions) were probably all because there\n> was an UNLEAK() in play...\n\nI don't see how UNLEAK() would impact stack traces. It should either\nmake something not-leaked-at-all (in which case LSan will no longer\nmention it), or it does nothing (it throws some wasted memory into a\nstructure which is itself not leaked).\n\n-Peff\n"},{"id":"439828","messageId":"xmqqy26euskb.fsf@gitster.g","threadId":"56744","inReplyTo":"YXkWhFGS/uTAnsI1@coredump.intra.peff.net","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-27T20:21:56Z","receivedAt":"2021-10-27T20:22:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think that's true at all. The annotations you showed in that\n> message, for example, are pointing at add_rev_cmdline(). But that is\n> _not_ the source of the leak, nor where it should be fixed. It is a\n> necessary part of how rev_info works. The leak is when the rev_info goes\n> out of scope without anybody cleaning it up.\n\nTrue.\n\n>> Hrm, now that I think about it I think that the cases where I had to\n>> resort to valgrind to get around such crappy stacktraces (when that was\n>> all I got, even without suppressions) were probably all because there\n>> was an UNLEAK() in play...\n>\n> I don't see how UNLEAK() would impact stack traces. It should either\n> make something not-leaked-at-all (in which case LSan will no longer\n> mention it), or it does nothing (it throws some wasted memory into a\n> structure which is itself not leaked).\n\nI'd prefer to see judicious use of UNLEAK() over LSAN suppressions\nthat lists things that are not the source of the problem. The list\nbeing kept in the tests that are far away from the actual source\ndoes not help, either.\n\nThanks.\n"},{"id":"439833","messageId":"211027.865ytixjcw.gmgdl@evledraar.gmail.com","threadId":"56744","inReplyTo":"YXkWhFGS/uTAnsI1@coredump.intra.peff.net","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-27T20:57:52Z","receivedAt":"2021-10-27T21:12:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 27 2021, Jeff King wrote:\n\n> On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > - it keeps the annotations near the code. Yes, that creates conflicts\n>> > when the code is changed (or the leak is actually fixed), but that's\n>> > a feature. It keeps them from going stale.\n>> \n>> I fully agree with you in general for \"good\" uses of UNLEAK(). E.g. consider:\n>> \n>> struct strbuf buf = xmalloc(...);\n>> [...]\n>> UNLEAK(buf);\n>> \n>> If I was fixing leaks in that sort of code what I pointed out downthread\n>> in [1] wouldn't apply.\n>\n> I'm OK with such a use of UNLEAK(), though of course you could probably\n> just free the buffer in that instance.\n>\n> But...\n>\n>> So to clarify, I'm not asking in [1] that UNLEAK() not be used at all\n>> while we have in-flight leak fixes. I.e. I'd run into a textual\n>> conflict, but that would be trivial to resolve, and obvious what the\n>> semantic & textual conflict was.\n>> \n>> Rather, it's not marking specific leaks, but UNLEAK() on a container\n>> struct that's problematic.\n>\n> ...that's the whole point of UNLEAK(), IMHO. You know that the container\n> struct is leaked, but you don't care because the program is about to\n> exit, or there's no easy way to avoid the leak.\n>\n> So it's not the \"container\" element, but rather it can be a problem if\n> people annotate too broadly (you will miss some leaks). In the case of\n> rev_info, there is no way to _not_ leak right now, because it has no\n> cleanup function.\n\nIt doesn't have one, but there are uses of setup_revisions() and\nrev_info usage that don't leak, as that builtin/rev-list.c case shows.\n\nI mean, in that case it's not doing much of anything, but at least we\ntest that setup_revisions() itself doesn't leak right now, but wouldn't\nwith UNLEAK().\n\n> In the long run we should probably add one.\n\nAfter the current in-flight patches I've got making diff_free() a bit\nmore useful, and then a revisions_release() that'll handle most cases,\nwhich gets us started on mass-marking the tests as leak free.\n\nSo just FWIW I'm not saying \"hey can we hold off on that UNLEAK() for\nfar future xyz\", but for a thing I've got queued up that I'd rather not\nstart rewriting...\n\n> [...]But in the meantime, annotating them so we can find the\n> _interesting_ leaks is worth doing (and that was the whole point of\n> adding UNLEAK in the first place.\n>\n> And I think the UNLEAK() calls in Taylor's patch are fine in that sense.\n> While yes, _some_ runs of those commands will not leak, the point is to\n> say that when they do leak, they are not interesting. And they are not\n> interesting because that rev_info is in scope until the program exits,\n> so anything it points to is only leaked at a moment where we no longer\n> care.\n\nI don't per-se care about leaks at program exit either, but am making\nthem leak-free as a proxy for making the relevant APIs useful for\npersistent use.\n\nSo I think the historical approach of just sprinkling UNLEAK() since\nwe're exiting anyway is taking too narrow of a view.\n\nIn that case if we'd like to see if revisions.[ch] is leak-free we'd\nneed parallel test-helper tests (or whatever) for all its features, or\nwe can just piggy-back on every \"git log\" invocation in the test\nsuite...\n\n>> So we wouldn't just be marking a known specific leak, but hiding all\n>> leaks & non-leaks in container from the top-level, and thus hide\n>> potential regressions, an addition to attaining the end-goal of marking\n>> some specific test as passing.\n>\n> I'd argue it _is_ a known specific leak. It is rev_info going out of\n> scope that causes the leak, not rev_info holding on to memory in things\n> like its pending array.\n>\n> Now those can be interesting, too (if it no longer needs the memory, can\n> it perhaps get rid of it). But IMHO all of that is pretty secondary to\n> clearing the noise so you can find \"true\" leaks (ones where some\n> sub-function really is allocating and then losing the pointer entirely,\n> especially if it's called an arbitrary number of times).\n\nI wasn't trying to get into the philosophical discussion of whether a\nstruct or a struct it contains can be said to be the \"real\" source of a\nleak :)\n\nYeah, I agree that from that POV there's no difference.\n\nI'm merely referring to the practical aspect of patches I've got queued\nup, which do things like fix all leaks of rev_info.pending, mark tests\nthat now pass, fix another rev_info.whatever, mark tests etc.\n\nFor rev_info in particular it's a bit iffy to UNLEAK() it, since it also\ncontains pointers to stuff it doesn't \"own\", sometimes things are\nallocated and handed off to it, sometimes for a while, sometimes\npermanently.\n\nSo by UNLEAK()ing it you're not just unleaking \"its\" memory, but\npotentially hiding leaks in APIs that need to interact with it in that\nway. E.g. mailmap.c is one of those cases.\n\n>> > - leak-checkers only know where things are allocated, not who is\n>> > supposed to own them. So you're often annotating the wrong thing;\n>> > it's not a strdup() call which is buggy and leaking, but the\n>> > function five layers up the stack which was supposed to take\n>> > ownership and didn't.\n>> \n>> As noted in [3] this case is because the LSAN suppressions list was in\n>> play, so we excluded the meaningful part of the stack trace (which is\n>> shown in that mail).\n>\n> I don't think that's true at all. The annotations you showed in that\n> message, for example, are pointing at add_rev_cmdline(). But that is\n> _not_ the source of the leak, nor where it should be fixed. It is a\n> necessary part of how rev_info works. The leak is when the rev_info goes\n> out of scope without anybody cleaning it up.\n>\n>> Hrm, now that I think about it I think that the cases where I had to\n>> resort to valgrind to get around such crappy stacktraces (when that was\n>> all I got, even without suppressions) were probably all because there\n>> was an UNLEAK() in play...\n>\n> I don't see how UNLEAK() would impact stack traces. It should either\n> make something not-leaked-at-all (in which case LSan will no longer\n> mention it), or it does nothing (it throws some wasted memory into a\n> structure which is itself not leaked).\n\nYes, I think either categorically wrong here, or it applies to some\nother case I wasn't able to dig up. Or maybe not, doesn't Taylor's\nexample take it from \"Direct leak\" to \"Indirect leak\" with the\nsuppression in play? I think those were related somehow (but don't have\nthat in front of me as I type this out).\n\nE.g. (to reinforce your point) try compiling with SANITIZE=leak and running:\n\n $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200\n 1466000000 -> +0000\n +0200 -> +0000\n \n =================================================================\n ==335188==ERROR: LeakSanitizer: detected memory leaks\n \n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42\n \n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n\nNow try with \"SANITIZE=\" and valgrind can give you the \"real\" source:\n\n $ TZ=UTC valgrind --leak-check=full t/helper/test-tool date show:format:%z 1466000000 +0200\n ==336515== Memcheck, a memory error detector\n ==336515== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.\n ==336515== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info\n ==336515== Command: t/helper/test-tool date show:format:%z 1466000000 +0200\n ==336515==\n 1466000000 -> +0000\n +0200 -> +0000\n ==336515==\n ==336515== HEAP SUMMARY:\n ==336515== in use at exit: 1,208 bytes in 13 blocks\n ==336515== total heap usage: 50 allocs, 37 frees, 27,593 bytes allocated\n ==336515==\n ==336515== 3 bytes in 1 blocks are definitely lost in loss record 1 of 13\n ==336515== at 0x483877F: malloc (vg_replace_malloc.c:307)\n ==336515== by 0x49C6E4A: strdup (strdup.c:42)\n ==336515== by 0x261E3A: xstrdup (wrapper.c:29)\n ==336515== by 0x152785: parse_date_format (date.c:991)\n ==336515== by 0x116A34: show_dates (test-date.c:39)\n ==336515== by 0x116DE9: cmd__date (test-date.c:116)\n ==336515== by 0x115227: cmd_main (test-tool.c:127)\n ==336515== by 0x115334: main (common-main.c:52)\n ==336515==\n ==336515== LEAK SUMMARY:\n ==336515== definitely lost: 3 bytes in 1 blocks\n ==336515== indirectly lost: 0 bytes in 0 blocks\n ==336515== possibly lost: 0 bytes in 0 blocks\n ==336515== still reachable: 1,205 bytes in 12 blocks\n ==336515== suppressed: 0 bytes in 0 blocks\n ==336515== Reachable blocks (those to which a pointer was found) are not shown.\n ==336515== To see them, rerun with: --leak-check=full --show-leak-kinds=all\n ==336515==\n ==336515== For lists of detected and suppressed errors, rerun with: -s\n ==336515== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)\n\nI don't know why it habitually loses track of seemingly easy-to-track\nleaks sometimes, but it does. There isn't an UNLEAK() in play here.\n\nWhen I run into these I try it with valgrind, fix the leak, then test\nagain with SANITIZE=leak. I haven't found cases where it actually misses\nthings, just cases where the stack traces suck.\n"},{"id":"440067","messageId":"YXxf/+dwx5giy6im@coredump.intra.peff.net","threadId":"56744","inReplyTo":"211027.865ytixjcw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-29T20:56:31Z","receivedAt":"2021-10-29T20:56:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 27, 2021 at 10:57:52PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > So it's not the \"container\" element, but rather it can be a problem if\n> > people annotate too broadly (you will miss some leaks). In the case of\n> > rev_info, there is no way to _not_ leak right now, because it has no\n> > cleanup function.\n> \n> It doesn't have one, but there are uses of setup_revisions() and\n> rev_info usage that don't leak, as that builtin/rev-list.c case shows.\n> \n> I mean, in that case it's not doing much of anything, but at least we\n> test that setup_revisions() itself doesn't leak right now, but wouldn't\n> with UNLEAK().\n\nI don't think that's true. If you UNLEAK() the rev_info in the caller,\nthen it will only affect allocations that are still reachable from\nrev_info. I.e., things that are by definition not a leak in\nsetup_revisions().\n\nNow you could argue that setup_revisions() is \"leaking\" by allocating\nthings and stuffing them into rev_info that it should not be. But we can\nnever know that until we have an actual function that cleans up a\nrev_info, which defines what it's \"supposed\" to have ownership of.\n\nMaybe we have callers that explicitly try to de-allocate bits of the\nrev_info. But IMHO that is the source of the whole problem: how is\nrandom code using rev_info supposed to know which of its internal\ndetails are owned or not? This should be documented and enforced with a\nsingle function.\n\n> So just FWIW I'm not saying \"hey can we hold off on that UNLEAK() for\n> far future xyz\", but for a thing I've got queued up that I'd rather not\n> start rewriting...\n\nJust to be clear: I am totally fine with dropping Taylor's UNLEAK\npatches (as I've said already). I was only arguing here that annotating\nvia external files is worse than just adding an UNLEAK().\n\nI'm also trying to combat what I see as mis-conceptions or inaccuracies\nabout what UNLEAK() does or its implications (or even what counts as a\n\"leak\"). But I hope in the long run that we don't need _any_ kind of\nannotation, because we'll actually be leak-free. And then we don't have\nto care about any of this.\n\n> > I don't see how UNLEAK() would impact stack traces. It should either\n> > make something not-leaked-at-all (in which case LSan will no longer\n> > mention it), or it does nothing (it throws some wasted memory into a\n> > structure which is itself not leaked).\n> \n> Yes, I think either categorically wrong here, or it applies to some\n> other case I wasn't able to dig up. Or maybe not, doesn't Taylor's\n> example take it from \"Direct leak\" to \"Indirect leak\" with the\n> suppression in play? I think those were related somehow (but don't have\n> that in front of me as I type this out).\n\nI don't think UNLEAK() can move something from \"direct\" to \"indirect\" in\nLSan's terminology. If rev_info points to an array of structs, and those\nstructs point to allocated strings, then the array itself is a \"direct\"\nleak, and the strings are \"indirect\" (they are leaked, but presumably\nfixing the direct leak would also deallocate them).\n\nIf UNLEAK() makes the array not-leaked, then those indirect leaks don't\nbecome direct. They should be transitively not-leaked, too.\n\n> E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:\n> \n> $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200\n> 1466000000 -> +0000\n> +0200 -> +0000\n> \n> =================================================================\n> ==335188==ERROR: LeakSanitizer: detected memory leaks\n> \n> Direct leak of 3 byte(s) in 1 object(s) allocated from:\n> #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n> #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42\n> \n> SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n\nSo these should be real leaks. Of course with the lousy stack trace it's\nhard to see what they are. But I don't see how UNLEAK() is responsible\nfor making the lousy stack trace. You could try compiling with LSan but\n_not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad\n(but I expect it to be, since test-date.c does not have any UNLEAK()\ncalls in it).\n\n-Peff\n"},{"id":"440068","messageId":"YXxh/GMuy+sBViVY@coredump.intra.peff.net","threadId":"56744","inReplyTo":"YXxf/+dwx5giy6im@coredump.intra.peff.net","subject":"Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS \"suppressions\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-29T21:05:00Z","receivedAt":"2021-10-29T21:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 29, 2021 at 04:56:31PM -0400, Jeff King wrote:\n\n> > E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:\n> > \n> > $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200\n> > 1466000000 -> +0000\n> > +0200 -> +0000\n> > \n> > =================================================================\n> > ==335188==ERROR: LeakSanitizer: detected memory leaks\n> > \n> > Direct leak of 3 byte(s) in 1 object(s) allocated from:\n> > #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n> > #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42\n> > \n> > SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n> \n> So these should be real leaks. Of course with the lousy stack trace it's\n> hard to see what they are. But I don't see how UNLEAK() is responsible\n> for making the lousy stack trace. You could try compiling with LSan but\n> _not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad\n> (but I expect it to be, since test-date.c does not have any UNLEAK()\n> calls in it).\n\nI just tested this, and it is unrelated to UNLEAK(). Interestingly,\ncompiling with SANITIZE=address does give the correct stack trace, so I\nthink LSan is just buggy here for some reason.\n\nWe usually suppress leaks with ASAN_OPTIONS because there are so many\n(and we want to get at the more important signal of actual memory\nerrors). But it wouldn't be hard to have that setting respect your\nPASSING_SANITIZE_LEAK variables. The big downside is that ASan runs of\nthe test suite are much more expensive.\n\nHmm. A little googling turns up LSan's fast_unwind_on_malloc option. And\nindeed:\n\n $ LSAN_OPTIONS=fast_unwind_on_malloc=0 t/helper/test-tool date show:format:%z 1466000000 +0200\n 1466000000 -> +0000\n +0200 -> +0000\n \n =================================================================\n ==39628==ERROR: LeakSanitizer: detected memory leaks\n \n Direct leak of 3 byte(s) in 1 object(s) allocated from:\n #0 0x7fa664e39b94 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:56\n #1 0x7fa664c124aa in __GI___strdup string/strdup.c:42\n #2 0x557636c40c2e in xstrdup /home/peff/compile/git/wrapper.c:29\n #3 0x557636b2ae52 in parse_date_format /home/peff/compile/git/date.c:991\n #4 0x557636aedb0e in show_dates t/helper/test-date.c:39\n #5 0x557636aedee1 in cmd__date t/helper/test-date.c:116\n #6 0x557636aec1f0 in cmd_main t/helper/test-tool.c:127\n #7 0x557636aec30d in main /home/peff/compile/git/common-main.c:52\n #8 0x7fa664babe49 in __libc_start_main ../csu/libc-start.c:314\n #9 0x557636aebec9 in _start (/home/peff/compile/git/t/helper/test-tool+0xcec9)\n \n SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).\n\nSo perhaps we ought to be setting that by default.\n\n-Peff\n"},{"id":"444888","messageId":"xmqq5yre7w5o.fsf@gitster.g","threadId":"56744","inReplyTo":"xmqqa6izcwio.fsf_-_@gitster.g","subject":"[PATCH v2] SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-23T23:12:51Z","receivedAt":"2021-12-23T23:12:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We encourage identifying what, among many topics on `next`, exact\ntopics a new work depends on, instead of building directly on\n`next`. Let's clarify this in the documentation.\n\nDevelopers should know what they are building on top of, and be\naware of which part of the system is currently being worked on.\nEncouraging them to make trial merges to `next` and `seen`\nthemselves will incentivize them to read others' changes and\nunderstand them, eventually helping the developers to coordinate\namong themselves and reviewing each others' changes.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------\n 1 file changed, 39 insertions(+), 14 deletions(-)\n\n * I've been trying to clear the deck, and noticed that this has\n been untended for quite some time. With some clarification to\n a place I was even confused myself while responding to Fabian's\n comments in the earlier round.\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex e409022d93..3c4c5d9f18 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -19,8 +19,10 @@ change is relevant to.\n base your work on the tip of the topic.\n \n * A new feature should be based on `master` in general. If the new\n- feature depends on a topic that is in `seen`, but not in `master`,\n- base your work on the tip of that topic.\n+ feature depends on other topics that are in `next`, but not in\n+ `master`, fork a branch from the tip of `master`, merge these topics\n+ to the branch, and work on that branch. You can remind yourself of\n+ how you prepared the base with `git log --first-parent master..`.\n \n * Corrections and enhancements to a topic not yet in `master` should\n be based on the tip of that topic. If the topic has not been merged\n@@ -28,10 +30,10 @@ change is relevant to.\n into the series.\n \n * In the exceptional case that a new feature depends on several topics\n- not in `master`, start working on `next` or `seen` privately and send\n- out patches for discussion. Before the final merge, you may have to\n- wait until some of the dependent topics graduate to `master`, and\n- rebase your work.\n+ not in `master`, start working on `next` or `seen` privately and\n+ send out patches only for discussion. Once your new feature starts\n+ to stabilize, you would have to rebase it (see the \"depends on other\n+ topics\" above).\n \n * Some parts of the system have dedicated maintainers with their own\n repositories (see the section \"Subsystems\" below). Changes to\n@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing. See\n [[tests]]\n When adding a new feature, make sure that you have new tests to show\n the feature triggers the new behavior when it should, and to show the\n-feature does not trigger when it shouldn't. After any code change, make\n-sure that the entire test suite passes.\n+feature does not trigger when it shouldn't. After any code change,\n+make sure that the entire test suite passes. When fixing a bug, make\n+sure you have new tests that breaks if somebody else breaks what you\n+fixed by accident to avoid regression. Also, try merging your work to\n+'next' and 'seen' and make sure the tests still pass; topics by others\n+that are still in flight may have unexpected interactions with what\n+you are trying to do in your topic.\n \n Pushing to a fork of https://github.com/git/git will use their CI\n integration to test your changes on Linux, Mac and Windows. See the\n@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list\n archive, summarize the relevant points of the discussion.\n \n [[commit-reference]]\n-If you want to reference a previous commit in the history of a stable\n-branch, use the format \"abbreviated hash (subject, date)\", like this:\n+\n+There are a few reasons why you may want to refer to another commit in\n+the \"more stable\" part of the history (i.e. on branches like `maint`,\n+`master`, and `next`):\n+\n+. A commit that introduced the root cause of a bug you are fixing.\n+\n+. A commit that introduced a feature that is what you are enhancing.\n+\n+. A commit that conflicts with your work when you made a trial merge\n+ of your work into `next` and `seen` for testing.\n+\n+When you reference a commit on a more stable branch (like `master`,\n+`maint` and `next`), use the format \"abbreviated hash (subject,\n+date)\", like this:\n \n ....\n \tCommit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,\n or include any extra files which do not relate to what your patch\n is trying to achieve. Make sure to review\n your patch after generating it, to ensure accuracy. Before\n-sending out, please make sure it cleanly applies to the `master`\n-branch head. If you are preparing a work based on \"next\" branch,\n-that is fine, but please mark it as such.\n+sending out, please make sure it cleanly applies to the base you\n+have chosen in the \"Decide what to base your work on\" section,\n+and unless it targets the `master` branch (which is the default),\n+mark your patches as such.\n+\n \n [[send-patches]]\n === Sending your patches.\n@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.\n Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n people who are involved in the area you are touching (the `git\n contacts` command in `contrib/contacts/` can help to\n-identify them), to solicit comments and reviews.\n+identify them), to solicit comments and reviews. Also, when you made\n+trial merges of your topic to `next` and `seen`, you may have noticed\n+work by others conflicting with your changes. There is a good possibility\n+that these people may know the area you are touching well.\n \n :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]\n :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n"},{"id":"445119","messageId":"CABPp-BFp=UnK=EEmr4GLp4m3e9pZ1yks0gt6yTk5OcNXD3LNKA@mail.gmail.com","threadId":"56744","inReplyTo":"xmqq5yre7w5o.fsf@gitster.g","subject":"Re: [PATCH v2] SubmittingPatchs: clarify choice of base and testing","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-28T17:47:08Z","receivedAt":"2021-12-28T17:47:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Dec 24, 2021 at 5:25 PM Junio C Hamano wrote:\n>\n> We encourage identifying what, among many topics on `next`, exact\n> topics a new work depends on, instead of building directly on\n> `next`. Let's clarify this in the documentation.\n>\n> Developers should know what they are building on top of, and be\n> aware of which part of the system is currently being worked on.\n> Encouraging them to make trial merges to `next` and `seen`\n> themselves will incentivize them to read others' changes and\n> understand them, eventually helping the developers to coordinate\n> among themselves and reviewing each others' changes.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------\n> 1 file changed, 39 insertions(+), 14 deletions(-)\n>\n> * I've been trying to clear the deck, and noticed that this has\n> been untended for quite some time. With some clarification to\n> a place I was even confused myself while responding to Fabian's\n> comments in the earlier round.\n>\n> diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\n> index e409022d93..3c4c5d9f18 100644\n> --- c/Documentation/SubmittingPatches\n> +++ w/Documentation/SubmittingPatches\n> @@ -19,8 +19,10 @@ change is relevant to.\n> base your work on the tip of the topic.\n>\n> * A new feature should be based on `master` in general. If the new\n> - feature depends on a topic that is in `seen`, but not in `master`,\n> - base your work on the tip of that topic.\n> + feature depends on other topics that are in `next`, but not in\n> + `master`, fork a branch from the tip of `master`, merge these topics\n> + to the branch, and work on that branch. You can remind yourself of\n> + how you prepared the base with `git log --first-parent master..`.\n>\n> * Corrections and enhancements to a topic not yet in `master` should\n> be based on the tip of that topic. If the topic has not been merged\n> @@ -28,10 +30,10 @@ change is relevant to.\n> into the series.\n>\n> * In the exceptional case that a new feature depends on several topics\n> - not in `master`, start working on `next` or `seen` privately and send\n> - out patches for discussion. Before the final merge, you may have to\n> - wait until some of the dependent topics graduate to `master`, and\n> - rebase your work.\n> + not in `master`, start working on `next` or `seen` privately and\n> + send out patches only for discussion. Once your new feature starts\n> + to stabilize, you would have to rebase it (see the \"depends on other\n> + topics\" above).\n>\n> * Some parts of the system have dedicated maintainers with their own\n> repositories (see the section \"Subsystems\" below). Changes to\n> @@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing. See\n> [[tests]]\n> When adding a new feature, make sure that you have new tests to show\n> the feature triggers the new behavior when it should, and to show the\n> -feature does not trigger when it shouldn't. After any code change, make\n> -sure that the entire test suite passes.\n> +feature does not trigger when it shouldn't. After any code change,\n> +make sure that the entire test suite passes. When fixing a bug, make\n> +sure you have new tests that breaks if somebody else breaks what you\n> +fixed by accident to avoid regression. Also, try merging your work to\n> +'next' and 'seen' and make sure the tests still pass; topics by others\n> +that are still in flight may have unexpected interactions with what\n> +you are trying to do in your topic.\n>\n> Pushing to a fork of https://github.com/git/git will use their CI\n> integration to test your changes on Linux, Mac and Windows. See the\n> @@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list\n> archive, summarize the relevant points of the discussion.\n>\n> [[commit-reference]]\n> -If you want to reference a previous commit in the history of a stable\n> -branch, use the format \"abbreviated hash (subject, date)\", like this:\n> +\n> +There are a few reasons why you may want to refer to another commit in\n> +the \"more stable\" part of the history (i.e. on branches like `maint`,\n> +`master`, and `next`):\n> +\n> +. A commit that introduced the root cause of a bug you are fixing.\n> +\n> +. A commit that introduced a feature that is what you are enhancing.\n> +\n> +. A commit that conflicts with your work when you made a trial merge\n> + of your work into `next` and `seen` for testing.\n> +\n> +When you reference a commit on a more stable branch (like `master`,\n> +`maint` and `next`), use the format \"abbreviated hash (subject,\n> +date)\", like this:\n\nI was going to comment that this would be a good place to mention\n--pretty=reference, but looking at the file in question, that is\nexactly what the text after this already does.\n\n> ....\n> Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n> @@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,\n> or include any extra files which do not relate to what your patch\n> is trying to achieve. Make sure to review\n> your patch after generating it, to ensure accuracy. Before\n> -sending out, please make sure it cleanly applies to the `master`\n> -branch head. If you are preparing a work based on \"next\" branch,\n> -that is fine, but please mark it as such.\n> +sending out, please make sure it cleanly applies to the base you\n> +have chosen in the \"Decide what to base your work on\" section,\n> +and unless it targets the `master` branch (which is the default),\n> +mark your patches as such.\n> +\n>\n> [[send-patches]]\n> === Sending your patches.\n> @@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.\n> Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n> people who are involved in the area you are touching (the `git\n> contacts` command in `contrib/contacts/` can help to\n> -identify them), to solicit comments and reviews.\n> +identify them), to solicit comments and reviews. Also, when you made\n> +trial merges of your topic to `next` and `seen`, you may have noticed\n> +work by others conflicting with your changes. There is a good possibility\n> +that these people may know the area you are touching well.\n>\n> :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]\n> :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n\nThis patch looks good to me.\n"},{"id":"445215","messageId":"20211230102050.oe2t5pavijaow5lx@fs","threadId":"56744","inReplyTo":"xmqq5yre7w5o.fsf@gitster.g","subject":"Re: [PATCH v2] SubmittingPatchs: clarify choice of base and testing","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-30T10:20:50Z","receivedAt":"2021-12-30T10:20:56Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 23.12.2021 15:12, Junio C Hamano wrote:\n\nJust some minor pointers/typos. Looks good.\n\n>We encourage identifying what, among many topics on `next`, exact\n>topics a new work depends on, instead of building directly on\n>`next`. Let's clarify this in the documentation.\n>\n>Developers should know what they are building on top of, and be\n>aware of which part of the system is currently being worked on.\n>Encouraging them to make trial merges to `next` and `seen`\n>themselves will incentivize them to read others' changes and\n>understand them, eventually helping the developers to coordinate\n>among themselves and reviewing each others' changes.\n>\n>Signed-off-by: Junio C Hamano \n>---\n> Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------\n> 1 file changed, 39 insertions(+), 14 deletions(-)\n>\n> * I've been trying to clear the deck, and noticed that this has\n> been untended for quite some time. With some clarification to\n> a place I was even confused myself while responding to Fabian's\n> comments in the earlier round.\n>\n>diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\n>index e409022d93..3c4c5d9f18 100644\n>--- c/Documentation/SubmittingPatches\n>+++ w/Documentation/SubmittingPatches\n>@@ -19,8 +19,10 @@ change is relevant to.\n> base your work on the tip of the topic.\n>\n> * A new feature should be based on `master` in general. If the new\n>- feature depends on a topic that is in `seen`, but not in `master`,\n>- base your work on the tip of that topic.\n>+ feature depends on other topics that are in `next`, but not in\n>+ `master`, fork a branch from the tip of `master`, merge these topics\n>+ to the branch, and work on that branch. You can remind yourself of\n>+ how you prepared the base with `git log --first-parent master..`.\n>\n> * Corrections and enhancements to a topic not yet in `master` should\n> be based on the tip of that topic. If the topic has not been merged\n>@@ -28,10 +30,10 @@ change is relevant to.\n> into the series.\n>\n> * In the exceptional case that a new feature depends on several topics\n>- not in `master`, start working on `next` or `seen` privately and send\n>- out patches for discussion. Before the final merge, you may have to\n>- wait until some of the dependent topics graduate to `master`, and\n>- rebase your work.\n>+ not in `master`, start working on `next` or `seen` privately and\n>+ send out patches only for discussion. Once your new feature starts\n>+ to stabilize, you would have to rebase it (see the \"depends on other\n>+ topics\" above).\n>\n> * Some parts of the system have dedicated maintainers with their own\n> repositories (see the section \"Subsystems\" below). Changes to\n>@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing. See\n> [[tests]]\n> When adding a new feature, make sure that you have new tests to show\n> the feature triggers the new behavior when it should, and to show the\n>-feature does not trigger when it shouldn't. After any code change, make\n>-sure that the entire test suite passes.\n>+feature does not trigger when it shouldn't. After any code change,\n>+make sure that the entire test suite passes. When fixing a bug, make\n>+sure you have new tests that breaks if somebody else breaks what you\n\ns/breaks/break\n\n>+fixed by accident to avoid regression. Also, try merging your work to\n>+'next' and 'seen' and make sure the tests still pass; topics by others\n>+that are still in flight may have unexpected interactions with what\n>+you are trying to do in your topic.\n>\n> Pushing to a fork of https://github.com/git/git will use their CI\n> integration to test your changes on Linux, Mac and Windows. See the\n>@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list\n> archive, summarize the relevant points of the discussion.\n>\n> [[commit-reference]]\n>-If you want to reference a previous commit in the history of a stable\n>-branch, use the format \"abbreviated hash (subject, date)\", like this:\n>+\n>+There are a few reasons why you may want to refer to another commit in\n>+the \"more stable\" part of the history (i.e. on branches like `maint`,\n>+`master`, and `next`):\n>+\n>+. A commit that introduced the root cause of a bug you are fixing.\n>+\n>+. A commit that introduced a feature that is what you are enhancing.\n\nI'm not a native speaker, but `that is what` sounds wrong to me.\n`feature which you are enhancing` or `feature that you are enhancing` maybe?\n\n>+\n>+. A commit that conflicts with your work when you made a trial merge\n>+ of your work into `next` and `seen` for testing.\n>+\n>+When you reference a commit on a more stable branch (like `master`,\n>+`maint` and `next`), use the format \"abbreviated hash (subject,\n>+date)\", like this:\n>\n> ....\n> \tCommit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n>@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,\n> or include any extra files which do not relate to what your patch\n> is trying to achieve. Make sure to review\n> your patch after generating it, to ensure accuracy. Before\n>-sending out, please make sure it cleanly applies to the `master`\n>-branch head. If you are preparing a work based on \"next\" branch,\n>-that is fine, but please mark it as such.\n>+sending out, please make sure it cleanly applies to the base you\n>+have chosen in the \"Decide what to base your work on\" section,\n>+and unless it targets the `master` branch (which is the default),\n>+mark your patches as such.\n>+\n\nMaybe add a hint to `--base=` for format-patch?\n\n>\n> [[send-patches]]\n> === Sending your patches.\n>@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.\n> Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n> people who are involved in the area you are touching (the `git\n> contacts` command in `contrib/contacts/` can help to\n>-identify them), to solicit comments and reviews.\n>+identify them), to solicit comments and reviews. Also, when you made\n>+trial merges of your topic to `next` and `seen`, you may have noticed\n>+work by others conflicting with your changes. There is a good possibility\n>+that these people may know the area you are touching well.\n>\n> :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]\n> :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n"},{"id":"445238","messageId":"xmqqh7apsv6s.fsf_-_@gitster.g","threadId":"56744","inReplyTo":"20211230102050.oe2t5pavijaow5lx@fs","subject":"Re* [PATCH v2] SubmittingPatchs: clarify choice of base and testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-30T20:18:35Z","receivedAt":"2021-12-30T20:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> On 23.12.2021 15:12, Junio C Hamano wrote:\n> ...\n>>+feature does not trigger when it shouldn't. After any code change,\n>>+make sure that the entire test suite passes. When fixing a bug, make\n>>+sure you have new tests that breaks if somebody else breaks what you\n>\n> s/breaks/break\n\nCorrect---we are adding multiple tests here.\n\n>>+. A commit that introduced the root cause of a bug you are fixing.\n>>+\n>>+. A commit that introduced a feature that is what you are enhancing.\n>\n> I'm not a native speaker, but `that is what` sounds wrong to me.\n> `feature which you are enhancing` or `feature that you are enhancing` maybe?\n\nI am not either, but it indeed does sound strange. \"A commit that\nintroduced a feature that you are enhancing\" sounds good.\n\n>>@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,\n>> or include any extra files which do not relate to what your patch\n>> is trying to achieve. Make sure to review\n>> your patch after generating it, to ensure accuracy. Before\n>>-sending out, please make sure it cleanly applies to the `master`\n>>-branch head. If you are preparing a work based on \"next\" branch,\n>>-that is fine, but please mark it as such.\n>>+sending out, please make sure it cleanly applies to the base you\n>>+have chosen in the \"Decide what to base your work on\" section,\n>>+and unless it targets the `master` branch (which is the default),\n>>+mark your patches as such.\n>>+\n>\n> Maybe add a hint to `--base=` for format-patch?\n\nI do not think it belongs to this step. There is a separate section\nabout sending patches that mentions the use of format-patch, where it\nmight be more appropriate.\n\nThanks.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH v3] SubmittingPatchs: clarify choice of base and testing\n\nWe encourage identifying what, among many topics on `next`, exact\ntopics a new work depends on, instead of building directly on\n`next`. Let's clarify this in the documentation.\n\nDevelopers should know what they are building on top of, and be\naware of which part of the system is currently being worked on.\nEncouraging them to make trial merges to `next` and `seen`\nthemselves will incentivize them to read others' changes and\nunderstand them, eventually helping the developers to coordinate\namong themselves and reviewing each others' changes.\n\nSigned-off-by: Junio C Hamano \n---\nRange-diff:\n1: 2a277ef302 ! 1: f387f92719 SubmittingPatchs: clarify choice of base and testing\n @@ Documentation/SubmittingPatches: Make sure that you have tests for the bug you a\n -sure that the entire test suite passes.\n +feature does not trigger when it shouldn't. After any code change,\n +make sure that the entire test suite passes. When fixing a bug, make\n -+sure you have new tests that breaks if somebody else breaks what you\n ++sure you have new tests that break if somebody else breaks what you\n +fixed by accident to avoid regression. Also, try merging your work to\n +'next' and 'seen' and make sure the tests still pass; topics by others\n +that are still in flight may have unexpected interactions with what\n @@ Documentation/SubmittingPatches: without external resources. Instead of giving a\n +\n +. A commit that introduced the root cause of a bug you are fixing.\n +\n -+. A commit that introduced a feature that is what you are enhancing.\n ++. A commit that introduced a feature that you are enhancing.\n +\n +. A commit that conflicts with your work when you made a trial merge\n + of your work into `next` and `seen` for testing.\n\n Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++---------\n 1 file changed, 39 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex e409022d93..ad3041da1a 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -19,8 +19,10 @@ change is relevant to.\n base your work on the tip of the topic.\n \n * A new feature should be based on `master` in general. If the new\n- feature depends on a topic that is in `seen`, but not in `master`,\n- base your work on the tip of that topic.\n+ feature depends on other topics that are in `next`, but not in\n+ `master`, fork a branch from the tip of `master`, merge these topics\n+ to the branch, and work on that branch. You can remind yourself of\n+ how you prepared the base with `git log --first-parent master..`.\n \n * Corrections and enhancements to a topic not yet in `master` should\n be based on the tip of that topic. If the topic has not been merged\n@@ -28,10 +30,10 @@ change is relevant to.\n into the series.\n \n * In the exceptional case that a new feature depends on several topics\n- not in `master`, start working on `next` or `seen` privately and send\n- out patches for discussion. Before the final merge, you may have to\n- wait until some of the dependent topics graduate to `master`, and\n- rebase your work.\n+ not in `master`, start working on `next` or `seen` privately and\n+ send out patches only for discussion. Once your new feature starts\n+ to stabilize, you would have to rebase it (see the \"depends on other\n+ topics\" above).\n \n * Some parts of the system have dedicated maintainers with their own\n repositories (see the section \"Subsystems\" below). Changes to\n@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing. See\n [[tests]]\n When adding a new feature, make sure that you have new tests to show\n the feature triggers the new behavior when it should, and to show the\n-feature does not trigger when it shouldn't. After any code change, make\n-sure that the entire test suite passes.\n+feature does not trigger when it shouldn't. After any code change,\n+make sure that the entire test suite passes. When fixing a bug, make\n+sure you have new tests that break if somebody else breaks what you\n+fixed by accident to avoid regression. Also, try merging your work to\n+'next' and 'seen' and make sure the tests still pass; topics by others\n+that are still in flight may have unexpected interactions with what\n+you are trying to do in your topic.\n \n Pushing to a fork of https://github.com/git/git will use their CI\n integration to test your changes on Linux, Mac and Windows. See the\n@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list\n archive, summarize the relevant points of the discussion.\n \n [[commit-reference]]\n-If you want to reference a previous commit in the history of a stable\n-branch, use the format \"abbreviated hash (subject, date)\", like this:\n+\n+There are a few reasons why you may want to refer to another commit in\n+the \"more stable\" part of the history (i.e. on branches like `maint`,\n+`master`, and `next`):\n+\n+. A commit that introduced the root cause of a bug you are fixing.\n+\n+. A commit that introduced a feature that you are enhancing.\n+\n+. A commit that conflicts with your work when you made a trial merge\n+ of your work into `next` and `seen` for testing.\n+\n+When you reference a commit on a more stable branch (like `master`,\n+`maint` and `next`), use the format \"abbreviated hash (subject,\n+date)\", like this:\n \n ....\n \tCommit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)\n@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,\n or include any extra files which do not relate to what your patch\n is trying to achieve. Make sure to review\n your patch after generating it, to ensure accuracy. Before\n-sending out, please make sure it cleanly applies to the `master`\n-branch head. If you are preparing a work based on \"next\" branch,\n-that is fine, but please mark it as such.\n+sending out, please make sure it cleanly applies to the base you\n+have chosen in the \"Decide what to base your work on\" section,\n+and unless it targets the `master` branch (which is the default),\n+mark your patches as such.\n+\n \n [[send-patches]]\n === Sending your patches.\n@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.\n Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n people who are involved in the area you are touching (the `git\n contacts` command in `contrib/contacts/` can help to\n-identify them), to solicit comments and reviews.\n+identify them), to solicit comments and reviews. Also, when you made\n+trial merges of your topic to `next` and `seen`, you may have noticed\n+work by others conflicting with your changes. There is a good possibility\n+that these people may know the area you are touching well.\n \n :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]\n :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n-- \n2.34.1-616-gbc337a5f45\n\n"}]}