| Message ID | 20220403182250.904933-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Importing and exporting stashes to refs | expand |
"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
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. Thanks.
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/
On 2022-04-04 at 06:20:29, Ævar Arnfjörð Bjarmason wrote: > 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(). Your resolutions seem reasonable. And yes, I always conflate strbuf_reset and strbuf_release. I'll put out a v4 with changes to my series and responses to some other feedback I've received.