Re: Weird revision walk behaviour
- From
Jeff King <peff@peff.net>
- Date
- May 29, 2018, 21:04 UTC
- Message-ID
- <20180529210434.GA3857@sigill.intra.peff.net>
- In-Reply-To
- <20180528220651.20287-1-szeder.dev@gmail.com>
On Tue, May 29, 2018 at 12:06:51AM +0200, SZEDER Gábor wrote:
Show 34 quoted lines
> 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:
Show 12 quoted lines
> @@ -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