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

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

[Marco Costalba]

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

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

[Linus Torvalds]

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)

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

[Junio C Hamano]

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.

> 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.

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

[Marco Costalba]

>
> Gaah. Does this trivial patch fix it for you?
>

Yes. It works.

Thanks
Marco