---

 revision.c |   10 ++++------

 1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/revision.c b/revision.c

index 4231ea2..bcfcd2a 100644

--- a/revision.c

+++ b/revision.c

@@ -415,7 +415,6 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str

 {

 	struct commit_list *parent = commit->parents;

 	unsigned left_flag;

-	int add, rest;
 	if (commit->object.flags & ADDED)

 		return 0;

@@ -462,19 +461,18 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str
 	left_flag = (commit->object.flags & SYMMETRIC_LEFT);
-	rest = !revs->first_parent_only;

-	for (parent = commit->parents, add = 1; parent; add = rest) {

+	for (parent = commit->parents; parent; parent = parent->next) {

 		struct commit *p = parent->item;
-		parent = parent->next;

 		if (parse_commit(p) < 0)

 			return -1;

 		p->object.flags |= left_flag;

 		if (p->object.flags & SEEN)

 			continue;

 		p->object.flags |= SEEN;

-		if (add)

-			insert_by_date(p, list);

+		insert_by_date(p, list);

+		if(revs->first_parent_only)

+			break;

 	}

 	return 0;

 }

--

1.5.5.1.83.ge77a4.dirty
--

No explanation of what the patch does, nor justification of why it is a

good change?
The original code makes sure all the parents of the given commits are

marked as SEEN (and SYMMETRIC_LEFT if needed), even when only it traverses

the first parent.  By leaving the loop early, you are changing the

semantics of the code.  Other parents, when reached from other paths while

traversing the commit ancestry chain, will behave differently between the

version with your patch and without.
You would need to explain how and why that behaviour change is a "fix" in

the proposed commit log message.  "Before the change, traversing a history

of this shape behaves like this, but that is wrong for such and such

reasons, and I identified the culprit to be this code.  With this patch,

the same traversal will instead behave this way, which is what I expect to

be the right behaviour".
--

Sorry, thought that one-line description was enough.  I'll resubmit this
Yes, I was and am aware of that.  The previous behaviour appears to be bogus.

I'll elaborate on resubmission.
P.S. No reaction on the other patches means that they're accepted, or do

I need to resubmit them as well (signed)?

--

Sincerely,                                                          srb@cuci.nl

           Stephen R. van den Berg.

--

No reaction means just that.  I either haven't looked at them, or after

having looked at them I did not find them interesting enough to comment

on.
The latter does not mean they are rejected, though.  Remember, my review

is NOT the only thing that counts here, other people's review and

comment too here, and it tends to take time.
The proposed commit log message would express why the change was needed.

It would present a use case that is useful (and argue why that use case is

worth supporting), that is not easily supported with the existing code,

and how the patch makes it so.  When I review a patch posted on the list,

here are the things I consider.  This is pretty much personal, and other

people may do things in different order:
 (0) The merit of the patch itself is not obvious from the diff, but there

     is no explanation; or
 (1) The argument to support the use case may not be convincing or may be

     outright wrong; or
 (2) Even if the argument is convincing, the claim that the current code

     does not support it may be false; or
 (3) The patch may not be the right way to support it and there may be

     better ways; or
 (4) The patch may make the new use case supported, but breaks existing

     use cases.
I look at (0) to _guess_ why the submitter thought the patch was a good

idea when I have nothing better to do (but that seldom happens these days)

or the submitter is new to the list.
Patches in (1) and (2) categories may get comments on what the patch tries

to achieve, and for that I do not have to look at the diff.  I tend start

to look at the diff for patches in categories (3) and (4).
The ideal case is obviously:
 (5) The merit of the patch itself is very clear and there is a good

     explanation in the commit log message.
--

I for one would have liked to see more explanation on your "check for

circular references" patch. I am not clear on what circumstances create

such circular references.
-Peff

--