Re: Weird revision walk behaviour

[Jeff King <peff@peff.net>]

On Tue, May 29, 2018 at 12:06:51AM +0200, SZEDER Gábor wrote:

> diff --git a/revision.c b/revision.c
> index 4e0e193e57..0ddd2c1e8a 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -605,7 +605,7 @@ static inline int limiting_can_increase_treesame(const struct rev_info *revs)
>  
>  static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  {
> -	struct commit_list **pp, *parent;
> +	struct commit_list **pp, *parent, *treesame_parents = NULL;
>  	struct treesame_state *ts = NULL;
>  	int relevant_change = 0, irrelevant_change = 0;
>  	int relevant_parents, nth_parent;
> @@ -672,6 +672,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  		switch (rev_compare_tree(revs, p, commit)) {
>  		case REV_TREE_SAME:
>  			if (!revs->simplify_history || !relevant_commit(p)) {
> +				struct commit_list *tp;
>  				/* Even if a merge with an uninteresting
>  				 * side branch brought the entire change
>  				 * we are interested in, we do not want
> @@ -680,6 +681,13 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  				 */
>  				if (ts)
>  					ts->treesame[nth_parent] = 1;
> +				/* But we note it for potential later
> +				 * simplification
> +				 */
> +				tp = treesame_parents;
> +				treesame_parents = xmalloc(sizeof(*treesame_parents));
> +				treesame_parents->item = p;
> +				treesame_parents->next = tp;
>  				continue;
>  			}

We hit this "if" if !relevant_commit(p), which I think is what we want.
But we'd also hit it if !revs->simplify_history. Would we want to avoid
doing the simplification in that case?

I guess later we do:

> @@ -716,6 +724,14 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  		die("bad tree compare for commit %s", oid_to_hex(&commit->object.oid));
>  	}
>  
> +	if (relevant_parents == 0 && revs->simplify_history &&
> +	    treesame_parents) {
> +		commit->parents = treesame_parents;
> +		commit->object.flags |= TREESAME;
> +		return;
> +	} else
> +		free_commit_list(treesame_parents);
> +

...which blocks the !simplify_history case from triggering. But then we
could avoid the allocation above in that case, I think (though I agree
with Kevin's later email that we may not need it at all).

Do we even need to do the parent rewriting here? By definition those
parents aren't interesting, and we're TREESAME to whatever is in
treesame_parents. So conceptually it seems like we just need a flag "I
found a treesame parent", but we only convert that into a TREESAME flag
if there are no relevant parents.

I wouldn't be surprised, though, if some code path really cares whether
we've simplified to a single uninteresting parent here, versus
simplifying to a root commit (I admit that the simplification code is
one of the areas of Git I'm least familiar with).

-Peff