Re: [PATCH v3 0/4] Importing and exporting stashes to refs
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 4, 2022, 06:20 UTC
- Message-ID
- <220404.86tub9jql5.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqpmlxsnc5.fsf@gitster.g>
On Sun, Apr 03 2022, Junio C Hamano wrote:
Show 25 quoted lines
> 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/