threads / patch / 13258

patchSimplify and fix --first-parent implementation

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

## tl;dr

5 messages between Apr 25, 2008 and Apr 26, 2008. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Stephen R. van den Berg· Apr 25, 2008, 18:10 UTC · lore
---
 revision.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Show changes to revision.c +4 −6
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
Junio C Hamano· Apr 26, 2008, 00:11 UTC · re: Stephen R. van den Berg · lore

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

"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.
Show 22 quoted lines
> @@ -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".

Stephen R. van den Berg· Apr 26, 2008, 11:59 UTC · re: Junio C Hamano · lore

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

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?
Show 6 quoted lines
>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.
Jeff King· Apr 26, 2008, 12:27 UTC · re: Stephen R. van den Berg · lore

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

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
Junio C Hamano· Apr 26, 2008, 19:13 UTC · re: Stephen R. van den Berg · lore

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

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

← back to recent threads