{"thread":{"id":"62119","subject":"[PATCH 0/6] apply: fix leaking buffer of `struct image`","startedAt":"2024-09-16T07:10:02Z","lastAt":"2024-09-17T20:57:18Z","messageCount":20,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"502824","messageId":"cover.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":null,"subject":"[PATCH 0/6] apply: fix leaking buffer of `struct image`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:09:58Z","receivedAt":"2024-09-16T07:10:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series addresses memory leaks in \"apply.c\" revolving around\n`struct image`. I've split the series out of the usual leak fix series\nbecause I found the code really hard to reason about. So instead of just\ntrying to plug the leak with a least-effort patch I refactored it a bit\nto make it more straight-forward. Most importantly, the last patch\nconverts the code to use a proper `struct strbuf` to modify the pre- and\npostimages, which is both easier and allows for proper lifecycle\nmanagement of its buffer.\n\nThis series builds on top of ed155187b4 (Sync with Git 2.46.1,\n2024-09-13).\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (6):\n apply: reorder functions to move image-related things together\n apply: rename functions operating on `struct image`\n apply: introduce macro and function to init images\n apply: refactor code to drop `line_allocated`\n apply: rename members that track line count and allocation length\n apply: refactor `struct image` to use a `struct strbuf`\n\n apply.c | 447 +++++++++++++----------------\n t/t3436-rebase-more-options.sh | 1 +\n t/t4107-apply-ignore-whitespace.sh | 4 +-\n t/t4124-apply-ws-rule.sh | 1 +\n t/t4125-apply-ws-fuzz.sh | 1 +\n t/t4138-apply-ws-expansion.sh | 1 +\n 6 files changed, 204 insertions(+), 251 deletions(-)\n\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502825","messageId":"7b6903ecdd471b9bc0dbf2d64847d4db492b4f25.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 1/6] apply: reorder functions to move image-related things together","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:01Z","receivedAt":"2024-09-16T07:10:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While most of the functions relating to `struct image` are relatively\nclose to one another, `fuzzy_matchlines()` sits in between those even\nthough it is rather unrelated.\n\nReorder functions such that `struct image`-related functions are next to\neach other. While at it, move `clear_image()` to the top such that it is\nclose to the struct definition itself. This makes this lifecycle-related\nthing easy to discover.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 106 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 53 insertions(+), 53 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 6e1060a952..9dd2f4d215 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -285,6 +285,13 @@ struct image {\n \tstruct line *line;\n };\n \n+static void clear_image(struct image *image)\n+{\n+\tfree(image->buf);\n+\tfree(image->line_allocated);\n+\tmemset(image, 0, sizeof(*image));\n+}\n+\n static uint32_t hash_line(const char *cp, size_t len)\n {\n \tsize_t i;\n@@ -297,42 +304,6 @@ static uint32_t hash_line(const char *cp, size_t len)\n \treturn h;\n }\n \n-/*\n- * Compare lines s1 of length n1 and s2 of length n2, ignoring\n- * whitespace difference. Returns 1 if they match, 0 otherwise\n- */\n-static int fuzzy_matchlines(const char *s1, size_t n1,\n-\t\t\t const char *s2, size_t n2)\n-{\n-\tconst char *end1 = s1 + n1;\n-\tconst char *end2 = s2 + n2;\n-\n-\t/* ignore line endings */\n-\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n-\t\tend1--;\n-\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n-\t\tend2--;\n-\n-\twhile (s1 < end1 && s2 < end2) {\n-\t\tif (isspace(*s1)) {\n-\t\t\t/*\n-\t\t\t * Skip whitespace. We check on both buffers\n-\t\t\t * because we don't want \"a b\" to match \"ab\".\n-\t\t\t */\n-\t\t\tif (!isspace(*s2))\n-\t\t\t\treturn 0;\n-\t\t\twhile (s1 < end1 && isspace(*s1))\n-\t\t\t\ts1++;\n-\t\t\twhile (s2 < end2 && isspace(*s2))\n-\t\t\t\ts2++;\n-\t\t} else if (*s1++ != *s2++)\n-\t\t\treturn 0;\n-\t}\n-\n-\t/* If we reached the end on one side only, lines don't match. */\n-\treturn s1 == end1 && s2 == end2;\n-}\n-\n static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n \tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n@@ -373,11 +344,17 @@ static void prepare_image(struct image *image, char *buf, size_t len,\n \timage->line = image->line_allocated;\n }\n \n-static void clear_image(struct image *image)\n+static void remove_first_line(struct image *img)\n {\n-\tfree(image->buf);\n-\tfree(image->line_allocated);\n-\tmemset(image, 0, sizeof(*image));\n+\timg->buf += img->line[0].len;\n+\timg->len -= img->line[0].len;\n+\timg->line++;\n+\timg->nr--;\n+}\n+\n+static void remove_last_line(struct image *img)\n+{\n+\timg->len -= img->line[--img->nr].len;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2419,6 +2396,42 @@ static void update_pre_post_images(struct image *preimage,\n \tpostimage->nr -= reduced;\n }\n \n+/*\n+ * Compare lines s1 of length n1 and s2 of length n2, ignoring\n+ * whitespace difference. Returns 1 if they match, 0 otherwise\n+ */\n+static int fuzzy_matchlines(const char *s1, size_t n1,\n+\t\t\t const char *s2, size_t n2)\n+{\n+\tconst char *end1 = s1 + n1;\n+\tconst char *end2 = s2 + n2;\n+\n+\t/* ignore line endings */\n+\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n+\t\tend1--;\n+\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n+\t\tend2--;\n+\n+\twhile (s1 < end1 && s2 < end2) {\n+\t\tif (isspace(*s1)) {\n+\t\t\t/*\n+\t\t\t * Skip whitespace. We check on both buffers\n+\t\t\t * because we don't want \"a b\" to match \"ab\".\n+\t\t\t */\n+\t\t\tif (!isspace(*s2))\n+\t\t\t\treturn 0;\n+\t\t\twhile (s1 < end1 && isspace(*s1))\n+\t\t\t\ts1++;\n+\t\t\twhile (s2 < end2 && isspace(*s2))\n+\t\t\t\ts2++;\n+\t\t} else if (*s1++ != *s2++)\n+\t\t\treturn 0;\n+\t}\n+\n+\t/* If we reached the end on one side only, lines don't match. */\n+\treturn s1 == end1 && s2 == end2;\n+}\n+\n static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t struct image *preimage,\n \t\t\t\t struct image *postimage,\n@@ -2804,19 +2817,6 @@ static int find_pos(struct apply_state *state,\n \treturn -1;\n }\n \n-static void remove_first_line(struct image *img)\n-{\n-\timg->buf += img->line[0].len;\n-\timg->len -= img->line[0].len;\n-\timg->line++;\n-\timg->nr--;\n-}\n-\n-static void remove_last_line(struct image *img)\n-{\n-\timg->len -= img->line[--img->nr].len;\n-}\n-\n /*\n * The change from \"preimage\" and \"postimage\" has been found to\n * apply at applied_pos (counts in line numbers) in \"img\".\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502826","messageId":"3f188412f61a98890798999b1351e06ca191c195.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 2/6] apply: rename functions operating on `struct image`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:04Z","receivedAt":"2024-09-16T07:10:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename functions operating on `struct image` to have a `image_` prefix\nto match our modern code style.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 61 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 30 insertions(+), 31 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 9dd2f4d215..ac21c21297 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -285,11 +285,10 @@ struct image {\n \tstruct line *line;\n };\n \n-static void clear_image(struct image *image)\n+static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n \tfree(image->line_allocated);\n-\tmemset(image, 0, sizeof(*image));\n }\n \n static uint32_t hash_line(const char *cp, size_t len)\n@@ -304,7 +303,7 @@ static uint32_t hash_line(const char *cp, size_t len)\n \treturn h;\n }\n \n-static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n+static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n \tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n \timg->line_allocated[img->nr].len = len;\n@@ -318,7 +317,7 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign\n * attach it to \"image\" and add line-based index to it.\n * \"image\" now owns the \"buf\".\n */\n-static void prepare_image(struct image *image, char *buf, size_t len,\n+static void image_prepare(struct image *image, char *buf, size_t len,\n \t\t\t int prepare_linetable)\n {\n \tconst char *cp, *ep;\n@@ -338,13 +337,13 @@ static void prepare_image(struct image *image, char *buf, size_t len,\n \t\t\t;\n \t\tif (next < ep)\n \t\t\tnext++;\n-\t\tadd_line_info(image, cp, next - cp, 0);\n+\t\timage_add_line(image, cp, next - cp, 0);\n \t\tcp = next;\n \t}\n \timage->line = image->line_allocated;\n }\n \n-static void remove_first_line(struct image *img)\n+static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n@@ -352,7 +351,7 @@ static void remove_first_line(struct image *img)\n \timg->nr--;\n }\n \n-static void remove_last_line(struct image *img)\n+static void image_remove_last_line(struct image *img)\n {\n \timg->len -= img->line[--img->nr].len;\n }\n@@ -2322,7 +2321,7 @@ static void update_pre_post_images(struct image *preimage,\n \t * are not losing preimage->buf -- apply_one_fragment() will\n \t * free \"oldlines\".\n \t */\n-\tprepare_image(&fixed_preimage, buf, len, 1);\n+\timage_prepare(&fixed_preimage, buf, len, 1);\n \tassert(postlen\n \t ? fixed_preimage.nr == preimage->nr\n \t : fixed_preimage.nr <= preimage->nr);\n@@ -2874,7 +2873,7 @@ static void update_image(struct apply_state *state,\n \tnr = img->nr + postimage->nr - preimage_limit;\n \tif (preimage_limit < postimage->nr) {\n \t\t/*\n-\t\t * NOTE: this knows that we never call remove_first_line()\n+\t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n@@ -2957,8 +2956,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t\tbreak;\n \t\t\t*old++ = '\\n';\n \t\t\tstrbuf_addch(&newlines, '\\n');\n-\t\t\tadd_line_info(&preimage, \"\\n\", 1, LINE_COMMON);\n-\t\t\tadd_line_info(&postimage, \"\\n\", 1, LINE_COMMON);\n+\t\t\timage_add_line(&preimage, \"\\n\", 1, LINE_COMMON);\n+\t\t\timage_add_line(&postimage, \"\\n\", 1, LINE_COMMON);\n \t\t\tis_blank_context = 1;\n \t\t\tbreak;\n \t\tcase ' ':\n@@ -2968,7 +2967,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t/* fallthrough */\n \t\tcase '-':\n \t\t\tmemcpy(old, patch + 1, plen);\n-\t\t\tadd_line_info(&preimage, old, plen,\n+\t\t\timage_add_line(&preimage, old, plen,\n \t\t\t\t (first == ' ' ? LINE_COMMON : 0));\n \t\t\told += plen;\n \t\t\tif (first == '-')\n@@ -2988,7 +2987,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\telse {\n \t\t\t\tws_fix_copy(&newlines, patch + 1, plen, ws_rule, &state->applied_after_fixing_ws);\n \t\t\t}\n-\t\t\tadd_line_info(&postimage, newlines.buf + start, newlines.len - start,\n+\t\t\timage_add_line(&postimage, newlines.buf + start, newlines.len - start,\n \t\t\t\t (first == '+' ? 0 : LINE_COMMON));\n \t\t\tif (first == '+' &&\n \t\t\t (ws_rule & WS_BLANK_AT_EOF) &&\n@@ -3082,14 +3081,14 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t * just reduce the larger context.\n \t\t */\n \t\tif (leading >= trailing) {\n-\t\t\tremove_first_line(&preimage);\n-\t\t\tremove_first_line(&postimage);\n+\t\t\timage_remove_first_line(&preimage);\n+\t\t\timage_remove_first_line(&postimage);\n \t\t\tpos--;\n \t\t\tleading--;\n \t\t}\n \t\tif (trailing > leading) {\n-\t\t\tremove_last_line(&preimage);\n-\t\t\tremove_last_line(&postimage);\n+\t\t\timage_remove_last_line(&preimage);\n+\t\t\timage_remove_last_line(&postimage);\n \t\t\ttrailing--;\n \t\t}\n \t}\n@@ -3103,7 +3102,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t\t\tfound_new_blank_lines_at_end);\n \t\t\tif (state->ws_error_action == correct_ws_error) {\n \t\t\t\twhile (new_blank_lines_at_end--)\n-\t\t\t\t\tremove_last_line(&postimage);\n+\t\t\t\t\timage_remove_last_line(&postimage);\n \t\t\t}\n \t\t\t/*\n \t\t\t * We would want to prevent write_out_results()\n@@ -3181,12 +3180,12 @@ static int apply_binary_fragment(struct apply_state *state,\n \t\t\t\t fragment->size, &len);\n \t\tif (!dst)\n \t\t\treturn -1;\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->buf = dst;\n \t\timg->len = len;\n \t\treturn 0;\n \tcase BINARY_LITERAL_DEFLATED:\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->len = fragment->size;\n \t\timg->buf = xmemdupz(fragment->patch, img->len);\n \t\treturn 0;\n@@ -3241,7 +3240,7 @@ static int apply_binary(struct apply_state *state,\n \n \tget_oid_hex(patch->new_oid_prefix, &oid);\n \tif (is_null_oid(&oid)) {\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\treturn 0; /* deletion patch */\n \t}\n \n@@ -3257,7 +3256,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\treturn error(_(\"the necessary postimage %s for \"\n \t\t\t\t \"'%s' cannot be read\"),\n \t\t\t\t patch->new_oid_prefix, name);\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->buf = result;\n \t\timg->len = size;\n \t} else {\n@@ -3533,7 +3532,7 @@ static int load_preimage(struct apply_state *state,\n \t}\n \n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(image, img, len, !patch->is_binary);\n+\timage_prepare(image, img, len, !patch->is_binary);\n \treturn 0;\n }\n \n@@ -3542,7 +3541,7 @@ static int resolve_to(struct image *image, const struct object_id *result_id)\n \tunsigned long size;\n \tenum object_type type;\n \n-\tclear_image(image);\n+\timage_clear(image);\n \n \timage->buf = repo_read_object_file(the_repository, result_id, &type,\n \t\t\t\t\t &size);\n@@ -3589,7 +3588,7 @@ static int three_way_merge(struct apply_state *state,\n \t\tfree(result.ptr);\n \t\treturn -1;\n \t}\n-\tclear_image(image);\n+\timage_clear(image);\n \timage->buf = result.ptr;\n \timage->len = result.size;\n \n@@ -3636,7 +3635,7 @@ static int load_current(struct apply_state *state,\n \telse if (status)\n \t\treturn -1;\n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(image, img, len, !patch->is_binary);\n+\timage_prepare(image, img, len, !patch->is_binary);\n \treturn 0;\n }\n \n@@ -3671,15 +3670,15 @@ static int try_threeway(struct apply_state *state,\n \t\tfprintf(stderr, _(\"Performing three-way merge...\\n\"));\n \n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(&tmp_image, img, len, 1);\n+\timage_prepare(&tmp_image, img, len, 1);\n \t/* Apply the patch to get the post image */\n \tif (apply_fragments(state, &tmp_image, patch) < 0) {\n-\t\tclear_image(&tmp_image);\n+\t\timage_clear(&tmp_image);\n \t\treturn -1;\n \t}\n \t/* post_oid is theirs */\n \twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);\n-\tclear_image(&tmp_image);\n+\timage_clear(&tmp_image);\n \n \t/* our_oid is ours */\n \tif (patch->is_new) {\n@@ -3692,7 +3691,7 @@ static int try_threeway(struct apply_state *state,\n \t\t\t\t patch->old_name);\n \t}\n \twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);\n-\tclear_image(&tmp_image);\n+\timage_clear(&tmp_image);\n \n \t/* in-core three-way merge between post and our using pre as base */\n \tstatus = three_way_merge(state, image, patch->new_name,\n@@ -3740,7 +3739,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n \t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {\n-\t\t\tclear_image(&image);\n+\t\t\timage_clear(&image);\n \t\t\treturn -1;\n \t\t}\n \t}\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502827","messageId":"1b49e39bcdcf43318682d8520e6bc3d1f40bd618.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 3/6] apply: introduce macro and function to init images","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:06Z","receivedAt":"2024-09-16T07:10:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to convert the `struct image` to gain a `struct strbuf`\nmember, which requires more careful initialization than just memsetting\nit to zeros. Introduce the `IMAGE_INIT` macro and `image_init()`\nfunction to prepare for this change.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex ac21c21297..76f7777d4c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -284,11 +284,19 @@ struct image {\n \tstruct line *line_allocated;\n \tstruct line *line;\n };\n+#define IMAGE_INIT { 0 }\n+\n+static void image_init(struct image *image)\n+{\n+\tstruct image empty = IMAGE_INIT;\n+\tmemcpy(image, &empty, sizeof(*image));\n+}\n \n static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n \tfree(image->line_allocated);\n+\timage_init(image);\n }\n \n static uint32_t hash_line(const char *cp, size_t len)\n@@ -322,7 +330,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n {\n \tconst char *cp, *ep;\n \n-\tmemset(image, 0, sizeof(*image));\n+\timage_clear(image);\n \timage->buf = buf;\n \timage->len = len;\n \n@@ -2314,7 +2322,7 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tint i, ctx, reduced;\n \tchar *new_buf, *old_buf, *fixed;\n-\tstruct image fixed_preimage;\n+\tstruct image fixed_preimage = IMAGE_INIT;\n \n \t/*\n \t * Update the preimage with whitespace fixes. Note that we\n@@ -2910,11 +2918,9 @@ static int apply_one_fragment(struct apply_state *state,\n \tint hunk_linenr = frag->linenr;\n \tunsigned long leading, trailing;\n \tint pos, applied_pos;\n-\tstruct image preimage;\n-\tstruct image postimage;\n+\tstruct image preimage = IMAGE_INIT;\n+\tstruct image postimage = IMAGE_INIT;\n \n-\tmemset(&preimage, 0, sizeof(preimage));\n-\tmemset(&postimage, 0, sizeof(postimage));\n \toldlines = xmalloc(size);\n \tstrbuf_init(&newlines, size);\n \n@@ -3650,7 +3656,7 @@ static int try_threeway(struct apply_state *state,\n \tsize_t len;\n \tint status;\n \tchar *img;\n-\tstruct image tmp_image;\n+\tstruct image tmp_image = IMAGE_INIT;\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n@@ -3727,7 +3733,7 @@ static int try_threeway(struct apply_state *state,\n static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t struct stat *st, const struct cache_entry *ce)\n {\n-\tstruct image image;\n+\tstruct image image = IMAGE_INIT;\n \n \tif (load_preimage(state, &image, patch, st, ce) < 0)\n \t\treturn -1;\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502828","messageId":"0427cb72507bba7ed62d13a5523fea351a0cb35f.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 4/6] apply: refactor code to drop `line_allocated`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:09Z","receivedAt":"2024-09-16T07:10:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` has two members `line` and `line_allocated`. The\nformer member is the one that should be used throughougt the code,\nwhereas the latter one is used to track whether the lines have been\nallocated or not.\n\nIn practice, the array of lines is always allocated. The reason why we\nhave `line_allocated` is that `remove_first_line()` will advance the\narray pointer to drop the first entry, and thus it point into the array\ninstead of to the array header.\n\nRefactor the function to use memmove(3P) instead, which allows us to get\nrid of this double bookkeeping. We call this function at most once per\nimage anyway, so this shouldn't cause any performance regressions.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 33 ++++++++++++++-------------------\n 1 file changed, 14 insertions(+), 19 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 76f7777d4c..985564ac76 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -281,7 +281,6 @@ struct image {\n \tsize_t len;\n \tsize_t nr;\n \tsize_t alloc;\n-\tstruct line *line_allocated;\n \tstruct line *line;\n };\n #define IMAGE_INIT { 0 }\n@@ -295,7 +294,7 @@ static void image_init(struct image *image)\n static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n-\tfree(image->line_allocated);\n+\tfree(image->line);\n \timage_init(image);\n }\n \n@@ -313,10 +312,10 @@ static uint32_t hash_line(const char *cp, size_t len)\n \n static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n-\tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n-\timg->line_allocated[img->nr].len = len;\n-\timg->line_allocated[img->nr].hash = hash_line(bol, len);\n-\timg->line_allocated[img->nr].flag = flag;\n+\tALLOC_GROW(img->line, img->nr + 1, img->alloc);\n+\timg->line[img->nr].len = len;\n+\timg->line[img->nr].hash = hash_line(bol, len);\n+\timg->line[img->nr].flag = flag;\n \timg->nr++;\n }\n \n@@ -348,15 +347,15 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \t\timage_add_line(image, cp, next - cp, 0);\n \t\tcp = next;\n \t}\n-\timage->line = image->line_allocated;\n }\n \n static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n-\timg->line++;\n \timg->nr--;\n+\tif (img->nr)\n+\t\tMOVE_ARRAY(img->line, img->line + 1, img->nr);\n }\n \n static void image_remove_last_line(struct image *img)\n@@ -2335,7 +2334,7 @@ static void update_pre_post_images(struct image *preimage,\n \t : fixed_preimage.nr <= preimage->nr);\n \tfor (i = 0; i < fixed_preimage.nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n-\tfree(preimage->line_allocated);\n+\tfree(preimage->line);\n \t*preimage = fixed_preimage;\n \n \t/*\n@@ -2879,14 +2878,12 @@ static void update_image(struct apply_state *state,\n \n \t/* Adjust the line table */\n \tnr = img->nr + postimage->nr - preimage_limit;\n-\tif (preimage_limit < postimage->nr) {\n+\tif (preimage_limit < postimage->nr)\n \t\t/*\n \t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n-\t\timg->line_allocated = img->line;\n-\t}\n \tif (preimage_limit != postimage->nr)\n \t\tMOVE_ARRAY(img->line + applied_pos + postimage->nr,\n \t\t\t img->line + applied_pos + preimage_limit,\n@@ -3027,8 +3024,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t newlines.len > 0 && newlines.buf[newlines.len - 1] == '\\n') {\n \t\told--;\n \t\tstrbuf_setlen(&newlines, newlines.len - 1);\n-\t\tpreimage.line_allocated[preimage.nr - 1].len--;\n-\t\tpostimage.line_allocated[postimage.nr - 1].len--;\n+\t\tpreimage.line[preimage.nr - 1].len--;\n+\t\tpostimage.line[postimage.nr - 1].len--;\n \t}\n \n \tleading = frag->leading;\n@@ -3062,8 +3059,6 @@ static int apply_one_fragment(struct apply_state *state,\n \tpreimage.len = old - oldlines;\n \tpostimage.buf = newlines.buf;\n \tpostimage.len = newlines.len;\n-\tpreimage.line = preimage.line_allocated;\n-\tpostimage.line = postimage.line_allocated;\n \n \tfor (;;) {\n \n@@ -3151,8 +3146,8 @@ static int apply_one_fragment(struct apply_state *state,\n out:\n \tfree(oldlines);\n \tstrbuf_release(&newlines);\n-\tfree(preimage.line_allocated);\n-\tfree(postimage.line_allocated);\n+\tfree(preimage.line);\n+\tfree(postimage.line);\n \n \treturn (applied_pos < 0);\n }\n@@ -3752,7 +3747,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \tpatch->result = image.buf;\n \tpatch->resultsize = image.len;\n \tadd_to_fn_table(state, patch);\n-\tfree(image.line_allocated);\n+\tfree(image.line);\n \n \tif (0 < patch->is_delete && patch->resultsize)\n \t\treturn error(_(\"removal patch leaves file contents\"));\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502829","messageId":"e35816ed568633470b049f77ad639ddf0d39379e.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 5/6] apply: rename members that track line count and allocation length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:14Z","receivedAt":"2024-09-16T07:10:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` has two members `nr` and `alloc` that track the\nnumber of lines as well as how large its array is. It is somewhat easy\nto confuse these members with `len` though, which tracks the length of\nthe `buf` member.\n\nRename these members to `line_nr` and `line_alloc` respectively to avoid\nconfusion. This is in line with how we typically name variables that\ntrack an array in this way.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 91 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 45 insertions(+), 46 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 985564ac76..3340bb1fc0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -279,9 +279,8 @@ struct line {\n struct image {\n \tchar *buf;\n \tsize_t len;\n-\tsize_t nr;\n-\tsize_t alloc;\n \tstruct line *line;\n+\tsize_t line_nr, line_alloc;\n };\n #define IMAGE_INIT { 0 }\n \n@@ -312,11 +311,11 @@ static uint32_t hash_line(const char *cp, size_t len)\n \n static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n-\tALLOC_GROW(img->line, img->nr + 1, img->alloc);\n-\timg->line[img->nr].len = len;\n-\timg->line[img->nr].hash = hash_line(bol, len);\n-\timg->line[img->nr].flag = flag;\n-\timg->nr++;\n+\tALLOC_GROW(img->line, img->line_nr + 1, img->line_alloc);\n+\timg->line[img->line_nr].len = len;\n+\timg->line[img->line_nr].hash = hash_line(bol, len);\n+\timg->line[img->line_nr].flag = flag;\n+\timg->line_nr++;\n }\n \n /*\n@@ -353,14 +352,14 @@ static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n-\timg->nr--;\n-\tif (img->nr)\n-\t\tMOVE_ARRAY(img->line, img->line + 1, img->nr);\n+\timg->line_nr--;\n+\tif (img->line_nr)\n+\t\tMOVE_ARRAY(img->line, img->line + 1, img->line_nr);\n }\n \n static void image_remove_last_line(struct image *img)\n {\n-\timg->len -= img->line[--img->nr].len;\n+\timg->len -= img->line[--img->line_nr].len;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2330,9 +2329,9 @@ static void update_pre_post_images(struct image *preimage,\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n \tassert(postlen\n-\t ? fixed_preimage.nr == preimage->nr\n-\t : fixed_preimage.nr <= preimage->nr);\n-\tfor (i = 0; i < fixed_preimage.nr; i++)\n+\t ? fixed_preimage.line_nr == preimage->line_nr\n+\t : fixed_preimage.line_nr <= preimage->line_nr);\n+\tfor (i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n \tfree(preimage->line);\n \t*preimage = fixed_preimage;\n@@ -2353,7 +2352,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\tnew_buf = old_buf;\n \tfixed = preimage->buf;\n \n-\tfor (i = reduced = ctx = 0; i < postimage->nr; i++) {\n+\tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n \t\t\t/* an added line -- no counterparts in preimage */\n@@ -2367,7 +2366,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\told_buf += l_len;\n \n \t\t/* and find the corresponding one in the fixed preimage */\n-\t\twhile (ctx < preimage->nr &&\n+\t\twhile (ctx < preimage->line_nr &&\n \t\t !(preimage->line[ctx].flag & LINE_COMMON)) {\n \t\t\tfixed += preimage->line[ctx].len;\n \t\t\tctx++;\n@@ -2377,7 +2376,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\t * preimage is expected to run out, if the caller\n \t\t * fixed addition of trailing blank lines.\n \t\t */\n-\t\tif (preimage->nr <= ctx) {\n+\t\tif (preimage->line_nr <= ctx) {\n \t\t\treduced++;\n \t\t\tcontinue;\n \t\t}\n@@ -2399,7 +2398,7 @@ static void update_pre_post_images(struct image *preimage,\n \n \t/* Fix the length of the whole thing */\n \tpostimage->len = new_buf - postimage->buf;\n-\tpostimage->nr -= reduced;\n+\tpostimage->line_nr -= reduced;\n }\n \n /*\n@@ -2482,7 +2481,7 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t * we are removing blank lines at the end of the file.)\n \t */\n \tbuf = preimage_eof = preimage->buf + preoff;\n-\tfor ( ; i < preimage->nr; i++)\n+\tfor ( ; i < preimage->line_nr; i++)\n \t\tpreoff += preimage->line[i].len;\n \tpreimage_end = preimage->buf + preoff;\n \tfor ( ; buf < preimage_end; buf++)\n@@ -2522,12 +2521,12 @@ static int match_fragment(struct apply_state *state,\n \tint preimage_limit;\n \tint ret;\n \n-\tif (preimage->nr + current_lno <= img->nr) {\n+\tif (preimage->line_nr + current_lno <= img->line_nr) {\n \t\t/*\n \t\t * The hunk falls within the boundaries of img.\n \t\t */\n-\t\tpreimage_limit = preimage->nr;\n-\t\tif (match_end && (preimage->nr + current_lno != img->nr)) {\n+\t\tpreimage_limit = preimage->line_nr;\n+\t\tif (match_end && (preimage->line_nr + current_lno != img->line_nr)) {\n \t\t\tret = 0;\n \t\t\tgoto out;\n \t\t}\n@@ -2540,7 +2539,7 @@ static int match_fragment(struct apply_state *state,\n \t\t * match with img, and the remainder of the preimage\n \t\t * must be blank.\n \t\t */\n-\t\tpreimage_limit = img->nr - current_lno;\n+\t\tpreimage_limit = img->line_nr - current_lno;\n \t} else {\n \t\t/*\n \t\t * The hunk extends beyond the end of the img and\n@@ -2565,7 +2564,7 @@ static int match_fragment(struct apply_state *state,\n \t\t}\n \t}\n \n-\tif (preimage_limit == preimage->nr) {\n+\tif (preimage_limit == preimage->line_nr) {\n \t\t/*\n \t\t * Do we have an exact match? If we were told to match\n \t\t * at the end, size must be exactly at current+fragsize,\n@@ -2637,7 +2636,7 @@ static int match_fragment(struct apply_state *state,\n \n \t/* First count added lines in postimage */\n \tpostlen = 0;\n-\tfor (i = 0; i < postimage->nr; i++) {\n+\tfor (i = 0; i < postimage->line_nr; i++) {\n \t\tif (!(postimage->line[i].flag & LINE_COMMON))\n \t\t\tpostlen += postimage->line[i].len;\n \t}\n@@ -2699,7 +2698,7 @@ static int match_fragment(struct apply_state *state,\n \t * empty or only contain whitespace (if WS_BLANK_AT_EOL is\n \t * false).\n \t */\n-\tfor ( ; i < preimage->nr; i++) {\n+\tfor ( ; i < preimage->line_nr; i++) {\n \t\tsize_t fixstart = fixed.len; /* start of the fixed preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n \t\tint j;\n@@ -2754,7 +2753,7 @@ static int find_pos(struct apply_state *state,\n \t * than `match_beginning`.\n \t */\n \tif (state->allow_overlap && match_beginning && match_end &&\n-\t img->nr - preimage->nr != 0)\n+\t img->line_nr - preimage->line_nr != 0)\n \t\tmatch_beginning = 0;\n \n \t/*\n@@ -2765,15 +2764,15 @@ static int find_pos(struct apply_state *state,\n \tif (match_beginning)\n \t\tline = 0;\n \telse if (match_end)\n-\t\tline = img->nr - preimage->nr;\n+\t\tline = img->line_nr - preimage->line_nr;\n \n \t/*\n \t * Because the comparison is unsigned, the following test\n \t * will also take care of a negative line number that can\n \t * result when match_end and preimage is larger than the target.\n \t */\n-\tif ((size_t) line > img->nr)\n-\t\tline = img->nr;\n+\tif ((size_t) line > img->line_nr)\n+\t\tline = img->line_nr;\n \n \tcurrent = 0;\n \tfor (i = 0; i < line; i++)\n@@ -2796,7 +2795,7 @@ static int find_pos(struct apply_state *state,\n \t\t\treturn current_lno;\n \n \tagain:\n-\t\tif (backwards_lno == 0 && forwards_lno == img->nr)\n+\t\tif (backwards_lno == 0 && forwards_lno == img->line_nr)\n \t\t\tbreak;\n \n \t\tif (i & 1) {\n@@ -2809,7 +2808,7 @@ static int find_pos(struct apply_state *state,\n \t\t\tcurrent = backwards;\n \t\t\tcurrent_lno = backwards_lno;\n \t\t} else {\n-\t\t\tif (forwards_lno == img->nr) {\n+\t\t\tif (forwards_lno == img->line_nr) {\n \t\t\t\ti++;\n \t\t\t\tgoto again;\n \t\t\t}\n@@ -2852,9 +2851,9 @@ static void update_image(struct apply_state *state,\n \t * to the number of lines in the preimage that falls\n \t * within the boundaries.\n \t */\n-\tpreimage_limit = preimage->nr;\n-\tif (preimage_limit > img->nr - applied_pos)\n-\t\tpreimage_limit = img->nr - applied_pos;\n+\tpreimage_limit = preimage->line_nr;\n+\tif (preimage_limit > img->line_nr - applied_pos)\n+\t\tpreimage_limit = img->line_nr - applied_pos;\n \n \tfor (i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n@@ -2877,22 +2876,22 @@ static void update_image(struct apply_state *state,\n \tresult[img->len] = '\\0';\n \n \t/* Adjust the line table */\n-\tnr = img->nr + postimage->nr - preimage_limit;\n-\tif (preimage_limit < postimage->nr)\n+\tnr = img->line_nr + postimage->line_nr - preimage_limit;\n+\tif (preimage_limit < postimage->line_nr)\n \t\t/*\n \t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n-\tif (preimage_limit != postimage->nr)\n-\t\tMOVE_ARRAY(img->line + applied_pos + postimage->nr,\n+\tif (preimage_limit != postimage->line_nr)\n+\t\tMOVE_ARRAY(img->line + applied_pos + postimage->line_nr,\n \t\t\t img->line + applied_pos + preimage_limit,\n-\t\t\t img->nr - (applied_pos + preimage_limit));\n-\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n+\t\t\t img->line_nr - (applied_pos + preimage_limit));\n+\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);\n \tif (!state->allow_overlap)\n-\t\tfor (i = 0; i < postimage->nr; i++)\n+\t\tfor (i = 0; i < postimage->line_nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n-\timg->nr = nr;\n+\timg->line_nr = nr;\n }\n \n /*\n@@ -3024,8 +3023,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t newlines.len > 0 && newlines.buf[newlines.len - 1] == '\\n') {\n \t\told--;\n \t\tstrbuf_setlen(&newlines, newlines.len - 1);\n-\t\tpreimage.line[preimage.nr - 1].len--;\n-\t\tpostimage.line[postimage.nr - 1].len--;\n+\t\tpreimage.line[preimage.line_nr - 1].len--;\n+\t\tpostimage.line[postimage.line_nr - 1].len--;\n \t}\n \n \tleading = frag->leading;\n@@ -3096,7 +3095,7 @@ static int apply_one_fragment(struct apply_state *state,\n \n \tif (applied_pos >= 0) {\n \t\tif (new_blank_lines_at_end &&\n-\t\t preimage.nr + applied_pos >= img->nr &&\n+\t\t preimage.line_nr + applied_pos >= img->line_nr &&\n \t\t (ws_rule & WS_BLANK_AT_EOF) &&\n \t\t state->ws_error_action != nowarn_ws_error) {\n \t\t\trecord_ws_error(state, WS_BLANK_AT_EOF, \"+\", 1,\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502830","messageId":"6cf45daf84af68f74d87a5b2ae7102d4cd8ceb57.1726470385.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH 6/6] apply: refactor `struct image` to use a `struct strbuf`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-16T07:10:16Z","receivedAt":"2024-09-16T07:10:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` uses a character array to track the pre- or postimage\nof a patch operation. This has multiple downsides:\n\n - It is somewhat hard to track memory ownership. In fact, we have\n several memory leaks in git-apply(1) because we do not (and cannot\n easily) free the buffer in all situations.\n\n - We have to reinvent the wheel and manually implement a lot of\n functionality that would already be provided by `struct strbuf`.\n\n - We have to carefully track whether `update_pre_post_images()` can do\n an in-place update of the postimage or whether it has to allocate a\n new buffer for it.\n\nThis is all rather cumbersome, and especially `update_pre_post_images()`\nis really hard to understand as a consequence even though what it is\ndoing is rather trivial.\n\nRefactor the code to use a `struct strbuf` instead, addressing all of\nthe above. Like this we can easily perform in-place updates in all\nsituations, the logic to perform those updates becomes way simpler and\nthe lifetime of the buffer becomes a ton easier to track.\n\nThis refactoring also plugs some leaking buffers as a side effect.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 192 +++++++++++------------------\n t/t3436-rebase-more-options.sh | 1 +\n t/t4107-apply-ignore-whitespace.sh | 4 +-\n t/t4124-apply-ws-rule.sh | 1 +\n t/t4125-apply-ws-fuzz.sh | 1 +\n t/t4138-apply-ws-expansion.sh | 1 +\n 6 files changed, 77 insertions(+), 123 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3340bb1fc0..d283dd4f5e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -277,12 +277,13 @@ struct line {\n * This represents a \"file\", which is an array of \"lines\".\n */\n struct image {\n-\tchar *buf;\n-\tsize_t len;\n+\tstruct strbuf buf;\n \tstruct line *line;\n \tsize_t line_nr, line_alloc;\n };\n-#define IMAGE_INIT { 0 }\n+#define IMAGE_INIT { \\\n+\t.buf = STRBUF_INIT, \\\n+}\n \n static void image_init(struct image *image)\n {\n@@ -292,7 +293,7 @@ static void image_init(struct image *image)\n \n static void image_clear(struct image *image)\n {\n-\tfree(image->buf);\n+\tstrbuf_release(&image->buf);\n \tfree(image->line);\n \timage_init(image);\n }\n@@ -329,14 +330,13 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \tconst char *cp, *ep;\n \n \timage_clear(image);\n-\timage->buf = buf;\n-\timage->len = len;\n+\tstrbuf_attach(&image->buf, buf, len, len + 1);\n \n \tif (!prepare_linetable)\n \t\treturn;\n \n-\tep = image->buf + image->len;\n-\tcp = image->buf;\n+\tep = image->buf.buf + image->buf.len;\n+\tcp = image->buf.buf;\n \twhile (cp < ep) {\n \t\tconst char *next;\n \t\tfor (next = cp; next < ep && *next != '\\n'; next++)\n@@ -350,8 +350,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \n static void image_remove_first_line(struct image *img)\n {\n-\timg->buf += img->line[0].len;\n-\timg->len -= img->line[0].len;\n+\tstrbuf_remove(&img->buf, 0, img->line[0].len);\n \timg->line_nr--;\n \tif (img->line_nr)\n \t\tMOVE_ARRAY(img->line, img->line + 1, img->line_nr);\n@@ -359,7 +358,7 @@ static void image_remove_first_line(struct image *img)\n \n static void image_remove_last_line(struct image *img)\n {\n-\timg->len -= img->line[--img->line_nr].len;\n+\tstrbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2308,19 +2307,16 @@ static int read_old_data(struct stat *st, struct patch *patch,\n \n /*\n * Update the preimage, and the common lines in postimage,\n- * from buffer buf of length len. If postlen is 0 the postimage\n- * is updated in place, otherwise it's updated on a new buffer\n- * of length postlen\n+ * from buffer buf of length len.\n */\n-\n static void update_pre_post_images(struct image *preimage,\n \t\t\t\t struct image *postimage,\n-\t\t\t\t char *buf,\n-\t\t\t\t size_t len, size_t postlen)\n+\t\t\t\t char *buf, size_t len)\n {\n-\tint i, ctx, reduced;\n-\tchar *new_buf, *old_buf, *fixed;\n \tstruct image fixed_preimage = IMAGE_INIT;\n+\tsize_t insert_pos = 0;\n+\tint i, ctx, reduced;\n+\tconst char *fixed;\n \n \t/*\n \t * Update the preimage with whitespace fixes. Note that we\n@@ -2328,43 +2324,24 @@ static void update_pre_post_images(struct image *preimage,\n \t * free \"oldlines\".\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n-\tassert(postlen\n-\t ? fixed_preimage.line_nr == preimage->line_nr\n-\t : fixed_preimage.line_nr <= preimage->line_nr);\n \tfor (i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n-\tfree(preimage->line);\n+\timage_clear(preimage);\n \t*preimage = fixed_preimage;\n+\tfixed = preimage->buf.buf;\n \n \t/*\n-\t * Adjust the common context lines in postimage. This can be\n-\t * done in-place when we are shrinking it with whitespace\n-\t * fixing, but needs a new buffer when ignoring whitespace or\n-\t * expanding leading tabs to spaces.\n-\t *\n-\t * We trust the caller to tell us if the update can be done\n-\t * in place (postlen==0) or not.\n+\t * Adjust the common context lines in postimage.\n \t */\n-\told_buf = postimage->buf;\n-\tif (postlen)\n-\t\tnew_buf = postimage->buf = xmalloc(postlen);\n-\telse\n-\t\tnew_buf = old_buf;\n-\tfixed = preimage->buf;\n-\n \tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n+\n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n \t\t\t/* an added line -- no counterparts in preimage */\n-\t\t\tmemmove(new_buf, old_buf, l_len);\n-\t\t\told_buf += l_len;\n-\t\t\tnew_buf += l_len;\n+\t\t\tinsert_pos += l_len;\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* a common context -- skip it in the original postimage */\n-\t\told_buf += l_len;\n-\n \t\t/* and find the corresponding one in the fixed preimage */\n \t\twhile (ctx < preimage->line_nr &&\n \t\t !(preimage->line[ctx].flag & LINE_COMMON)) {\n@@ -2383,21 +2360,15 @@ static void update_pre_post_images(struct image *preimage,\n \n \t\t/* and copy it in, while fixing the line length */\n \t\tl_len = preimage->line[ctx].len;\n-\t\tmemcpy(new_buf, fixed, l_len);\n-\t\tnew_buf += l_len;\n+\t\tstrbuf_splice(&postimage->buf, insert_pos, postimage->line[i].len,\n+\t\t\t fixed, l_len);\n+\t\tinsert_pos += l_len;\n \t\tfixed += l_len;\n \t\tpostimage->line[i].len = l_len;\n \t\tctx++;\n \t}\n \n-\tif (postlen\n-\t ? postlen < new_buf - postimage->buf\n-\t : postimage->len < new_buf - postimage->buf)\n-\t\tBUG(\"caller miscounted postlen: asked %d, orig = %d, used = %d\",\n-\t\t (int)postlen, (int) postimage->len, (int)(new_buf - postimage->buf));\n-\n \t/* Fix the length of the whole thing */\n-\tpostimage->len = new_buf - postimage->buf;\n \tpostimage->line_nr -= reduced;\n }\n \n@@ -2447,7 +2418,6 @@ static int line_by_line_fuzzy_match(struct image *img,\n \tint i;\n \tsize_t imgoff = 0;\n \tsize_t preoff = 0;\n-\tsize_t postlen = postimage->len;\n \tsize_t extra_chars;\n \tchar *buf;\n \tchar *preimage_eof;\n@@ -2460,11 +2430,9 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\tsize_t prelen = preimage->line[i].len;\n \t\tsize_t imglen = img->line[current_lno+i].len;\n \n-\t\tif (!fuzzy_matchlines(img->buf + current + imgoff, imglen,\n-\t\t\t\t preimage->buf + preoff, prelen))\n+\t\tif (!fuzzy_matchlines(img->buf.buf + current + imgoff, imglen,\n+\t\t\t\t preimage->buf.buf + preoff, prelen))\n \t\t\treturn 0;\n-\t\tif (preimage->line[i].flag & LINE_COMMON)\n-\t\t\tpostlen += imglen - prelen;\n \t\timgoff += imglen;\n \t\tpreoff += prelen;\n \t}\n@@ -2480,10 +2448,10 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t * are whitespace characters. (This can only happen if\n \t * we are removing blank lines at the end of the file.)\n \t */\n-\tbuf = preimage_eof = preimage->buf + preoff;\n+\tbuf = preimage_eof = preimage->buf.buf + preoff;\n \tfor ( ; i < preimage->line_nr; i++)\n \t\tpreoff += preimage->line[i].len;\n-\tpreimage_end = preimage->buf + preoff;\n+\tpreimage_end = preimage->buf.buf + preoff;\n \tfor ( ; buf < preimage_end; buf++)\n \t\tif (!isspace(*buf))\n \t\t\treturn 0;\n@@ -2497,11 +2465,11 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t */\n \textra_chars = preimage_end - preimage_eof;\n \tstrbuf_init(&fixed, imgoff + extra_chars);\n-\tstrbuf_add(&fixed, img->buf + current, imgoff);\n+\tstrbuf_add(&fixed, img->buf.buf + current, imgoff);\n \tstrbuf_add(&fixed, preimage_eof, extra_chars);\n \tfixed_buf = strbuf_detach(&fixed, &fixed_len);\n \tupdate_pre_post_images(preimage, postimage,\n-\t\t\t fixed_buf, fixed_len, postlen);\n+\t\t\t fixed_buf, fixed_len);\n \treturn 1;\n }\n \n@@ -2517,7 +2485,8 @@ static int match_fragment(struct apply_state *state,\n \tint i;\n \tconst char *orig, *target;\n \tstruct strbuf fixed = STRBUF_INIT;\n-\tsize_t postlen;\n+\tchar *fixed_buf;\n+\tsize_t fixed_len;\n \tint preimage_limit;\n \tint ret;\n \n@@ -2573,9 +2542,9 @@ static int match_fragment(struct apply_state *state,\n \t\t * exactly.\n \t\t */\n \t\tif ((match_end\n-\t\t ? (current + preimage->len == img->len)\n-\t\t : (current + preimage->len <= img->len)) &&\n-\t\t !memcmp(img->buf + current, preimage->buf, preimage->len)) {\n+\t\t ? (current + preimage->buf.len == img->buf.len)\n+\t\t : (current + preimage->buf.len <= img->buf.len)) &&\n+\t\t !memcmp(img->buf.buf + current, preimage->buf.buf, preimage->buf.len)) {\n \t\t\tret = 1;\n \t\t\tgoto out;\n \t\t}\n@@ -2589,7 +2558,7 @@ static int match_fragment(struct apply_state *state,\n \t\t */\n \t\tconst char *buf, *buf_end;\n \n-\t\tbuf = preimage->buf;\n+\t\tbuf = preimage->buf.buf;\n \t\tbuf_end = buf;\n \t\tfor (i = 0; i < preimage_limit; i++)\n \t\t\tbuf_end += preimage->line[i].len;\n@@ -2634,21 +2603,14 @@ static int match_fragment(struct apply_state *state,\n \t * fixed.\n \t */\n \n-\t/* First count added lines in postimage */\n-\tpostlen = 0;\n-\tfor (i = 0; i < postimage->line_nr; i++) {\n-\t\tif (!(postimage->line[i].flag & LINE_COMMON))\n-\t\t\tpostlen += postimage->line[i].len;\n-\t}\n-\n \t/*\n \t * The preimage may extend beyond the end of the file,\n \t * but in this loop we will only handle the part of the\n \t * preimage that falls within the file.\n \t */\n-\tstrbuf_grow(&fixed, preimage->len + 1);\n-\torig = preimage->buf;\n-\ttarget = img->buf + current;\n+\tstrbuf_grow(&fixed, preimage->buf.len + 1);\n+\torig = preimage->buf.buf;\n+\ttarget = img->buf.buf + current;\n \tfor (i = 0; i < preimage_limit; i++) {\n \t\tsize_t oldlen = preimage->line[i].len;\n \t\tsize_t tgtlen = img->line[current_lno + i].len;\n@@ -2677,10 +2639,6 @@ static int match_fragment(struct apply_state *state,\n \t\t\t !memcmp(tgtfix.buf, fixed.buf + fixstart,\n \t\t\t\t\t fixed.len - fixstart));\n \n-\t\t/* Add the length if this is common with the postimage */\n-\t\tif (preimage->line[i].flag & LINE_COMMON)\n-\t\t\tpostlen += tgtfix.len;\n-\n \t\tstrbuf_release(&tgtfix);\n \t\tif (!match) {\n \t\t\tret = 0;\n@@ -2722,10 +2680,9 @@ static int match_fragment(struct apply_state *state,\n \t * has whitespace breakages unfixed, and fixing them makes the\n \t * hunk match. Update the context lines in the postimage.\n \t */\n-\tif (postlen < postimage->len)\n-\t\tpostlen = 0;\n+\tfixed_buf = strbuf_detach(&fixed, &fixed_len);\n \tupdate_pre_post_images(preimage, postimage,\n-\t\t\t fixed.buf, fixed.len, postlen);\n+\t\t\t fixed_buf, fixed_len);\n \n \tret = 1;\n \n@@ -2839,6 +2796,7 @@ static void update_image(struct apply_state *state,\n \t */\n \tint i, nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n+\tsize_t result_alloc;\n \tchar *result;\n \tint preimage_limit;\n \n@@ -2861,19 +2819,18 @@ static void update_image(struct apply_state *state,\n \tremove_count = 0;\n \tfor (i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n-\tinsert_count = postimage->len;\n+\tinsert_count = postimage->buf.len;\n \n \t/* Adjust the contents */\n-\tresult = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 1));\n-\tmemcpy(result, img->buf, applied_at);\n-\tmemcpy(result + applied_at, postimage->buf, postimage->len);\n-\tmemcpy(result + applied_at + postimage->len,\n-\t img->buf + (applied_at + remove_count),\n-\t img->len - (applied_at + remove_count));\n-\tfree(img->buf);\n-\timg->buf = result;\n-\timg->len += insert_count - remove_count;\n-\tresult[img->len] = '\\0';\n+\tresult_alloc = st_add3(st_sub(img->buf.len, remove_count), insert_count, 1);\n+\tresult = xmalloc(result_alloc);\n+\tmemcpy(result, img->buf.buf, applied_at);\n+\tmemcpy(result + applied_at, postimage->buf.buf, postimage->buf.len);\n+\tmemcpy(result + applied_at + postimage->buf.len,\n+\t img->buf.buf + (applied_at + remove_count),\n+\t img->buf.len - (applied_at + remove_count));\n+\tstrbuf_attach(&img->buf, result, postimage->buf.len + img->buf.len - remove_count,\n+\t\t result_alloc);\n \n \t/* Adjust the line table */\n \tnr = img->line_nr + postimage->line_nr - preimage_limit;\n@@ -3054,10 +3011,8 @@ static int apply_one_fragment(struct apply_state *state,\n \tmatch_end = !state->unidiff_zero && !trailing;\n \n \tpos = frag->newpos ? (frag->newpos - 1) : 0;\n-\tpreimage.buf = oldlines;\n-\tpreimage.len = old - oldlines;\n-\tpostimage.buf = newlines.buf;\n-\tpostimage.len = newlines.len;\n+\tstrbuf_add(&preimage.buf, oldlines, old - oldlines);\n+\tstrbuf_swap(&postimage.buf, &newlines);\n \n \tfor (;;) {\n \n@@ -3145,8 +3100,8 @@ static int apply_one_fragment(struct apply_state *state,\n out:\n \tfree(oldlines);\n \tstrbuf_release(&newlines);\n-\tfree(preimage.line);\n-\tfree(postimage.line);\n+\timage_clear(&preimage);\n+\timage_clear(&postimage);\n \n \treturn (applied_pos < 0);\n }\n@@ -3176,18 +3131,16 @@ static int apply_binary_fragment(struct apply_state *state,\n \t}\n \tswitch (fragment->binary_patch_method) {\n \tcase BINARY_DELTA_DEFLATED:\n-\t\tdst = patch_delta(img->buf, img->len, fragment->patch,\n+\t\tdst = patch_delta(img->buf.buf, img->buf.len, fragment->patch,\n \t\t\t\t fragment->size, &len);\n \t\tif (!dst)\n \t\t\treturn -1;\n \t\timage_clear(img);\n-\t\timg->buf = dst;\n-\t\timg->len = len;\n+\t\tstrbuf_attach(&img->buf, dst, len, len + 1);\n \t\treturn 0;\n \tcase BINARY_LITERAL_DEFLATED:\n \t\timage_clear(img);\n-\t\timg->len = fragment->size;\n-\t\timg->buf = xmemdupz(fragment->patch, img->len);\n+\t\tstrbuf_add(&img->buf, fragment->patch, fragment->size);\n \t\treturn 0;\n \t}\n \treturn -1;\n@@ -3223,8 +3176,8 @@ static int apply_binary(struct apply_state *state,\n \t\t * See if the old one matches what the patch\n \t\t * applies to.\n \t\t */\n-\t\thash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,\n-\t\t\t\t &oid);\n+\t\thash_object_file(the_hash_algo, img->buf.buf, img->buf.len,\n+\t\t\t\t OBJ_BLOB, &oid);\n \t\tif (strcmp(oid_to_hex(&oid), patch->old_oid_prefix))\n \t\t\treturn error(_(\"the patch applies to '%s' (%s), \"\n \t\t\t\t \"which does not match the \"\n@@ -3233,7 +3186,7 @@ static int apply_binary(struct apply_state *state,\n \t}\n \telse {\n \t\t/* Otherwise, the old one must be empty. */\n-\t\tif (img->len)\n+\t\tif (img->buf.len)\n \t\t\treturn error(_(\"the patch applies to an empty \"\n \t\t\t\t \"'%s' but it is not empty\"), name);\n \t}\n@@ -3257,8 +3210,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\t\t \"'%s' cannot be read\"),\n \t\t\t\t patch->new_oid_prefix, name);\n \t\timage_clear(img);\n-\t\timg->buf = result;\n-\t\timg->len = size;\n+\t\tstrbuf_attach(&img->buf, result, size, size + 1);\n \t} else {\n \t\t/*\n \t\t * We have verified buf matches the preimage;\n@@ -3270,7 +3222,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\t\t name);\n \n \t\t/* verify that the result matches */\n-\t\thash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,\n+\t\thash_object_file(the_hash_algo, img->buf.buf, img->buf.len, OBJ_BLOB,\n \t\t\t\t &oid);\n \t\tif (strcmp(oid_to_hex(&oid), patch->new_oid_prefix))\n \t\t\treturn error(_(\"binary patch to '%s' creates incorrect result (expecting %s, got %s)\"),\n@@ -3540,14 +3492,14 @@ static int resolve_to(struct image *image, const struct object_id *result_id)\n {\n \tunsigned long size;\n \tenum object_type type;\n+\tchar *data;\n \n \timage_clear(image);\n \n-\timage->buf = repo_read_object_file(the_repository, result_id, &type,\n-\t\t\t\t\t &size);\n-\tif (!image->buf || type != OBJ_BLOB)\n+\tdata = repo_read_object_file(the_repository, result_id, &type, &size);\n+\tif (!data || type != OBJ_BLOB)\n \t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n-\timage->len = size;\n+\tstrbuf_attach(&image->buf, data, size, size + 1);\n \n \treturn 0;\n }\n@@ -3589,8 +3541,7 @@ static int three_way_merge(struct apply_state *state,\n \t\treturn -1;\n \t}\n \timage_clear(image);\n-\timage->buf = result.ptr;\n-\timage->len = result.size;\n+\tstrbuf_attach(&image->buf, result.ptr, result.size, result.size);\n \n \treturn status;\n }\n@@ -3677,7 +3628,7 @@ static int try_threeway(struct apply_state *state,\n \t\treturn -1;\n \t}\n \t/* post_oid is theirs */\n-\twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);\n+\twrite_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &post_oid);\n \timage_clear(&tmp_image);\n \n \t/* our_oid is ours */\n@@ -3690,7 +3641,7 @@ static int try_threeway(struct apply_state *state,\n \t\t\treturn error(_(\"cannot read the current contents of '%s'\"),\n \t\t\t\t patch->old_name);\n \t}\n-\twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);\n+\twrite_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &our_oid);\n \timage_clear(&tmp_image);\n \n \t/* in-core three-way merge between post and our using pre as base */\n@@ -3743,8 +3694,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tpatch->result = image.buf;\n-\tpatch->resultsize = image.len;\n+\tpatch->result = strbuf_detach(&image.buf, &patch->resultsize);\n \tadd_to_fn_table(state, patch);\n \tfree(image.line);\n \ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 94671d3c46..4d9744e5fc 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -5,6 +5,7 @@\n \n test_description='tests to ensure compatibility between am and interactive backends'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n . \"$TEST_DIRECTORY\"/lib-rebase.sh\ndiff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh\nindex ac72eeaf27..5e6e203aa5 100755\n--- a/t/t4107-apply-ignore-whitespace.sh\n+++ b/t/t4107-apply-ignore-whitespace.sh\n@@ -3,9 +3,9 @@\n # Copyright (c) 2009 Giuseppe Bilotta\n #\n \n-test_description='git-apply --ignore-whitespace.\n+test_description='git-apply --ignore-whitespace.'\n \n-'\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # This primes main.c file that indents without using HT at all.\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 485c7d2d12..cdffee0273 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -2,6 +2,7 @@\n \n test_description='core.whitespace rules and git apply'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n prepare_test_file () {\ndiff --git a/t/t4125-apply-ws-fuzz.sh b/t/t4125-apply-ws-fuzz.sh\nindex 090987c89b..f248cc2a00 100755\n--- a/t/t4125-apply-ws-fuzz.sh\n+++ b/t/t4125-apply-ws-fuzz.sh\n@@ -2,6 +2,7 @@\n \n test_description='applying patch that has broken whitespaces in context'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t4138-apply-ws-expansion.sh b/t/t4138-apply-ws-expansion.sh\nindex 8bbf8260fa..7981931b4e 100755\n--- a/t/t4138-apply-ws-expansion.sh\n+++ b/t/t4138-apply-ws-expansion.sh\n@@ -5,6 +5,7 @@\n \n test_description='git apply test patches with whitespace expansion.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502902","messageId":"xmqqldzrmnqn.fsf@gitster.g","threadId":"62119","inReplyTo":"0427cb72507bba7ed62d13a5523fea351a0cb35f.1726470385.git.ps@pks.im","subject":"Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-16T18:56:16Z","receivedAt":"2024-09-16T18:56:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Refactor the function to use memmove(3P) instead, which allows us to get\n> rid of this double bookkeeping. We call this function at most once per\n> image anyway, so this shouldn't cause any performance regressions.\n\nDon't we call remove_first_line() as long as leading is larger than\ntrailing repeatedly? Is \"at most once\" accurate?\n\nAs to the correctness, I think nobody takes the address of an\nelement in the line[] array and expects the address to stay valid\nacross a call to remove_first_line(), so this should be safe.\n\nThanks.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> apply.c | 33 ++++++++++++++-------------------\n> 1 file changed, 14 insertions(+), 19 deletions(-)\n"},{"id":"502904","messageId":"xmqq8qvrmm5e.fsf@gitster.g","threadId":"62119","inReplyTo":"6cf45daf84af68f74d87a5b2ae7102d4cd8ceb57.1726470385.git.ps@pks.im","subject":"Re: [PATCH 6/6] apply: refactor `struct image` to use a `struct strbuf`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-16T19:30:37Z","receivedAt":"2024-09-16T19:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Refactor the code to use a `struct strbuf` instead, addressing all of\n> the above. Like this we can easily perform in-place updates in all\n> situations, the logic to perform those updates becomes way simpler and\n> the lifetime of the buffer becomes a ton easier to track.\n>\n> This refactoring also plugs some leaking buffers as a side effect.\n\nNice.\n\nIn short, the leaks were in the original code where it was making\ndirect assignment to image->buf, and we now use strbuf_attach(),\nwhich releases the current buffer before replacing it with a piece\nof memory allocated outside the control of strbuf API?\n\n> static void image_remove_last_line(struct image *img)\n> {\n> -\timg->len -= img->line[--img->line_nr].len;\n> +\tstrbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);\n> }\n\nI feel that this, while technically is faithful to the original, got\na bit too complex to understand what is going on. Perhaps split it\ninto two statements with an intermediate variable? I dunno.\n\nThanks.\n"},{"id":"502910","messageId":"xmqqzfo7l1l7.fsf@gitster.g","threadId":"62119","inReplyTo":"0427cb72507bba7ed62d13a5523fea351a0cb35f.1726470385.git.ps@pks.im","subject":"Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-16T21:40:04Z","receivedAt":"2024-09-16T21:40:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> former member is the one that should be used throughougt the code,\n\n\"throughout\" (I'll amend while queuing).\n"},{"id":"502931","messageId":"ZulRAiILnz8WZSz4@pks.im","threadId":"62119","inReplyTo":"xmqqldzrmnqn.fsf@gitster.g","subject":"Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T09:50:58Z","receivedAt":"2024-09-17T09:51:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 16, 2024 at 11:56:16AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Refactor the function to use memmove(3P) instead, which allows us to get\n> > rid of this double bookkeeping. We call this function at most once per\n> > image anyway, so this shouldn't cause any performance regressions.\n> \n> Don't we call remove_first_line() as long as leading is larger than\n> trailing repeatedly? Is \"at most once\" accurate?\n> \n> As to the correctness, I think nobody takes the address of an\n> element in the line[] array and expects the address to stay valid\n> across a call to remove_first_line(), so this should be safe.\n\nOh, you're right. I did search for a loop surrounding\n`image_remove_first_line()`, but somehow I completely missed the obvious\n`for (;;)` loop around it. No idea how I was able to miss it.\n\nI still very much doubt that this will cause performance issues in\npractice (even though it's only by gut feeling), but the statement is\nobviously incorrect. In case the assumption ever turns out to be wrong\nwe can likely refactor the loop to only trim contents after we have\nfound how many lines to remove, at which point we can remove them with a\nsingle call to `strbuf_remove()`.\n\nPatrick\n"},{"id":"502938","messageId":"cover.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726470385.git.ps@pks.im","subject":"[PATCH v2 0/6] apply: fix leaking buffer of `struct image`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:07:48Z","receivedAt":"2024-09-17T10:07:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that refactors lifecycle\nmanagement of `struct image` in \"apply.c\" to plug a bunch of memory\nleaks.\n\nChanges compared to v1:\n \n - Fix two typos.\n\n - Correct the statement that we don't loop around\n `image_remove_first_line()`. No idea how I missed that loop.\n\n - Split up an overly complex line into multiple lines.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (6):\n apply: reorder functions to move image-related things together\n apply: rename functions operating on `struct image`\n apply: introduce macro and function to init images\n apply: refactor code to drop `line_allocated`\n apply: rename members that track line count and allocation length\n apply: refactor `struct image` to use a `struct strbuf`\n\n apply.c | 449 +++++++++++++----------------\n t/t3436-rebase-more-options.sh | 1 +\n t/t4107-apply-ignore-whitespace.sh | 4 +-\n t/t4124-apply-ws-rule.sh | 1 +\n t/t4125-apply-ws-fuzz.sh | 1 +\n t/t4138-apply-ws-expansion.sh | 1 +\n 6 files changed, 206 insertions(+), 251 deletions(-)\n\nRange-diff against v1:\n1: 7b6903ecdd = 1: a713a7aef0 apply: reorder functions to move image-related things together\n2: 3f188412f6 = 2: be8f98881f apply: rename functions operating on `struct image`\n3: 1b49e39bcd = 3: 506c787d68 apply: introduce macro and function to init images\n4: 0427cb7250 ! 4: 6ac37186f2 apply: refactor code to drop `line_allocated`\n @@ Commit message\n apply: refactor code to drop `line_allocated`\n \n The `struct image` has two members `line` and `line_allocated`. The\n - former member is the one that should be used throughougt the code,\n + former member is the one that should be used throughout the code,\n whereas the latter one is used to track whether the lines have been\n allocated or not.\n \n In practice, the array of lines is always allocated. The reason why we\n have `line_allocated` is that `remove_first_line()` will advance the\n - array pointer to drop the first entry, and thus it point into the array\n + array pointer to drop the first entry, and thus it points into the array\n instead of to the array header.\n \n Refactor the function to use memmove(3P) instead, which allows us to get\n - rid of this double bookkeeping. We call this function at most once per\n - image anyway, so this shouldn't cause any performance regressions.\n + rid of this double bookkeeping. This is less efficient, but I doubt that\n + this matters much in practice. If this judgement call is found to be\n + wrong at a later point in time we can likely refactor the surrounding\n + loop such that we first calculate the number of leading context lines to\n + remove and then remove them in a single call to memmove(3P).\n \n Signed-off-by: Patrick Steinhardt \n \n5: e35816ed56 = 5: 5497708428 apply: rename members that track line count and allocation length\n6: 6cf45daf84 ! 6: 42880dcf04 apply: refactor `struct image` to use a `struct strbuf`\n @@ apply.c: static void image_remove_first_line(struct image *img)\n static void image_remove_last_line(struct image *img)\n {\n -\timg->len -= img->line[--img->line_nr].len;\n -+\tstrbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);\n ++\tsize_t last_line_len = img->line[img->line_nr - 1].len;\n ++\tstrbuf_setlen(&img->buf, img->buf.len - last_line_len);\n ++\timg->line_nr--;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n\nbase-commit: ed155187b429a2a6b6475efe1767053df37ccfe1\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502939","messageId":"a713a7aef0302a6c844fb36890c4ca60ad8a77f7.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 1/6] apply: reorder functions to move image-related things together","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:07:52Z","receivedAt":"2024-09-17T10:07:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While most of the functions relating to `struct image` are relatively\nclose to one another, `fuzzy_matchlines()` sits in between those even\nthough it is rather unrelated.\n\nReorder functions such that `struct image`-related functions are next to\neach other. While at it, move `clear_image()` to the top such that it is\nclose to the struct definition itself. This makes this lifecycle-related\nthing easy to discover.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 106 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 53 insertions(+), 53 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 6e1060a952..9dd2f4d215 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -285,6 +285,13 @@ struct image {\n \tstruct line *line;\n };\n \n+static void clear_image(struct image *image)\n+{\n+\tfree(image->buf);\n+\tfree(image->line_allocated);\n+\tmemset(image, 0, sizeof(*image));\n+}\n+\n static uint32_t hash_line(const char *cp, size_t len)\n {\n \tsize_t i;\n@@ -297,42 +304,6 @@ static uint32_t hash_line(const char *cp, size_t len)\n \treturn h;\n }\n \n-/*\n- * Compare lines s1 of length n1 and s2 of length n2, ignoring\n- * whitespace difference. Returns 1 if they match, 0 otherwise\n- */\n-static int fuzzy_matchlines(const char *s1, size_t n1,\n-\t\t\t const char *s2, size_t n2)\n-{\n-\tconst char *end1 = s1 + n1;\n-\tconst char *end2 = s2 + n2;\n-\n-\t/* ignore line endings */\n-\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n-\t\tend1--;\n-\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n-\t\tend2--;\n-\n-\twhile (s1 < end1 && s2 < end2) {\n-\t\tif (isspace(*s1)) {\n-\t\t\t/*\n-\t\t\t * Skip whitespace. We check on both buffers\n-\t\t\t * because we don't want \"a b\" to match \"ab\".\n-\t\t\t */\n-\t\t\tif (!isspace(*s2))\n-\t\t\t\treturn 0;\n-\t\t\twhile (s1 < end1 && isspace(*s1))\n-\t\t\t\ts1++;\n-\t\t\twhile (s2 < end2 && isspace(*s2))\n-\t\t\t\ts2++;\n-\t\t} else if (*s1++ != *s2++)\n-\t\t\treturn 0;\n-\t}\n-\n-\t/* If we reached the end on one side only, lines don't match. */\n-\treturn s1 == end1 && s2 == end2;\n-}\n-\n static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n \tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n@@ -373,11 +344,17 @@ static void prepare_image(struct image *image, char *buf, size_t len,\n \timage->line = image->line_allocated;\n }\n \n-static void clear_image(struct image *image)\n+static void remove_first_line(struct image *img)\n {\n-\tfree(image->buf);\n-\tfree(image->line_allocated);\n-\tmemset(image, 0, sizeof(*image));\n+\timg->buf += img->line[0].len;\n+\timg->len -= img->line[0].len;\n+\timg->line++;\n+\timg->nr--;\n+}\n+\n+static void remove_last_line(struct image *img)\n+{\n+\timg->len -= img->line[--img->nr].len;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2419,6 +2396,42 @@ static void update_pre_post_images(struct image *preimage,\n \tpostimage->nr -= reduced;\n }\n \n+/*\n+ * Compare lines s1 of length n1 and s2 of length n2, ignoring\n+ * whitespace difference. Returns 1 if they match, 0 otherwise\n+ */\n+static int fuzzy_matchlines(const char *s1, size_t n1,\n+\t\t\t const char *s2, size_t n2)\n+{\n+\tconst char *end1 = s1 + n1;\n+\tconst char *end2 = s2 + n2;\n+\n+\t/* ignore line endings */\n+\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n+\t\tend1--;\n+\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n+\t\tend2--;\n+\n+\twhile (s1 < end1 && s2 < end2) {\n+\t\tif (isspace(*s1)) {\n+\t\t\t/*\n+\t\t\t * Skip whitespace. We check on both buffers\n+\t\t\t * because we don't want \"a b\" to match \"ab\".\n+\t\t\t */\n+\t\t\tif (!isspace(*s2))\n+\t\t\t\treturn 0;\n+\t\t\twhile (s1 < end1 && isspace(*s1))\n+\t\t\t\ts1++;\n+\t\t\twhile (s2 < end2 && isspace(*s2))\n+\t\t\t\ts2++;\n+\t\t} else if (*s1++ != *s2++)\n+\t\t\treturn 0;\n+\t}\n+\n+\t/* If we reached the end on one side only, lines don't match. */\n+\treturn s1 == end1 && s2 == end2;\n+}\n+\n static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t struct image *preimage,\n \t\t\t\t struct image *postimage,\n@@ -2804,19 +2817,6 @@ static int find_pos(struct apply_state *state,\n \treturn -1;\n }\n \n-static void remove_first_line(struct image *img)\n-{\n-\timg->buf += img->line[0].len;\n-\timg->len -= img->line[0].len;\n-\timg->line++;\n-\timg->nr--;\n-}\n-\n-static void remove_last_line(struct image *img)\n-{\n-\timg->len -= img->line[--img->nr].len;\n-}\n-\n /*\n * The change from \"preimage\" and \"postimage\" has been found to\n * apply at applied_pos (counts in line numbers) in \"img\".\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502940","messageId":"be8f98881fb5836d58a7f4819d6fad65415e2c34.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 2/6] apply: rename functions operating on `struct image`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:07:55Z","receivedAt":"2024-09-17T10:07:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename functions operating on `struct image` to have a `image_` prefix\nto match our modern code style.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 61 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 30 insertions(+), 31 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 9dd2f4d215..ac21c21297 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -285,11 +285,10 @@ struct image {\n \tstruct line *line;\n };\n \n-static void clear_image(struct image *image)\n+static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n \tfree(image->line_allocated);\n-\tmemset(image, 0, sizeof(*image));\n }\n \n static uint32_t hash_line(const char *cp, size_t len)\n@@ -304,7 +303,7 @@ static uint32_t hash_line(const char *cp, size_t len)\n \treturn h;\n }\n \n-static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n+static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n \tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n \timg->line_allocated[img->nr].len = len;\n@@ -318,7 +317,7 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign\n * attach it to \"image\" and add line-based index to it.\n * \"image\" now owns the \"buf\".\n */\n-static void prepare_image(struct image *image, char *buf, size_t len,\n+static void image_prepare(struct image *image, char *buf, size_t len,\n \t\t\t int prepare_linetable)\n {\n \tconst char *cp, *ep;\n@@ -338,13 +337,13 @@ static void prepare_image(struct image *image, char *buf, size_t len,\n \t\t\t;\n \t\tif (next < ep)\n \t\t\tnext++;\n-\t\tadd_line_info(image, cp, next - cp, 0);\n+\t\timage_add_line(image, cp, next - cp, 0);\n \t\tcp = next;\n \t}\n \timage->line = image->line_allocated;\n }\n \n-static void remove_first_line(struct image *img)\n+static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n@@ -352,7 +351,7 @@ static void remove_first_line(struct image *img)\n \timg->nr--;\n }\n \n-static void remove_last_line(struct image *img)\n+static void image_remove_last_line(struct image *img)\n {\n \timg->len -= img->line[--img->nr].len;\n }\n@@ -2322,7 +2321,7 @@ static void update_pre_post_images(struct image *preimage,\n \t * are not losing preimage->buf -- apply_one_fragment() will\n \t * free \"oldlines\".\n \t */\n-\tprepare_image(&fixed_preimage, buf, len, 1);\n+\timage_prepare(&fixed_preimage, buf, len, 1);\n \tassert(postlen\n \t ? fixed_preimage.nr == preimage->nr\n \t : fixed_preimage.nr <= preimage->nr);\n@@ -2874,7 +2873,7 @@ static void update_image(struct apply_state *state,\n \tnr = img->nr + postimage->nr - preimage_limit;\n \tif (preimage_limit < postimage->nr) {\n \t\t/*\n-\t\t * NOTE: this knows that we never call remove_first_line()\n+\t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n@@ -2957,8 +2956,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t\tbreak;\n \t\t\t*old++ = '\\n';\n \t\t\tstrbuf_addch(&newlines, '\\n');\n-\t\t\tadd_line_info(&preimage, \"\\n\", 1, LINE_COMMON);\n-\t\t\tadd_line_info(&postimage, \"\\n\", 1, LINE_COMMON);\n+\t\t\timage_add_line(&preimage, \"\\n\", 1, LINE_COMMON);\n+\t\t\timage_add_line(&postimage, \"\\n\", 1, LINE_COMMON);\n \t\t\tis_blank_context = 1;\n \t\t\tbreak;\n \t\tcase ' ':\n@@ -2968,7 +2967,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t/* fallthrough */\n \t\tcase '-':\n \t\t\tmemcpy(old, patch + 1, plen);\n-\t\t\tadd_line_info(&preimage, old, plen,\n+\t\t\timage_add_line(&preimage, old, plen,\n \t\t\t\t (first == ' ' ? LINE_COMMON : 0));\n \t\t\told += plen;\n \t\t\tif (first == '-')\n@@ -2988,7 +2987,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\telse {\n \t\t\t\tws_fix_copy(&newlines, patch + 1, plen, ws_rule, &state->applied_after_fixing_ws);\n \t\t\t}\n-\t\t\tadd_line_info(&postimage, newlines.buf + start, newlines.len - start,\n+\t\t\timage_add_line(&postimage, newlines.buf + start, newlines.len - start,\n \t\t\t\t (first == '+' ? 0 : LINE_COMMON));\n \t\t\tif (first == '+' &&\n \t\t\t (ws_rule & WS_BLANK_AT_EOF) &&\n@@ -3082,14 +3081,14 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t * just reduce the larger context.\n \t\t */\n \t\tif (leading >= trailing) {\n-\t\t\tremove_first_line(&preimage);\n-\t\t\tremove_first_line(&postimage);\n+\t\t\timage_remove_first_line(&preimage);\n+\t\t\timage_remove_first_line(&postimage);\n \t\t\tpos--;\n \t\t\tleading--;\n \t\t}\n \t\tif (trailing > leading) {\n-\t\t\tremove_last_line(&preimage);\n-\t\t\tremove_last_line(&postimage);\n+\t\t\timage_remove_last_line(&preimage);\n+\t\t\timage_remove_last_line(&postimage);\n \t\t\ttrailing--;\n \t\t}\n \t}\n@@ -3103,7 +3102,7 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t\t\tfound_new_blank_lines_at_end);\n \t\t\tif (state->ws_error_action == correct_ws_error) {\n \t\t\t\twhile (new_blank_lines_at_end--)\n-\t\t\t\t\tremove_last_line(&postimage);\n+\t\t\t\t\timage_remove_last_line(&postimage);\n \t\t\t}\n \t\t\t/*\n \t\t\t * We would want to prevent write_out_results()\n@@ -3181,12 +3180,12 @@ static int apply_binary_fragment(struct apply_state *state,\n \t\t\t\t fragment->size, &len);\n \t\tif (!dst)\n \t\t\treturn -1;\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->buf = dst;\n \t\timg->len = len;\n \t\treturn 0;\n \tcase BINARY_LITERAL_DEFLATED:\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->len = fragment->size;\n \t\timg->buf = xmemdupz(fragment->patch, img->len);\n \t\treturn 0;\n@@ -3241,7 +3240,7 @@ static int apply_binary(struct apply_state *state,\n \n \tget_oid_hex(patch->new_oid_prefix, &oid);\n \tif (is_null_oid(&oid)) {\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\treturn 0; /* deletion patch */\n \t}\n \n@@ -3257,7 +3256,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\treturn error(_(\"the necessary postimage %s for \"\n \t\t\t\t \"'%s' cannot be read\"),\n \t\t\t\t patch->new_oid_prefix, name);\n-\t\tclear_image(img);\n+\t\timage_clear(img);\n \t\timg->buf = result;\n \t\timg->len = size;\n \t} else {\n@@ -3533,7 +3532,7 @@ static int load_preimage(struct apply_state *state,\n \t}\n \n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(image, img, len, !patch->is_binary);\n+\timage_prepare(image, img, len, !patch->is_binary);\n \treturn 0;\n }\n \n@@ -3542,7 +3541,7 @@ static int resolve_to(struct image *image, const struct object_id *result_id)\n \tunsigned long size;\n \tenum object_type type;\n \n-\tclear_image(image);\n+\timage_clear(image);\n \n \timage->buf = repo_read_object_file(the_repository, result_id, &type,\n \t\t\t\t\t &size);\n@@ -3589,7 +3588,7 @@ static int three_way_merge(struct apply_state *state,\n \t\tfree(result.ptr);\n \t\treturn -1;\n \t}\n-\tclear_image(image);\n+\timage_clear(image);\n \timage->buf = result.ptr;\n \timage->len = result.size;\n \n@@ -3636,7 +3635,7 @@ static int load_current(struct apply_state *state,\n \telse if (status)\n \t\treturn -1;\n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(image, img, len, !patch->is_binary);\n+\timage_prepare(image, img, len, !patch->is_binary);\n \treturn 0;\n }\n \n@@ -3671,15 +3670,15 @@ static int try_threeway(struct apply_state *state,\n \t\tfprintf(stderr, _(\"Performing three-way merge...\\n\"));\n \n \timg = strbuf_detach(&buf, &len);\n-\tprepare_image(&tmp_image, img, len, 1);\n+\timage_prepare(&tmp_image, img, len, 1);\n \t/* Apply the patch to get the post image */\n \tif (apply_fragments(state, &tmp_image, patch) < 0) {\n-\t\tclear_image(&tmp_image);\n+\t\timage_clear(&tmp_image);\n \t\treturn -1;\n \t}\n \t/* post_oid is theirs */\n \twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);\n-\tclear_image(&tmp_image);\n+\timage_clear(&tmp_image);\n \n \t/* our_oid is ours */\n \tif (patch->is_new) {\n@@ -3692,7 +3691,7 @@ static int try_threeway(struct apply_state *state,\n \t\t\t\t patch->old_name);\n \t}\n \twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);\n-\tclear_image(&tmp_image);\n+\timage_clear(&tmp_image);\n \n \t/* in-core three-way merge between post and our using pre as base */\n \tstatus = three_way_merge(state, image, patch->new_name,\n@@ -3740,7 +3739,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n \t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {\n-\t\t\tclear_image(&image);\n+\t\t\timage_clear(&image);\n \t\t\treturn -1;\n \t\t}\n \t}\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502941","messageId":"506c787d6828c028b3a4403a9a65468f1bbc030b.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 3/6] apply: introduce macro and function to init images","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:08:01Z","receivedAt":"2024-09-17T10:08:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to convert the `struct image` to gain a `struct strbuf`\nmember, which requires more careful initialization than just memsetting\nit to zeros. Introduce the `IMAGE_INIT` macro and `image_init()`\nfunction to prepare for this change.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex ac21c21297..76f7777d4c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -284,11 +284,19 @@ struct image {\n \tstruct line *line_allocated;\n \tstruct line *line;\n };\n+#define IMAGE_INIT { 0 }\n+\n+static void image_init(struct image *image)\n+{\n+\tstruct image empty = IMAGE_INIT;\n+\tmemcpy(image, &empty, sizeof(*image));\n+}\n \n static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n \tfree(image->line_allocated);\n+\timage_init(image);\n }\n \n static uint32_t hash_line(const char *cp, size_t len)\n@@ -322,7 +330,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n {\n \tconst char *cp, *ep;\n \n-\tmemset(image, 0, sizeof(*image));\n+\timage_clear(image);\n \timage->buf = buf;\n \timage->len = len;\n \n@@ -2314,7 +2322,7 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tint i, ctx, reduced;\n \tchar *new_buf, *old_buf, *fixed;\n-\tstruct image fixed_preimage;\n+\tstruct image fixed_preimage = IMAGE_INIT;\n \n \t/*\n \t * Update the preimage with whitespace fixes. Note that we\n@@ -2910,11 +2918,9 @@ static int apply_one_fragment(struct apply_state *state,\n \tint hunk_linenr = frag->linenr;\n \tunsigned long leading, trailing;\n \tint pos, applied_pos;\n-\tstruct image preimage;\n-\tstruct image postimage;\n+\tstruct image preimage = IMAGE_INIT;\n+\tstruct image postimage = IMAGE_INIT;\n \n-\tmemset(&preimage, 0, sizeof(preimage));\n-\tmemset(&postimage, 0, sizeof(postimage));\n \toldlines = xmalloc(size);\n \tstrbuf_init(&newlines, size);\n \n@@ -3650,7 +3656,7 @@ static int try_threeway(struct apply_state *state,\n \tsize_t len;\n \tint status;\n \tchar *img;\n-\tstruct image tmp_image;\n+\tstruct image tmp_image = IMAGE_INIT;\n \n \t/* No point falling back to 3-way merge in these cases */\n \tif (patch->is_delete ||\n@@ -3727,7 +3733,7 @@ static int try_threeway(struct apply_state *state,\n static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t struct stat *st, const struct cache_entry *ce)\n {\n-\tstruct image image;\n+\tstruct image image = IMAGE_INIT;\n \n \tif (load_preimage(state, &image, patch, st, ce) < 0)\n \t\treturn -1;\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502942","messageId":"6ac37186f284f9a4fcc4c9e25a1eaf102317f38a.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 4/6] apply: refactor code to drop `line_allocated`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:08:03Z","receivedAt":"2024-09-17T10:08:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` has two members `line` and `line_allocated`. The\nformer member is the one that should be used throughout the code,\nwhereas the latter one is used to track whether the lines have been\nallocated or not.\n\nIn practice, the array of lines is always allocated. The reason why we\nhave `line_allocated` is that `remove_first_line()` will advance the\narray pointer to drop the first entry, and thus it points into the array\ninstead of to the array header.\n\nRefactor the function to use memmove(3P) instead, which allows us to get\nrid of this double bookkeeping. This is less efficient, but I doubt that\nthis matters much in practice. If this judgement call is found to be\nwrong at a later point in time we can likely refactor the surrounding\nloop such that we first calculate the number of leading context lines to\nremove and then remove them in a single call to memmove(3P).\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 33 ++++++++++++++-------------------\n 1 file changed, 14 insertions(+), 19 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 76f7777d4c..985564ac76 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -281,7 +281,6 @@ struct image {\n \tsize_t len;\n \tsize_t nr;\n \tsize_t alloc;\n-\tstruct line *line_allocated;\n \tstruct line *line;\n };\n #define IMAGE_INIT { 0 }\n@@ -295,7 +294,7 @@ static void image_init(struct image *image)\n static void image_clear(struct image *image)\n {\n \tfree(image->buf);\n-\tfree(image->line_allocated);\n+\tfree(image->line);\n \timage_init(image);\n }\n \n@@ -313,10 +312,10 @@ static uint32_t hash_line(const char *cp, size_t len)\n \n static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n-\tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n-\timg->line_allocated[img->nr].len = len;\n-\timg->line_allocated[img->nr].hash = hash_line(bol, len);\n-\timg->line_allocated[img->nr].flag = flag;\n+\tALLOC_GROW(img->line, img->nr + 1, img->alloc);\n+\timg->line[img->nr].len = len;\n+\timg->line[img->nr].hash = hash_line(bol, len);\n+\timg->line[img->nr].flag = flag;\n \timg->nr++;\n }\n \n@@ -348,15 +347,15 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \t\timage_add_line(image, cp, next - cp, 0);\n \t\tcp = next;\n \t}\n-\timage->line = image->line_allocated;\n }\n \n static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n-\timg->line++;\n \timg->nr--;\n+\tif (img->nr)\n+\t\tMOVE_ARRAY(img->line, img->line + 1, img->nr);\n }\n \n static void image_remove_last_line(struct image *img)\n@@ -2335,7 +2334,7 @@ static void update_pre_post_images(struct image *preimage,\n \t : fixed_preimage.nr <= preimage->nr);\n \tfor (i = 0; i < fixed_preimage.nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n-\tfree(preimage->line_allocated);\n+\tfree(preimage->line);\n \t*preimage = fixed_preimage;\n \n \t/*\n@@ -2879,14 +2878,12 @@ static void update_image(struct apply_state *state,\n \n \t/* Adjust the line table */\n \tnr = img->nr + postimage->nr - preimage_limit;\n-\tif (preimage_limit < postimage->nr) {\n+\tif (preimage_limit < postimage->nr)\n \t\t/*\n \t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n-\t\timg->line_allocated = img->line;\n-\t}\n \tif (preimage_limit != postimage->nr)\n \t\tMOVE_ARRAY(img->line + applied_pos + postimage->nr,\n \t\t\t img->line + applied_pos + preimage_limit,\n@@ -3027,8 +3024,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t newlines.len > 0 && newlines.buf[newlines.len - 1] == '\\n') {\n \t\told--;\n \t\tstrbuf_setlen(&newlines, newlines.len - 1);\n-\t\tpreimage.line_allocated[preimage.nr - 1].len--;\n-\t\tpostimage.line_allocated[postimage.nr - 1].len--;\n+\t\tpreimage.line[preimage.nr - 1].len--;\n+\t\tpostimage.line[postimage.nr - 1].len--;\n \t}\n \n \tleading = frag->leading;\n@@ -3062,8 +3059,6 @@ static int apply_one_fragment(struct apply_state *state,\n \tpreimage.len = old - oldlines;\n \tpostimage.buf = newlines.buf;\n \tpostimage.len = newlines.len;\n-\tpreimage.line = preimage.line_allocated;\n-\tpostimage.line = postimage.line_allocated;\n \n \tfor (;;) {\n \n@@ -3151,8 +3146,8 @@ static int apply_one_fragment(struct apply_state *state,\n out:\n \tfree(oldlines);\n \tstrbuf_release(&newlines);\n-\tfree(preimage.line_allocated);\n-\tfree(postimage.line_allocated);\n+\tfree(preimage.line);\n+\tfree(postimage.line);\n \n \treturn (applied_pos < 0);\n }\n@@ -3752,7 +3747,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \tpatch->result = image.buf;\n \tpatch->resultsize = image.len;\n \tadd_to_fn_table(state, patch);\n-\tfree(image.line_allocated);\n+\tfree(image.line);\n \n \tif (0 < patch->is_delete && patch->resultsize)\n \t\treturn error(_(\"removal patch leaves file contents\"));\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502943","messageId":"5497708428c5cf6c35c3ea8e17110ce01ec19845.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 5/6] apply: rename members that track line count and allocation length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:08:06Z","receivedAt":"2024-09-17T10:08:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` has two members `nr` and `alloc` that track the\nnumber of lines as well as how large its array is. It is somewhat easy\nto confuse these members with `len` though, which tracks the length of\nthe `buf` member.\n\nRename these members to `line_nr` and `line_alloc` respectively to avoid\nconfusion. This is in line with how we typically name variables that\ntrack an array in this way.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 91 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 45 insertions(+), 46 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 985564ac76..3340bb1fc0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -279,9 +279,8 @@ struct line {\n struct image {\n \tchar *buf;\n \tsize_t len;\n-\tsize_t nr;\n-\tsize_t alloc;\n \tstruct line *line;\n+\tsize_t line_nr, line_alloc;\n };\n #define IMAGE_INIT { 0 }\n \n@@ -312,11 +311,11 @@ static uint32_t hash_line(const char *cp, size_t len)\n \n static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n-\tALLOC_GROW(img->line, img->nr + 1, img->alloc);\n-\timg->line[img->nr].len = len;\n-\timg->line[img->nr].hash = hash_line(bol, len);\n-\timg->line[img->nr].flag = flag;\n-\timg->nr++;\n+\tALLOC_GROW(img->line, img->line_nr + 1, img->line_alloc);\n+\timg->line[img->line_nr].len = len;\n+\timg->line[img->line_nr].hash = hash_line(bol, len);\n+\timg->line[img->line_nr].flag = flag;\n+\timg->line_nr++;\n }\n \n /*\n@@ -353,14 +352,14 @@ static void image_remove_first_line(struct image *img)\n {\n \timg->buf += img->line[0].len;\n \timg->len -= img->line[0].len;\n-\timg->nr--;\n-\tif (img->nr)\n-\t\tMOVE_ARRAY(img->line, img->line + 1, img->nr);\n+\timg->line_nr--;\n+\tif (img->line_nr)\n+\t\tMOVE_ARRAY(img->line, img->line + 1, img->line_nr);\n }\n \n static void image_remove_last_line(struct image *img)\n {\n-\timg->len -= img->line[--img->nr].len;\n+\timg->len -= img->line[--img->line_nr].len;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2330,9 +2329,9 @@ static void update_pre_post_images(struct image *preimage,\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n \tassert(postlen\n-\t ? fixed_preimage.nr == preimage->nr\n-\t : fixed_preimage.nr <= preimage->nr);\n-\tfor (i = 0; i < fixed_preimage.nr; i++)\n+\t ? fixed_preimage.line_nr == preimage->line_nr\n+\t : fixed_preimage.line_nr <= preimage->line_nr);\n+\tfor (i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n \tfree(preimage->line);\n \t*preimage = fixed_preimage;\n@@ -2353,7 +2352,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\tnew_buf = old_buf;\n \tfixed = preimage->buf;\n \n-\tfor (i = reduced = ctx = 0; i < postimage->nr; i++) {\n+\tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n \t\t\t/* an added line -- no counterparts in preimage */\n@@ -2367,7 +2366,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\told_buf += l_len;\n \n \t\t/* and find the corresponding one in the fixed preimage */\n-\t\twhile (ctx < preimage->nr &&\n+\t\twhile (ctx < preimage->line_nr &&\n \t\t !(preimage->line[ctx].flag & LINE_COMMON)) {\n \t\t\tfixed += preimage->line[ctx].len;\n \t\t\tctx++;\n@@ -2377,7 +2376,7 @@ static void update_pre_post_images(struct image *preimage,\n \t\t * preimage is expected to run out, if the caller\n \t\t * fixed addition of trailing blank lines.\n \t\t */\n-\t\tif (preimage->nr <= ctx) {\n+\t\tif (preimage->line_nr <= ctx) {\n \t\t\treduced++;\n \t\t\tcontinue;\n \t\t}\n@@ -2399,7 +2398,7 @@ static void update_pre_post_images(struct image *preimage,\n \n \t/* Fix the length of the whole thing */\n \tpostimage->len = new_buf - postimage->buf;\n-\tpostimage->nr -= reduced;\n+\tpostimage->line_nr -= reduced;\n }\n \n /*\n@@ -2482,7 +2481,7 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t * we are removing blank lines at the end of the file.)\n \t */\n \tbuf = preimage_eof = preimage->buf + preoff;\n-\tfor ( ; i < preimage->nr; i++)\n+\tfor ( ; i < preimage->line_nr; i++)\n \t\tpreoff += preimage->line[i].len;\n \tpreimage_end = preimage->buf + preoff;\n \tfor ( ; buf < preimage_end; buf++)\n@@ -2522,12 +2521,12 @@ static int match_fragment(struct apply_state *state,\n \tint preimage_limit;\n \tint ret;\n \n-\tif (preimage->nr + current_lno <= img->nr) {\n+\tif (preimage->line_nr + current_lno <= img->line_nr) {\n \t\t/*\n \t\t * The hunk falls within the boundaries of img.\n \t\t */\n-\t\tpreimage_limit = preimage->nr;\n-\t\tif (match_end && (preimage->nr + current_lno != img->nr)) {\n+\t\tpreimage_limit = preimage->line_nr;\n+\t\tif (match_end && (preimage->line_nr + current_lno != img->line_nr)) {\n \t\t\tret = 0;\n \t\t\tgoto out;\n \t\t}\n@@ -2540,7 +2539,7 @@ static int match_fragment(struct apply_state *state,\n \t\t * match with img, and the remainder of the preimage\n \t\t * must be blank.\n \t\t */\n-\t\tpreimage_limit = img->nr - current_lno;\n+\t\tpreimage_limit = img->line_nr - current_lno;\n \t} else {\n \t\t/*\n \t\t * The hunk extends beyond the end of the img and\n@@ -2565,7 +2564,7 @@ static int match_fragment(struct apply_state *state,\n \t\t}\n \t}\n \n-\tif (preimage_limit == preimage->nr) {\n+\tif (preimage_limit == preimage->line_nr) {\n \t\t/*\n \t\t * Do we have an exact match? If we were told to match\n \t\t * at the end, size must be exactly at current+fragsize,\n@@ -2637,7 +2636,7 @@ static int match_fragment(struct apply_state *state,\n \n \t/* First count added lines in postimage */\n \tpostlen = 0;\n-\tfor (i = 0; i < postimage->nr; i++) {\n+\tfor (i = 0; i < postimage->line_nr; i++) {\n \t\tif (!(postimage->line[i].flag & LINE_COMMON))\n \t\t\tpostlen += postimage->line[i].len;\n \t}\n@@ -2699,7 +2698,7 @@ static int match_fragment(struct apply_state *state,\n \t * empty or only contain whitespace (if WS_BLANK_AT_EOL is\n \t * false).\n \t */\n-\tfor ( ; i < preimage->nr; i++) {\n+\tfor ( ; i < preimage->line_nr; i++) {\n \t\tsize_t fixstart = fixed.len; /* start of the fixed preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n \t\tint j;\n@@ -2754,7 +2753,7 @@ static int find_pos(struct apply_state *state,\n \t * than `match_beginning`.\n \t */\n \tif (state->allow_overlap && match_beginning && match_end &&\n-\t img->nr - preimage->nr != 0)\n+\t img->line_nr - preimage->line_nr != 0)\n \t\tmatch_beginning = 0;\n \n \t/*\n@@ -2765,15 +2764,15 @@ static int find_pos(struct apply_state *state,\n \tif (match_beginning)\n \t\tline = 0;\n \telse if (match_end)\n-\t\tline = img->nr - preimage->nr;\n+\t\tline = img->line_nr - preimage->line_nr;\n \n \t/*\n \t * Because the comparison is unsigned, the following test\n \t * will also take care of a negative line number that can\n \t * result when match_end and preimage is larger than the target.\n \t */\n-\tif ((size_t) line > img->nr)\n-\t\tline = img->nr;\n+\tif ((size_t) line > img->line_nr)\n+\t\tline = img->line_nr;\n \n \tcurrent = 0;\n \tfor (i = 0; i < line; i++)\n@@ -2796,7 +2795,7 @@ static int find_pos(struct apply_state *state,\n \t\t\treturn current_lno;\n \n \tagain:\n-\t\tif (backwards_lno == 0 && forwards_lno == img->nr)\n+\t\tif (backwards_lno == 0 && forwards_lno == img->line_nr)\n \t\t\tbreak;\n \n \t\tif (i & 1) {\n@@ -2809,7 +2808,7 @@ static int find_pos(struct apply_state *state,\n \t\t\tcurrent = backwards;\n \t\t\tcurrent_lno = backwards_lno;\n \t\t} else {\n-\t\t\tif (forwards_lno == img->nr) {\n+\t\t\tif (forwards_lno == img->line_nr) {\n \t\t\t\ti++;\n \t\t\t\tgoto again;\n \t\t\t}\n@@ -2852,9 +2851,9 @@ static void update_image(struct apply_state *state,\n \t * to the number of lines in the preimage that falls\n \t * within the boundaries.\n \t */\n-\tpreimage_limit = preimage->nr;\n-\tif (preimage_limit > img->nr - applied_pos)\n-\t\tpreimage_limit = img->nr - applied_pos;\n+\tpreimage_limit = preimage->line_nr;\n+\tif (preimage_limit > img->line_nr - applied_pos)\n+\t\tpreimage_limit = img->line_nr - applied_pos;\n \n \tfor (i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n@@ -2877,22 +2876,22 @@ static void update_image(struct apply_state *state,\n \tresult[img->len] = '\\0';\n \n \t/* Adjust the line table */\n-\tnr = img->nr + postimage->nr - preimage_limit;\n-\tif (preimage_limit < postimage->nr)\n+\tnr = img->line_nr + postimage->line_nr - preimage_limit;\n+\tif (preimage_limit < postimage->line_nr)\n \t\t/*\n \t\t * NOTE: this knows that we never call image_remove_first_line()\n \t\t * on anything other than pre/post image.\n \t\t */\n \t\tREALLOC_ARRAY(img->line, nr);\n-\tif (preimage_limit != postimage->nr)\n-\t\tMOVE_ARRAY(img->line + applied_pos + postimage->nr,\n+\tif (preimage_limit != postimage->line_nr)\n+\t\tMOVE_ARRAY(img->line + applied_pos + postimage->line_nr,\n \t\t\t img->line + applied_pos + preimage_limit,\n-\t\t\t img->nr - (applied_pos + preimage_limit));\n-\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);\n+\t\t\t img->line_nr - (applied_pos + preimage_limit));\n+\tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);\n \tif (!state->allow_overlap)\n-\t\tfor (i = 0; i < postimage->nr; i++)\n+\t\tfor (i = 0; i < postimage->line_nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n-\timg->nr = nr;\n+\timg->line_nr = nr;\n }\n \n /*\n@@ -3024,8 +3023,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t newlines.len > 0 && newlines.buf[newlines.len - 1] == '\\n') {\n \t\told--;\n \t\tstrbuf_setlen(&newlines, newlines.len - 1);\n-\t\tpreimage.line[preimage.nr - 1].len--;\n-\t\tpostimage.line[postimage.nr - 1].len--;\n+\t\tpreimage.line[preimage.line_nr - 1].len--;\n+\t\tpostimage.line[postimage.line_nr - 1].len--;\n \t}\n \n \tleading = frag->leading;\n@@ -3096,7 +3095,7 @@ static int apply_one_fragment(struct apply_state *state,\n \n \tif (applied_pos >= 0) {\n \t\tif (new_blank_lines_at_end &&\n-\t\t preimage.nr + applied_pos >= img->nr &&\n+\t\t preimage.line_nr + applied_pos >= img->line_nr &&\n \t\t (ws_rule & WS_BLANK_AT_EOF) &&\n \t\t state->ws_error_action != nowarn_ws_error) {\n \t\t\trecord_ws_error(state, WS_BLANK_AT_EOF, \"+\", 1,\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502944","messageId":"42880dcf04a34833f22dd4a1c641f5476b77fda0.1726567217.git.ps@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"[PATCH v2 6/6] apply: refactor `struct image` to use a `struct strbuf`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:08:08Z","receivedAt":"2024-09-17T10:08:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct image` uses a character array to track the pre- or postimage\nof a patch operation. This has multiple downsides:\n\n - It is somewhat hard to track memory ownership. In fact, we have\n several memory leaks in git-apply(1) because we do not (and cannot\n easily) free the buffer in all situations.\n\n - We have to reinvent the wheel and manually implement a lot of\n functionality that would already be provided by `struct strbuf`.\n\n - We have to carefully track whether `update_pre_post_images()` can do\n an in-place update of the postimage or whether it has to allocate a\n new buffer for it.\n\nThis is all rather cumbersome, and especially `update_pre_post_images()`\nis really hard to understand as a consequence even though what it is\ndoing is rather trivial.\n\nRefactor the code to use a `struct strbuf` instead, addressing all of\nthe above. Like this we can easily perform in-place updates in all\nsituations, the logic to perform those updates becomes way simpler and\nthe lifetime of the buffer becomes a ton easier to track.\n\nThis refactoring also plugs some leaking buffers as a side effect.\n\nSigned-off-by: Patrick Steinhardt \n---\n apply.c | 194 +++++++++++------------------\n t/t3436-rebase-more-options.sh | 1 +\n t/t4107-apply-ignore-whitespace.sh | 4 +-\n t/t4124-apply-ws-rule.sh | 1 +\n t/t4125-apply-ws-fuzz.sh | 1 +\n t/t4138-apply-ws-expansion.sh | 1 +\n 6 files changed, 79 insertions(+), 123 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 3340bb1fc0..06a75ea034 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -277,12 +277,13 @@ struct line {\n * This represents a \"file\", which is an array of \"lines\".\n */\n struct image {\n-\tchar *buf;\n-\tsize_t len;\n+\tstruct strbuf buf;\n \tstruct line *line;\n \tsize_t line_nr, line_alloc;\n };\n-#define IMAGE_INIT { 0 }\n+#define IMAGE_INIT { \\\n+\t.buf = STRBUF_INIT, \\\n+}\n \n static void image_init(struct image *image)\n {\n@@ -292,7 +293,7 @@ static void image_init(struct image *image)\n \n static void image_clear(struct image *image)\n {\n-\tfree(image->buf);\n+\tstrbuf_release(&image->buf);\n \tfree(image->line);\n \timage_init(image);\n }\n@@ -329,14 +330,13 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \tconst char *cp, *ep;\n \n \timage_clear(image);\n-\timage->buf = buf;\n-\timage->len = len;\n+\tstrbuf_attach(&image->buf, buf, len, len + 1);\n \n \tif (!prepare_linetable)\n \t\treturn;\n \n-\tep = image->buf + image->len;\n-\tcp = image->buf;\n+\tep = image->buf.buf + image->buf.len;\n+\tcp = image->buf.buf;\n \twhile (cp < ep) {\n \t\tconst char *next;\n \t\tfor (next = cp; next < ep && *next != '\\n'; next++)\n@@ -350,8 +350,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,\n \n static void image_remove_first_line(struct image *img)\n {\n-\timg->buf += img->line[0].len;\n-\timg->len -= img->line[0].len;\n+\tstrbuf_remove(&img->buf, 0, img->line[0].len);\n \timg->line_nr--;\n \tif (img->line_nr)\n \t\tMOVE_ARRAY(img->line, img->line + 1, img->line_nr);\n@@ -359,7 +358,9 @@ static void image_remove_first_line(struct image *img)\n \n static void image_remove_last_line(struct image *img)\n {\n-\timg->len -= img->line[--img->line_nr].len;\n+\tsize_t last_line_len = img->line[img->line_nr - 1].len;\n+\tstrbuf_setlen(&img->buf, img->buf.len - last_line_len);\n+\timg->line_nr--;\n }\n \n /* fmt must contain _one_ %s and no other substitution */\n@@ -2308,19 +2309,16 @@ static int read_old_data(struct stat *st, struct patch *patch,\n \n /*\n * Update the preimage, and the common lines in postimage,\n- * from buffer buf of length len. If postlen is 0 the postimage\n- * is updated in place, otherwise it's updated on a new buffer\n- * of length postlen\n+ * from buffer buf of length len.\n */\n-\n static void update_pre_post_images(struct image *preimage,\n \t\t\t\t struct image *postimage,\n-\t\t\t\t char *buf,\n-\t\t\t\t size_t len, size_t postlen)\n+\t\t\t\t char *buf, size_t len)\n {\n-\tint i, ctx, reduced;\n-\tchar *new_buf, *old_buf, *fixed;\n \tstruct image fixed_preimage = IMAGE_INIT;\n+\tsize_t insert_pos = 0;\n+\tint i, ctx, reduced;\n+\tconst char *fixed;\n \n \t/*\n \t * Update the preimage with whitespace fixes. Note that we\n@@ -2328,43 +2326,24 @@ static void update_pre_post_images(struct image *preimage,\n \t * free \"oldlines\".\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n-\tassert(postlen\n-\t ? fixed_preimage.line_nr == preimage->line_nr\n-\t : fixed_preimage.line_nr <= preimage->line_nr);\n \tfor (i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n-\tfree(preimage->line);\n+\timage_clear(preimage);\n \t*preimage = fixed_preimage;\n+\tfixed = preimage->buf.buf;\n \n \t/*\n-\t * Adjust the common context lines in postimage. This can be\n-\t * done in-place when we are shrinking it with whitespace\n-\t * fixing, but needs a new buffer when ignoring whitespace or\n-\t * expanding leading tabs to spaces.\n-\t *\n-\t * We trust the caller to tell us if the update can be done\n-\t * in place (postlen==0) or not.\n+\t * Adjust the common context lines in postimage.\n \t */\n-\told_buf = postimage->buf;\n-\tif (postlen)\n-\t\tnew_buf = postimage->buf = xmalloc(postlen);\n-\telse\n-\t\tnew_buf = old_buf;\n-\tfixed = preimage->buf;\n-\n \tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n+\n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n \t\t\t/* an added line -- no counterparts in preimage */\n-\t\t\tmemmove(new_buf, old_buf, l_len);\n-\t\t\told_buf += l_len;\n-\t\t\tnew_buf += l_len;\n+\t\t\tinsert_pos += l_len;\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* a common context -- skip it in the original postimage */\n-\t\told_buf += l_len;\n-\n \t\t/* and find the corresponding one in the fixed preimage */\n \t\twhile (ctx < preimage->line_nr &&\n \t\t !(preimage->line[ctx].flag & LINE_COMMON)) {\n@@ -2383,21 +2362,15 @@ static void update_pre_post_images(struct image *preimage,\n \n \t\t/* and copy it in, while fixing the line length */\n \t\tl_len = preimage->line[ctx].len;\n-\t\tmemcpy(new_buf, fixed, l_len);\n-\t\tnew_buf += l_len;\n+\t\tstrbuf_splice(&postimage->buf, insert_pos, postimage->line[i].len,\n+\t\t\t fixed, l_len);\n+\t\tinsert_pos += l_len;\n \t\tfixed += l_len;\n \t\tpostimage->line[i].len = l_len;\n \t\tctx++;\n \t}\n \n-\tif (postlen\n-\t ? postlen < new_buf - postimage->buf\n-\t : postimage->len < new_buf - postimage->buf)\n-\t\tBUG(\"caller miscounted postlen: asked %d, orig = %d, used = %d\",\n-\t\t (int)postlen, (int) postimage->len, (int)(new_buf - postimage->buf));\n-\n \t/* Fix the length of the whole thing */\n-\tpostimage->len = new_buf - postimage->buf;\n \tpostimage->line_nr -= reduced;\n }\n \n@@ -2447,7 +2420,6 @@ static int line_by_line_fuzzy_match(struct image *img,\n \tint i;\n \tsize_t imgoff = 0;\n \tsize_t preoff = 0;\n-\tsize_t postlen = postimage->len;\n \tsize_t extra_chars;\n \tchar *buf;\n \tchar *preimage_eof;\n@@ -2460,11 +2432,9 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\tsize_t prelen = preimage->line[i].len;\n \t\tsize_t imglen = img->line[current_lno+i].len;\n \n-\t\tif (!fuzzy_matchlines(img->buf + current + imgoff, imglen,\n-\t\t\t\t preimage->buf + preoff, prelen))\n+\t\tif (!fuzzy_matchlines(img->buf.buf + current + imgoff, imglen,\n+\t\t\t\t preimage->buf.buf + preoff, prelen))\n \t\t\treturn 0;\n-\t\tif (preimage->line[i].flag & LINE_COMMON)\n-\t\t\tpostlen += imglen - prelen;\n \t\timgoff += imglen;\n \t\tpreoff += prelen;\n \t}\n@@ -2480,10 +2450,10 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t * are whitespace characters. (This can only happen if\n \t * we are removing blank lines at the end of the file.)\n \t */\n-\tbuf = preimage_eof = preimage->buf + preoff;\n+\tbuf = preimage_eof = preimage->buf.buf + preoff;\n \tfor ( ; i < preimage->line_nr; i++)\n \t\tpreoff += preimage->line[i].len;\n-\tpreimage_end = preimage->buf + preoff;\n+\tpreimage_end = preimage->buf.buf + preoff;\n \tfor ( ; buf < preimage_end; buf++)\n \t\tif (!isspace(*buf))\n \t\t\treturn 0;\n@@ -2497,11 +2467,11 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t */\n \textra_chars = preimage_end - preimage_eof;\n \tstrbuf_init(&fixed, imgoff + extra_chars);\n-\tstrbuf_add(&fixed, img->buf + current, imgoff);\n+\tstrbuf_add(&fixed, img->buf.buf + current, imgoff);\n \tstrbuf_add(&fixed, preimage_eof, extra_chars);\n \tfixed_buf = strbuf_detach(&fixed, &fixed_len);\n \tupdate_pre_post_images(preimage, postimage,\n-\t\t\t fixed_buf, fixed_len, postlen);\n+\t\t\t fixed_buf, fixed_len);\n \treturn 1;\n }\n \n@@ -2517,7 +2487,8 @@ static int match_fragment(struct apply_state *state,\n \tint i;\n \tconst char *orig, *target;\n \tstruct strbuf fixed = STRBUF_INIT;\n-\tsize_t postlen;\n+\tchar *fixed_buf;\n+\tsize_t fixed_len;\n \tint preimage_limit;\n \tint ret;\n \n@@ -2573,9 +2544,9 @@ static int match_fragment(struct apply_state *state,\n \t\t * exactly.\n \t\t */\n \t\tif ((match_end\n-\t\t ? (current + preimage->len == img->len)\n-\t\t : (current + preimage->len <= img->len)) &&\n-\t\t !memcmp(img->buf + current, preimage->buf, preimage->len)) {\n+\t\t ? (current + preimage->buf.len == img->buf.len)\n+\t\t : (current + preimage->buf.len <= img->buf.len)) &&\n+\t\t !memcmp(img->buf.buf + current, preimage->buf.buf, preimage->buf.len)) {\n \t\t\tret = 1;\n \t\t\tgoto out;\n \t\t}\n@@ -2589,7 +2560,7 @@ static int match_fragment(struct apply_state *state,\n \t\t */\n \t\tconst char *buf, *buf_end;\n \n-\t\tbuf = preimage->buf;\n+\t\tbuf = preimage->buf.buf;\n \t\tbuf_end = buf;\n \t\tfor (i = 0; i < preimage_limit; i++)\n \t\t\tbuf_end += preimage->line[i].len;\n@@ -2634,21 +2605,14 @@ static int match_fragment(struct apply_state *state,\n \t * fixed.\n \t */\n \n-\t/* First count added lines in postimage */\n-\tpostlen = 0;\n-\tfor (i = 0; i < postimage->line_nr; i++) {\n-\t\tif (!(postimage->line[i].flag & LINE_COMMON))\n-\t\t\tpostlen += postimage->line[i].len;\n-\t}\n-\n \t/*\n \t * The preimage may extend beyond the end of the file,\n \t * but in this loop we will only handle the part of the\n \t * preimage that falls within the file.\n \t */\n-\tstrbuf_grow(&fixed, preimage->len + 1);\n-\torig = preimage->buf;\n-\ttarget = img->buf + current;\n+\tstrbuf_grow(&fixed, preimage->buf.len + 1);\n+\torig = preimage->buf.buf;\n+\ttarget = img->buf.buf + current;\n \tfor (i = 0; i < preimage_limit; i++) {\n \t\tsize_t oldlen = preimage->line[i].len;\n \t\tsize_t tgtlen = img->line[current_lno + i].len;\n@@ -2677,10 +2641,6 @@ static int match_fragment(struct apply_state *state,\n \t\t\t !memcmp(tgtfix.buf, fixed.buf + fixstart,\n \t\t\t\t\t fixed.len - fixstart));\n \n-\t\t/* Add the length if this is common with the postimage */\n-\t\tif (preimage->line[i].flag & LINE_COMMON)\n-\t\t\tpostlen += tgtfix.len;\n-\n \t\tstrbuf_release(&tgtfix);\n \t\tif (!match) {\n \t\t\tret = 0;\n@@ -2722,10 +2682,9 @@ static int match_fragment(struct apply_state *state,\n \t * has whitespace breakages unfixed, and fixing them makes the\n \t * hunk match. Update the context lines in the postimage.\n \t */\n-\tif (postlen < postimage->len)\n-\t\tpostlen = 0;\n+\tfixed_buf = strbuf_detach(&fixed, &fixed_len);\n \tupdate_pre_post_images(preimage, postimage,\n-\t\t\t fixed.buf, fixed.len, postlen);\n+\t\t\t fixed_buf, fixed_len);\n \n \tret = 1;\n \n@@ -2839,6 +2798,7 @@ static void update_image(struct apply_state *state,\n \t */\n \tint i, nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n+\tsize_t result_alloc;\n \tchar *result;\n \tint preimage_limit;\n \n@@ -2861,19 +2821,18 @@ static void update_image(struct apply_state *state,\n \tremove_count = 0;\n \tfor (i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n-\tinsert_count = postimage->len;\n+\tinsert_count = postimage->buf.len;\n \n \t/* Adjust the contents */\n-\tresult = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 1));\n-\tmemcpy(result, img->buf, applied_at);\n-\tmemcpy(result + applied_at, postimage->buf, postimage->len);\n-\tmemcpy(result + applied_at + postimage->len,\n-\t img->buf + (applied_at + remove_count),\n-\t img->len - (applied_at + remove_count));\n-\tfree(img->buf);\n-\timg->buf = result;\n-\timg->len += insert_count - remove_count;\n-\tresult[img->len] = '\\0';\n+\tresult_alloc = st_add3(st_sub(img->buf.len, remove_count), insert_count, 1);\n+\tresult = xmalloc(result_alloc);\n+\tmemcpy(result, img->buf.buf, applied_at);\n+\tmemcpy(result + applied_at, postimage->buf.buf, postimage->buf.len);\n+\tmemcpy(result + applied_at + postimage->buf.len,\n+\t img->buf.buf + (applied_at + remove_count),\n+\t img->buf.len - (applied_at + remove_count));\n+\tstrbuf_attach(&img->buf, result, postimage->buf.len + img->buf.len - remove_count,\n+\t\t result_alloc);\n \n \t/* Adjust the line table */\n \tnr = img->line_nr + postimage->line_nr - preimage_limit;\n@@ -3054,10 +3013,8 @@ static int apply_one_fragment(struct apply_state *state,\n \tmatch_end = !state->unidiff_zero && !trailing;\n \n \tpos = frag->newpos ? (frag->newpos - 1) : 0;\n-\tpreimage.buf = oldlines;\n-\tpreimage.len = old - oldlines;\n-\tpostimage.buf = newlines.buf;\n-\tpostimage.len = newlines.len;\n+\tstrbuf_add(&preimage.buf, oldlines, old - oldlines);\n+\tstrbuf_swap(&postimage.buf, &newlines);\n \n \tfor (;;) {\n \n@@ -3145,8 +3102,8 @@ static int apply_one_fragment(struct apply_state *state,\n out:\n \tfree(oldlines);\n \tstrbuf_release(&newlines);\n-\tfree(preimage.line);\n-\tfree(postimage.line);\n+\timage_clear(&preimage);\n+\timage_clear(&postimage);\n \n \treturn (applied_pos < 0);\n }\n@@ -3176,18 +3133,16 @@ static int apply_binary_fragment(struct apply_state *state,\n \t}\n \tswitch (fragment->binary_patch_method) {\n \tcase BINARY_DELTA_DEFLATED:\n-\t\tdst = patch_delta(img->buf, img->len, fragment->patch,\n+\t\tdst = patch_delta(img->buf.buf, img->buf.len, fragment->patch,\n \t\t\t\t fragment->size, &len);\n \t\tif (!dst)\n \t\t\treturn -1;\n \t\timage_clear(img);\n-\t\timg->buf = dst;\n-\t\timg->len = len;\n+\t\tstrbuf_attach(&img->buf, dst, len, len + 1);\n \t\treturn 0;\n \tcase BINARY_LITERAL_DEFLATED:\n \t\timage_clear(img);\n-\t\timg->len = fragment->size;\n-\t\timg->buf = xmemdupz(fragment->patch, img->len);\n+\t\tstrbuf_add(&img->buf, fragment->patch, fragment->size);\n \t\treturn 0;\n \t}\n \treturn -1;\n@@ -3223,8 +3178,8 @@ static int apply_binary(struct apply_state *state,\n \t\t * See if the old one matches what the patch\n \t\t * applies to.\n \t\t */\n-\t\thash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,\n-\t\t\t\t &oid);\n+\t\thash_object_file(the_hash_algo, img->buf.buf, img->buf.len,\n+\t\t\t\t OBJ_BLOB, &oid);\n \t\tif (strcmp(oid_to_hex(&oid), patch->old_oid_prefix))\n \t\t\treturn error(_(\"the patch applies to '%s' (%s), \"\n \t\t\t\t \"which does not match the \"\n@@ -3233,7 +3188,7 @@ static int apply_binary(struct apply_state *state,\n \t}\n \telse {\n \t\t/* Otherwise, the old one must be empty. */\n-\t\tif (img->len)\n+\t\tif (img->buf.len)\n \t\t\treturn error(_(\"the patch applies to an empty \"\n \t\t\t\t \"'%s' but it is not empty\"), name);\n \t}\n@@ -3257,8 +3212,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\t\t \"'%s' cannot be read\"),\n \t\t\t\t patch->new_oid_prefix, name);\n \t\timage_clear(img);\n-\t\timg->buf = result;\n-\t\timg->len = size;\n+\t\tstrbuf_attach(&img->buf, result, size, size + 1);\n \t} else {\n \t\t/*\n \t\t * We have verified buf matches the preimage;\n@@ -3270,7 +3224,7 @@ static int apply_binary(struct apply_state *state,\n \t\t\t\t name);\n \n \t\t/* verify that the result matches */\n-\t\thash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,\n+\t\thash_object_file(the_hash_algo, img->buf.buf, img->buf.len, OBJ_BLOB,\n \t\t\t\t &oid);\n \t\tif (strcmp(oid_to_hex(&oid), patch->new_oid_prefix))\n \t\t\treturn error(_(\"binary patch to '%s' creates incorrect result (expecting %s, got %s)\"),\n@@ -3540,14 +3494,14 @@ static int resolve_to(struct image *image, const struct object_id *result_id)\n {\n \tunsigned long size;\n \tenum object_type type;\n+\tchar *data;\n \n \timage_clear(image);\n \n-\timage->buf = repo_read_object_file(the_repository, result_id, &type,\n-\t\t\t\t\t &size);\n-\tif (!image->buf || type != OBJ_BLOB)\n+\tdata = repo_read_object_file(the_repository, result_id, &type, &size);\n+\tif (!data || type != OBJ_BLOB)\n \t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n-\timage->len = size;\n+\tstrbuf_attach(&image->buf, data, size, size + 1);\n \n \treturn 0;\n }\n@@ -3589,8 +3543,7 @@ static int three_way_merge(struct apply_state *state,\n \t\treturn -1;\n \t}\n \timage_clear(image);\n-\timage->buf = result.ptr;\n-\timage->len = result.size;\n+\tstrbuf_attach(&image->buf, result.ptr, result.size, result.size);\n \n \treturn status;\n }\n@@ -3677,7 +3630,7 @@ static int try_threeway(struct apply_state *state,\n \t\treturn -1;\n \t}\n \t/* post_oid is theirs */\n-\twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);\n+\twrite_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &post_oid);\n \timage_clear(&tmp_image);\n \n \t/* our_oid is ours */\n@@ -3690,7 +3643,7 @@ static int try_threeway(struct apply_state *state,\n \t\t\treturn error(_(\"cannot read the current contents of '%s'\"),\n \t\t\t\t patch->old_name);\n \t}\n-\twrite_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);\n+\twrite_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &our_oid);\n \timage_clear(&tmp_image);\n \n \t/* in-core three-way merge between post and our using pre as base */\n@@ -3743,8 +3696,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tpatch->result = image.buf;\n-\tpatch->resultsize = image.len;\n+\tpatch->result = strbuf_detach(&image.buf, &patch->resultsize);\n \tadd_to_fn_table(state, patch);\n \tfree(image.line);\n \ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 94671d3c46..4d9744e5fc 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -5,6 +5,7 @@\n \n test_description='tests to ensure compatibility between am and interactive backends'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n . \"$TEST_DIRECTORY\"/lib-rebase.sh\ndiff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh\nindex ac72eeaf27..5e6e203aa5 100755\n--- a/t/t4107-apply-ignore-whitespace.sh\n+++ b/t/t4107-apply-ignore-whitespace.sh\n@@ -3,9 +3,9 @@\n # Copyright (c) 2009 Giuseppe Bilotta\n #\n \n-test_description='git-apply --ignore-whitespace.\n+test_description='git-apply --ignore-whitespace.'\n \n-'\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # This primes main.c file that indents without using HT at all.\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 485c7d2d12..cdffee0273 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -2,6 +2,7 @@\n \n test_description='core.whitespace rules and git apply'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n prepare_test_file () {\ndiff --git a/t/t4125-apply-ws-fuzz.sh b/t/t4125-apply-ws-fuzz.sh\nindex 090987c89b..f248cc2a00 100755\n--- a/t/t4125-apply-ws-fuzz.sh\n+++ b/t/t4125-apply-ws-fuzz.sh\n@@ -2,6 +2,7 @@\n \n test_description='applying patch that has broken whitespaces in context'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t4138-apply-ws-expansion.sh b/t/t4138-apply-ws-expansion.sh\nindex 8bbf8260fa..7981931b4e 100755\n--- a/t/t4138-apply-ws-expansion.sh\n+++ b/t/t4138-apply-ws-expansion.sh\n@@ -5,6 +5,7 @@\n \n test_description='git apply test patches with whitespace expansion.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502945","messageId":"ZulVL7oBf2eFRWwu@pks.im","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"Re: [PATCH v2 0/6] apply: fix leaking buffer of `struct image`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T10:08:47Z","receivedAt":"2024-09-17T10:08:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 12:07:48PM +0200, Patrick Steinhardt wrote:\n> Hi,\n> \n> this is the second version of my patch series that refactors lifecycle\n> management of `struct image` in \"apply.c\" to plug a bunch of memory\n> leaks.\n> \n> Changes compared to v1:\n> \n> - Fix two typos.\n> \n> - Correct the statement that we don't loop around\n> `image_remove_first_line()`. No idea how I missed that loop.\n> \n> - Split up an overly complex line into multiple lines.\n> \n> Thanks!\n\nUgh, sorry. I meant to Cc Junio, not Taylor.\n\nPatrick\n"},{"id":"502969","messageId":"xmqqy13qj8wk.fsf@gitster.g","threadId":"62119","inReplyTo":"cover.1726567217.git.ps@pks.im","subject":"Re: [PATCH v2 0/6] apply: fix leaking buffer of `struct image`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-17T20:57:15Z","receivedAt":"2024-09-17T20:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is the second version of my patch series that refactors lifecycle\n> management of `struct image` in \"apply.c\" to plug a bunch of memory\n> leaks.\n>\n> Changes compared to v1:\n> \n> - Fix two typos.\n>\n> - Correct the statement that we don't loop around\n> `image_remove_first_line()`. No idea how I missed that loop.\n>\n> - Split up an overly complex line into multiple lines.\n\nLooking good. Will queue. Thanks.\n"}]}