# [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix" 4 messages from 2006-07-02 to 2006-07-02. Participants: Marco Costalba, Linus Torvalds, Junio C Hamano. Thread: https://gitlist.dev/t/4738 ## Marco Costalba, 2006-07-02 12:19 Subject: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix" Message-ID: URL: https://gitlist.dev/e/e5bfff550607020519k6007f41bne34d10c0e919f3c8%40mail.gmail.com ``` After patch "revision.c: --full-history fix" (6631c73) I have this in git tree. I have _manually_ ( --abbrev=nr does not apply in this case) truncated to 7 chars the SHA to be more readable) $ git-rev-list --topo-order --parents --remove-empty HEAD -- builtin-add.c 021b6e4 93872e0 93872e0 3c6a370 3c6a370 e8f990b e8f990b f259339 f259339 0d78153 0d78153 b4189aa 283c8ee $ git-rev-list --topo-order --remove-empty HEAD -- builtin-add.c 021b6e4 93872e0 3c6a370 e8f990b f259339 0d78153 $ What it seems is that with --parents option two more spurious revs are printed out. This revs have nothing to do with builtin-add.c, the file is newer then both of them. I have tested with different files and always we have spurious revisions in case of files that were added after initial commit, and always the erroneous revisions are older then file creation one. This is a possible regression that breaks things (I've found it thanks to an assert in qgit). Marco ``` ## Linus Torvalds, 2006-07-02 17:14 Subject: Re: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix" Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0607021012180.12404%40g5.osdl.org In-Reply-To: ``` On Sun, 2 Jul 2006, Marco Costalba wrote: > > What it seems is that with --parents option two more spurious revs > are printed out. This revs have nothing to do with builtin-add.c, the > file is newer then both of them. Gaah. Does this trivial patch fix it for you? It had the wrong test for whether a commit was a merge. What it did was to say that a non-merge has exactly one parent (which sounds almost right), but the fact is, initial trees have no parent at all, but they're obviously not merges. So the test for non-merge should be "!parents || !parents->next", not "parents && !parents->next". Linus ---- diff --git a/revision.c b/revision.c index 1cf6276..880fb7b 100644 --- a/revision.c +++ b/revision.c @@ -997,7 +997,7 @@ struct commit *get_revision(struct rev_i if (!revs->parents) continue; /* non-merge - always ignore it */ - if (commit->parents && !commit->parents->next) + if (!commit->parents || !commit->parents->next) continue; } if (revs->parents) ``` ## Junio C Hamano, 2006-07-02 17:48 Subject: Re: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix" Message-ID: <7v64if6gpr.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64if6gpr.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > Gaah. Does this trivial patch fix it for you? > > It had the wrong test for whether a commit was a merge. Gaah indeed -- I did not notice the logic error when I picked it up either, sorry. > diff --git a/revision.c b/revision.c > index 1cf6276..880fb7b 100644 > --- a/revision.c > +++ b/revision.c > @@ -997,7 +997,7 @@ struct commit *get_revision(struct rev_i > if (!revs->parents) > continue; > /* non-merge - always ignore it */ > - if (commit->parents && !commit->parents->next) > + if (!commit->parents || !commit->parents->next) > continue; > } > if (revs->parents) For a casual reader who is curious, the reason it matters to treat the "root" commit sanely in this example is because with the --remove-empty option the commits that add the specified paths are already made into "fake" root commits when the above function sees them (done in try_to_simplify_commit()). Thanks, Linus and Marco. ``` ## Marco Costalba, 2006-07-02 21:23 Subject: Re: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix" Message-ID: URL: https://gitlist.dev/e/e5bfff550607021423n34f1c407qec67b9f86a0d6381%40mail.gmail.com In-Reply-To: ``` > > Gaah. Does this trivial patch fix it for you? > Yes. It works. Thanks Marco ```