On 02/09/2025 14:22, Phillip Wood wrote:

On 01/09/2025 21:43, Colin Stagner wrote:

quoted

On 9/1/25 08:54, Phillip Wood wrote:
Colin Stagner [off-list ref] writes:

The outer loop in git-subtree.sh:983 appears to iterate from the root 
commit forwards… and not from the HEAD backwards.

     git rev-list --topo-order --reverse --parents $rev $unrevs
     #                         ^^^^^^^^^

Since the iteration is ancestor-first, I'm having difficulty seeing 
why `should_ignore_subtree_split_commit()` would want to do an 
ancestor traversal at all. It already sees the commits ancestor-first. 
But there could be a reason that I don't know.

Ah, I was only looking at this patch, not how it was called. That begs 
the question "what's the point of these checks if we've already visited 
all the ancestors anyway". I think the answer is that it is pruning the 
recursion that happens in check_parent() and checking the commits that 
come from that rev-list command is pointless. The regression test 
introduced with this function only looks at $extracount which comes from 
the recursion. I haven't looked too closely but it would be nice if we 
could move this check so it is only run when check_parents() is recursing.

Sorry that's not quite right. check_parents() recurses into 
process_split_commit() rather than the loop that call 
should_ignore_subtree_split_commit(). I think what this check does do is 
prune some parents which stops check_parents() from recursing into other 
subtrees so the check is in the right place.

Thanks

Phillip