threads / discuss / 4738

[POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix"

Subject: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix"

## tl;dr

4 messages between Jul 2, 2006 and Jul 2, 2006.

replies: 3people: 3as markdown or json

Marco Costalba· Jul 2, 2006, 12:19 UTC · lore
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· Jul 2, 2006, 17:14 UTC · re: Marco Costalba · lore

Re: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix"

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· Jul 2, 2006, 17:48 UTC · re: Linus Torvalds · lore

Re: [POSSIBLE REGRESSION] Spurious revs after patch "revision.c: --full-history fix"

Linus Torvalds <torvalds@osdl.org> 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.

Show 13 quoted lines
> 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.

← back to recent threads