{"thread":{"id":"8676","subject":"Fix up \"git log --follow\" a bit..","startedAt":"2007-06-21T17:22:59Z","lastAt":"2007-06-21T19:30:46Z","messageCount":2,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"45519","messageId":"alpine.LFD.0.98.0706211017190.3593@woody.linux-foundation.org","threadId":"8676","inReplyTo":null,"subject":"Fix up \"git log --follow\" a bit..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-21T17:22:59Z","receivedAt":"2007-06-21T17:22:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis fixes \"git log --follow\" to hopefully not leak memory any more, and \nalso cleans it up a bit to look more like some of the other functions that \nuse \"diff_queued_diff\" (by *not* using it directly as a global in the \ncode, but by instead just taking a pointer to the diff queue and using \nthat).\n\nAs to \"diff_queued_diff\", I think it would be better off not as a global \nat all, but as being just an entry in the \"struct diff_options\" structure, \nbut that's a separate issue, and there may be some subtle reason for why \nit's currently a global.\n\nAnyway, no real changes. Instead of having a magical first entry in the \ndiff-queue, we now end up just keeping the diff-queue clean, and keeping \nour \"preferred\" file pairing in an internal \"choice\" variable. That makes \nit easy to switch the choice around when we find a better one.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nNo changes outside of \"try_to_follow_renames()\", and most of the diff here \nreally is just trivial re-organizations and the addition of \"free all the \nfilepairs we're not interested in.\n\nUpdated the comments a bit to match.\n\n tree-diff.c |   37 ++++++++++++++++++++++++++++---------\n 1 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 42924e9..26bdbdd 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -305,9 +305,15 @@ static inline int diff_might_be_rename(void)\n static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt)\n {\n \tstruct diff_options diff_opts;\n-\tconst char *paths[2];\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct diff_filepair *choice;\n+\tconst char *paths[1];\n \tint i;\n \n+\t/* Remove the file creation entry from the diff queue, and remember it */\n+\tchoice = q->queue[0];\n+\tq->nr = 0;\n+\n \tdiff_setup(&diff_opts);\n \tdiff_opts.recursive = 1;\n \tdiff_opts.detect_rename = DIFF_DETECT_RENAME;\n@@ -320,17 +326,21 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_tree(t1, t2, base, &diff_opts);\n \tdiffcore_std(&diff_opts);\n \n-\t/* NOTE! Ignore the first diff! That was the old one! */\n-\tfor (i = 1; i < diff_queued_diff.nr; i++) {\n-\t\tstruct diff_filepair *p = diff_queued_diff.queue[i];\n+\t/* Go through the new set of filepairing, and see if we find a more interesting one */\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n \n \t\t/*\n \t\t * Found a source? Not only do we use that for the new\n-\t\t * diff_queued_diff, we also use that as the path in\n+\t\t * diff_queued_diff, we will also use that as the path in\n \t\t * the future!\n \t\t */\n \t\tif ((p->status == 'R' || p->status == 'C') && !strcmp(p->two->path, opt->paths[0])) {\n-\t\t\tdiff_queued_diff.queue[0] = p;\n+\t\t\t/* Switch the file-pairs around */\n+\t\t\tq->queue[i] = choice;\n+\t\t\tchoice = p;\n+\n+\t\t\t/* Update the path we use from now on.. */\n \t\t\topt->paths[0] = xstrdup(p->one->path);\n \t\t\tdiff_tree_setup_paths(opt->paths, opt);\n \t\t\tbreak;\n@@ -338,10 +348,19 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \t}\n \n \t/*\n-\t * Then, ignore any but the first entry! It might be the old one,\n-\t * or it might be the rename/copy we found\n+\t * Then, discard all the non-relevane file pairs...\n+\t */\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tdiff_free_filepair(p);\n+\t}\n+\n+\t/*\n+\t * .. and re-instate the one we want (which might be either the\n+\t * original one, or the rename/copy we found)\n \t */\n-\tdiff_queued_diff.nr = 1;\n+\tq->queue[0] = choice;\n+\tq->nr = 1;\n }\n \n int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)\n"},{"id":"45523","messageId":"7vbqf9w1eh.fsf@assigned-by-dhcp.pobox.com","threadId":"8676","inReplyTo":"alpine.LFD.0.98.0706211017190.3593@woody.linux-foundation.org","subject":"Re: Fix up \"git log --follow\" a bit..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-21T19:30:46Z","receivedAt":"2007-06-21T19:30:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> As to \"diff_queued_diff\", I think it would be better off not as a global \n> at all, but as being just an entry in the \"struct diff_options\" structure, \n> but that's a separate issue, and there may be some subtle reason for why \n> it's currently a global.\n\nThere is no subtle issue that I know of, but I have to double\ncheck.  It is just that the code is one of the most ancient part\nof the system by now.\n"}]}