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

[Junio C Hamano <gitster@pobox.com>]

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