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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2008, 00:11 UTC
Message-ID: <7viqy5o4om.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080425234556.D60FD5461@aristoteles.cuci.nl>

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