Rewritten commits missing from ancestry

[dastbe]

Sharing an example from a coworker

> git sl
◇ 34c1ac8 28m (main) init
┣━┓
┃ ✕ 578fbe1 25m (rewritten as 3bced589) commit (a)
┃ ┣━┓
┃ ┃ ◯ 107ede8 24m commit (b)
┃ ┃ ┃
┃ ┃ ● 3d6a46b 24m (feature_1) commit (c)
┃ ┃
┃ ◯ f603f49 20m commit (d)
┃ ┃
┃ ◯ 04b7ef3 20m (feature_2) commit (e)
┃
◯ 3bced58 17m commit (a)
hint: there are 2 abandoned commits in your commit graph
hint: to fix this, run: git restack
hint: disable this hint by running: git config --global branchless.hint.smartlogFixAbandoned false

and then running the following

> git query 'ancestors(107ede8)'
34c1ac8 init
107ede8 commit (b)

It's surprising behavior that no form of commit (a) appears in the ancestry of commit (b). Is this an unintended effect of #430?

[arxanas]

Hi @dastbe, yeah, I think it's an unintended effect of #430. The decision of which commits to include is a little difficult; I guess we actually want to respect the definition of "visible" commit?

There should also be a --hidden option to git query (and other revset-accepting commands) to produce hidden commits in the results.

If you want to fix this issue, I'm happy to guide you. It should be as simple as replacing the computation of "difference with hidden commits" to "intersection with active commits", where Context::query_active_commits is already implemented and just needs to be used.

[dastbe]

sure! though looking at the code it seems like https://github.com/arxanas/git-branchless/blob/master/git-branchless/src/revset/resolve.rs#L104 is the point that needs to be changed, and context isn't wired into resolve.

it seems like https://github.com/arxanas/git-branchless/blob/master/git-branchless/src/revset/eval.rs#L64-L71 should be implemented on Dag directly?

[arxanas]

@dastbe It looks like you're right about whether it should be changed.

For the query_active_heads function, there's already some similar functions in Dag. Actually, you might be able to remove and consolidate some of the functions, because it looks like there's several other places that manually difference the obsolete commits.

For example, we might not need the observed_commits parameter to Dag::query_active_heads. My guess is that it exists because of how we handled rendering the smartlog for git undo, which needs to render smartlogs at arbitrary points in the past, but it probably became unnecessary at some point.

Feel free to join the Discord server if you prefer live chat for asking questions.

[dastbe]

finally got around to trying to fix this, and it looks like your refactor to revsets and querying has handled this much more comprehensively by this change and others. thanks!