[PATCH] Simplify and fix --first-parent implementation

[PATCH] Simplify and fix --first-parent implementation

[Stephen R. van den Berg]

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

Re: [PATCH] Simplify and fix --first-parent implementation

[Junio C Hamano]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> ---

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

Please also sign your patch.

> @@ -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;
>  	}

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

Re: [PATCH] Simplify and fix --first-parent implementation

[Stephen R. van den Berg]

Junio C Hamano wrote:
>"Stephen R. van den Berg" <srb@cuci.nl> writes:
>> ---
>No explanation of what the patch does, nor justification of why it is a
>good change?

Sorry, thought that one-line description was enough.  I'll resubmit this
one more verbosely.

>Please also sign your patch.

Just with git-commit -s ?  Or do I need to get gpg involved?

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

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.

Re: [PATCH] Simplify and fix --first-parent implementation

[Jeff King]

On Sat, Apr 26, 2008 at 01:59:56PM +0200, Stephen R. van den Berg wrote:

> >Please also sign your patch.
> 
> Just with git-commit -s ?  Or do I need to get gpg involved?

With git-commit -s; check out Documentation/SubmittingPatches.

> P.S. No reaction on the other patches means that they're accepted, or do
> I need to resubmit them as well (signed)?

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

Re: [PATCH] Simplify and fix --first-parent implementation

[Junio C Hamano]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> P.S. No reaction on the other patches means that they're accepted, or do
> I need to resubmit them as well (signed)?

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.