Re: [PATCH v3 0/4] Importing and exporting stashes to refs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Apr 03 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>>> Changes from v2:
>>> * Fix uninitialized strbuf.
>>> * Avoid C99-style initializations.
>>
>> Thanks.
>>
>> [1] is a CI run of 'seen' without this topic, while [2] is the same
>> but with this topic.
>>
>> t3903.115-117 (stash export) are not very happy in the latter.
>>
>> e.g. https://github.com/git/git/runs/5808828105?check_suite_focus=true#step:7:6623
>>
>> [References]
>>
>> *1* https://github.com/git/git/actions/runs/2086776970 
>> *2* https://github.com/git/git/actions/runs/2086887176 
>
> I think this is coming from a mismerge, conflicting topic being
> Ævar's revisions leakage series.  I'll push out an updated
> resolution later today.

It looks like it, I looked at your resolution. It seems that since the
failure you pushed out this on top:
	
	diff --git a/builtin/stash.c b/builtin/stash.c
	index 35d7c2e828b..b4da17265a1 100644
	--- a/builtin/stash.c
	+++ b/builtin/stash.c
	@@ -150,6 +150,7 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
	 
	 static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	 {
	+	strbuf_init(revision, 0);
	 	if (!commit) {
	 		if (!ref_exists(ref_stash)) {
	 			fprintf_ln(stderr, _("No stash entries found."));
	@@ -192,8 +193,10 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	if (parse_revision(&info->revision, commit, 0))
	+	if (parse_revision(&info->revision, commit, 0)) {
	+		free_stash_info(info);
	 		return -1;
	+	}
	 
	 	revision = info->revision.buf;

Which per my [1] is odd. I.e. in my topic I'd gotten rid of that
strbuf_init() entirely, see [2].

But looking again the odd interaction with brian's topic is that in his
[2] and [3] he refactored the parse_revision() into existance.

In the version he based things on we always init'd the strbuf there, but
between [2] and [3] that "strbuf_init()" was tasked with the work of a
strbuf_reset().

And on "seen" this (new code) still leaks:
	
	$ ~/g/git/git stash export --print
	6b0514ae1f670e55f450518c4b4421a997c8d082
	
	=================================================================
	==25882==ERROR: LeakSanitizer: detected memory leaks
	
	Direct leak of 910 byte(s) in 14 object(s) allocated from:
	    #0 0x45658d in __interceptor_realloc (/home/avar/g/git/git+0x45658d)
	    #1 0x763f6d in xrealloc /home/avar/g/git/wrapper.c:136:8
	    #2 0x71c031 in strbuf_grow /home/avar/g/git/strbuf.c:99:2
	    #3 0x71d224 in strbuf_vaddf /home/avar/g/git/strbuf.c:395:3
	    #4 0x71d1e1 in strbuf_addf /home/avar/g/git/strbuf.c:336:2
	    #5 0x546f3e in parse_revision /home/avar/g/git/builtin/stash.c:162:3
	    #6 0x547f99 in do_export_stash /home/avar/g/git/builtin/stash.c:1987:8
	    #7 0x543b2b in export_stash /home/avar/g/git/builtin/stash.c:2061:8
	    #8 0x541846 in cmd_stash /home/avar/g/git/builtin/stash.c:2109:12
	    #9 0x45a38a in run_builtin /home/avar/g/git/git.c:466:11
	    #10 0x458e21 in handle_builtin /home/avar/g/git/git.c:720:3
	    #11 0x459d65 in run_argv /home/avar/g/git/git.c:787:4
	    #12 0x458bda in cmd_main /home/avar/g/git/git.c:920:19
	    #13 0x567839 in main /home/avar/g/git/common-main.c:56:11
	    #14 0x7fb9a380f7ec in __libc_start_main csu/../csu/libc-start.c:332:16
	    #15 0x431119 in _start (/home/avar/g/git/git+0x431119)

We just don't test t3903 under linux-leaks yet.

Anyway. I belive that the correct resolution is (but see caveats below):
	
	diff --git a/builtin/stash.c b/builtin/stash.c
	index b4da17265a1..3b9a6c5b753 100644
	--- a/builtin/stash.c
	+++ b/builtin/stash.c
	@@ -150,7 +150,6 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
	 
	 static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	 {
	-	strbuf_init(revision, 0);
	 	if (!commit) {
	 		if (!ref_exists(ref_stash)) {
	 			fprintf_ln(stderr, _("No stash entries found."));
	@@ -193,10 +192,8 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	if (parse_revision(&info->revision, commit, 0)) {
	-		free_stash_info(info);
	+	if (parse_revision(&info->revision, commit, 0))
	 		return -1;
	-	}
	 
	 	revision = info->revision.buf;
	 
	@@ -1827,7 +1824,7 @@ static int write_commit_with_parents(struct object_id *out, const struct object_
	 		goto out;
	 	}
	 out:
	-	strbuf_reset(&msg);
	+	strbuf_release(&msg);
	 	unuse_commit_buffer(this, buffer);
	 	free(author);
	 	free(committer);
	@@ -1965,6 +1962,7 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 		 */
	 		for (i = 0; i < argc; i++) {
	 			ALLOC_GROW_BY(items, nitems, 1, nalloc);
	+			strbuf_reset(&revision);
	 			if (parse_revision(&revision, argv[i], 0) ||
	 			    get_oid_with_context(the_repository, revision.buf,
	 						 GET_OID_QUIETLY | GET_OID_GENTLY,
	@@ -1983,6 +1981,8 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 			char buf[32];
	 			struct object_id oid;
	 
	+			strbuf_reset(&revision);
	+
	 			snprintf(buf, sizeof(buf), "%d", i);
	 			if (parse_revision(&revision, buf, 1) ||
	 			    get_oid_with_context(the_repository, revision.buf,
	@@ -2018,6 +2018,7 @@ static int do_export_stash(const char *ref, size_t argc, const char **argv)
	 		puts(oid_to_hex(&prev->object.oid));
	 out:
	 	free(items);
	+	strbuf_release(&revision);
	 
	 	return res;
	 }

I.e. most of that is really feedback/a bug report on [3] and [4],and the
third hunk here is really an unrelated fix where [4] conflated
strbuf_reset() with strbuf_release().

1. https://lore.kernel.org/git/cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v5-10.27-4c5404912e9-20220402T102002Z-avarab@gmail.com/
2. https://lore.kernel.org/git/20220403182250.904933-3-sandals@crustytoothpaste.net/
4. https://lore.kernel.org/git/20220403182250.904933-4-sandals@crustytoothpaste.net/