[PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[Taylor Blau]

Here is a topic that Ævar got me interested when he mentioned that t5319 is
leaky[1].

This is the result of trying to get t5319 to pass when compiling with
SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in
the pack-bitmap code, and the remaining 10% are sort of random.

I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.

This is based on tb/fix-midx-rename-while-mapped, with a back-merge of
js/windows-ci-path-fix in order to appease CI. It also contains a little bit of
overlap with a patch[2] from Ævar in another series, which he and I discovered
independently. I cherry-picked his patch, since I haven't seen much action on
that series lately.

[1]: https://lore.kernel.org/git/87wnmph73b.fsf@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/patch-v3-08.10-e0a3510dd88-20211013T222329Z-avarab@gmail.com/

Taylor Blau (10):
  midx.c: clean up chunkfile after reading the MIDX
  midx.c: don't leak MIDX from verify_midx_file
  t/helper/test-read-midx.c: free MIDX within read_midx_file()
  builtin/pack-objects.c: don't leak memory via arguments
  builtin/repack.c: avoid leaking child arguments
  builtin/multi-pack-index.c: don't leak concatenated options
  pack-bitmap.c: avoid leaking via midx_bitmap_filename()
  pack-bitmap.c: don't leak type-level bitmaps
  pack-bitmap.c: more aggressively free in free_bitmap_index()
  t5319: UNLEAK() the remaining leaks

Ævar Arnfjörð Bjarmason (1):
  pack-bitmap-write.c: don't return without stop_progress()

 builtin/log.c               |  1 +
 builtin/multi-pack-index.c  |  4 ++++
 builtin/pack-objects.c      | 12 ++++++++----
 builtin/repack.c            | 35 ++++++++++++++++++++++++-----------
 builtin/rev-list.c          |  2 ++
 midx.c                      |  7 +++++--
 pack-bitmap-write.c         |  8 +++++---
 pack-bitmap.c               | 18 ++++++++++++++++--
 t/helper/test-read-midx.c   |  3 ++-
 t/t5319-multi-pack-index.sh |  1 +
 10 files changed, 68 insertions(+), 23 deletions(-)

-- 
2.33.0.96.g73915697e6

[PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX

[Taylor Blau]

In order to read the contents of a MIDX, we initialize a chunkfile
structure which can read the table of contents and assign pointers into
different sections of the file for us.

We do call free(), since the chunkfile struct is heap allocated, but not
the more appropriate free_chunkfile(), which also frees memory that the
structure itself owns.

Call that instead to avoid leaking memory in this function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
The patch contents are from Ævar, but the message is mine. I hope that
he doesn't mind me forging his sign-off here.

 midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 8433086ac1..36e4754767 100644
--- a/midx.c
+++ b/midx.c
@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 	trace2_data_intmax("midx", the_repository, "load/num_packs", m->num_packs);
 	trace2_data_intmax("midx", the_repository, "load/num_objects", m->num_objects);

+	free_chunkfile(cf);
 	return m;

 cleanup_fail:
 	free(m);
 	free(midx_name);
-	free(cf);
+	free_chunkfile(cf);
 	if (midx_map)
 		munmap(midx_map, midx_size);
 	if (0 <= fd)
--
2.33.0.96.g73915697e6

[PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file

[Taylor Blau]

The function midx.c:verify_midx_file() allocate a MIDX struct by calling
load_multi_pack_index(). But when cleaning up, it calls free() without
freeing any resources associated with the MIDX.

Call the more appropriate close_midx() which does free those resources,
which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 36e4754767..ad60e48468 100644
--- a/midx.c
+++ b/midx.c
@@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		 * Remaining tests assume that we have objects, so we can
 		 * return here.
 		 */
-		return verify_midx_error;
+		goto cleanup;
 	}
 
 	if (flags & MIDX_PROGRESS)
@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 	}
 	stop_progress(&progress);
 
+cleanup:
 	free(pairs);
+	close_midx(m);
 
 	return verify_midx_error;
 }
-- 
2.33.0.96.g73915697e6

[PATCH 03/11] t/helper/test-read-midx.c: free MIDX within read_midx_file()

[Taylor Blau]

When calling `read_midx_file()` to show information about a MIDX or list
the objects contained within it we fail to call `close_midx()`, leaking
the memory allocated to store that MIDX.

Fix this by calling `close_midx()` before exiting the function. We can
drop the "early" return when `show_objects` is non-zero, since the next
instruction is also a return.

(We could just as easily put a `cleanup` label here as with previous
patches. But the only other time we terminate the function early is
when we fail to load a MIDX in the first place. `close_midx()` does
handle a NULL argument, but the extra complexity is likely not
warranted).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/helper/test-read-midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index 9d6fa7a377..27072ba94d 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -55,9 +55,10 @@ static int read_midx_file(const char *object_dir, int show_objects)
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);
 		}
-		return 0;
 	}
 
+	close_midx(m);
+
 	return 0;
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH 04/11] builtin/pack-objects.c: don't leak memory via arguments

[Taylor Blau]

When constructing arguments to pass to setup_revision(), pack-objects
only frees the memory used by that array after calling
get_object_list().

Ensure that we call strvec_clear() whether or not we use the arguments
array by cleaning up whenever we exit the function (and rewriting one
early return to jump to a label which frees the memory and then
returns).

We could avoid setting this array up altogether unless we are in the
if-else block that calls get_object_list(), but setting up the argument
array is intermingled with lots of other side-effects, e.g.:

    if (exclude_promisor_objects) {
      use_internal_rev_list = 1;
      fetch_if_missing = 0;
      strvec_push(&rp, "--exclude-promisor-objects");
    }

So it would be awkward to check exclude_promisor_objects twice: first to
set use_internal_rev_list and fetch_if_missing, and then again above
get_object_list() to push the relevant argument onto the array.

Instead, leave the array's construction alone and make sure to free it
unconditionally.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/pack-objects.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1a3dd445f8..857be7826f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4148,11 +4148,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		read_packs_list_from_stdin();
 		if (rev_list_unpacked)
 			add_unreachable_loose_objects();
-	} else if (!use_internal_rev_list)
+	} else if (!use_internal_rev_list) {
 		read_object_list_from_stdin();
-	else {
+	} else {
 		get_object_list(rp.nr, rp.v);
-		strvec_clear(&rp);
 	}
 	cleanup_preferred_base();
 	if (include_tag && nr_result)
@@ -4162,7 +4161,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			    the_repository);
 
 	if (non_empty && !nr_result)
-		return 0;
+		goto cleanup;
 	if (nr_result) {
 		trace2_region_enter("pack-objects", "prepare-pack",
 				    the_repository);
@@ -4183,5 +4182,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			     " pack-reused %"PRIu32),
 			   written, written_delta, reused, reused_delta,
 			   reuse_packfile_objects);
+
+cleanup:
+	strvec_clear(&rp);
+
 	return 0;
 }
-- 
2.33.0.96.g73915697e6

[PATCH 05/11] builtin/repack.c: avoid leaking child arguments

[Taylor Blau]

`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.

In none of these cases do we bother to call child_process_clear(), which
frees the memory associated with each child's arguments and environment.

In order to do so, tweak each function that spawns a child process to
have a `cleanup` label that we always visit before returning from each
function. Then, make sure that we call child_process_clear() as a part
of that label.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 0b2d1e5d82..d16bab09a4 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	FILE *out;
 	struct strbuf line = STRBUF_INIT;
+	int ret = 0;
 
 	prepare_pack_objects(&cmd, args);
 	cmd.in = -1;
@@ -260,7 +261,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 
 	if (cmd.in == -1)
 		/* No packed objects; cmd was never started */
-		return;
+		goto cleanup;
 
 	close(cmd.in);
 
@@ -293,7 +294,12 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 		free(promisor_name);
 	}
 	fclose(out);
-	if (finish_command(&cmd))
+	ret = finish_command(&cmd);
+
+cleanup:
+	child_process_clear(&cmd);
+
+	if (ret)
 		die(_("could not finish pack-objects to repack promisor objects"));
 }
 
@@ -559,10 +565,10 @@ static int write_midx_included_packs(struct string_list *include,
 	struct string_list_item *item;
 	struct packed_git *largest = get_largest_active_pack(geometry);
 	FILE *in;
-	int ret;
+	int ret = 0;
 
 	if (!include->nr)
-		return 0;
+		goto cleanup;
 
 	cmd.in = -1;
 	cmd.git_cmd = 1;
@@ -587,14 +593,19 @@ static int write_midx_included_packs(struct string_list *include,
 
 	ret = start_command(&cmd);
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	in = xfdopen(cmd.in, "w");
 	for_each_string_list_item(item, include)
 		fprintf(in, "%s\n", item->string);
 	fclose(in);
 
-	return finish_command(&cmd);
+	ret = finish_command(&cmd);
+
+cleanup:
+	child_process_clear(&cmd);
+
+	return ret;
 }
 
 int cmd_repack(int argc, const char **argv, const char *prefix)
@@ -608,7 +619,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct pack_geometry *geometry = NULL;
 	struct strbuf line = STRBUF_INIT;
 	struct tempfile *refs_snapshot = NULL;
-	int i, ext, ret;
+	int i, ext, ret = 0;
 	FILE *out;
 	int show_progress = isatty(2);
 
@@ -794,7 +805,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	ret = start_command(&cmd);
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	if (geometry) {
 		FILE *in = xfdopen(cmd.in, "w");
@@ -819,7 +830,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	fclose(out);
 	ret = finish_command(&cmd);
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	if (!names.nr && !po_args.quiet)
 		printf_ln(_("Nothing new to pack."));
@@ -893,7 +904,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		string_list_clear(&include, 0);
 
 		if (ret)
-			return ret;
+			goto cleanup;
 	}
 
 	reprepare_packed_git(the_repository);
@@ -946,12 +957,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_midx_file(get_object_directory(), NULL, NULL, flags);
 	}
 
+cleanup:
 	string_list_clear(&names, 0);
 	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
 	strbuf_release(&line);
+	child_process_clear(&cmd);
 
-	return 0;
+	return ret;
 }
-- 
2.33.0.96.g73915697e6

[PATCH 06/11] builtin/multi-pack-index.c: don't leak concatenated options

[Taylor Blau]

The `multi-pack-index` builtin dynamically allocates an array of
command-line option for each of its separate modes by calling
add_common_options() to concatante the common options with sub-command
specific ones.

Because this operation allocates a new array, we have to be careful to
remember to free it. We already do this in the repack and write
sub-commands, but verify and expire don't. Rectify this by calling
FREE_AND_NULL as the other modes do.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 075d15d706..4480ba3982 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -167,6 +167,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)
 		usage_with_options(builtin_multi_pack_index_verify_usage,
 				   options);
 
+	FREE_AND_NULL(options);
+
 	return verify_midx_file(the_repository, opts.object_dir, opts.flags);
 }
 
@@ -191,6 +193,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)
 		usage_with_options(builtin_multi_pack_index_expire_usage,
 				   options);
 
+	FREE_AND_NULL(options);
+
 	return expire_midx_packs(the_repository, opts.object_dir, opts.flags);
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()

[Taylor Blau]

To construct the path for a MIDX bitmap, midx_bitmap_filename() calls
get_midx_filename(), and uses the result as an argument to xstrfmt().
But get_midx_filename() also uses xstrfmt, and it assumes that the
caller will free the results.

get_midx_filename() could take a strbuf and write to that, but it would
make more work for every other caller. midx_bitmap_filename() could
store the result of get_midx_filename() in a temporary variable which is
freed before returning.

But that intermediate allocation feels wasteful. Since this function is
simple enough, we can instead "inline" its implementation into the
xstrfmt call itself. That is a little gross, but arguably better than
allocating memory unnecessarily.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index f47a0a7db4..d292e81af1 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -292,8 +292,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 
 char *midx_bitmap_filename(struct multi_pack_index *midx)
 {
-	return xstrfmt("%s-%s.bitmap",
-		       get_midx_filename(midx->object_dir),
+	return xstrfmt("%s/pack/multi-pack-index-%s.bitmap",
+		       midx->object_dir,
 		       hash_to_hex(get_midx_checksum(midx)));
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps

[Taylor Blau]

test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,
which compares the result of the on-disk bitmaps with ones generated
on-the-fly during a revision walk.

In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type
bitmaps, 2021-08-24), we hardened those tests to also check the four
special type-level bitmaps, but never freed those bitmaps. We should
have, since each required an allocation when we EWAH-decompressed them.

Free those, plugging that leak, and also free the base (the scratch-pad
bitmap), too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index d292e81af1..0f6656db02 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1721,6 +1721,12 @@ void test_bitmap_walk(struct rev_info *revs)
 	else
 		die("mismatch in bitmap results");
 
+	bitmap_free(result);
+	bitmap_free(tdata.base);
+	bitmap_free(tdata.commits);
+	bitmap_free(tdata.trees);
+	bitmap_free(tdata.blobs);
+	bitmap_free(tdata.tags);
 	free_bitmap_index(bitmap_git);
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Taylor Blau]

The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:

  - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    maps) commit OIDs to their corresponding bitmaps, the bitmaps
    themselves are not freed. Note here that we recycle already-freed
    ewah_bitmaps into a pool, but these are handled correctly by
    ewah_pool_free().

  - We never bother to free the extended index's "positions" map, which
    we always allocate in load_bitmap().

Fix both of these.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 0f6656db02..451ca3512c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b)
 	ewah_pool_free(b->trees);
 	ewah_pool_free(b->blobs);
 	ewah_pool_free(b->tags);
+	if (b->bitmaps) {
+		struct stored_bitmap *sb;
+		kh_foreach_value(b->bitmaps, sb, {
+			ewah_pool_free(sb->root);
+			free(sb);
+		});
+	}
 	kh_destroy_oid_map(b->bitmaps);
 	free(b->ext_index.objects);
 	free(b->ext_index.hashes);
+	kh_destroy_oid_pos(b->ext_index.positions);
 	bitmap_free(b->result);
 	bitmap_free(b->haves);
 	if (bitmap_is_midx(b)) {
-- 
2.33.0.96.g73915697e6

[PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Taylor Blau]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.

We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.

This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e..cab3eaa2ac 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
 
 	QSORT(indexed_commits, indexed_commits_nr, date_compare);
 
-	if (writer.show_progress)
-		writer.progress = start_progress("Selecting bitmap commits", 0);
-
 	if (indexed_commits_nr < 100) {
 		for (i = 0; i < indexed_commits_nr; ++i)
 			push_bitmapped_commit(indexed_commits[i]);
 		return;
 	}
 
+	if (writer.show_progress)
+		writer.progress = start_progress("Selecting bitmap commits", 0);
+
 	for (;;) {
 		struct commit *chosen = NULL;
 
-- 
2.33.0.96.g73915697e6

[PATCH 11/11] t5319: UNLEAK() the remaining leaks

[Taylor Blau]

This resolves the remaining leaks uncovered by running t5319 when
building with SANITIZE=leak. All of the leaks below are related to
allocated memory stored in 'struct rev_info' which is never freed.

We could alternatively implement a function which frees all allocated
memory associated with the rev_info struct, but doing this properly is a
bigger undertaking than I want to deal with right now.

Instead, let's just UNLEAK() the few ones that are holding up t5319,
which enables us to mark that test as TEST_PASSES_SANITIZE_LEAK.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/log.c               | 1 +
 builtin/pack-objects.c      | 1 +
 builtin/rev-list.c          | 2 ++
 pack-bitmap-write.c         | 2 ++
 t/t5319-multi-pack-index.sh | 1 +
 5 files changed, 7 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index f75d87e8d7..ad6dfacf77 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -757,6 +757,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	opt.revarg_opt = REVARG_COMMITTISH;
 	opt.tweak = log_setup_revisions_tweak;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+	UNLEAK(rev);
 	return cmd_log_walk(&rev);
 }
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 857be7826f..6128a2e2a5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3759,6 +3759,7 @@ static void get_object_list(int ac, const char **av)
 		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
 			die(_("bad revision '%s'"), line);
 	}
+	UNLEAK(revs);
 
 	warn_on_object_refname_ambiguity = save_warning;
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 36cb909eba..df3811e763 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	argc = setup_revisions(argc, argv, &revs, &s_r_opt);
 
+	UNLEAK(revs);
+
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;
 	if (revs.bisect)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index cab3eaa2ac..742bae4f57 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -234,6 +234,8 @@ static void bitmap_builder_init(struct bitmap_builder *bb,
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 
+	UNLEAK(revs);
+
 	while ((commit = get_revision(&revs))) {
 		struct commit_list *p = commit->parents;
 		struct bb_commit *c_ent;
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index a3c72b68f7..9cfc3d6661 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -3,6 +3,7 @@
 test_description='multi-pack-indexes'
 . ./test-lib.sh
 
+TEST_PASSES_SANITIZE_LEAK=true
 GIT_TEST_MULTI_PACK_INDEX=0
 objdir=.git/objects
 
-- 
2.33.0.96.g73915697e6

Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file

[Eric Sunshine]

On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau <me@ttaylorr.com> wrote:
> The function midx.c:verify_midx_file() allocate a MIDX struct by calling
> load_multi_pack_index(). But when cleaning up, it calls free() without
> freeing any resources associated with the MIDX.

s/allocate/allocates/

> Call the more appropriate close_midx() which does free those resources,
> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/midx.c b/midx.c
> @@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
> -               return verify_midx_error;
> +               goto cleanup;
>         }
> @@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
>         stop_progress(&progress);
>
> +cleanup:
>         free(pairs);
> +       close_midx(m);
>
>         return verify_midx_error;
>  }

Using the `goto` idiom to ensure cleanup makes perfect sense. For a
few reasons[*], I did spend a some moments wondering if the cognitive
load might be a bit lower by instead adding two close_midx() calls --
one at the early return and one just before the normal return --
rather than using a `goto`, but it's so subjective that it's not worth
worrying about.

FOOTNOTES

[*] First, unlike most cases in which the `goto` jumps over relatively
short blocks of code, the distance in this case between `goto` and the
new label is significant and it's not easy to see at a glance what is
being skipped and how important it might be. Second, `pairs` is still
NULL at the point of the `goto`; I spent extra time checking and
double-checking what free(pairs) was doing in this instance. Finally,
there are enough start/stop-progress calls in this function that it
requires extra mental effort to determine that this `goto` is indeed
safe (at least for the present).

Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Eric Sunshine]

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:
> The function free_bitmap_index() is somewhat lax in what it frees. There
> are two notable examples:
>
>   - While it does call kh_destroy_oid_map on the "bitmaps" map (which
>     maps) commit OIDs to their corresponding bitmaps, the bitmaps
>     themselves are not freed. Note here that we recycle already-freed
>     ewah_bitmaps into a pool, but these are handled correctly by
>     ewah_pool_free().

The parentheses placement seems off; it's not clear what the intent
is. Perhaps either move the closing parenthesis to just before the
comma or drop them altogether.

>   - We never bother to free the extended index's "positions" map, which
>     we always allocate in load_bitmap().
>
> Fix both of these.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Eric Sunshine]

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
> bitmap writing, 2013-12-21), we did not call stop_progress() if we
> reached the early exit in this function.
>
> We could call stop_progress() before we return, but better yet is to
> defer calling start_progress() until we need it.
>
> This will matter in a subsequent commit where we BUG(...) out if this
> happens, and matters now e.g. because we don't have a corresponding
> "region_end" for the progress trace2 event.

Nit: There is no subsequent commit in this series which "BUGS out", so
this comment is confusing.

> Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> The patch contents are from Ævar, but the message is mine. I hope that
> he doesn't mind me forging his sign-off here.
>
>  midx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/midx.c b/midx.c
> index 8433086ac1..36e4754767 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
>  	trace2_data_intmax("midx", the_repository, "load/num_packs", m->num_packs);
>  	trace2_data_intmax("midx", the_repository, "load/num_objects", m->num_objects);
>
> +	free_chunkfile(cf);
>  	return m;
>
>  cleanup_fail:
>  	free(m);
>  	free(midx_name);
> -	free(cf);
> +	free_chunkfile(cf);
>  	if (midx_map)
>  		munmap(midx_map, midx_size);
>  	if (0 <= fd)

The former is not something we can mechanically locate, but the
latter we should be able to.  And indeed this is the only instance
the following experiment finds.

	$ cat >contrib/coccinelle/chunkfile.cocci <<-\EOF
	@@
	identifier f !~ "^free_chunkfile$";
	struct chunkfile *cf;
	@@
	  f(...) {<...
	- free(cf)
	+ free_chunkfile(cf)
	  ...>}
	EOF
	$ make contrib/coccinelle/chunkfile.cocci.patch

Thanks.

Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau <me@ttaylorr.com> wrote:
>> The function midx.c:verify_midx_file() allocate a MIDX struct by calling
>> load_multi_pack_index(). But when cleaning up, it calls free() without
>> freeing any resources associated with the MIDX.
>
> s/allocate/allocates/
>
>> Call the more appropriate close_midx() which does free those resources,
>> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
>>
>> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>> ---
>> diff --git a/midx.c b/midx.c
>> @@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
>> -               return verify_midx_error;
>> +               goto cleanup;
>>         }
>> @@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
>>         stop_progress(&progress);
>>
>> +cleanup:
>>         free(pairs);
>> +       close_midx(m);
>>
>>         return verify_midx_error;
>>  }
>
> Using the `goto` idiom to ensure cleanup makes perfect sense. For a
> few reasons[*], I did spend a some moments wondering if the cognitive
> load might be a bit lower by instead adding two close_midx() calls --
> one at the early return and one just before the normal return --
> rather than using a `goto`, but it's so subjective that it's not worth
> worrying about.
>
> FOOTNOTES
>
> [*] First, unlike most cases in which the `goto` jumps over relatively
> short blocks of code, the distance in this case between `goto` and the
> new label is significant and it's not easy to see at a glance what is
> being skipped and how important it might be. Second, `pairs` is still
> NULL at the point of the `goto`; I spent extra time checking and
> double-checking what free(pairs) was doing in this instance. Finally,
> there are enough start/stop-progress calls in this function that it
> requires extra mental effort to determine that this `goto` is indeed
> safe (at least for the present).

Carefully reading the patch is very good, but at least in a patch
like this, it is immediately obvious that the patch is not making
any of the things the footnote worries about any worse than the
original, by replacing "return" (which by definition will skip all
the things the goto did jump over) with a "goto" to the bottom, no?

Thanks.

Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 20 2021, Taylor Blau wrote:

> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
> bitmap writing, 2013-12-21), we did not call stop_progress() if we
> reached the early exit in this function.
>
> We could call stop_progress() before we return, but better yet is to
> defer calling start_progress() until we need it.
>
> This will matter in a subsequent commit where we BUG(...) out if this
> happens, and matters now e.g. because we don't have a corresponding
> "region_end" for the progress trace2 event.
>
> Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pack-bitmap-write.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 9c55c1531e..cab3eaa2ac 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
>  
>  	QSORT(indexed_commits, indexed_commits_nr, date_compare);
>  
> -	if (writer.show_progress)
> -		writer.progress = start_progress("Selecting bitmap commits", 0);
> -
>  	if (indexed_commits_nr < 100) {
>  		for (i = 0; i < indexed_commits_nr; ++i)
>  			push_bitmapped_commit(indexed_commits[i]);
>  		return;
>  	}
>  
> +	if (writer.show_progress)
> +		writer.progress = start_progress("Selecting bitmap commits", 0);
> +
>  	for (;;) {
>  		struct commit *chosen = NULL;

Looks good :) Just a note that this is in
"ab/only-single-progress-at-once" already in case you didn't spot it.

That series is marked for "next?" (with a question mark) by Junio in the
latest what's cooking, so *hint* *hint* for any last minute review :)

Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 20 2021, Taylor Blau wrote:

> The patch contents are from Ævar, but the message is mine. I hope that
> he doesn't mind me forging his sign-off here.

I don't mind, and (and maybe we should have an in-tree text file for
this) like I think Jeff King has noted for his inline-posted patches for
discussion you can assume my SOB for anything I post here on list
(except noted otherwise) in terms of copyright.

Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 20 2021, Taylor Blau wrote:

> I tried to separate these out based on their respective areas. The last 10% are
> all from leaking memory in the rev_info structure, which I punted on for now by
> just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
> that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
> little gross, so I'm happy to leave it out if others would prefer.

I'll defer to you, but I've got a mild preference for keeping it out. An
upcoming series of patches I'm submitting (I'm waiting on the current
leak fixes to land) will fix most of those rev_info leaks. So not having
UNLEAK() markings in-flight makes things a bit easier to manage.

Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments

[Derrick Stolee]

On 10/20/2021 11:39 PM, Taylor Blau wrote:
> `git repack` invokes a handful of child processes: one to write the
> actual pack, and optionally ones to repack promisor objects and update
> the MIDX.
> 
> In none of these cases do we bother to call child_process_clear(), which
> frees the memory associated with each child's arguments and environment.
> 
> In order to do so, tweak each function that spawns a child process to
> have a `cleanup` label that we always visit before returning from each
> function. Then, make sure that we call child_process_clear() as a part
> of that label.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/repack.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0b2d1e5d82..d16bab09a4 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
>  	struct child_process cmd = CHILD_PROCESS_INIT;
>  	FILE *out;
>  	struct strbuf line = STRBUF_INIT;
> +	int ret = 0;

nit: "ret" is short for "return" which makes me think "this will
be used as 'return ret;'" but we don't do that. Instead, we use
it as the _result_ of an inner call:

> -	if (finish_command(&cmd))
> +	ret = finish_command(&cmd);
> +
> +cleanup:
> +	child_process_clear(&cmd);
> +
> +	if (ret)
>  		die(_("could not finish pack-objects to repack promisor objects"));

For that reason, I would rename this to "res" or "result".

> -	return finish_command(&cmd);
> +	ret = finish_command(&cmd);
> +
> +cleanup:
> +	child_process_clear(&cmd);
> +
> +	return ret;

Here, you are taking the result of an inner call, but actually
returning it. This makes sense to be "ret".

> +cleanup:
>  	string_list_clear(&names, 0);
>  	string_list_clear(&rollback, 0);
>  	string_list_clear(&existing_nonkept_packs, 0);
>  	string_list_clear(&existing_kept_packs, 0);
>  	clear_pack_geometry(geometry);
>  	strbuf_release(&line);
> +	child_process_clear(&cmd);
>  
> -	return 0;
> +	return ret;

Also here is good.

Thanks,
-Stolee

Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[Derrick Stolee]

On 10/20/2021 11:39 PM, Taylor Blau wrote:
> Here is a topic that Ævar got me interested when he mentioned that t5319 is
> leaky[1].
> 
> This is the result of trying to get t5319 to pass when compiling with
> SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in
> the pack-bitmap code, and the remaining 10% are sort of random.
> 
> I tried to separate these out based on their respective areas. The last 10% are
> all from leaking memory in the rev_info structure, which I punted on for now by
> just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
> that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
> little gross, so I'm happy to leave it out if others would prefer.
> 
> This is based on tb/fix-midx-rename-while-mapped, with a back-merge of
> js/windows-ci-path-fix in order to appease CI. It also contains a little bit of
> overlap with a patch[2] from Ævar in another series, which he and I discovered
> independently. I cherry-picked his patch, since I haven't seen much action on
> that series lately.

In my reading, I only found one nitpick that wasn't already caught by
other reviewers. Feel free to ignore my comment as it was just something
that caused me to pause while reading, but isn't that unusual.

Thanks,
-Stolee

Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>  cleanup_fail:
>  	free(m);
>  	free(midx_name);
> -	free(cf);
> +	free_chunkfile(cf);
>  	if (midx_map)
>  		munmap(midx_map, midx_size);
>  	if (0 <= fd)

Not a fault of this patch, but I think the code is calling close()
on an already closed file descriptor in cleanup_fail codepath, when
"goto cleanup_fail" is reached after xmmap() returned, e.g. when
oid_version() does not match hash_version, when we failed to read
the ToC.

Also, it is not clear why is it a dying offence if we do not find
packnames chunk, but it is just a "pretend as if we do not have this
midx file and everybody else is happy" when we failed to read the
ToC.

Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> The function midx.c:verify_midx_file() allocate a MIDX struct by calling
> load_multi_pack_index(). But when cleaning up, it calls free() without
> freeing any resources associated with the MIDX.
>
> Call the more appropriate close_midx() which does free those resources,
> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Nice.

By the way, the function starts like so:

    int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags)
    {
            struct pair_pos_vs_id *pairs = NULL;
            uint32_t i;
            struct progress *progress = NULL;
            struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
            verify_midx_error = 0;

            if (!m) {
                    int result = 0;
                    struct stat sb;
                    char *filename = get_midx_filename(object_dir);
                    if (!stat(filename, &sb)) {
                            error(_("multi-pack-index file exists, but failed to parse"));
                            result = 1;
                    }
                    free(filename);
                    return result;
            }

but after seeing die() sprinkled in load_multi_pack_index() with
checks during parsing, I am not sure if this is a good error
reporting mechanism we are seeing here.

It is wonderful to plug leaks here and there, but to be honest, even
with only a very little parts I saw in this code, I think there are
other things that need clean-up here.

Also, the way the file-scope global verify_midx_error is used is
beyond words _ugly_, if the only reason for its use is to make
midx_report() look simpler, which is what I think is happening.

Not very impressed, but all of the above is not a new issue
introduced by this patch.

Thanks.

Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> `git repack` invokes a handful of child processes: one to write the
> actual pack, and optionally ones to repack promisor objects and update
> the MIDX.
>
> In none of these cases do we bother to call child_process_clear(), which
> frees the memory associated with each child's arguments and environment.
>
> In order to do so, tweak each function that spawns a child process to
> have a `cleanup` label that we always visit before returning from each
> function. Then, make sure that we call child_process_clear() as a part
> of that label.

I have a slight aversion against the fact that this patch makes us
call child_process_clear(), when we know we may have called
finish_command().  Callers of finish_command() in other codepaths
rely on the fact that finish_command() calls child_process_clear(),
which means that because of this change, we are now constrained to
keep child_process_clear() idempotent?

Re: [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> But that intermediate allocation feels wasteful. Since this function is
> simple enough, we can instead "inline" its implementation into the
> xstrfmt call itself. That is a little gross, but arguably better than
> allocating memory unnecessarily.

The function being "simple enough" does not help maintenance burden
this change brings in at all, does it?  Unless it is accompanied by
a comment "the template string used here must be kept in sync with
what is used in the get_midx_filename() defined elsewhere" near the
lines this patch touches, perhaps in big red letters, that is.

Without this caller, get_midx_filename() could be file-scope static
in midx.c and can be eliminated from midx.h, it seems.  If there are
many other filenames like midx_bitmap that are derrived from
midx_filename, it might be worth extending get_midx_filename to take
an extra format parameter (i.e. "I want midx-related filename for
this object directory, but I am getting this .bitmap kind").  Another
remedy may be to move this function to midx.c and make it sit next
to what this function must be always in sync with.

Or use a C preprocessor macro for "%s/pack/multi-pack-index" in
midx.h and use it here and there to make sure they will stay in
sync.

But after reading all the users of get_midx_filename(), especially
given that it is mostly internal implementation detail inside
midx.c, I think rewriting

	f() {
	char *midx = get_midx_filename(...);

	... use midx ...

	cleanup:
	free(midx);
	}

into

	f() {
	struct strbuf midx;

	get_midx_filename(&midx, ...);

	... use midx.buf ...

	cleanup:
	strbuf_release(&midx);
	}

is not too bad a fix for this.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pack-bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index f47a0a7db4..d292e81af1 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -292,8 +292,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>  
>  char *midx_bitmap_filename(struct multi_pack_index *midx)
>  {
> -	return xstrfmt("%s-%s.bitmap",
> -		       get_midx_filename(midx->object_dir),
> +	return xstrfmt("%s/pack/multi-pack-index-%s.bitmap",
> +		       midx->object_dir,
>  		       hash_to_hex(get_midx_checksum(midx)));
>  }

Re: [PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,
> which compares the result of the on-disk bitmaps with ones generated
> on-the-fly during a revision walk.
>
> In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type
> bitmaps, 2021-08-24), we hardened those tests to also check the four
> special type-level bitmaps, but never freed those bitmaps. We should
> have, since each required an allocation when we EWAH-decompressed them.
>
> Free those, plugging that leak, and also free the base (the scratch-pad
> bitmap), too.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---

I've looked at the patches up to this point.  The ones I did not
comment (including this one ;-) looked perfect, and all of them
(including the ones I did comment) were cleanly described to easily
read the reasoning behind these changes, which I highly appreciated.

Especially if I did not quite agree with the reasoning, it helped me
clarify my thought where I found it not agreeable.

Thanks.

Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:
>> The function free_bitmap_index() is somewhat lax in what it frees. There
>> are two notable examples:
>>
>>   - While it does call kh_destroy_oid_map on the "bitmaps" map (which
>>     maps) commit OIDs to their corresponding bitmaps, the bitmaps
>>     themselves are not freed. Note here that we recycle already-freed
>>     ewah_bitmaps into a pool, but these are handled correctly by
>>     ewah_pool_free().
>
> The parentheses placement seems off; it's not clear what the intent
> is. Perhaps either move the closing parenthesis to just before the
> comma or drop them altogether.

Yeah, I think we can do without them and the sentence becomes
clearer (we can add a comma before "which", too).

>>   - We never bother to free the extended index's "positions" map, which
>>     we always allocate in load_bitmap().
>>
>> Fix both of these.
>>
>> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Looks good :) Just a note that this is in
> "ab/only-single-progress-at-once" already in case you didn't spot it.
>
> That series is marked for "next?" (with a question mark) by Junio in the
> latest what's cooking, so *hint* *hint* for any last minute review :)

I wonder if it would help contributors if we give them a simple rule
to follow before sending their patches out:

 - You develop on an appropriate base (either on maint, or master,
   or a merge of selected topics in flight you absolutely have to
   depend on) as usual.

 - You test merge the result to 'seen' and to 'next'.  If you do not
   get heavy conflicts and the tests pass, you are OK.

 - Otherwise, you study what is conflicting, and should review it
   before you submit your topic.

Without understanding what others are doing in the same area, it is
hard to make productive use of your time to develop new things, and
if you are reading others' work in the same area anyway, it would be
a minimum additional cost to write about what you learned in their
work.

Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 0f6656db02..451ca3512c 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1854,9 +1854,17 @@ void free_bitmap_index(struct bitmap_index *b)
>  	ewah_pool_free(b->trees);
>  	ewah_pool_free(b->blobs);
>  	ewah_pool_free(b->tags);
> +	if (b->bitmaps) {
> +		struct stored_bitmap *sb;
> +		kh_foreach_value(b->bitmaps, sb, {
> +			ewah_pool_free(sb->root);
> +			free(sb);
> +		});
> +	}
>  	kh_destroy_oid_map(b->bitmaps);
>  	free(b->ext_index.objects);
>  	free(b->ext_index.hashes);
> +	kh_destroy_oid_pos(b->ext_index.positions);
>  	bitmap_free(b->result);
>  	bitmap_free(b->haves);
>  	if (bitmap_is_midx(b)) {

It is not a fault of this patch, but I was reading the corresponding
load_bitmap() to see if we are covering everything here.

On the error path of that function, where it does "got failed", it
does not clean after itself very well, it seems.  If we read commits
bitmap successfully but failed to read trees bitmap, for example,
the resource held by commits bitmap is never reclaimed.

Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>>  	struct child_process cmd = CHILD_PROCESS_INIT;
>>  	FILE *out;
>>  	struct strbuf line = STRBUF_INIT;
>> +	int ret = 0;
>
> nit: "ret" is short for "return" which makes me think "this will
> be used as 'return ret;'" but we don't do that. Instead, we use
> it as the _result_ of an inner call:

Yup, my reading hiccupped there, too.

>
>> -	if (finish_command(&cmd))
>> +	ret = finish_command(&cmd);
>> +
>> +cleanup:
>> +	child_process_clear(&cmd);
>> +
>> +	if (ret)
>>  		die(_("could not finish pack-objects to repack promisor objects"));
>
> For that reason, I would rename this to "res" or "result".

Yeah, if we are introducing a new variable, result is a good name.

Re: [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX

[Taylor Blau]

On Thu, Oct 21, 2021 at 09:16:27AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >  cleanup_fail:
> >  	free(m);
> >  	free(midx_name);
> > -	free(cf);
> > +	free_chunkfile(cf);
> >  	if (midx_map)
> >  		munmap(midx_map, midx_size);
> >  	if (0 <= fd)
>
> Not a fault of this patch, but I think the code is calling close()
> on an already closed file descriptor in cleanup_fail codepath, when
> "goto cleanup_fail" is reached after xmmap() returned, e.g. when
> oid_version() does not match hash_version, when we failed to read
> the ToC.
>
> Also, it is not clear why is it a dying offence if we do not find
> packnames chunk, but it is just a "pretend as if we do not have this
> midx file and everybody else is happy" when we failed to read the
> ToC.

Yep, I agree with you on both of those. I would be happier to see fewer
die()s deep within midx.c. I'll start a list for myself of some
potential future cleanups in this area that don't involve memory leaks.

My hunch is that there's enough subtlty here that we shouldn't tie the
two (fixing leaks, and other general issues/tidiness in the MIDX code)
together. So I'll keep track of the latter separately and address those
in future series.

Thanks,
Taylor

Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments

[Taylor Blau]

On Thu, Oct 21, 2021 at 09:37:07AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > `git repack` invokes a handful of child processes: one to write the
> > actual pack, and optionally ones to repack promisor objects and update
> > the MIDX.
> >
> > In none of these cases do we bother to call child_process_clear(), which
> > frees the memory associated with each child's arguments and environment.
> >
> > In order to do so, tweak each function that spawns a child process to
> > have a `cleanup` label that we always visit before returning from each
> > function. Then, make sure that we call child_process_clear() as a part
> > of that label.
>
> I have a slight aversion against the fact that this patch makes us
> call child_process_clear(), when we know we may have called
> finish_command().  Callers of finish_command() in other codepaths
> rely on the fact that finish_command() calls child_process_clear(),
> which means that because of this change, we are now constrained to
> keep child_process_clear() idempotent?

Good point. In functions besides cmd_repack(), it's easy enough to just
call child_process_clear() once before returning instead of using a
label. That has the added benefit of side-stepping the variable name
question that you and Stolee raised (but I think whether we call it
"result", "res", or "ret" is besides the point).

In cmd_repack(), I'd like to wrap the other cleanup-related calls
underneath a 'cleanup' label. So there we want to track whether or not
we have called finish_command(), so that we only call
child_process_clear() when we *haven't* already called it via
finish_command().

Thanks,
Taylor

Re: [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename()

[Taylor Blau]

On Thu, Oct 21, 2021 at 09:54:39AM -0700, Junio C Hamano wrote:
> But after reading all the users of get_midx_filename(), especially
> given that it is mostly internal implementation detail inside
> midx.c, I think rewriting
>
> 	f() {
> 	char *midx = get_midx_filename(...);
>
> 	... use midx ...
>
> 	cleanup:
> 	free(midx);
> 	}
>
> into
>
> 	f() {
> 	struct strbuf midx;
>
> 	get_midx_filename(&midx, ...);
>
> 	... use midx.buf ...
>
> 	cleanup:
> 	strbuf_release(&midx);
> 	}
>
> is not too bad a fix for this.

I was initially worried that this was too heavy-handed an approach. But
I hadn't actually tried it, and updating the half-dozen callers of
get_midx_filename() took only a couple of minutes.

But just because something is easy doesn't necessarily mean that it is a
good idea to do ;). Luckily, this approach ended up being clean, and
allows us to implement midx_bitmap_filename() without copying,
open-coding, or leaking.

Thanks for the nice suggestion.

Taylor

Re: [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Taylor Blau]

On Thu, Oct 21, 2021 at 11:32:16AM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:
> >> The function free_bitmap_index() is somewhat lax in what it frees. There
> >> are two notable examples:
> >>
> >>   - While it does call kh_destroy_oid_map on the "bitmaps" map (which
> >>     maps) commit OIDs to their corresponding bitmaps, the bitmaps
> >>     themselves are not freed. Note here that we recycle already-freed
> >>     ewah_bitmaps into a pool, but these are handled correctly by
> >>     ewah_pool_free().
> >
> > The parentheses placement seems off; it's not clear what the intent
> > is. Perhaps either move the closing parenthesis to just before the
> > comma or drop them altogether.
>
> Yeah, I think we can do without them and the sentence becomes
> clearer (we can add a comma before "which", too).

Yep, thanks both.

Thanks,
Taylor

Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Taylor Blau]

On Thu, Oct 21, 2021 at 11:39:34AM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > Looks good :) Just a note that this is in
> > "ab/only-single-progress-at-once" already in case you didn't spot it.
> >
> > That series is marked for "next?" (with a question mark) by Junio in the
> > latest what's cooking, so *hint* *hint* for any last minute review :)
>
> I wonder if it would help contributors if we give them a simple rule
> to follow before sending their patches out:
>
>  - You develop on an appropriate base (either on maint, or master,
>    or a merge of selected topics in flight you absolutely have to
>    depend on) as usual.

That seems like sensible advice to me. When I sent the series out last
night, I figured that it would be preferable to cherry-pick the commit
from Ævar's series, since it wasn't clear whether or not all or parts of
that series were actively moving forward (besides being marked as
"next?" in your What's Cooking email).

But I'll send the next iteration based on a combined merge of my topic
and Ævar's, and note your branch names for both of them clearly. To
encourage both of our topics to move forward, I'll throw Ævar's onto my
review queue.

Thanks,
Taylor

Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[Taylor Blau]

On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Oct 20 2021, Taylor Blau wrote:
>
> > I tried to separate these out based on their respective areas. The last 10% are
> > all from leaking memory in the rev_info structure, which I punted on for now by
> > just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
> > that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
> > little gross, so I'm happy to leave it out if others would prefer.
>
> I'll defer to you, but I've got a mild preference for keeping it out. An
> upcoming series of patches I'm submitting (I'm waiting on the current
> leak fixes to land) will fix most of those rev_info leaks. So not having
> UNLEAK() markings in-flight makes things a bit easier to manage.

If you don't mind, I think keeping this in (as a way to verify that we
really did make t5319 leak-free) may be good for reviewers. When you
clean these areas up, you'll have to remember to remove those UNLEAK()s,
but hopefully they produce conflicts with your in-flight work that serve
as not-too-annoying reminders.

I would certainly rather not have to UNLEAK() those at all, so I am very
excited to see a series from you which handles freeing these resources
appropriately. It was just too big a bite for me to chew off when
preparing this quick series.

Thanks,
Taylor

Re: [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak'

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 22 2021, Taylor Blau wrote:

> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Oct 20 2021, Taylor Blau wrote:
>>
>> > I tried to separate these out based on their respective areas. The last 10% are
>> > all from leaking memory in the rev_info structure, which I punted on for now by
>> > just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
>> > that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
>> > little gross, so I'm happy to leave it out if others would prefer.
>>
>> I'll defer to you, but I've got a mild preference for keeping it out. An
>> upcoming series of patches I'm submitting (I'm waiting on the current
>> leak fixes to land) will fix most of those rev_info leaks. So not having
>> UNLEAK() markings in-flight makes things a bit easier to manage.
>
> If you don't mind, I think keeping this in (as a way to verify that we
> really did make t5319 leak-free) may be good for reviewers. When you
> clean these areas up, you'll have to remember to remove those UNLEAK()s,
> but hopefully they produce conflicts with your in-flight work that serve
> as not-too-annoying reminders.
>
> I would certainly rather not have to UNLEAK() those at all, so I am very
> excited to see a series from you which handles freeing these resources
> appropriately. It was just too big a bite for me to chew off when
> preparing this quick series.

Having looked at this a bit more carefully I've upgraded that a bit from
"I'll defer to you" to strongly objecting to this specific approach (but
read to the end, I think you can have your cake and eat it too).

I just glanced at this series in my previous pass-over, and evidently
didn't read the CL to the end. I thought based on the commit subject
that it was just a change to some midx code impacting t5319.

But it's a bigger change across the whole test suite than that.

If you run:

    rm .prove; GIT_SKIP_TESTS=t0027 prove -j8 --state=save t[0-9]*.sh :: --immediate

Without this patch (t0027 will take forever), and then:

    GIT_SKIP_TESTS=t0027 prove -j8 --state=failed

You'll see that (I use GNU screen's logging feature to get the output)
we made these tests pass, not just your t5319:
    
    $ grep -w ok screenlog.11 
    t0056-git-C.sh .................................... ok                  
    t1060-object-corruption.sh ........................ ok                  
    t4212-log-corrupt.sh .............................. ok                  
    t4207-log-decoration-colors.sh .................... ok                  
    t5313-pack-bounds-checks.sh ....................... ok                  
    t5506-remote-groups.sh ............................ ok                  
    t5513-fetch-track.sh .............................. ok                  
    t5319-multi-pack-index.sh ......................... ok                  
    t5532-fetch-proxy.sh .............................. ok                  
    t5900-repo-selection.sh ........................... ok                  
    t6011-rev-list-with-bad-commit.sh ................. ok                  
    t6100-rev-list-in-order.sh ........................ ok                  
    t6114-keep-packs.sh ............................... ok                  
    t6131-pathspec-icase.sh ........................... ok                  
    t6003-rev-list-topo-order.sh ...................... ok                  
    t7702-repack-cyclic-alternate.sh .................. ok                  
    t9108-git-svn-glob.sh ............................. ok                  
    t9109-git-svn-multi-glob.sh ....................... ok                  
    t9127-git-svn-partial-rebuild.sh .................. ok                  
    t9133-git-svn-nested-git-repo.sh .................. ok                  
    t9168-git-svn-partially-globbed-names.sh .......... ok                  

I've got subsequent patches on top of what's now in-flight that fix
those bigger leaks incrementally, and as part of that add
"TEST_PASSES_SANITIZE_LEAK=true" to /all/ the relevant passing
tests.

I.e. there isn't a 1=1 correspondance between all current passing tests
and "TEST_PASSES_SANITIZE_LEAK=true" markings (some are still left
unmarked). But there has been a 1=1 mapping between freshly pasing tests
as a result of leak fixes and tests that get that
"TEST_PASSES_SANITIZE_LEAK=true" marking.

I think it helps a lot to review those patches & assess their impact if
code changes can be paired with relevant test suite changes, which isn't
the case if we've got simultaneous UNLEAK() markings for major leaks
in-flight.

So the practical effect of that is that I'll need to rebase all that,
change my testing setup to test those one-at-at-time with "git rebase -i
--exec" before submission (which I do anyway), but now with some
selective revert of an earlier UNLEAK() to assert that I'm still fixing
the things I expected.

Then either drop the parts of the commit messages that say "this fixes
leak XYZ, making tests A, B, C pass" to omit any mention of "A, B, C",
or reword it for the earlier now-landed UNLEAK() markings.

These are also just the fully passing tests I ran as a one-off for this
E-Mail. For those I do more exhaustive runs where I'll e.g. spot if a
test goes from N failing to N-X, and note it if that gets us much closer
to 0.

But on the "cake and eat it too" front I also realize that it sucks to
not be able to mark tests you made leak-free as passing just because
I've got some larger more general leak fixes planned, and even if none
of your code leaks, it might just be "git log" or whatever.

But we can just use LSAN_OPTIONS with a suppressions file for that. This
diff below (assume my SOB yadayada) makes your tests pass if I eject the
11/11 and replace that with this:

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index a3c72b68f7c..3f6d716f825 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1,8 +1,23 @@
 #!/bin/sh
 
 test_description='multi-pack-indexes'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+test_expect_success SANITIZE_LEAK 'setup LSAN whitelisting' '
+	suppressions=".git/lsan-suppressions.txt" &&	    
+	cat >"$suppressions" <<-\EOF &&
+	leak:add_rev_cmdline
+	leak:cmd_log_init_finish
+	leak:prep_parse_options
+	leak:prepare_revision_walk
+	leak:strdup
+	EOF
+	LSAN_OPTIONS="$LSAN_OPTIONS:suppressions=\"$PWD/$suppressions\"" &&
+	export LSAN_OPTIONS
+'
+
 GIT_TEST_MULTI_PACK_INDEX=0
 objdir=.git/objects
 
I think that would be a much better approach here, and would really
prefer if we went for some version of that if you really want to test
these right away with the linux-leaks job.

I think the better thing to do here is to just leave it, i.e. we've got
tons of these tests that don't leak themselves, but leak due to "git
log" or whatever already, but anyway, if what you're doing in
t5319-multi-pack-index.sh doesn't cause much more work for me as noted
above I really don't mind.

If you want to pick that approach up and run with it I think it would
probably make sense to factor that suggested test_expect_success out
into a function in test-lib-functions.sh or whatever, and call it as
e.g.:
    
    TEST_PASSES_SANITIZE_LEAK=true
     . ./test-lib.sh
    declare_known_leaks <<-\EOF
    add_rev_cmdline
    [...]
    EOF

Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
you.

Doing it that way would be less boilerplate for each test that wants it,
and is also more likely to work with other non-LSAN leak appoaches,
i.e. as long as something can take a list of lines matching stack traces
we can feed that to that leak checker's idea of a whitelist.

[PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Ævar Arnfjörð Bjarmason]

Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
be able to add a "suppressions" file to the $LSAN_OPTIONS.

This allows for marking tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
general widespread memory leaks, such as from the "git log" family of
commands. We can now mark the "git -C" tests as passing.

For getting specific tests to pass this is preferable to using
UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
and being able to atomically mark relevant tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
[1] for more details.

1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Oct 22 2021, Taylor Blau wrote:
>
>> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Wed, Oct 20 2021, Taylor Blau wrote:
[...]
> If you want to pick that approach up and run with it I think it would
> probably make sense to factor that suggested test_expect_success out
> into a function in test-lib-functions.sh or whatever, and call it as
> e.g.:
>     
>     TEST_PASSES_SANITIZE_LEAK=true
>      . ./test-lib.sh
>     declare_known_leaks <<-\EOF
>     add_rev_cmdline
>     [...]
>     EOF
>
> Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
> you.
>
> Doing it that way would be less boilerplate for each test that wants it,
> and is also more likely to work with other non-LSAN leak appoaches,
> i.e. as long as something can take a list of lines matching stack traces
> we can feed that to that leak checker's idea of a whitelist.

I just went ahead and hacked that up. If you're OK with that approach
it would really help reduce the work for leak changes I've got
planned, and as noted gives you the end-state of a passing 5319.

I don't know if it makes more sense for you to base on top of this
if/when Junio picks it up, or to integrate it into your series
etc. Maybe Junio will chime in ...

 t/t0056-git-C.sh        |  4 ++++
 t/test-lib-functions.sh | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh
index 2630e756dab..490aefa81a1 100755
--- a/t/t0056-git-C.sh
+++ b/t/t0056-git-C.sh
@@ -2,7 +2,11 @@
 
 test_description='"-C <path>" option and its effects on other path-related options'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
+todo_leaks <<-\EOF
+^cmd_log_init_finish$
+EOF
 
 test_expect_success '"git -C <path>" runs git from the directory <path>' '
 	test_create_repo dir1 &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index eef2262a360..d89bf5da7dc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -243,6 +243,48 @@ debug () {
 	done
 }
 
+# Declare known "general" memory leaks, for use with TEST_PASSES_SANITIZE_LEAK=true.
+#
+# Matches lines in a stack trace that leaks. Intended for
+# LSAN_OPTIONS, but the format is intended to be easy to use with
+# other leak checkers, so the "leak:" prefix is omitted (and added for
+# you).
+#
+# Use it immediately after sourcing test-lib.sh (or equivalent), and
+# after a "TEST_PASSES_SANITIZE_LEAK=true" has been set. E.g:
+#
+#    TEST_PASSES_SANITIZE_LEAK=true
+#    . ./test-lib.sh
+#    todo_leaks <<-\EOF
+#    ^cmd_log_init_finish$
+#    EOF
+#
+# The "^" and "$" anchors don't suggest full regex syntax support,
+# that's the only anchoring (or other metacharacter) understood by
+# LSAN_OPTIONS,.
+#
+# See
+# https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions
+# for the relevant LSAN_OPTIONS documentation.
+todo_leaks () {
+	if ! test_have_prereq SANITIZE_LEAK
+	then
+		return 0
+	fi
+
+	# Try not to interfere with any test logic
+	suppressions=.lsan-suppressions.txt
+	if test -d .git
+	then
+		suppressions=".git/$suppressions"
+	fi
+	suppressions="$PWD/$suppressions"
+
+	sed 's/^/leak:/' >"$suppressions" &&
+	LSAN_OPTIONS="$LSAN_OPTIONS:suppressions=\"$suppressions\"" &&
+	export LSAN_OPTIONS
+}
+
 # Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]
 #   -C <dir>:
 #	Run all git commands in directory <dir>
-- 
2.33.1.1494.g88b39a443e1

Re: [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Looks good :) Just a note that this is in
>> "ab/only-single-progress-at-once" already in case you didn't spot it.
>>
>> That series is marked for "next?" (with a question mark) by Junio in the
>> latest what's cooking, so *hint* *hint* for any last minute review :)
>
> I wonder if it would help contributors if we give them a simple rule
> to follow before sending their patches out:
>
>  - You develop on an appropriate base (either on maint, or master,
>    or a merge of selected topics in flight you absolutely have to
>    depend on) as usual.
>
>  - You test merge the result to 'seen' and to 'next'.  If you do not
>    get heavy conflicts and the tests pass, you are OK.
>
>  - Otherwise, you study what is conflicting, and should review it
>    before you submit your topic.

Addendum to this third step.

     ... Also, if you chose to depend on other topics when you
     decided on the base in the first step, giving them your review
     would be a good way to show others that _you_ understand the
     code that you are building on top of, which in turn may give
     others confidence in your topic.

Thanks.

SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

We encourage identifying what, among many topics on `next`, exact
topics a new work depends on, instead of building directly on
`next`.  Let's clarify this in the documentation.

Developers should know what they are building on top of, and be
aware of which part of the system is currently being worked on.
Encouraging them to make trial merges to `next` and `seen`
themselves will incentivize them to read others' changes and
understand them, eventually helping the developers to coordinate
among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This, being a guide for those who submit their work, stops short
   of telling them to send reviews on others' patch that interacts
   with their work, but reading and understanding others' work is
   both necessary to make their own work to play well together with
   other topics, and to start reviewing others' work.

 Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
index e409022d93..2de8f80dc5 100644
--- c/Documentation/SubmittingPatches
+++ w/Documentation/SubmittingPatches
@@ -19,8 +19,11 @@ change is relevant to.
   base your work on the tip of the topic.
 
 * A new feature should be based on `master` in general. If the new
-  feature depends on a topic that is in `seen`, but not in `master`,
-  base your work on the tip of that topic.
+  feature depends on other topics that are in `next`, but not in
+  `master`, fork a branch from the tip of `master`, merge these topics
+  to the branch, and work on that branch.  You can remind yourself of
+  how you prepared the base with `git log --first-parent master..`
+  easily by doing so.
 
 * Corrections and enhancements to a topic not yet in `master` should
   be based on the tip of that topic. If the topic has not been merged
@@ -28,10 +31,10 @@ change is relevant to.
   into the series.
 
 * In the exceptional case that a new feature depends on several topics
-  not in `master`, start working on `next` or `seen` privately and send
-  out patches for discussion. Before the final merge, you may have to
-  wait until some of the dependent topics graduate to `master`, and
-  rebase your work.
+  not in `master`, start working on `next` or `seen` privately and
+  send out patches only for discussion. Once your new feature starts
+  to stabilize, you would have to rebase it (see the "depends on other
+  topics" above).
 
 * Some parts of the system have dedicated maintainers with their own
   repositories (see the section "Subsystems" below).  Changes to
@@ -71,8 +74,13 @@ Make sure that you have tests for the bug you are fixing.  See
 [[tests]]
 When adding a new feature, make sure that you have new tests to show
 the feature triggers the new behavior when it should, and to show the
-feature does not trigger when it shouldn't.  After any code change, make
-sure that the entire test suite passes.
+feature does not trigger when it shouldn't.  After any code change,
+make sure that the entire test suite passes.  When fixing a bug, make
+sure you have new tests that breaks if somebody else breaks what you
+fixed by accident to avoid regression.  Also, try merging your work to
+'next' and 'seen' and make sure the tests still pass; topics by others
+that are still in flight may have unexpected interactions with what
+you are trying to do in your topic.
 
 Pushing to a fork of https://github.com/git/git will use their CI
 integration to test your changes on Linux, Mac and Windows. See the
@@ -144,8 +152,21 @@ without external resources. Instead of giving a URL to a mailing list
 archive, summarize the relevant points of the discussion.
 
 [[commit-reference]]
-If you want to reference a previous commit in the history of a stable
-branch, use the format "abbreviated hash (subject, date)", like this:
+
+There are a few reasons why you may want to refer to another commit in
+the "more stable" part of the history (i.e. on branches like `maint`,
+`master`, and `next`):
+
+. A commit that introduced the root cause of a bug you are fixing.
+
+. A commit that introduced a feature that is what you are enhancing.
+
+. A commit that conflicts with your work when you made a trial merge
+  of your work into `next` and `seen` for testing.
+
+When you reference a commit on a more stable branch (like `master`,
+`maint` and `next`), use the format "abbreviated hash (subject,
+date)", like this:
 
 ....
 	Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
@@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch
 is trying to achieve. Make sure to review
 your patch after generating it, to ensure accuracy.  Before
 sending out, please make sure it cleanly applies to the `master`
-branch head.  If you are preparing a work based on "next" branch,
-that is fine, but please mark it as such.
+branch head.  If you are preparing a work based on selected topics
+merged to `master`, please mark your patch as such.
 
 [[send-patches]]
 === Sending your patches.
@@ -365,7 +386,10 @@ Security mailing list{security-ml-ref}.
 Send your patch with "To:" set to the mailing list, with "cc:" listing
 people who are involved in the area you are touching (the `git
 contacts` command in `contrib/contacts/` can help to
-identify them), to solicit comments and reviews.
+identify them), to solicit comments and reviews.  Also, when you made
+trial merges of your topic to `next` and `seen`, you may have noticed
+work by others conflicting with your changes.  There is a good possibility
+that these people may know the area you are touching well.
 
 :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
 :git-ml: footnote:[The mailing list: git@vger.kernel.org]

Re: SubmittingPatchs: clarify choice of base and testing

[Ævar Arnfjörð Bjarmason]

On Sat, Oct 23 2021, Junio C Hamano wrote:

>  Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 13 deletions(-)
>
> diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
> index e409022d93..2de8f80dc5 100644
> --- c/Documentation/SubmittingPatches
> +++ w/Documentation/SubmittingPatches
> @@ -19,8 +19,11 @@ change is relevant to.
>    base your work on the tip of the topic.
>  
>  * A new feature should be based on `master` in general. If the new
> -  feature depends on a topic that is in `seen`, but not in `master`,
> -  base your work on the tip of that topic.
> +  feature depends on other topics that are in `next`, but not in
> +  `master`, fork a branch from the tip of `master`, merge these topics
> +  to the branch, and work on that branch.  You can remind yourself of
> +  how you prepared the base with `git log --first-parent master..`
> +  easily by doing so.

For what it's worth when I do this I base things on one either your
gitster/* branches, what I've applied from the ML, or grabbed from the
author's own repo, and then usually rebase it on the latest "master" or
<topic> as appropriate so I don't have on old base, particularly in the
case of gitster/*, you don't rebase those unless needed.

Anyway, I can see why you suggest the "base on master and merge", it has
its benefits, but it is in apparent conflict with existing advice added
in 76644e3268b (documentation: add tutorial for first contribution,
2019-05-17).

I.e. the "After Review Approval" section (of all places) discusses what
to do in that situation. It is narrowly talking about submitting
something on top of your own topic, but the advice should be the same in
either case I'd think.

>  * Corrections and enhancements to a topic not yet in `master` should
>    be based on the tip of that topic. If the topic has not been merged
> @@ -28,10 +31,10 @@ change is relevant to.
>    into the series.
>  
>  * In the exceptional case that a new feature depends on several topics
> -  not in `master`, start working on `next` or `seen` privately and send
> -  out patches for discussion. Before the final merge, you may have to
> -  wait until some of the dependent topics graduate to `master`, and
> -  rebase your work.
> +  not in `master`, start working on `next` or `seen` privately and
> +  send out patches only for discussion. Once your new feature starts
> +  to stabilize, you would have to rebase it (see the "depends on other
> +  topics" above).
>  
>  * Some parts of the system have dedicated maintainers with their own
>    repositories (see the section "Subsystems" below).  Changes to
> @@ -71,8 +74,13 @@ Make sure that you have tests for the bug you are fixing.  See
>  [[tests]]
>  When adding a new feature, make sure that you have new tests to show
>  the feature triggers the new behavior when it should, and to show the
> -feature does not trigger when it shouldn't.  After any code change, make
> -sure that the entire test suite passes.
> +feature does not trigger when it shouldn't.  After any code change,
> +make sure that the entire test suite passes.  When fixing a bug, make
> +sure you have new tests that breaks if somebody else breaks what you
> +fixed by accident to avoid regression.  Also, try merging your work to
> +'next' and 'seen' and make sure the tests still pass; topics by others
> +that are still in flight may have unexpected interactions with what
> +you are trying to do in your topic.

Maybe it would be useful to have a CI target that merged to next/master
and reported how that went? It would have to be a soft failure, as we
might have an easy merge conflict, or someone's pushing a revision to a
topic that's already in "next" or "seen" (and might conflict). But
having it as an FYI might be helpful.

>  Pushing to a fork of https://github.com/git/git will use their CI
>  integration to test your changes on Linux, Mac and Windows. See the
> @@ -144,8 +152,21 @@ without external resources. Instead of giving a URL to a mailing list
>  archive, summarize the relevant points of the discussion.
>  
>  [[commit-reference]]
> -If you want to reference a previous commit in the history of a stable
> -branch, use the format "abbreviated hash (subject, date)", like this:
> +
> +There are a few reasons why you may want to refer to another commit in
> +the "more stable" part of the history (i.e. on branches like `maint`,
> +`master`, and `next`):
> +
> +. A commit that introduced the root cause of a bug you are fixing.
> +
> +. A commit that introduced a feature that is what you are enhancing.
> +
> +. A commit that conflicts with your work when you made a trial merge
> +  of your work into `next` and `seen` for testing.
> +
> +When you reference a commit on a more stable branch (like `master`,
> +`maint` and `next`), use the format "abbreviated hash (subject,
> +date)", like this:

This seems like a good clarification, but partially unrelated to the
$subject, i.e. just the last bullet point is directly relevant, the
first two are new general advice. Perhaps split those out into another
commit?

Re: SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Maybe it would be useful to have a CI target that merged to next/master
> and reported how that went? It would have to be a soft failure, as we
> might have an easy merge conflict, or someone's pushing a revision to a
> topic that's already in "next" or "seen" (and might conflict). But
> having it as an FYI might be helpful.

Sorry, but it would defeat the whole point of this suggestion.
Letting CI do it to allow contributors not to worry about other
people's work is the *last* thing I want to encourage.  I want
to see people get in the habit of making trial merges and ensuring
their work works in the wider context than just "my patches work in
'master'---it is not my problem if it does not work when patches
other people already wrote are present". 

> This seems like a good clarification, but partially unrelated to the
> $subject, i.e. just the last bullet point is directly relevant, the
> first two are new general advice. Perhaps split those out into another
> commit?

Not really.  The original didn't say in what situations you would
want to refer to other commits.  Clarifying that made a three-bullet
list, and all three are equally relevant.

Thanks.

Re: SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Anyway, I can see why you suggest the "base on master and merge", it has
> its benefits, but it is in apparent conflict with existing advice added
> in 76644e3268b (documentation: add tutorial for first contribution,
> 2019-05-17).

I do not mind if you help the effort by updating that document as
well to match, but I have a feeling that the first contribution,
with a follow-up to fix, is sufficiently different from a more
advanced "I need something more than and not yet in 'master' for my
new topic" use case.

> I.e. the "After Review Approval" section (of all places) discusses what
> to do in that situation. It is narrowly talking about submitting
> something on top of your own topic, but the advice should be the same in
> either case I'd think.

No, the advice should be totally different between these two cases.

The "fix or enhance what you already sent" is a continuation of the
same topic, just you, your reviewers and your maintainer were all
not competent enough to catch the shortcomings before the topic hits
'master', and it does make sense to make correction directly on top.

What I am describing here is "I am doing something new, but uses
what is not yet in 'master'.  The author of that other thing I am
depending on may or may not mean to help this new thing when they
wrote it, but the important point is that this is not a continuation
of their topic. It merely depends on them".

Thanks.

Re: SubmittingPatchs: clarify choice of base and testing

[Fabian Stelzer]

On 23.10.21 22:32, Junio C Hamano wrote:>  * A new feature should be
based on `master` in general. If the new
> -  feature depends on a topic that is in `seen`, but not in `master`,
> -  base your work on the tip of that topic.
> +  feature depends on other topics that are in `next`, but not in
> +  `master`, fork a branch from the tip of `master`, merge these topics
> +  to the branch, and work on that branch.  You can remind yourself of
> +  how you prepared the base with `git log --first-parent master..`
> +  easily by doing so.

Using the topic branches from gitster/git that were merged? Or by
selecting the specific commits from the merge into next?

> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch
>  is trying to achieve. Make sure to review
>  your patch after generating it, to ensure accuracy.  Before
>  sending out, please make sure it cleanly applies to the `master`
> -branch head.  If you are preparing a work based on "next" branch,
> -that is fine, but please mark it as such.
> +branch head.  If you are preparing a work based on selected topics
> +merged to `master`, please mark your patch as such.

I think this meant to say 'merged to "next|maint|seen"'?
Or topics selected for being merged into master?

Thanks,
Fabian

Re: SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> On 23.10.21 22:32, Junio C Hamano wrote:>  * A new feature should be
> based on `master` in general. If the new
>> -  feature depends on a topic that is in `seen`, but not in `master`,
>> -  base your work on the tip of that topic.
>> +  feature depends on other topics that are in `next`, but not in
>> +  `master`, fork a branch from the tip of `master`, merge these topics
>> +  to the branch, and work on that branch.  You can remind yourself of
>> +  how you prepared the base with `git log --first-parent master..`
>> +  easily by doing so.
>
> Using the topic branches from gitster/git that were merged? Or by
> selecting the specific commits from the merge into next?

If I were doing this, I would find the tip(s) of things I would
depend on out of the output from

  $ git log --oneline --first-parent origin/master..origin/next

This lists a series of merge commits and the second parent of each
merge commit is the tip of the topic that was merged to 'next'.

>> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch
>>  is trying to achieve. Make sure to review
>>  your patch after generating it, to ensure accuracy.  Before
>>  sending out, please make sure it cleanly applies to the `master`
>> -branch head.  If you are preparing a work based on "next" branch,
>> -that is fine, but please mark it as such.
>> +branch head.  If you are preparing a work based on selected topics
>> +merged to `master`, please mark your patch as such.
>
> I think this meant to say 'merged to "next|maint|seen"'?
> Or topics selected for being merged into master?

Ah, thanks for catching.  I meant "not merged to 'master'" (will fix
locally).

Depending on stuff that are already in 'master', unless you are
preparing a fix that would also apply to the maintenance track, is
rather easy---you can just build on top of 'master'.

And in general, I do not want to see a new topic based on another
topic that is not yet in 'next'.  If a developer has such a topic,
I'd appreciate if the developer waits and shifts their attention to
help the other topics they are planning to depend on---and one way
to do so is to review these other topics ;-)

Thanks.

Re: SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>> @@ -260,8 +281,8 @@ or include any extra files which do not relate to what your patch
>>>  is trying to achieve. Make sure to review
>>>  your patch after generating it, to ensure accuracy.  Before
>>>  sending out, please make sure it cleanly applies to the `master`
>>> -branch head.  If you are preparing a work based on "next" branch,
>>> -that is fine, but please mark it as such.
>>> +branch head.  If you are preparing a work based on selected topics
>>> +merged to `master`, please mark your patch as such.
>>
>> I think this meant to say 'merged to "next|maint|seen"'?
>> Or topics selected for being merged into master?
>
> Ah, thanks for catching.  I meant "not merged to 'master'" (will fix
> locally).

Sorry, should have re-read it more carefully.  The original text
is saying what I wanted to say, but I misread it ;-)

We earlier in the document said that there are three possible bases;
fixes are usually based on 'maint', new features on 'master', but as
an exception, if your new feature need to depend on something not
yet in 'master', then you start a branch from 'master', merge
selected topics into that branch, and use the resulting branch as
the 'base' of your topic.  We do not want a new topic based on
'next', so preparing such a synthetic base by starting from 'next'
and merging topics that are not yet in 'next' is unwelcome.

Perhaps there needs some rewording to clarify that the sentence is
referring to that case.

Thanks again.

Re: SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps there needs some rewording to clarify that the sentence is
> referring to that case.

So, I came up with this incremental clarification.  I'd wait for
other comments before moving the topic further.

 Documentation/SubmittingPatches | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
index 2de8f80dc5..aea01bf36b 100644
--- c/Documentation/SubmittingPatches
+++ w/Documentation/SubmittingPatches
@@ -280,9 +280,11 @@ Please make sure your patch does not add commented out debugging code,
 or include any extra files which do not relate to what your patch
 is trying to achieve. Make sure to review
 your patch after generating it, to ensure accuracy.  Before
-sending out, please make sure it cleanly applies to the `master`
-branch head.  If you are preparing a work based on selected topics
-merged to `master`, please mark your patch as such.
+sending out, please make sure it cleanly applies to the base you
+have chosen in the "Decide what to base your work on" section,
+and unless it targets the `master` branch (which is the default),
+mark your patches as such.
+
 
 [[send-patches]]
 === Sending your patches.

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Taylor Blau]

On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
> be able to add a "suppressions" file to the $LSAN_OPTIONS.
>
> This allows for marking tests as passing with
> "TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
> general widespread memory leaks, such as from the "git log" family of
> commands. We can now mark the "git -C" tests as passing.
>
> For getting specific tests to pass this is preferable to using
> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
> and being able to atomically mark relevant tests as passing with
> "TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
> [1] for more details.
>
> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
>
> > On Fri, Oct 22 2021, Taylor Blau wrote:
> >
> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >>>
> >>> On Wed, Oct 20 2021, Taylor Blau wrote:
> [...]
> > If you want to pick that approach up and run with it I think it would
> > probably make sense to factor that suggested test_expect_success out
> > into a function in test-lib-functions.sh or whatever, and call it as
> > e.g.:
> >
> >     TEST_PASSES_SANITIZE_LEAK=true
> >      . ./test-lib.sh
> >     declare_known_leaks <<-\EOF
> >     add_rev_cmdline
> >     [...]
> >     EOF
> >
> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
> > you.
> >
> > Doing it that way would be less boilerplate for each test that wants it,
> > and is also more likely to work with other non-LSAN leak appoaches,
> > i.e. as long as something can take a list of lines matching stack traces
> > we can feed that to that leak checker's idea of a whitelist.
>
> I just went ahead and hacked that up. If you're OK with that approach
> it would really help reduce the work for leak changes I've got
> planned, and as noted gives you the end-state of a passing 5319.
>
> I don't know if it makes more sense for you to base on top of this
> if/when Junio picks it up, or to integrate it into your series
> etc. Maybe Junio will chime in ...

Hmm. This seems neat, but I haven't been able to get it to work without
encountering a rather annoying bug along the way.

Here is the preamble of my modified t5319 to include all of the leaks I
found in the previous round (but decided not to fix):

TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
todo_leaks <<-\EOF
^add_rev_cmdline$
^cmd_log_init_finish$
^prepare_revision_walk$
^prep_parse_options$
EOF

So running that when git is compiled with SANITIZE=leak *should* work,
but instead produces this rather annoying leak detection after t5319.7:

  =================================================================
  ==1785298==ERROR: LeakSanitizer: detected memory leaks

  Indirect leak of 41 byte(s) in 3 object(s) allocated from:
      #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
      #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42

  -----------------------------------------------------
  Suppressions used:
    count      bytes template
        1        576 ^add_rev_cmdline$
  -----------------------------------------------------

  SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
  Aborted

Huh? Looking past __GI___strdup (which I assume is just a
symbolification failure on ASan's part), who calls strdup()? That trace
is annoyingly incomplete, and doesn't really give us anything to go off
of.

It does seem to remind me of this:

  https://github.com/google/sanitizers/issues/148

though that issue goes in so many different directions that I'm not sure
whether it really is the same issue or not. In any case, that leak
*still* shows up even when suppressing xmalloc() and xrealloc(), so I
almost think that it's a leak within ASan itself.

But most interesting is that those calls go away when I stop setting
`suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.

So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.

Thanks,
Taylor

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Jeff King]

On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:

> So this all feels like a bug in ASan to me. I'm curious if it works on
> your system, but in the meantime I think the best path forward is to
> drop the last patch of my original series (the one with the three
> UNLEAK() calls) and to avoid relying on this patch for the time being.

Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
for all the reasons we introduced UNLEAK() in the first place:

  - it keeps the annotations near the code. Yes, that creates conflicts
    when the code is changed (or the leak is actually fixed), but that's
    a feature. It keeps them from going stale.

  - leak-checkers only know where things are allocated, not who is
    supposed to own them. So you're often annotating the wrong thing;
    it's not a strdup() call which is buggy and leaking, but the
    function five layers up the stack which was supposed to take
    ownership and didn't.

And if we avoid any annotation bugs by doing so, that's icing on the
cake. :)

-Peff

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Taylor Blau]

On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:
> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
>
> > So this all feels like a bug in ASan to me. I'm curious if it works on
> > your system, but in the meantime I think the best path forward is to
> > drop the last patch of my original series (the one with the three
> > UNLEAK() calls) and to avoid relying on this patch for the time being.
>
> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
> for all the reasons we introduced UNLEAK() in the first place:
>
>   - it keeps the annotations near the code. Yes, that creates conflicts
>     when the code is changed (or the leak is actually fixed), but that's
>     a feature. It keeps them from going stale.

I agree completely. I noted as much in my message here:

    https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/

but Ævar made it sound like his work would be made much easier without
the conflict. Since I'm not in any kind of rush to make t5319 leak-free,
I figured that queueing the parts of that series that wouldn't conflict
with Ævar's ongoing work would be a net-positive.

>   - leak-checkers only know where things are allocated, not who is
>     supposed to own them. So you're often annotating the wrong thing;
>     it's not a strdup() call which is buggy and leaking, but the
>     function five layers up the stack which was supposed to take
>     ownership and didn't.

TBH, I find this much more compelling. Either way, I don't feel strongly
enough to deviate from v2 much, and I don't want to create more work for
Ævar along the way.

Thanks,
Taylor

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Jeff King]

On Tue, Oct 26, 2021 at 05:30:47PM -0400, Taylor Blau wrote:

> > Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
> > for all the reasons we introduced UNLEAK() in the first place:
> >
> >   - it keeps the annotations near the code. Yes, that creates conflicts
> >     when the code is changed (or the leak is actually fixed), but that's
> >     a feature. It keeps them from going stale.
> 
> I agree completely. I noted as much in my message here:
> 
>     https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/
> 
> but Ævar made it sound like his work would be made much easier without
> the conflict. Since I'm not in any kind of rush to make t5319 leak-free,
> I figured that queueing the parts of that series that wouldn't conflict
> with Ævar's ongoing work would be a net-positive.

Yeah, to be clear, if there's work in progress in an area, then _not_
annotating it (with either method) is perfectly fine with me in the
meantime.

-Peff

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 26 2021, Taylor Blau wrote:

> On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
>> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
>> be able to add a "suppressions" file to the $LSAN_OPTIONS.
>>
>> This allows for marking tests as passing with
>> "TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
>> general widespread memory leaks, such as from the "git log" family of
>> commands. We can now mark the "git -C" tests as passing.
>>
>> For getting specific tests to pass this is preferable to using
>> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
>> and being able to atomically mark relevant tests as passing with
>> "TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
>> [1] for more details.
>>
>> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
>>
>> > On Fri, Oct 22 2021, Taylor Blau wrote:
>> >
>> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >>>
>> >>> On Wed, Oct 20 2021, Taylor Blau wrote:
>> [...]
>> > If you want to pick that approach up and run with it I think it would
>> > probably make sense to factor that suggested test_expect_success out
>> > into a function in test-lib-functions.sh or whatever, and call it as
>> > e.g.:
>> >
>> >     TEST_PASSES_SANITIZE_LEAK=true
>> >      . ./test-lib.sh
>> >     declare_known_leaks <<-\EOF
>> >     add_rev_cmdline
>> >     [...]
>> >     EOF
>> >
>> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
>> > you.
>> >
>> > Doing it that way would be less boilerplate for each test that wants it,
>> > and is also more likely to work with other non-LSAN leak appoaches,
>> > i.e. as long as something can take a list of lines matching stack traces
>> > we can feed that to that leak checker's idea of a whitelist.
>>
>> I just went ahead and hacked that up. If you're OK with that approach
>> it would really help reduce the work for leak changes I've got
>> planned, and as noted gives you the end-state of a passing 5319.
>>
>> I don't know if it makes more sense for you to base on top of this
>> if/when Junio picks it up, or to integrate it into your series
>> etc. Maybe Junio will chime in ...
>
> Hmm. This seems neat, but I haven't been able to get it to work without
> encountering a rather annoying bug along the way.
>
> Here is the preamble of my modified t5319 to include all of the leaks I
> found in the previous round (but decided not to fix):
>
> TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> todo_leaks <<-\EOF
> ^add_rev_cmdline$
> ^cmd_log_init_finish$
> ^prepare_revision_walk$
> ^prep_parse_options$
> EOF
>
> So running that when git is compiled with SANITIZE=leak *should* work,
> but instead produces this rather annoying leak detection after t5319.7:
>
>   =================================================================
>   ==1785298==ERROR: LeakSanitizer: detected memory leaks
>
>   Indirect leak of 41 byte(s) in 3 object(s) allocated from:
>       #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
>       #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42
>
>   -----------------------------------------------------
>   Suppressions used:
>     count      bytes template
>         1        576 ^add_rev_cmdline$
>   -----------------------------------------------------
>
>   SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
>   Aborted
>
> Huh? Looking past __GI___strdup (which I assume is just a
> symbolification failure on ASan's part), who calls strdup()? That trace
> is annoyingly incomplete, and doesn't really give us anything to go off
> of.
>
> It does seem to remind me of this:
>
>   https://github.com/google/sanitizers/issues/148
>
> though that issue goes in so many different directions that I'm not sure
> whether it really is the same issue or not. In any case, that leak
> *still* shows up even when suppressing xmalloc() and xrealloc(), so I
> almost think that it's a leak within ASan itself.
>
> But most interesting is that those calls go away when I stop setting
> `suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.
>
> So this all feels like a bug in ASan to me. I'm curious if it works on
> your system, but in the meantime I think the best path forward is to
> drop the last patch of my original series (the one with the three
> UNLEAK() calls) and to avoid relying on this patch for the time being.

There are similar cases where LSAN doesn't provide as meaningful of a
stacktrace as valgrind, sometimes when tracing leaks I get a relatively
bad stacktrace like that ending in some __GI___*<stdlib-name>
function. I'll usually compile without SANITIZE=leak and just run a
slower:

    valgrind --leak-check=full --show-leak-kinds=all

In this case however it's not a bug or bad leak tracing, but an artifact
of us using these stacktrace exclusions.

If you run this manually you'll see:
    
    $ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all
    cd0747a9352b58d112f0010134351efc7bbad4a6
    [... snipped output ...]
    
    =================================================================
    ==58023==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 576 byte(s) in 1 object(s) allocated from:
        #0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82
        #1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126
        #2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482
        #3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534
        #4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483
        #5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418
        #6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498
        #7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504
        #8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578
        #9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597
        #10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738
        #11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550
        #12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461
        #13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713
        #14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780
        #15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911
        #16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52
        #17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308
    
    Indirect leak of 41 byte(s) in 3 object(s) allocated from:
        #0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s).

Which is why I put the "strdup" there, but forgot to tell you when I
submitted the real PATCH version that that one shouldn't have the ^$
anchoring.

FWIW this will work on top of your series:
    
    diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
    index a3c72b68f7c..891de720b2c 100755
    --- a/t/t5319-multi-pack-index.sh
    +++ b/t/t5319-multi-pack-index.sh
    @@ -2,6 +2,17 @@
     
     test_description='multi-pack-indexes'
     . ./test-lib.sh
    +todo_leaks <<-\EOF
    +^add_rev_cmdline$
    +^cmd_log_init_finish$
    +^prep_parse_options$
    +^prepare_revision_walk$
    +^start_progress_delay$
    +
    +# An indirect leak reported because we excluded the leaks above
    +strdup$
    +EOF
    +
     
     GIT_TEST_MULTI_PACK_INDEX=0
     objdir=.git/objects

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 26 2021, Jeff King wrote:

It seems that I've gotten what I wanted from this side-thread, yay!
Thanks both, not to pile on, but just to clarify:

> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
>
>> So this all feels like a bug in ASan to me. I'm curious if it works on
>> your system, but in the meantime I think the best path forward is to
>> drop the last patch of my original series (the one with the three
>> UNLEAK() calls) and to avoid relying on this patch for the time being.
>
> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
> for all the reasons we introduced UNLEAK() in the first place:
>
>   - it keeps the annotations near the code. Yes, that creates conflicts
>     when the code is changed (or the leak is actually fixed), but that's
>     a feature. It keeps them from going stale.

I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:

    struct strbuf buf = xmalloc(...);
    [...]
    UNLEAK(buf);

If I was fixing leaks in that sort of code what I pointed out downthread
in [1] wouldn't apply.

So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
while we have in-flight leak fixes. I.e. I'd run into a textual
conflict, but that would be trivial to resolve, and obvious what the
semantic & textual conflict was.

Rather, it's not marking specific leaks, but UNLEAK() on a container
struct that's problematic.

Depending on how they're used those structs may or may not leak. So
e.g. Taylor's upthread 11/11[2] contained:
    
    diff --git a/builtin/rev-list.c b/builtin/rev-list.c
    index 36cb909eba..df3811e763 100644
    --- a/builtin/rev-list.c
    +++ b/builtin/rev-list.c
    @@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
     
            argc = setup_revisions(argc, argv, &revs, &s_r_opt);
     
    +       UNLEAK(revs);
    +
            memset(&info, 0, sizeof(info));
            info.revs = &revs;
            if (revs.bisect)

Which if you on master replace that with:
    
    diff --git a/builtin/rev-list.c b/builtin/rev-list.c
    index 36cb909ebaa..3cce87e0eb7 100644
    --- a/builtin/rev-list.c
    +++ b/builtin/rev-list.c
    @@ -548,6 +548,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
                    revs.do_not_die_on_missing_tree = 1;
     
            argc = setup_revisions(argc, argv, &revs, &s_r_opt);
    +       BUG("using this?");
     
            memset(&info, 0, sizeof(info));
            info.revs = &revs;

You'll run into a failure with:

    GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0002-gitfile.sh

I.e. our existing leak tests already take that codepath and don't run
into a leak from using "revs".

So we wouldn't just be marking a known specific leak, but hiding all
leaks & non-leaks in container from the top-level, and thus hide
potential regressions, an addition to attaining the end-goal of marking
some specific test as passing.

Now, that specific example isn't very useful right now, since we don't
have a release_revisions() at all, but e.g. the first batch of fixes
I've got for the revisions.[ch] leak fix most common cases (e.g. the
pending array), but not some obscure ones.

Being able to incrementally mark those leaks as fixed on a test-by-test
basis has the advantage over UNLEAK() that we won't have regressions
while we're not at a 100% leak free (at which point we could remove the
UNLEAK()).

>   - leak-checkers only know where things are allocated, not who is
>     supposed to own them. So you're often annotating the wrong thing;
>     it's not a strdup() call which is buggy and leaking, but the
>     function five layers up the stack which was supposed to take
>     ownership and didn't.

As noted in [3] this case is because the LSAN suppressions list was in
play, so we excluded the meaningful part of the stack trace (which is
shown in that mail).

Hrm, now that I think about it I think that the cases where I had to
resort to valgrind to get around such crappy stacktraces (when that was
all I got, even without suppressions) were probably all because there
was an UNLEAK() in play...

1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/f1bb8b73ffdb78995dc5653791f9a64adf216e21.1634787555.git.me@ttaylorr.com/
3. https://lore.kernel.org/git/211027.86zgquyk52.gmgdl@evledraar.gmail.com/

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Jeff King]

On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >   - it keeps the annotations near the code. Yes, that creates conflicts
> >     when the code is changed (or the leak is actually fixed), but that's
> >     a feature. It keeps them from going stale.
> 
> I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
> 
>     struct strbuf buf = xmalloc(...);
>     [...]
>     UNLEAK(buf);
> 
> If I was fixing leaks in that sort of code what I pointed out downthread
> in [1] wouldn't apply.

I'm OK with such a use of UNLEAK(), though of course you could probably
just free the buffer in that instance.

But...

> So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
> while we have in-flight leak fixes. I.e. I'd run into a textual
> conflict, but that would be trivial to resolve, and obvious what the
> semantic & textual conflict was.
> 
> Rather, it's not marking specific leaks, but UNLEAK() on a container
> struct that's problematic.

...that's the whole point of UNLEAK(), IMHO. You know that the container
struct is leaked, but you don't care because the program is about to
exit, or there's no easy way to avoid the leak.

So it's not the "container" element, but rather it can be a problem if
people annotate too broadly (you will miss some leaks). In the case of
rev_info, there is no way to _not_ leak right now, because it has no
cleanup function. In the long run we should probably add one. But in the
meantime, annotating them so we can find the _interesting_ leaks is
worth doing (and that was the whole point of adding UNLEAK in the first
place.

And I think the UNLEAK() calls in Taylor's patch are fine in that sense.
While yes, _some_ runs of those commands will not leak, the point is to
say that when they do leak, they are not interesting. And they are not
interesting because that rev_info is in scope until the program exits,
so anything it points to is only leaked at a moment where we no longer
care.

> So we wouldn't just be marking a known specific leak, but hiding all
> leaks & non-leaks in container from the top-level, and thus hide
> potential regressions, an addition to attaining the end-goal of marking
> some specific test as passing.

I'd argue it _is_ a known specific leak. It is rev_info going out of
scope that causes the leak, not rev_info holding on to memory in things
like its pending array.

Now those can be interesting, too (if it no longer needs the memory, can
it perhaps get rid of it). But IMHO all of that is pretty secondary to
clearing the noise so you can find "true" leaks (ones where some
sub-function really is allocating and then losing the pointer entirely,
especially if it's called an arbitrary number of times).

> >   - leak-checkers only know where things are allocated, not who is
> >     supposed to own them. So you're often annotating the wrong thing;
> >     it's not a strdup() call which is buggy and leaking, but the
> >     function five layers up the stack which was supposed to take
> >     ownership and didn't.
> 
> As noted in [3] this case is because the LSAN suppressions list was in
> play, so we excluded the meaningful part of the stack trace (which is
> shown in that mail).

I don't think that's true at all. The annotations you showed in that
message, for example, are pointing at add_rev_cmdline(). But that is
_not_ the source of the leak, nor where it should be fixed. It is a
necessary part of how rev_info works. The leak is when the rev_info goes
out of scope without anybody cleaning it up.

> Hrm, now that I think about it I think that the cases where I had to
> resort to valgrind to get around such crappy stacktraces (when that was
> all I got, even without suppressions) were probably all because there
> was an UNLEAK() in play...

I don't see how UNLEAK() would impact stack traces. It should either
make something not-leaked-at-all (in which case LSan will no longer
mention it), or it does nothing (it throws some wasted memory into a
structure which is itself not leaked).

-Peff

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think that's true at all. The annotations you showed in that
> message, for example, are pointing at add_rev_cmdline(). But that is
> _not_ the source of the leak, nor where it should be fixed. It is a
> necessary part of how rev_info works. The leak is when the rev_info goes
> out of scope without anybody cleaning it up.

True.

>> Hrm, now that I think about it I think that the cases where I had to
>> resort to valgrind to get around such crappy stacktraces (when that was
>> all I got, even without suppressions) were probably all because there
>> was an UNLEAK() in play...
>
> I don't see how UNLEAK() would impact stack traces. It should either
> make something not-leaked-at-all (in which case LSan will no longer
> mention it), or it does nothing (it throws some wasted memory into a
> structure which is itself not leaked).

I'd prefer to see judicious use of UNLEAK() over LSAN suppressions
that lists things that are not the source of the problem.  The list
being kept in the tests that are far away from the actual source
does not help, either.

Thanks.

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 27 2021, Jeff King wrote:

> On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >   - it keeps the annotations near the code. Yes, that creates conflicts
>> >     when the code is changed (or the leak is actually fixed), but that's
>> >     a feature. It keeps them from going stale.
>> 
>> I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
>> 
>>     struct strbuf buf = xmalloc(...);
>>     [...]
>>     UNLEAK(buf);
>> 
>> If I was fixing leaks in that sort of code what I pointed out downthread
>> in [1] wouldn't apply.
>
> I'm OK with such a use of UNLEAK(), though of course you could probably
> just free the buffer in that instance.
>
> But...
>
>> So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
>> while we have in-flight leak fixes. I.e. I'd run into a textual
>> conflict, but that would be trivial to resolve, and obvious what the
>> semantic & textual conflict was.
>> 
>> Rather, it's not marking specific leaks, but UNLEAK() on a container
>> struct that's problematic.
>
> ...that's the whole point of UNLEAK(), IMHO. You know that the container
> struct is leaked, but you don't care because the program is about to
> exit, or there's no easy way to avoid the leak.
>
> So it's not the "container" element, but rather it can be a problem if
> people annotate too broadly (you will miss some leaks). In the case of
> rev_info, there is no way to _not_ leak right now, because it has no
> cleanup function.

It doesn't have one, but there are uses of setup_revisions() and
rev_info usage that don't leak, as that builtin/rev-list.c case shows.

I mean, in that case it's not doing much of anything, but at least we
test that setup_revisions() itself doesn't leak right now, but wouldn't
with UNLEAK().

> In the long run we should probably add one.

After the current in-flight patches I've got making diff_free() a bit
more useful, and then a revisions_release() that'll handle most cases,
which gets us started on mass-marking the tests as leak free.

So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for
far future xyz", but for a thing I've got queued up that I'd rather not
start rewriting...

> [...]But in the meantime, annotating them so we can find the
> _interesting_ leaks is worth doing (and that was the whole point of
> adding UNLEAK in the first place.
>
> And I think the UNLEAK() calls in Taylor's patch are fine in that sense.
> While yes, _some_ runs of those commands will not leak, the point is to
> say that when they do leak, they are not interesting. And they are not
> interesting because that rev_info is in scope until the program exits,
> so anything it points to is only leaked at a moment where we no longer
> care.

I don't per-se care about leaks at program exit either, but am making
them leak-free as a proxy for making the relevant APIs useful for
persistent use.

So I think the historical approach of just sprinkling UNLEAK() since
we're exiting anyway is taking too narrow of a view.

In that case if we'd like to see if revisions.[ch] is leak-free we'd
need parallel test-helper tests (or whatever) for all its features, or
we can just piggy-back on every "git log" invocation in the test
suite...

>> So we wouldn't just be marking a known specific leak, but hiding all
>> leaks & non-leaks in container from the top-level, and thus hide
>> potential regressions, an addition to attaining the end-goal of marking
>> some specific test as passing.
>
> I'd argue it _is_ a known specific leak. It is rev_info going out of
> scope that causes the leak, not rev_info holding on to memory in things
> like its pending array.
>
> Now those can be interesting, too (if it no longer needs the memory, can
> it perhaps get rid of it). But IMHO all of that is pretty secondary to
> clearing the noise so you can find "true" leaks (ones where some
> sub-function really is allocating and then losing the pointer entirely,
> especially if it's called an arbitrary number of times).

I wasn't trying to get into the philosophical discussion of whether a
struct or a struct it contains can be said to be the "real" source of a
leak :)

Yeah, I agree that from that POV there's no difference.

I'm merely referring to the practical aspect of patches I've got queued
up, which do things like fix all leaks of rev_info.pending, mark tests
that now pass, fix another rev_info.whatever, mark tests etc.

For rev_info in particular it's a bit iffy to UNLEAK() it, since it also
contains pointers to stuff it doesn't "own", sometimes things are
allocated and handed off to it, sometimes for a while, sometimes
permanently.

So by UNLEAK()ing it you're not just unleaking "its" memory, but
potentially hiding leaks in APIs that need to interact with it in that
way. E.g. mailmap.c is one of those cases.

>> >   - leak-checkers only know where things are allocated, not who is
>> >     supposed to own them. So you're often annotating the wrong thing;
>> >     it's not a strdup() call which is buggy and leaking, but the
>> >     function five layers up the stack which was supposed to take
>> >     ownership and didn't.
>> 
>> As noted in [3] this case is because the LSAN suppressions list was in
>> play, so we excluded the meaningful part of the stack trace (which is
>> shown in that mail).
>
> I don't think that's true at all. The annotations you showed in that
> message, for example, are pointing at add_rev_cmdline(). But that is
> _not_ the source of the leak, nor where it should be fixed. It is a
> necessary part of how rev_info works. The leak is when the rev_info goes
> out of scope without anybody cleaning it up.
>
>> Hrm, now that I think about it I think that the cases where I had to
>> resort to valgrind to get around such crappy stacktraces (when that was
>> all I got, even without suppressions) were probably all because there
>> was an UNLEAK() in play...
>
> I don't see how UNLEAK() would impact stack traces. It should either
> make something not-leaked-at-all (in which case LSan will no longer
> mention it), or it does nothing (it throws some wasted memory into a
> structure which is itself not leaked).

Yes, I think either categorically wrong here, or it applies to some
other case I wasn't able to dig up. Or maybe not, doesn't Taylor's
example take it from "Direct leak" to "Indirect leak" with the
suppression in play? I think those were related somehow (but don't have
that in front of me as I type this out).

E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:

    $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
    1466000000 -> +0000
    +0200 -> +0000
    
    =================================================================
    ==335188==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

Now try with "SANITIZE=" and valgrind can give you the "real" source:

    $ TZ=UTC valgrind --leak-check=full t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515== Memcheck, a memory error detector
    ==336515== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
    ==336515== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
    ==336515== Command: t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515==
    1466000000 -> +0000
    +0200 -> +0000
    ==336515==
    ==336515== HEAP SUMMARY:
    ==336515==     in use at exit: 1,208 bytes in 13 blocks
    ==336515==   total heap usage: 50 allocs, 37 frees, 27,593 bytes allocated
    ==336515==
    ==336515== 3 bytes in 1 blocks are definitely lost in loss record 1 of 13
    ==336515==    at 0x483877F: malloc (vg_replace_malloc.c:307)
    ==336515==    by 0x49C6E4A: strdup (strdup.c:42)
    ==336515==    by 0x261E3A: xstrdup (wrapper.c:29)
    ==336515==    by 0x152785: parse_date_format (date.c:991)
    ==336515==    by 0x116A34: show_dates (test-date.c:39)
    ==336515==    by 0x116DE9: cmd__date (test-date.c:116)
    ==336515==    by 0x115227: cmd_main (test-tool.c:127)
    ==336515==    by 0x115334: main (common-main.c:52)
    ==336515==
    ==336515== LEAK SUMMARY:
    ==336515==    definitely lost: 3 bytes in 1 blocks
    ==336515==    indirectly lost: 0 bytes in 0 blocks
    ==336515==      possibly lost: 0 bytes in 0 blocks
    ==336515==    still reachable: 1,205 bytes in 12 blocks
    ==336515==         suppressed: 0 bytes in 0 blocks
    ==336515== Reachable blocks (those to which a pointer was found) are not shown.
    ==336515== To see them, rerun with: --leak-check=full --show-leak-kinds=all
    ==336515==
    ==336515== For lists of detected and suppressed errors, rerun with: -s
    ==336515== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I don't know why it habitually loses track of seemingly easy-to-track
leaks sometimes, but it does. There isn't an UNLEAK() in play here.

When I run into these I try it with valgrind, fix the leak, then test
again with SANITIZE=leak. I haven't found cases where it actually misses
things, just cases where the stack traces suck.

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Jeff King]

On Wed, Oct 27, 2021 at 10:57:52PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > So it's not the "container" element, but rather it can be a problem if
> > people annotate too broadly (you will miss some leaks). In the case of
> > rev_info, there is no way to _not_ leak right now, because it has no
> > cleanup function.
> 
> It doesn't have one, but there are uses of setup_revisions() and
> rev_info usage that don't leak, as that builtin/rev-list.c case shows.
> 
> I mean, in that case it's not doing much of anything, but at least we
> test that setup_revisions() itself doesn't leak right now, but wouldn't
> with UNLEAK().

I don't think that's true. If you UNLEAK() the rev_info in the caller,
then it will only affect allocations that are still reachable from
rev_info. I.e., things that are by definition not a leak in
setup_revisions().

Now you could argue that setup_revisions() is "leaking" by allocating
things and stuffing them into rev_info that it should not be. But we can
never know that until we have an actual function that cleans up a
rev_info, which defines what it's "supposed" to have ownership of.

Maybe we have callers that explicitly try to de-allocate bits of the
rev_info. But IMHO that is the source of the whole problem: how is
random code using rev_info supposed to know which of its internal
details are owned or not? This should be documented and enforced with a
single function.

> So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for
> far future xyz", but for a thing I've got queued up that I'd rather not
> start rewriting...

Just to be clear: I am totally fine with dropping Taylor's UNLEAK
patches (as I've said already). I was only arguing here that annotating
via external files is worse than just adding an UNLEAK().

I'm also trying to combat what I see as mis-conceptions or inaccuracies
about what UNLEAK() does or its implications (or even what counts as a
"leak"). But I hope in the long run that we don't need _any_ kind of
annotation, because we'll actually be leak-free. And then we don't have
to care about any of this.

> > I don't see how UNLEAK() would impact stack traces. It should either
> > make something not-leaked-at-all (in which case LSan will no longer
> > mention it), or it does nothing (it throws some wasted memory into a
> > structure which is itself not leaked).
> 
> Yes, I think either categorically wrong here, or it applies to some
> other case I wasn't able to dig up. Or maybe not, doesn't Taylor's
> example take it from "Direct leak" to "Indirect leak" with the
> suppression in play? I think those were related somehow (but don't have
> that in front of me as I type this out).

I don't think UNLEAK() can move something from "direct" to "indirect" in
LSan's terminology. If rev_info points to an array of structs, and those
structs point to allocated strings, then the array itself is a "direct"
leak, and the strings are "indirect" (they are leaked, but presumably
fixing the direct leak would also deallocate them).

If UNLEAK() makes the array not-leaked, then those indirect leaks don't
become direct. They should be transitively not-leaked, too.

> E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:
> 
>     $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
>     1466000000 -> +0000
>     +0200 -> +0000
>     
>     =================================================================
>     ==335188==ERROR: LeakSanitizer: detected memory leaks
>     
>     Direct leak of 3 byte(s) in 1 object(s) allocated from:
>         #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
>         #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
>     
>     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

So these should be real leaks. Of course with the lousy stack trace it's
hard to see what they are. But I don't see how UNLEAK() is responsible
for making the lousy stack trace. You could try compiling with LSan but
_not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad
(but I expect it to be, since test-date.c does not have any UNLEAK()
calls in it).

-Peff

Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

[Jeff King]

On Fri, Oct 29, 2021 at 04:56:31PM -0400, Jeff King wrote:

> > E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:
> > 
> >     $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
> >     1466000000 -> +0000
> >     +0200 -> +0000
> >     
> >     =================================================================
> >     ==335188==ERROR: LeakSanitizer: detected memory leaks
> >     
> >     Direct leak of 3 byte(s) in 1 object(s) allocated from:
> >         #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
> >         #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
> >     
> >     SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
> 
> So these should be real leaks. Of course with the lousy stack trace it's
> hard to see what they are. But I don't see how UNLEAK() is responsible
> for making the lousy stack trace. You could try compiling with LSan but
> _not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad
> (but I expect it to be, since test-date.c does not have any UNLEAK()
> calls in it).

I just tested this, and it is unrelated to UNLEAK(). Interestingly,
compiling with SANITIZE=address does give the correct stack trace, so I
think LSan is just buggy here for some reason.

We usually suppress leaks with ASAN_OPTIONS because there are so many
(and we want to get at the more important signal of actual memory
errors). But it wouldn't be hard to have that setting respect your
PASSING_SANITIZE_LEAK variables. The big downside is that ASan runs of
the test suite are much more expensive.

Hmm. A little googling turns up LSan's fast_unwind_on_malloc option. And
indeed:

  $ LSAN_OPTIONS=fast_unwind_on_malloc=0 t/helper/test-tool date show:format:%z 1466000000 +0200
  1466000000 -> +0000
  +0200 -> +0000
  
  =================================================================
  ==39628==ERROR: LeakSanitizer: detected memory leaks
  
  Direct leak of 3 byte(s) in 1 object(s) allocated from:
      #0 0x7fa664e39b94 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:56
      #1 0x7fa664c124aa in __GI___strdup string/strdup.c:42
      #2 0x557636c40c2e in xstrdup /home/peff/compile/git/wrapper.c:29
      #3 0x557636b2ae52 in parse_date_format /home/peff/compile/git/date.c:991
      #4 0x557636aedb0e in show_dates t/helper/test-date.c:39
      #5 0x557636aedee1 in cmd__date t/helper/test-date.c:116
      #6 0x557636aec1f0 in cmd_main t/helper/test-tool.c:127
      #7 0x557636aec30d in main /home/peff/compile/git/common-main.c:52
      #8 0x7fa664babe49 in __libc_start_main ../csu/libc-start.c:314
      #9 0x557636aebec9 in _start (/home/peff/compile/git/t/helper/test-tool+0xcec9)
  
  SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

So perhaps we ought to be setting that by default.

-Peff

[PATCH v2] SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

We encourage identifying what, among many topics on `next`, exact
topics a new work depends on, instead of building directly on
`next`.  Let's clarify this in the documentation.

Developers should know what they are building on top of, and be
aware of which part of the system is currently being worked on.
Encouraging them to make trial merges to `next` and `seen`
themselves will incentivize them to read others' changes and
understand them, eventually helping the developers to coordinate
among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

 * I've been trying to clear the deck, and noticed that this has
   been untended for quite some time.  With some clarification to
   a place I was even confused myself while responding to Fabian's
   comments in the earlier round.

diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
index e409022d93..3c4c5d9f18 100644
--- c/Documentation/SubmittingPatches
+++ w/Documentation/SubmittingPatches
@@ -19,8 +19,10 @@ change is relevant to.
   base your work on the tip of the topic.
 
 * A new feature should be based on `master` in general. If the new
-  feature depends on a topic that is in `seen`, but not in `master`,
-  base your work on the tip of that topic.
+  feature depends on other topics that are in `next`, but not in
+  `master`, fork a branch from the tip of `master`, merge these topics
+  to the branch, and work on that branch.  You can remind yourself of
+  how you prepared the base with `git log --first-parent master..`.
 
 * Corrections and enhancements to a topic not yet in `master` should
   be based on the tip of that topic. If the topic has not been merged
@@ -28,10 +30,10 @@ change is relevant to.
   into the series.
 
 * In the exceptional case that a new feature depends on several topics
-  not in `master`, start working on `next` or `seen` privately and send
-  out patches for discussion. Before the final merge, you may have to
-  wait until some of the dependent topics graduate to `master`, and
-  rebase your work.
+  not in `master`, start working on `next` or `seen` privately and
+  send out patches only for discussion. Once your new feature starts
+  to stabilize, you would have to rebase it (see the "depends on other
+  topics" above).
 
 * Some parts of the system have dedicated maintainers with their own
   repositories (see the section "Subsystems" below).  Changes to
@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing.  See
 [[tests]]
 When adding a new feature, make sure that you have new tests to show
 the feature triggers the new behavior when it should, and to show the
-feature does not trigger when it shouldn't.  After any code change, make
-sure that the entire test suite passes.
+feature does not trigger when it shouldn't.  After any code change,
+make sure that the entire test suite passes.  When fixing a bug, make
+sure you have new tests that breaks if somebody else breaks what you
+fixed by accident to avoid regression.  Also, try merging your work to
+'next' and 'seen' and make sure the tests still pass; topics by others
+that are still in flight may have unexpected interactions with what
+you are trying to do in your topic.
 
 Pushing to a fork of https://github.com/git/git will use their CI
 integration to test your changes on Linux, Mac and Windows. See the
@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list
 archive, summarize the relevant points of the discussion.
 
 [[commit-reference]]
-If you want to reference a previous commit in the history of a stable
-branch, use the format "abbreviated hash (subject, date)", like this:
+
+There are a few reasons why you may want to refer to another commit in
+the "more stable" part of the history (i.e. on branches like `maint`,
+`master`, and `next`):
+
+. A commit that introduced the root cause of a bug you are fixing.
+
+. A commit that introduced a feature that is what you are enhancing.
+
+. A commit that conflicts with your work when you made a trial merge
+  of your work into `next` and `seen` for testing.
+
+When you reference a commit on a more stable branch (like `master`,
+`maint` and `next`), use the format "abbreviated hash (subject,
+date)", like this:
 
 ....
 	Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,
 or include any extra files which do not relate to what your patch
 is trying to achieve. Make sure to review
 your patch after generating it, to ensure accuracy.  Before
-sending out, please make sure it cleanly applies to the `master`
-branch head.  If you are preparing a work based on "next" branch,
-that is fine, but please mark it as such.
+sending out, please make sure it cleanly applies to the base you
+have chosen in the "Decide what to base your work on" section,
+and unless it targets the `master` branch (which is the default),
+mark your patches as such.
+
 
 [[send-patches]]
 === Sending your patches.
@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.
 Send your patch with "To:" set to the mailing list, with "cc:" listing
 people who are involved in the area you are touching (the `git
 contacts` command in `contrib/contacts/` can help to
-identify them), to solicit comments and reviews.
+identify them), to solicit comments and reviews.  Also, when you made
+trial merges of your topic to `next` and `seen`, you may have noticed
+work by others conflicting with your changes.  There is a good possibility
+that these people may know the area you are touching well.
 
 :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
 :git-ml: footnote:[The mailing list: git@vger.kernel.org]

Re: [PATCH v2] SubmittingPatchs: clarify choice of base and testing

[Elijah Newren]

On Fri, Dec 24, 2021 at 5:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> We encourage identifying what, among many topics on `next`, exact
> topics a new work depends on, instead of building directly on
> `next`.  Let's clarify this in the documentation.
>
> Developers should know what they are building on top of, and be
> aware of which part of the system is currently being worked on.
> Encouraging them to make trial merges to `next` and `seen`
> themselves will incentivize them to read others' changes and
> understand them, eventually helping the developers to coordinate
> among themselves and reviewing each others' changes.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
>  * I've been trying to clear the deck, and noticed that this has
>    been untended for quite some time.  With some clarification to
>    a place I was even confused myself while responding to Fabian's
>    comments in the earlier round.
>
> diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
> index e409022d93..3c4c5d9f18 100644
> --- c/Documentation/SubmittingPatches
> +++ w/Documentation/SubmittingPatches
> @@ -19,8 +19,10 @@ change is relevant to.
>    base your work on the tip of the topic.
>
>  * A new feature should be based on `master` in general. If the new
> -  feature depends on a topic that is in `seen`, but not in `master`,
> -  base your work on the tip of that topic.
> +  feature depends on other topics that are in `next`, but not in
> +  `master`, fork a branch from the tip of `master`, merge these topics
> +  to the branch, and work on that branch.  You can remind yourself of
> +  how you prepared the base with `git log --first-parent master..`.
>
>  * Corrections and enhancements to a topic not yet in `master` should
>    be based on the tip of that topic. If the topic has not been merged
> @@ -28,10 +30,10 @@ change is relevant to.
>    into the series.
>
>  * In the exceptional case that a new feature depends on several topics
> -  not in `master`, start working on `next` or `seen` privately and send
> -  out patches for discussion. Before the final merge, you may have to
> -  wait until some of the dependent topics graduate to `master`, and
> -  rebase your work.
> +  not in `master`, start working on `next` or `seen` privately and
> +  send out patches only for discussion. Once your new feature starts
> +  to stabilize, you would have to rebase it (see the "depends on other
> +  topics" above).
>
>  * Some parts of the system have dedicated maintainers with their own
>    repositories (see the section "Subsystems" below).  Changes to
> @@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing.  See
>  [[tests]]
>  When adding a new feature, make sure that you have new tests to show
>  the feature triggers the new behavior when it should, and to show the
> -feature does not trigger when it shouldn't.  After any code change, make
> -sure that the entire test suite passes.
> +feature does not trigger when it shouldn't.  After any code change,
> +make sure that the entire test suite passes.  When fixing a bug, make
> +sure you have new tests that breaks if somebody else breaks what you
> +fixed by accident to avoid regression.  Also, try merging your work to
> +'next' and 'seen' and make sure the tests still pass; topics by others
> +that are still in flight may have unexpected interactions with what
> +you are trying to do in your topic.
>
>  Pushing to a fork of https://github.com/git/git will use their CI
>  integration to test your changes on Linux, Mac and Windows. See the
> @@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list
>  archive, summarize the relevant points of the discussion.
>
>  [[commit-reference]]
> -If you want to reference a previous commit in the history of a stable
> -branch, use the format "abbreviated hash (subject, date)", like this:
> +
> +There are a few reasons why you may want to refer to another commit in
> +the "more stable" part of the history (i.e. on branches like `maint`,
> +`master`, and `next`):
> +
> +. A commit that introduced the root cause of a bug you are fixing.
> +
> +. A commit that introduced a feature that is what you are enhancing.
> +
> +. A commit that conflicts with your work when you made a trial merge
> +  of your work into `next` and `seen` for testing.
> +
> +When you reference a commit on a more stable branch (like `master`,
> +`maint` and `next`), use the format "abbreviated hash (subject,
> +date)", like this:

I was going to comment that this would be a good place to mention
--pretty=reference, but looking at the file in question, that is
exactly what the text after this already does.

>  ....
>         Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
> @@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,
>  or include any extra files which do not relate to what your patch
>  is trying to achieve. Make sure to review
>  your patch after generating it, to ensure accuracy.  Before
> -sending out, please make sure it cleanly applies to the `master`
> -branch head.  If you are preparing a work based on "next" branch,
> -that is fine, but please mark it as such.
> +sending out, please make sure it cleanly applies to the base you
> +have chosen in the "Decide what to base your work on" section,
> +and unless it targets the `master` branch (which is the default),
> +mark your patches as such.
> +
>
>  [[send-patches]]
>  === Sending your patches.
> @@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.
>  Send your patch with "To:" set to the mailing list, with "cc:" listing
>  people who are involved in the area you are touching (the `git
>  contacts` command in `contrib/contacts/` can help to
> -identify them), to solicit comments and reviews.
> +identify them), to solicit comments and reviews.  Also, when you made
> +trial merges of your topic to `next` and `seen`, you may have noticed
> +work by others conflicting with your changes.  There is a good possibility
> +that these people may know the area you are touching well.
>
>  :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
>  :git-ml: footnote:[The mailing list: git@vger.kernel.org]

This patch looks good to me.

Re: [PATCH v2] SubmittingPatchs: clarify choice of base and testing

[Fabian Stelzer]

On 23.12.2021 15:12, Junio C Hamano wrote:

Just some minor pointers/typos. Looks good.

>We encourage identifying what, among many topics on `next`, exact
>topics a new work depends on, instead of building directly on
>`next`.  Let's clarify this in the documentation.
>
>Developers should know what they are building on top of, and be
>aware of which part of the system is currently being worked on.
>Encouraging them to make trial merges to `next` and `seen`
>themselves will incentivize them to read others' changes and
>understand them, eventually helping the developers to coordinate
>among themselves and reviewing each others' changes.
>
>Signed-off-by: Junio C Hamano <gitster@pobox.com>
>---
> Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 39 insertions(+), 14 deletions(-)
>
> * I've been trying to clear the deck, and noticed that this has
>   been untended for quite some time.  With some clarification to
>   a place I was even confused myself while responding to Fabian's
>   comments in the earlier round.
>
>diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
>index e409022d93..3c4c5d9f18 100644
>--- c/Documentation/SubmittingPatches
>+++ w/Documentation/SubmittingPatches
>@@ -19,8 +19,10 @@ change is relevant to.
>   base your work on the tip of the topic.
>
> * A new feature should be based on `master` in general. If the new
>-  feature depends on a topic that is in `seen`, but not in `master`,
>-  base your work on the tip of that topic.
>+  feature depends on other topics that are in `next`, but not in
>+  `master`, fork a branch from the tip of `master`, merge these topics
>+  to the branch, and work on that branch.  You can remind yourself of
>+  how you prepared the base with `git log --first-parent master..`.
>
> * Corrections and enhancements to a topic not yet in `master` should
>   be based on the tip of that topic. If the topic has not been merged
>@@ -28,10 +30,10 @@ change is relevant to.
>   into the series.
>
> * In the exceptional case that a new feature depends on several topics
>-  not in `master`, start working on `next` or `seen` privately and send
>-  out patches for discussion. Before the final merge, you may have to
>-  wait until some of the dependent topics graduate to `master`, and
>-  rebase your work.
>+  not in `master`, start working on `next` or `seen` privately and
>+  send out patches only for discussion. Once your new feature starts
>+  to stabilize, you would have to rebase it (see the "depends on other
>+  topics" above).
>
> * Some parts of the system have dedicated maintainers with their own
>   repositories (see the section "Subsystems" below).  Changes to
>@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing.  See
> [[tests]]
> When adding a new feature, make sure that you have new tests to show
> the feature triggers the new behavior when it should, and to show the
>-feature does not trigger when it shouldn't.  After any code change, make
>-sure that the entire test suite passes.
>+feature does not trigger when it shouldn't.  After any code change,
>+make sure that the entire test suite passes.  When fixing a bug, make
>+sure you have new tests that breaks if somebody else breaks what you

s/breaks/break

>+fixed by accident to avoid regression.  Also, try merging your work to
>+'next' and 'seen' and make sure the tests still pass; topics by others
>+that are still in flight may have unexpected interactions with what
>+you are trying to do in your topic.
>
> Pushing to a fork of https://github.com/git/git will use their CI
> integration to test your changes on Linux, Mac and Windows. See the
>@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list
> archive, summarize the relevant points of the discussion.
>
> [[commit-reference]]
>-If you want to reference a previous commit in the history of a stable
>-branch, use the format "abbreviated hash (subject, date)", like this:
>+
>+There are a few reasons why you may want to refer to another commit in
>+the "more stable" part of the history (i.e. on branches like `maint`,
>+`master`, and `next`):
>+
>+. A commit that introduced the root cause of a bug you are fixing.
>+
>+. A commit that introduced a feature that is what you are enhancing.

I'm not a native speaker, but `that is what` sounds wrong to me.
`feature which you are enhancing` or `feature that you are enhancing` maybe?

>+
>+. A commit that conflicts with your work when you made a trial merge
>+  of your work into `next` and `seen` for testing.
>+
>+When you reference a commit on a more stable branch (like `master`,
>+`maint` and `next`), use the format "abbreviated hash (subject,
>+date)", like this:
>
> ....
> 	Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
>@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,
> or include any extra files which do not relate to what your patch
> is trying to achieve. Make sure to review
> your patch after generating it, to ensure accuracy.  Before
>-sending out, please make sure it cleanly applies to the `master`
>-branch head.  If you are preparing a work based on "next" branch,
>-that is fine, but please mark it as such.
>+sending out, please make sure it cleanly applies to the base you
>+have chosen in the "Decide what to base your work on" section,
>+and unless it targets the `master` branch (which is the default),
>+mark your patches as such.
>+

Maybe add a hint to `--base=` for format-patch?

>
> [[send-patches]]
> === Sending your patches.
>@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.
> Send your patch with "To:" set to the mailing list, with "cc:" listing
> people who are involved in the area you are touching (the `git
> contacts` command in `contrib/contacts/` can help to
>-identify them), to solicit comments and reviews.
>+identify them), to solicit comments and reviews.  Also, when you made
>+trial merges of your topic to `next` and `seen`, you may have noticed
>+work by others conflicting with your changes.  There is a good possibility
>+that these people may know the area you are touching well.
>
> :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
> :git-ml: footnote:[The mailing list: git@vger.kernel.org]

Re* [PATCH v2] SubmittingPatchs: clarify choice of base and testing

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> On 23.12.2021 15:12, Junio C Hamano wrote:
> ...
>>+feature does not trigger when it shouldn't.  After any code change,
>>+make sure that the entire test suite passes.  When fixing a bug, make
>>+sure you have new tests that breaks if somebody else breaks what you
>
> s/breaks/break

Correct---we are adding multiple tests here.

>>+. A commit that introduced the root cause of a bug you are fixing.
>>+
>>+. A commit that introduced a feature that is what you are enhancing.
>
> I'm not a native speaker, but `that is what` sounds wrong to me.
> `feature which you are enhancing` or `feature that you are enhancing` maybe?

I am not either, but it indeed does sound strange.  "A commit that
introduced a feature that you are enhancing" sounds good.

>>@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,
>> or include any extra files which do not relate to what your patch
>> is trying to achieve. Make sure to review
>> your patch after generating it, to ensure accuracy.  Before
>>-sending out, please make sure it cleanly applies to the `master`
>>-branch head.  If you are preparing a work based on "next" branch,
>>-that is fine, but please mark it as such.
>>+sending out, please make sure it cleanly applies to the base you
>>+have chosen in the "Decide what to base your work on" section,
>>+and unless it targets the `master` branch (which is the default),
>>+mark your patches as such.
>>+
>
> Maybe add a hint to `--base=` for format-patch?

I do not think it belongs to this step.  There is a separate section
about sending patches that mentions the use of format-patch, where it
might be more appropriate.

Thanks.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH v3] SubmittingPatchs: clarify choice of base and testing

We encourage identifying what, among many topics on `next`, exact
topics a new work depends on, instead of building directly on
`next`.  Let's clarify this in the documentation.

Developers should know what they are building on top of, and be
aware of which part of the system is currently being worked on.
Encouraging them to make trial merges to `next` and `seen`
themselves will incentivize them to read others' changes and
understand them, eventually helping the developers to coordinate
among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1:  2a277ef302 ! 1:  f387f92719 SubmittingPatchs: clarify choice of base and testing
    @@ Documentation/SubmittingPatches: Make sure that you have tests for the bug you a
     -sure that the entire test suite passes.
     +feature does not trigger when it shouldn't.  After any code change,
     +make sure that the entire test suite passes.  When fixing a bug, make
    -+sure you have new tests that breaks if somebody else breaks what you
    ++sure you have new tests that break if somebody else breaks what you
     +fixed by accident to avoid regression.  Also, try merging your work to
     +'next' and 'seen' and make sure the tests still pass; topics by others
     +that are still in flight may have unexpected interactions with what
    @@ Documentation/SubmittingPatches: without external resources. Instead of giving a
     +
     +. A commit that introduced the root cause of a bug you are fixing.
     +
    -+. A commit that introduced a feature that is what you are enhancing.
    ++. A commit that introduced a feature that you are enhancing.
     +
     +. A commit that conflicts with your work when you made a trial merge
     +  of your work into `next` and `seen` for testing.

 Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index e409022d93..ad3041da1a 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -19,8 +19,10 @@ change is relevant to.
   base your work on the tip of the topic.
 
 * A new feature should be based on `master` in general. If the new
-  feature depends on a topic that is in `seen`, but not in `master`,
-  base your work on the tip of that topic.
+  feature depends on other topics that are in `next`, but not in
+  `master`, fork a branch from the tip of `master`, merge these topics
+  to the branch, and work on that branch.  You can remind yourself of
+  how you prepared the base with `git log --first-parent master..`.
 
 * Corrections and enhancements to a topic not yet in `master` should
   be based on the tip of that topic. If the topic has not been merged
@@ -28,10 +30,10 @@ change is relevant to.
   into the series.
 
 * In the exceptional case that a new feature depends on several topics
-  not in `master`, start working on `next` or `seen` privately and send
-  out patches for discussion. Before the final merge, you may have to
-  wait until some of the dependent topics graduate to `master`, and
-  rebase your work.
+  not in `master`, start working on `next` or `seen` privately and
+  send out patches only for discussion. Once your new feature starts
+  to stabilize, you would have to rebase it (see the "depends on other
+  topics" above).
 
 * Some parts of the system have dedicated maintainers with their own
   repositories (see the section "Subsystems" below).  Changes to
@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing.  See
 [[tests]]
 When adding a new feature, make sure that you have new tests to show
 the feature triggers the new behavior when it should, and to show the
-feature does not trigger when it shouldn't.  After any code change, make
-sure that the entire test suite passes.
+feature does not trigger when it shouldn't.  After any code change,
+make sure that the entire test suite passes.  When fixing a bug, make
+sure you have new tests that break if somebody else breaks what you
+fixed by accident to avoid regression.  Also, try merging your work to
+'next' and 'seen' and make sure the tests still pass; topics by others
+that are still in flight may have unexpected interactions with what
+you are trying to do in your topic.
 
 Pushing to a fork of https://github.com/git/git will use their CI
 integration to test your changes on Linux, Mac and Windows. See the
@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list
 archive, summarize the relevant points of the discussion.
 
 [[commit-reference]]
-If you want to reference a previous commit in the history of a stable
-branch, use the format "abbreviated hash (subject, date)", like this:
+
+There are a few reasons why you may want to refer to another commit in
+the "more stable" part of the history (i.e. on branches like `maint`,
+`master`, and `next`):
+
+. A commit that introduced the root cause of a bug you are fixing.
+
+. A commit that introduced a feature that you are enhancing.
+
+. A commit that conflicts with your work when you made a trial merge
+  of your work into `next` and `seen` for testing.
+
+When you reference a commit on a more stable branch (like `master`,
+`maint` and `next`), use the format "abbreviated hash (subject,
+date)", like this:
 
 ....
 	Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code,
 or include any extra files which do not relate to what your patch
 is trying to achieve. Make sure to review
 your patch after generating it, to ensure accuracy.  Before
-sending out, please make sure it cleanly applies to the `master`
-branch head.  If you are preparing a work based on "next" branch,
-that is fine, but please mark it as such.
+sending out, please make sure it cleanly applies to the base you
+have chosen in the "Decide what to base your work on" section,
+and unless it targets the `master` branch (which is the default),
+mark your patches as such.
+
 
 [[send-patches]]
 === Sending your patches.
@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.
 Send your patch with "To:" set to the mailing list, with "cc:" listing
 people who are involved in the area you are touching (the `git
 contacts` command in `contrib/contacts/` can help to
-identify them), to solicit comments and reviews.
+identify them), to solicit comments and reviews.  Also, when you made
+trial merges of your topic to `next` and `seen`, you may have noticed
+work by others conflicting with your changes.  There is a good possibility
+that these people may know the area you are touching well.
 
 :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
 :git-ml: footnote:[The mailing list: git@vger.kernel.org]
-- 
2.34.1-616-gbc337a5f45