{"thread":{"id":"58717","subject":"Bug report: git -L requires excessive memory.","startedAt":"2022-10-29T17:00:27Z","lastAt":"2022-11-08T02:14:55Z","messageCount":17,"participants":["man dog","SZEDER Gábor","Taylor Blau","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"466023","messageId":"CAFOPqVXz2XwzX8vGU7wLuqb2ZuwTuOFAzBLRM_QPk+NJa=eC-g@mail.gmail.com","threadId":"58717","inReplyTo":null,"subject":"Bug report: git -L requires excessive memory.","fromName":"man dog","fromEmail":"dogman888888@gmail.com","sentAt":"2022-10-29T16:59:41Z","receivedAt":"2022-10-29T17:00:27Z","isPatch":false,"sender":{"key":"dogman888888@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\ngit log -L /regex/,/regex/:myfile to a repo in which 2MB text file is\ncommitted about 2800 times.\n\nWhat did you expect to happen? (Expected behavior)\nget the result.\n\nWhat happened instead? (Actual behavior)\nfatal: Out of memory, malloc failed (tried to allocate 2346801 bytes)\n\nWhat's different between what you expected and what actually happened?\nThe function requires too much memory.\n-n option should work for -L function.\n\nAnything else you want to add:\nI made a script to reproduce this. Please run the script below.\nResults in each environments are in its header.\nA workaround which is given in other BBS is included also.\n\n\n\n\n#!/bin/bash\n#\n# Bug report: git -L requires excessive memory.\n# Run this script to reproduce\n#\n# MINGW32(git version 2.38.1.windows.1) fatal: Out of memory, malloc\nfailed (tried to allocate 2346801 bytes)\n# MINGW64(git version 2.38.1.windows.1) requires  8.6GB\n# Linux64(git version 2.20.1          ) requires 13.1GB\n#\n\ngit --version\n\nif [ ! -d .git ]; then\n  git init\n  c=${1:-3000}\n  for (( i=0;i<c;i++)); do\n    gawk -v r=\"$i\" '\n      BEGIN{\n        for (i=0;i<100;i++) {\n          if (r>=i) {\n            printf(\"function func_%03d(){ // revised at %d\\n\",i,\nint((r-i)/100)*100+i%100)\n            printf(\"  // contents of function\\n\")\n            printf(\"}\\n\")\n            make_subfuncs(i);\n          }\n        }\n        exit\n      }\n      function make_subfuncs(i,    j){\n        for (j=0;j<300;j++) {\n          printf(\"function func_%03d_sub%03d(){\\n\",i,j)\n          printf(\"  // contents of sub functions are NOT revised.\\n\")\n          printf(\"}\\n\")\n        }\n      }' > test.txt\n    git add test.txt\n    git commit -m \"revision $i\"\n  done\n  git gc\nfi\n\ngit log -L /func_007\\(/,/}$/:test.txt # this command requires excessive memory.\ngit log -L /func_007\\(/,/}$/:test.txt -n 10 # -n option doesn't work also.\n#git log -L /func_007\\(/,/}$/:test.txt HEAD~10..HEAD~0 # this works.\n\n#\n# This can be a workaround\n#\nstep=50\nnum=`git log | grep -c commit`\nfor ((i=0;i<$num;i+=$step)); do\n  end=$((i+$step))\n  range=HEAD~$end..HEAD~$i\n  if [ $end -ge $num ]; then\n    range=HEAD~$i\n  fi\n#  echo $range\n  git --no-pager log -L /func_007\\(/,/}$/:test.txt $range\ndone\n\n\n\n\n[System Info]\n[Enabled Hooks]\n"},{"id":"466121","messageId":"20221031214554.GA1714@szeder.dev","threadId":"58717","inReplyTo":"CAFOPqVXz2XwzX8vGU7wLuqb2ZuwTuOFAzBLRM_QPk+NJa=eC-g@mail.gmail.com","subject":"Re: Bug report: git -L requires excessive memory.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-10-31T21:45:54Z","receivedAt":"2022-10-31T21:46:21Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Oct 30, 2022 at 01:59:41AM +0900, man dog wrote:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n> \n> What did you do before the bug happened? (Steps to reproduce your issue)\n> git log -L /regex/,/regex/:myfile to a repo in which 2MB text file is\n> committed about 2800 times.\n> \n> What did you expect to happen? (Expected behavior)\n> get the result.\n> \n> What happened instead? (Actual behavior)\n> fatal: Out of memory, malloc failed (tried to allocate 2346801 bytes)\n\nThanks for the report and the reproduction recipe.\n\nThis is not a buggy allocation (the size matches the size of the test\nfile + 1 byte), but the line-level log apparently leaks some memory\nfor each commit modifying the file in question, and in your case their\ncombined size is excessive because of the somewhat big file that is\nmodified in every commit.\n\n'line-log.c' contains two \"NEEDSWORK leaking like a sieve\" comments,\nbut you managed to stumble upon yet another case (those two are in the\ncode path handling merge commits, but your history is linear).\n\nThe patch below plugs this leak.\n\n  ---  >8  ---\n\ndiff --git a/line-log.c b/line-log.c\nindex 51d93310a4..b6ea82ac6b 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1195,6 +1195,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \tif (parent)\n \t\tadd_line_range(rev, parent, parent_range);\n \tfree_line_log_data(parent_range);\n+\tfor (int i = 0; i < queue.nr; i++)\n+\t\tdiff_free_filepair(queue.queue[i]);\n+\tfree(queue.queue);\n \treturn changed;\n }\n \n  ---  8<  ---\n\n> What's different between what you expected and what actually happened?\n> The function requires too much memory.\n> -n option should work for -L function.\n\nLine-level log does work with '-n', but the file is so big and is\nmodified so many times between commits that do touch the specified\nline range, that by the time it gets to the 10th commit to show it has\nalready leaked over 4GB memory.  Had you specified an even smaller\nnumber of commits to show it might have worked:\n\n  $ for i in {1..7} ; do /usr/bin/time -f \"n: $i  maxRSS: %M\" git log -L /func_007\\(/,/}$/:test.txt -n $i >/dev/null || break ; done\n  n: 1  maxRSS: 531192\n  n: 2  maxRSS: 989504\n  n: 3  maxRSS: 1447900\n  n: 4  maxRSS: 1906408\n  n: 5  maxRSS: 2364740\n  n: 6  maxRSS: 2823148\n  n: 7  maxRSS: 3282360\n\nIn you reproduction recipe the given line range is modified every 100\ncommit and there are 3000 commits in total, so I estimate the total\nmemory usage to be somewhere around 13.5GB.  With the patch above it\ntops out at around 260MB.\n\n\n> Anything else you want to add:\n> I made a script to reproduce this. Please run the script below.\n> Results in each environments are in its header.\n> A workaround which is given in other BBS is included also.\n> \n> \n> \n> \n> #!/bin/bash\n> #\n> # Bug report: git -L requires excessive memory.\n> # Run this script to reproduce\n> #\n> # MINGW32(git version 2.38.1.windows.1) fatal: Out of memory, malloc\n> failed (tried to allocate 2346801 bytes)\n> # MINGW64(git version 2.38.1.windows.1) requires  8.6GB\n> # Linux64(git version 2.20.1          ) requires 13.1GB\n> #\n> \n> git --version\n> \n> if [ ! -d .git ]; then\n>   git init\n>   c=${1:-3000}\n>   for (( i=0;i<c;i++)); do\n>     gawk -v r=\"$i\" '\n>       BEGIN{\n>         for (i=0;i<100;i++) {\n>           if (r>=i) {\n>             printf(\"function func_%03d(){ // revised at %d\\n\",i,\n> int((r-i)/100)*100+i%100)\n>             printf(\"  // contents of function\\n\")\n>             printf(\"}\\n\")\n>             make_subfuncs(i);\n>           }\n>         }\n>         exit\n>       }\n>       function make_subfuncs(i,    j){\n>         for (j=0;j<300;j++) {\n>           printf(\"function func_%03d_sub%03d(){\\n\",i,j)\n>           printf(\"  // contents of sub functions are NOT revised.\\n\")\n>           printf(\"}\\n\")\n>         }\n>       }' > test.txt\n>     git add test.txt\n>     git commit -m \"revision $i\"\n>   done\n>   git gc\n> fi\n> \n> git log -L /func_007\\(/,/}$/:test.txt # this command requires excessive memory.\n> git log -L /func_007\\(/,/}$/:test.txt -n 10 # -n option doesn't work also.\n> #git log -L /func_007\\(/,/}$/:test.txt HEAD~10..HEAD~0 # this works.\n\nPerhaps I misunderstood, but I got the impression that you think that\n'HEAD~10..HEAD~0' and '-n 10' do the same.\n\nThey are not: 'HEAD~10..HEAD~0' means to process only the last ten\ncommits, so it can't leak all that much, and that's why it worked.\n'-n 10', however, means to _show_ only ten commits, but process as\nmany commits as necessary to find those ten.  In your case, with the\nline range being modified every 100 commit, that amounts to processing\nover 1000 commits.\n\n> #\n> # This can be a workaround\n> #\n> step=50\n> num=`git log | grep -c commit`\n> for ((i=0;i<$num;i+=$step)); do\n>   end=$((i+$step))\n>   range=HEAD~$end..HEAD~$i\n>   if [ $end -ge $num ]; then\n>     range=HEAD~$i\n>   fi\n> #  echo $range\n>   git --no-pager log -L /func_007\\(/,/}$/:test.txt $range\n> done\n> \n> \n> \n> \n> [System Info]\n> [Enabled Hooks]\n"},{"id":"466122","messageId":"Y2BElOFGJ8JinYxC@nand.local","threadId":"58717","inReplyTo":"20221031214554.GA1714@szeder.dev","subject":"Re: Bug report: git -L requires excessive memory.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-31T21:56:36Z","receivedAt":"2022-10-31T21:56:42Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 10:45:54PM +0100, SZEDER Gábor wrote:\n> 'line-log.c' contains two \"NEEDSWORK leaking like a sieve\" comments,\n> but you managed to stumble upon yet another case (those two are in the\n> code path handling merge commits, but your history is linear).\n\n;-).\n\n> The patch below plugs this leak.\n>\n>   ---  >8  ---\n>\n> diff --git a/line-log.c b/line-log.c\n> index 51d93310a4..b6ea82ac6b 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -1195,6 +1195,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n>  \tif (parent)\n>  \t\tadd_line_range(rev, parent, parent_range);\n>  \tfree_line_log_data(parent_range);\n> +\tfor (int i = 0; i < queue.nr; i++)\n> +\t\tdiff_free_filepair(queue.queue[i]);\n> +\tfree(queue.queue);\n>  \treturn changed;\n>  }\n>\n>   ---  8<  ---\n\nVery nice. I wouldn't be surprised if there are other leaks, in the\nline-log code, but fixing one is good regardless. If you resend with\nyour S-o-b, I'd be happy to queue it.\n\nThanks,\nTaylor\n"},{"id":"466333","messageId":"20221102220142.574890-1-szeder.dev@gmail.com","threadId":"58717","inReplyTo":"Y2BElOFGJ8JinYxC@nand.local","subject":"[PATCH 0/3] line-log: plug some memory leaks","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-02T22:01:39Z","receivedAt":"2022-11-02T22:01:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The first patch plugs the reported big memory leak, the second one\nplugs a minor leak, and the little cleanup in the third puts the\ncherry on top.\n\nSZEDER Gábor (3):\n  line-log: free diff queue when processing non-merge commits\n  line-log: free the diff queues' arrays when processing merge commits\n  diff.c: use diff_free_queue()\n\n diff.c     | 17 +++++++++--------\n diffcore.h |  1 +\n line-log.c |  7 +++----\n 3 files changed, 13 insertions(+), 12 deletions(-)\n\n-- \n2.38.1.564.g99c012faba\n\n"},{"id":"466334","messageId":"20221102220142.574890-2-szeder.dev@gmail.com","threadId":"58717","inReplyTo":"20221102220142.574890-1-szeder.dev@gmail.com","subject":"[PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-02T22:01:40Z","receivedAt":"2022-11-02T22:01:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When processing a non-merge commit, the line-level log first asks the\ntree-diff machinery whether any of the files in the given line ranges\nwere modified between the current commit and its parent, and if some\nof them were, then it loads the contents of those files from both\ncommits to see whether their line ranges were modified and/or need to\nbe adjusted.  Alas, it doesn't free() the diff queue holding the\nresults of that query and the contents of those files once its done.\nThis can add up to a substantial amount of leaked memory, especially\nwhen the file in question is big and is frequently modified: a user\nreported \"Out of memory, malloc failed\" errors with a 2MB text file\nthat was modified ~2800 times [1] (I estimate the leak would use up\nalmost 11GB memory in that case).\n\nFree that diff queue to plug this memory leak.  However, instead of\nsimply open-coding the necessary three lines, add them as a helper\nfunction to the diff API, because it will be useful elsewhere as well.\n\n[1] https://public-inbox.org/git/CAFOPqVXz2XwzX8vGU7wLuqb2ZuwTuOFAzBLRM_QPk+NJa=eC-g@mail.gmail.com/\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n diff.c     | 7 +++++++\n diffcore.h | 1 +\n line-log.c | 1 +\n 3 files changed, 9 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 35e46dd968..ef94175163 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5773,6 +5773,13 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n+void diff_free_queue(struct diff_queue_struct *q)\n+{\n+\tfor (int i = 0; i < q->nr; i++)\n+\t\tdiff_free_filepair(q->queue[i]);\n+\tfree(q->queue);\n+}\n+\n const char *diff_aligned_abbrev(const struct object_id *oid, int len)\n {\n \tint abblen;\ndiff --git a/diffcore.h b/diffcore.h\nindex badc2261c2..9b588a1ee1 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -162,6 +162,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t struct diff_filespec *,\n \t\t\t\t struct diff_filespec *);\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n+void diff_free_queue(struct diff_queue_struct *q);\n \n /* dir_rename_relevance: the reason we want rename information for a dir */\n enum dir_rename_relevance {\ndiff --git a/line-log.c b/line-log.c\nindex 51d93310a4..7a74daf2e8 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1195,6 +1195,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \tif (parent)\n \t\tadd_line_range(rev, parent, parent_range);\n \tfree_line_log_data(parent_range);\n+\tdiff_free_queue(&queue);\n \treturn changed;\n }\n \n-- \n2.38.1.564.g99c012faba\n\n"},{"id":"466335","messageId":"20221102220142.574890-3-szeder.dev@gmail.com","threadId":"58717","inReplyTo":"20221102220142.574890-1-szeder.dev@gmail.com","subject":"[PATCH 2/3] line-log: free the diff queues' arrays when processing merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-02T22:01:41Z","receivedAt":"2022-11-02T22:01:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When processing merge commits, the line-level log first creates an\narray of diff queues, each comparing the merge commit with one of its\nparents, to check whether any of the files in the given line ranges\nwere modified.  Alas, when freeing these queues it only frees the\nfilepairs in the queues, but not the queues' internal arrays holding\npointers to those filepairs.\n\nUse the diff_free_queue() helper function introduced in the previous\ncommit to free the diff queues' internal arrays as well.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n line-log.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 7a74daf2e8..a7f3e7f6ce 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1089,10 +1089,8 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)\n \n static void free_diffqueues(int n, struct diff_queue_struct *dq)\n {\n-\tint i, j;\n-\tfor (i = 0; i < n; i++)\n-\t\tfor (j = 0; j < dq[i].nr; j++)\n-\t\t\tdiff_free_filepair(dq[i].queue[j]);\n+\tfor (int i = 0; i < n; i++)\n+\t\tdiff_free_queue(&dq[i]);\n \tfree(dq);\n }\n \n-- \n2.38.1.564.g99c012faba\n\n"},{"id":"466336","messageId":"20221102220142.574890-4-szeder.dev@gmail.com","threadId":"58717","inReplyTo":"20221102220142.574890-1-szeder.dev@gmail.com","subject":"[PATCH 3/3] diff.c: use diff_free_queue()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-02T22:01:42Z","receivedAt":"2022-11-02T22:01:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Use diff_free_queue() instead of open-coding it.  This shortens the\ncode and make it less repetitive.\n\nNote that the second hunk in diff_flush() is interesting, because the\n'free_queue' label separates the loop freeing the queue's filepairs\nfrom free()-ing the queue's internal array.  This is somewhat\nsuspicious, but it was not an issue before: there is only one place\nfrom where we jump to this label with a goto, and that is protected by\nan 'if (!q->nr && ...)' condition, i.e. we only skipped the loop\nfreeing the filepairs when there were no filepairs in the queue to\nbegin with.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n diff.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ef94175163..03e6ffb5e4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6337,13 +6337,9 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint i;\n \tint result = diff_get_patch_id(options, oid, diff_header_only);\n \n-\tfor (i = 0; i < q->nr; i++)\n-\t\tdiff_free_filepair(q->queue[i]);\n-\n-\tfree(q->queue);\n+\tdiff_free_queue(q);\n \tDIFF_QUEUE_CLEAR(q);\n \n \treturn result;\n@@ -6612,10 +6608,8 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_CALLBACK)\n \t\toptions->format_callback(q, options, options->format_callback_data);\n \n-\tfor (i = 0; i < q->nr; i++)\n-\t\tdiff_free_filepair(q->queue[i]);\n free_queue:\n-\tfree(q->queue);\n+\tdiff_free_queue(q);\n \tDIFF_QUEUE_CLEAR(q);\n \tdiff_free(options);\n \n-- \n2.38.1.564.g99c012faba\n\n"},{"id":"466360","messageId":"Y2MJRRfwG7rSp6Ra@nand.local","threadId":"58717","inReplyTo":"20221102220142.574890-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T00:20:21Z","receivedAt":"2022-11-03T00:20:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 11:01:40PM +0100, SZEDER Gábor wrote:\n> When processing a non-merge commit, the line-level log first asks the\n> tree-diff machinery whether any of the files in the given line ranges\n> were modified between the current commit and its parent, and if some\n> of them were, then it loads the contents of those files from both\n> commits to see whether their line ranges were modified and/or need to\n> be adjusted.  Alas, it doesn't free() the diff queue holding the\n> results of that query and the contents of those files once its done.\n> This can add up to a substantial amount of leaked memory, especially\n> when the file in question is big and is frequently modified: a user\n> reported \"Out of memory, malloc failed\" errors with a 2MB text file\n> that was modified ~2800 times [1] (I estimate the leak would use up\n> almost 11GB memory in that case).\n>\n> Free that diff queue to plug this memory leak.  However, instead of\n> simply open-coding the necessary three lines, add them as a helper\n> function to the diff API, because it will be useful elsewhere as well.\n\nNicely explained.\n\n> ---\n>  diff.c     | 7 +++++++\n>  diffcore.h | 1 +\n>  line-log.c | 1 +\n>  3 files changed, 9 insertions(+)\n\nAnd all looks reasonable here, good...\n\n> diff --git a/diff.c b/diff.c\n> index 35e46dd968..ef94175163 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5773,6 +5773,13 @@ void diff_free_filepair(struct diff_filepair *p)\n>  \tfree(p);\n>  }\n>\n> +void diff_free_queue(struct diff_queue_struct *q)\n> +{\n> +\tfor (int i = 0; i < q->nr; i++)\n> +\t\tdiff_free_filepair(q->queue[i]);\n> +\tfree(q->queue);\n> +}\n\nThough I wonder, should diff_free_queue() be a noop when q is NULL? The\ncaller in process_ranges_ordinary_commit() doesn't care, of course,\nsince q is always non-NULL there.\n\nBut if we're making it part of the diff API, we should probably err on\nthe side of flexibility.\n\nThanks,\nTaylor\n"},{"id":"466361","messageId":"Y2MJc6wIQIlK44a1@nand.local","threadId":"58717","inReplyTo":"20221102220142.574890-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] line-log: free the diff queues' arrays when processing merge commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T00:21:07Z","receivedAt":"2022-11-03T00:21:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 11:01:41PM +0100, SZEDER Gábor wrote:\n> ---\n>  line-log.c | 6 ++----\n>  1 file changed, 2 insertions(+), 4 deletions(-)\n\nLooking good.\n\nThanks,\nTaylor\n"},{"id":"466363","messageId":"Y2MKKTz4nK0L8uW5@nand.local","threadId":"58717","inReplyTo":"20221102220142.574890-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] diff.c: use diff_free_queue()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T00:24:09Z","receivedAt":"2022-11-03T00:24:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:\n> Use diff_free_queue() instead of open-coding it.  This shortens the\n> code and make it less repetitive.\n>\n> Note that the second hunk in diff_flush() is interesting, because the\n> 'free_queue' label separates the loop freeing the queue's filepairs\n> from free()-ing the queue's internal array.  This is somewhat\n> suspicious, but it was not an issue before: there is only one place\n> from where we jump to this label with a goto, and that is protected by\n> an 'if (!q->nr && ...)' condition, i.e. we only skipped the loop\n> freeing the filepairs when there were no filepairs in the queue to\n> begin with.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  diff.c | 10 ++--------\n>  1 file changed, 2 insertions(+), 8 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index ef94175163..03e6ffb5e4 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6337,13 +6337,9 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n>  int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n>  {\n>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n> -\tint i;\n>  \tint result = diff_get_patch_id(options, oid, diff_header_only);\n>\n> -\tfor (i = 0; i < q->nr; i++)\n> -\t\tdiff_free_filepair(q->queue[i]);\n> -\n> -\tfree(q->queue);\n> +\tdiff_free_queue(q);\n>  \tDIFF_QUEUE_CLEAR(q);\n\nSo, this all looks fine to me. But I did a quick grep around for\nDIFF_QUEUE_CLEAR(), and this macro is used in quite a few places.\nMostly, as far as I can tell, to \"empty\" out the diff-queue by setting\nits 'queue' pointer to NULL, and its 'nr' back to 0.\n\nShould we be freeing the memory held by the queue there more\naggressively? I.e., should we make sure that there is a\ndiff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR()\nmacro?\n\nThanks,\nTaylor\n"},{"id":"466381","messageId":"221103.864jvg2yit.gmgdl@evledraar.gmail.com","threadId":"58717","inReplyTo":"20221102220142.574890-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/3] line-log: plug some memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-03T09:05:18Z","receivedAt":"2022-11-03T09:11:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 02 2022, SZEDER Gábor wrote:\n\n> The first patch plugs the reported big memory leak, the second one\n> plugs a minor leak, and the little cleanup in the third puts the\n> cherry on top.\n\nLooks good as far as it goes.\n\nThe \"further\" part seems a real mess though, e.g. I came up with the\nbelow on top as a quick test, i.e. we have other existing users that\nalso loop over the same struct, and free \"queue[n]\", but do so\ndifferently.\n\nI handled the combine-diff.c one (rough WIP, only compiled it), but then\ndiffcore-rename.c has another such case.\n\nI wonder if you looked at that further, and if the free function we're\nadding now should anticipate that case or not.\n\nAnd, orthagonally I came up with this rough WIP yesterday:\n\t\n\t@@ -6640,9 +6633,7 @@ static void diffcore_apply_filter(struct diff_options *options)\n\t {\n\t \tint i;\n\t \tstruct diff_queue_struct *q = &diff_queued_diff;\n\t-\tstruct diff_queue_struct outq;\n\t-\n\t-\tDIFF_QUEUE_CLEAR(&outq);\n\t+\tstruct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;\n\t \n\t \tif (!options->filter)\n\t \t\treturn;\n\t@@ -6735,8 +6726,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n\t {\n\t \tint i;\n\t \tstruct diff_queue_struct *q = &diff_queued_diff;\n\t-\tstruct diff_queue_struct outq;\n\t-\tDIFF_QUEUE_CLEAR(&outq);\n\t+\tstruct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;\n\t \n\t \tfor (i = 0; i < q->nr; i++) {\n\t \t\tstruct diff_filepair *p = q->queue[i];\n\tdiff --git a/diffcore.h b/diffcore.h\n\tindex badc2261c20..a0a89568cec 100644\n\t--- a/diffcore.h\n\t+++ b/diffcore.h\n\t@@ -150,12 +150,7 @@ struct diff_queue_struct {\n\t \tint alloc;\n\t \tint nr;\n\t };\n\t-\n\t-#define DIFF_QUEUE_CLEAR(q) \\\n\t-\tdo { \\\n\t-\t\t(q)->queue = NULL; \\\n\t-\t\t(q)->nr = (q)->alloc = 0; \\\n\t-\t} while (0)\n\t+\t#define DIFF_QUEUE_STRUCT_INIT { 0 }\n\nYou leave the DIFF_QUEUE_CLEAR in place, but I wonder given that that's\nthe common pattern whether you shouldn't have a *_reset() and\n*_{free,release}() (one of the two, your current naming is fine) which\nresets it too, as some callers seen below & in your diff context want\nthat.\n\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex b0ece954808..509c58ad556 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1297,12 +1297,6 @@ void show_combined_diff(struct combine_diff_path *p,\n \t\tshow_patch_diff(p, num_parent, 1, rev);\n }\n \n-static void free_combined_pair(struct diff_filepair *pair)\n-{\n-\tfree(pair->two);\n-\tfree(pair);\n-}\n-\n /*\n  * A combine_diff_path expresses N parents on the LHS against 1 merge\n  * result. Synthesize a diff_filepair that has N entries on the \"one\"\n@@ -1355,9 +1349,7 @@ static void handle_combined_callback(struct diff_options *opt,\n \tfor (i = 0, p = paths; p; p = p->next)\n \t\tq.queue[i++] = combined_pair(p, num_parent);\n \topt->format_callback(&q, opt, opt->format_callback_data);\n-\tfor (i = 0; i < num_paths; i++)\n-\t\tfree_combined_pair(q.queue[i]);\n-\tfree(q.queue);\n+\tdiff_free_queue(&q, 1);\n }\n \n static const char *path_path(void *obj)\ndiff --git a/diff.c b/diff.c\nindex 03e6ffb5e4e..b48105c070f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5773,10 +5773,20 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n-void diff_free_queue(struct diff_queue_struct *q)\n+static void free_combined_pair(struct diff_filepair *pair)\n {\n-\tfor (int i = 0; i < q->nr; i++)\n-\t\tdiff_free_filepair(q->queue[i]);\n+\tfree(pair->two);\n+\tfree(pair);\n+}\n+\n+void diff_free_queue(struct diff_queue_struct *q, int combined)\n+{\n+\tfor (int i = 0; i < q->nr; i++) {\n+\t\tif (combined)\n+\t\t\tdiff_free_filepair(q->queue[i]);\n+\t\telse\n+\t\t\tfree_combined_pair(q->queue[i]);\n+\t}\n \tfree(q->queue);\n }\n \n@@ -6339,7 +6349,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint result = diff_get_patch_id(options, oid, diff_header_only);\n \n-\tdiff_free_queue(q);\n+\tdiff_free_queue(q, 0);\n \tDIFF_QUEUE_CLEAR(q);\n \n \treturn result;\n@@ -6609,7 +6619,7 @@ void diff_flush(struct diff_options *options)\n \t\toptions->format_callback(q, options, options->format_callback_data);\n \n free_queue:\n-\tdiff_free_queue(q);\n+\tdiff_free_queue(q, 0);\n \tDIFF_QUEUE_CLEAR(q);\n \tdiff_free(options);\n \ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex c0422d9e709..1754411a916 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1686,6 +1686,7 @@ void diffcore_rename_extended(struct diff_options *options,\n \t\t\tpair_to_free = p;\n \n \t\tif (pair_to_free)\n+\t\t\t/* ??? */\n \t\t\tpool_diff_free_filepair(pool, pair_to_free);\n \t}\n \tdiff_debug_queue(\"done copying original\", &outq);\ndiff --git a/diffcore.h b/diffcore.h\nindex 9b588a1ee15..6cb74e6eadf 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -162,7 +162,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t struct diff_filespec *,\n \t\t\t\t struct diff_filespec *);\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n-void diff_free_queue(struct diff_queue_struct *q);\n+void diff_free_queue(struct diff_queue_struct *q, int combined);\n \n /* dir_rename_relevance: the reason we want rename information for a dir */\n enum dir_rename_relevance {\ndiff --git a/line-log.c b/line-log.c\nindex a7f3e7f6ce4..88c22b20c0f 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1090,7 +1090,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)\n static void free_diffqueues(int n, struct diff_queue_struct *dq)\n {\n \tfor (int i = 0; i < n; i++)\n-\t\tdiff_free_queue(&dq[i]);\n+\t\tdiff_free_queue(&dq[i], 0);\n \tfree(dq);\n }\n \n@@ -1193,7 +1193,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \tif (parent)\n \t\tadd_line_range(rev, parent, parent_range);\n \tfree_line_log_data(parent_range);\n-\tdiff_free_queue(&queue);\n+\tdiff_free_queue(&queue, 0);\n \treturn changed;\n }\n \n"},{"id":"466669","messageId":"20221107151115.GA1951@szeder.dev","threadId":"58717","inReplyTo":"Y2MJRRfwG7rSp6Ra@nand.local","subject":"Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-07T15:11:15Z","receivedAt":"2022-11-07T15:11:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:\n> > +void diff_free_queue(struct diff_queue_struct *q)\n> > +{\n> > +\tfor (int i = 0; i < q->nr; i++)\n> > +\t\tdiff_free_filepair(q->queue[i]);\n> > +\tfree(q->queue);\n> > +}\n> \n> Though I wonder, should diff_free_queue() be a noop when q is NULL? The\n> caller in process_ranges_ordinary_commit() doesn't care, of course,\n> since q is always non-NULL there.\n> \n> But if we're making it part of the diff API, we should probably err on\n> the side of flexibility.\n\nOn one hand, strbuf_reset(), string_list_clear(), or strvec_clear()\nwould all segfault on a NULL strbuf, string_list, or strvec pointer.\n\nOn the other hand, given the usage patterns of the diff API, and that\nit mostly only works on the dreaded global 'diff_queued_diff'\ninstance, I don't think there is any flexibility to be gained with\nthis; indeed it is already more flexible than many diff API functions\nas it works on the diff queue given as parameter instead of that\nglobal instance.\n\n"},{"id":"466671","messageId":"221107.86r0yevkt5.gmgdl@evledraar.gmail.com","threadId":"58717","inReplyTo":"20221107151115.GA1951@szeder.dev","subject":"Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T15:29:39Z","receivedAt":"2022-11-07T15:36:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, SZEDER Gábor wrote:\n\n> On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:\n>> > +void diff_free_queue(struct diff_queue_struct *q)\n>> > +{\n>> > +\tfor (int i = 0; i < q->nr; i++)\n>> > +\t\tdiff_free_filepair(q->queue[i]);\n>> > +\tfree(q->queue);\n>> > +}\n>> \n>> Though I wonder, should diff_free_queue() be a noop when q is NULL? The\n>> caller in process_ranges_ordinary_commit() doesn't care, of course,\n>> since q is always non-NULL there.\n>> \n>> But if we're making it part of the diff API, we should probably err on\n>> the side of flexibility.\n>\n> On one hand, strbuf_reset(), string_list_clear(), or strvec_clear()\n> would all segfault on a NULL strbuf, string_list, or strvec pointer.\n\nBut the reason we do that is because those APIs will always ensure that\nthe struct is never in an inconsistent state, as opposed to the\ndestructor you're adding here.\n\nI.e. if you were to work with the queue after this diff_free_queue()\ncall in process_ranges_ordinary_commit() you'd segfault, not so with\nthose other APIs.\n\n> On the other hand, given the usage patterns of the diff API, and that\n> it mostly only works on the dreaded global 'diff_queued_diff'\n> instance, I don't think there is any flexibility to be gained with\n> this; indeed it is already more flexible than many diff API functions\n> as it works on the diff queue given as parameter instead of that\n> global instance.\n\nI pointed how this could be nicer if you made it work like those other\nAPIs in\nhttps://lore.kernel.org/git/221103.864jvg2yit.gmgdl@evledraar.gmail.com/;\nI.e. we could do away with DIFF_QUEUE_CLEAR() after calling this\n\"free()\".\n\nBut in lieu of such a larger change, just adding a call to\n\"DIFF_QUEUE_CLEAR()\" in this new free() function seems like it could\nmake thing safer at very little cost.\n\nWe're also far from consistent about this, but I wish it worked like\nthat and were called:\n\n\tdiff_queue_struct_{release,clear}()\n\nI.e. the usual naming is:\n\n\t<struct name>_{release,clear}()\n\nIn cases where we don't free() the pointer itself, but assume that we're\nworking on a struct on the stack, whereas *_free() functions will free\nthe malloc'd pointer itself, as well as anything it contains.\n\n"},{"id":"466674","messageId":"20221107155721.GB1951@szeder.dev","threadId":"58717","inReplyTo":"221107.86r0yevkt5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-07T15:57:21Z","receivedAt":"2022-11-07T15:57:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 07, 2022 at 04:29:39PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Nov 07 2022, SZEDER Gábor wrote:\n> \n> > On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:\n> >> > +void diff_free_queue(struct diff_queue_struct *q)\n> >> > +{\n> >> > +\tfor (int i = 0; i < q->nr; i++)\n> >> > +\t\tdiff_free_filepair(q->queue[i]);\n> >> > +\tfree(q->queue);\n> >> > +}\n> >> \n> >> Though I wonder, should diff_free_queue() be a noop when q is NULL? The\n> >> caller in process_ranges_ordinary_commit() doesn't care, of course,\n> >> since q is always non-NULL there.\n> >> \n> >> But if we're making it part of the diff API, we should probably err on\n> >> the side of flexibility.\n> >\n> > On one hand, strbuf_reset(), string_list_clear(), or strvec_clear()\n> > would all segfault on a NULL strbuf, string_list, or strvec pointer.\n> \n> But the reason we do that is because those APIs will always ensure that\n> the struct is never in an inconsistent state, as opposed to the\n> destructor you're adding here.\n\nTaylor's suggestion quoted above is not about the internal state of\nthe diff queue, but about a NULL pointer passed to diff_free_queue().\n\n> I.e. if you were to work with the queue after this diff_free_queue()\n> call in process_ranges_ordinary_commit() you'd segfault, not so with\n> those other APIs.\n> \n> > On the other hand, given the usage patterns of the diff API, and that\n> > it mostly only works on the dreaded global 'diff_queued_diff'\n> > instance, I don't think there is any flexibility to be gained with\n> > this; indeed it is already more flexible than many diff API functions\n> > as it works on the diff queue given as parameter instead of that\n> > global instance.\n> \n> I pointed how this could be nicer if you made it work like those other\n> APIs in\n> https://lore.kernel.org/git/221103.864jvg2yit.gmgdl@evledraar.gmail.com/;\n> I.e. we could do away with DIFF_QUEUE_CLEAR() after calling this\n> \"free()\".\n> \n> But in lieu of such a larger change, just adding a call to\n> \"DIFF_QUEUE_CLEAR()\" in this new free() function seems like it could\n> make thing safer at very little cost.\n> \n> We're also far from consistent about this, but I wish it worked like\n> that and were called:\n> \n> \tdiff_queue_struct_{release,clear}()\n> \n> I.e. the usual naming is:\n> \n> \t<struct name>_{release,clear}()\n> \n> In cases where we don't free() the pointer itself, but assume that we're\n> working on a struct on the stack, whereas *_free() functions will free\n> the malloc'd pointer itself, as well as anything it contains.\n> \n"},{"id":"466675","messageId":"20221107161311.GC1951@szeder.dev","threadId":"58717","inReplyTo":"Y2MKKTz4nK0L8uW5@nand.local","subject":"Re: [PATCH 3/3] diff.c: use diff_free_queue()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-11-07T16:13:11Z","receivedAt":"2022-11-07T16:13:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Nov 02, 2022 at 08:24:09PM -0400, Taylor Blau wrote:\n> On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:\n> >  int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n> >  {\n> >  \tstruct diff_queue_struct *q = &diff_queued_diff;\n> > -\tint i;\n> >  \tint result = diff_get_patch_id(options, oid, diff_header_only);\n> >\n> > -\tfor (i = 0; i < q->nr; i++)\n> > -\t\tdiff_free_filepair(q->queue[i]);\n> > -\n> > -\tfree(q->queue);\n> > +\tdiff_free_queue(q);\n> >  \tDIFF_QUEUE_CLEAR(q);\n> \n> So, this all looks fine to me. But I did a quick grep around for\n> DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places.\n> Mostly, as far as I can tell, to \"empty\" out the diff-queue by setting\n> its 'queue' pointer to NULL, and its 'nr' back to 0.\n> \n> Should we be freeing the memory held by the queue there more\n> aggressively? I.e., should we make sure that there is a\n> diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR()\n> macro?\n\nDefinitely not.  DIFF_QUEUE_CLEAR is often used to initialize a just\ncreated 'struct diff_queue_struct' instance; by adding a\ndiff_free_queue() above those it would operate on uninitialized\nmemory.\n\n"},{"id":"466780","messageId":"Y2m7gielFOvTYbvi@nand.local","threadId":"58717","inReplyTo":"20221107155721.GB1951@szeder.dev","subject":"Re: [PATCH 1/3] line-log: free diff queue when processing non-merge commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T02:14:26Z","receivedAt":"2022-11-08T02:14:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 04:57:21PM +0100, SZEDER Gábor wrote:\n> On Mon, Nov 07, 2022 at 04:29:39PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >\n> > On Mon, Nov 07 2022, SZEDER Gábor wrote:\n> >\n> > > On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:\n> > >> > +void diff_free_queue(struct diff_queue_struct *q)\n> > >> > +{\n> > >> > +\tfor (int i = 0; i < q->nr; i++)\n> > >> > +\t\tdiff_free_filepair(q->queue[i]);\n> > >> > +\tfree(q->queue);\n> > >> > +}\n> > >>\n> > >> Though I wonder, should diff_free_queue() be a noop when q is NULL? The\n> > >> caller in process_ranges_ordinary_commit() doesn't care, of course,\n> > >> since q is always non-NULL there.\n> > >>\n> > >> But if we're making it part of the diff API, we should probably err on\n> > >> the side of flexibility.\n> > >\n> > > On one hand, strbuf_reset(), string_list_clear(), or strvec_clear()\n> > > would all segfault on a NULL strbuf, string_list, or strvec pointer.\n> >\n> > But the reason we do that is because those APIs will always ensure that\n> > the struct is never in an inconsistent state, as opposed to the\n> > destructor you're adding here.\n>\n> Taylor's suggestion quoted above is not about the internal state of\n> the diff queue, but about a NULL pointer passed to diff_free_queue().\n\nI think your perspective that strbuf_reset(), string_list_clear(), etc.\nall segfault on a NULL argument is extremely reasonable. Let's start\nmerging this down.\n\nThanks,\nTaylor\n"},{"id":"466781","messageId":"Y2m7mmlL1RWkp6m3@nand.local","threadId":"58717","inReplyTo":"20221107161311.GC1951@szeder.dev","subject":"Re: [PATCH 3/3] diff.c: use diff_free_queue()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T02:14:50Z","receivedAt":"2022-11-08T02:14:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 05:13:11PM +0100, SZEDER Gábor wrote:\n> On Wed, Nov 02, 2022 at 08:24:09PM -0400, Taylor Blau wrote:\n> > On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:\n> > >  int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n> > >  {\n> > >  \tstruct diff_queue_struct *q = &diff_queued_diff;\n> > > -\tint i;\n> > >  \tint result = diff_get_patch_id(options, oid, diff_header_only);\n> > >\n> > > -\tfor (i = 0; i < q->nr; i++)\n> > > -\t\tdiff_free_filepair(q->queue[i]);\n> > > -\n> > > -\tfree(q->queue);\n> > > +\tdiff_free_queue(q);\n> > >  \tDIFF_QUEUE_CLEAR(q);\n> >\n> > So, this all looks fine to me. But I did a quick grep around for\n> > DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places.\n> > Mostly, as far as I can tell, to \"empty\" out the diff-queue by setting\n> > its 'queue' pointer to NULL, and its 'nr' back to 0.\n> >\n> > Should we be freeing the memory held by the queue there more\n> > aggressively? I.e., should we make sure that there is a\n> > diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR()\n> > macro?\n>\n> Definitely not.  DIFF_QUEUE_CLEAR is often used to initialize a just\n> created 'struct diff_queue_struct' instance; by adding a\n> diff_free_queue() above those it would operate on uninitialized\n> memory.\n\nThanks for pointing it out.\n\nThanks,\nTaylor\n"}]}