{"thread":{"id":"4738","subject":"[POSSIBLE REGRESSION] Spurious revs after patch \"revision.c: --full-history fix\"","startedAt":"2006-07-02T12:19:01Z","lastAt":"2006-07-02T21:23:39Z","messageCount":4,"participants":["Marco Costalba","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"23034","messageId":"e5bfff550607020519k6007f41bne34d10c0e919f3c8@mail.gmail.com","threadId":"4738","inReplyTo":null,"subject":"[POSSIBLE REGRESSION] Spurious revs after patch \"revision.c: --full-history fix\"","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-07-02T12:19:01Z","receivedAt":"2006-07-02T12:19:01Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"After patch \"revision.c: --full-history fix\" (6631c73)\n\nI have this in git tree.\n\nI have _manually_  ( --abbrev=nr does not apply in this case)\ntruncated to 7 chars the SHA to be more readable)\n\n$ git-rev-list --topo-order --parents --remove-empty HEAD -- builtin-add.c\n021b6e4 93872e0\n93872e0 3c6a370\n3c6a370 e8f990b\ne8f990b f259339\nf259339 0d78153\n0d78153\nb4189aa\n283c8ee\n$ git-rev-list --topo-order --remove-empty HEAD -- builtin-add.c\n021b6e4\n93872e0\n3c6a370\ne8f990b\nf259339\n0d78153\n$\n\nWhat it seems is that with --parents  option two more spurious revs\nare printed out. This revs have nothing to do with builtin-add.c, the\nfile is newer then both of them.\n\nI have tested with different files and always we have spurious\nrevisions in case of files that were added after initial commit, and\nalways the erroneous revisions are older then file creation one.\n\nThis is a possible regression that breaks things (I've found it thanks\nto an assert in qgit).\n\n   Marco\n"},{"id":"23036","messageId":"Pine.LNX.4.64.0607021012180.12404@g5.osdl.org","threadId":"4738","inReplyTo":"e5bfff550607020519k6007f41bne34d10c0e919f3c8@mail.gmail.com","subject":"Re: [POSSIBLE REGRESSION] Spurious revs after patch \"revision.c: --full-history fix\"","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-07-02T17:14:35Z","receivedAt":"2006-07-02T17:14:35Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Jul 2006, Marco Costalba wrote:\n> \n> What it seems is that with --parents  option two more spurious revs\n> are printed out. This revs have nothing to do with builtin-add.c, the\n> file is newer then both of them.\n\nGaah. Does this trivial patch fix it for you?\n\nIt had the wrong test for whether a commit was a merge. What it did was to \nsay that a non-merge has exactly one parent (which sounds almost right), \nbut the fact is, initial trees have no parent at all, but they're \nobviously not merges.\n\nSo the test for non-merge should be \"!parents || !parents->next\", not \n\"parents && !parents->next\".\n\n\t\tLinus\n\n----\ndiff --git a/revision.c b/revision.c\nindex 1cf6276..880fb7b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -997,7 +997,7 @@ struct commit *get_revision(struct rev_i\n \t\t\t\tif (!revs->parents)\n \t\t\t\t\tcontinue;\n \t\t\t\t/* non-merge - always ignore it */\n-\t\t\t\tif (commit->parents && !commit->parents->next)\n+\t\t\t\tif (!commit->parents || !commit->parents->next)\n \t\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (revs->parents)\n"},{"id":"23039","messageId":"7v64if6gpr.fsf@assigned-by-dhcp.cox.net","threadId":"4738","inReplyTo":"Pine.LNX.4.64.0607021012180.12404@g5.osdl.org","subject":"Re: [POSSIBLE REGRESSION] Spurious revs after patch \"revision.c: --full-history fix\"","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-02T17:48:16Z","receivedAt":"2006-07-02T17:48:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> Gaah. Does this trivial patch fix it for you?\n>\n> It had the wrong test for whether a commit was a merge.\n\nGaah indeed -- I did not notice the logic error when I picked it\nup either, sorry.\n\n> diff --git a/revision.c b/revision.c\n> index 1cf6276..880fb7b 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -997,7 +997,7 @@ struct commit *get_revision(struct rev_i\n>  \t\t\t\tif (!revs->parents)\n>  \t\t\t\t\tcontinue;\n>  \t\t\t\t/* non-merge - always ignore it */\n> -\t\t\t\tif (commit->parents && !commit->parents->next)\n> +\t\t\t\tif (!commit->parents || !commit->parents->next)\n>  \t\t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tif (revs->parents)\n\nFor a casual reader who is curious, the reason it matters to\ntreat the \"root\" commit sanely in this example is because with\nthe --remove-empty option the commits that add the specified\npaths are already made into \"fake\" root commits when the above\nfunction sees them (done in try_to_simplify_commit()).\n\nThanks, Linus and Marco.\n"},{"id":"23050","messageId":"e5bfff550607021423n34f1c407qec67b9f86a0d6381@mail.gmail.com","threadId":"4738","inReplyTo":"Pine.LNX.4.64.0607021012180.12404@g5.osdl.org","subject":"Re: [POSSIBLE REGRESSION] Spurious revs after patch \"revision.c: --full-history fix\"","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-07-02T21:23:39Z","receivedAt":"2006-07-02T21:23:39Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":">\n> Gaah. Does this trivial patch fix it for you?\n>\n\nYes. It works.\n\nThanks\nMarco\n"}]}