Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

This revision now requires changes to proceed.Dec 2 2019, 6:45 PM

In D2383#56176, @olasd wrote:

Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

We should also add a comment in the id generation to explain that we're using the cycle detection feature, because that's not obvious at all here.

In D2383#56176, @olasd wrote:

Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

Oh I see, I need to dig further on this then.

We should also add a comment in the id generation to explain that we're using the cycle detection feature, because that's not obvious at all here.

Indeed, I thought it was related to another error I did not know of. I will update accordingly.

If there's a bug here it should be fixed in that function (and a regression test should be added).

Turns out I can not reproduce anymore the cycles issue I encountered so I must have introduced it by hacking on the strategy implementation.

Initially I stumbled across aliases chain in the snapshot content generated by hypothesis that was making a test failed for swh-web
as it does not handle such cases (fixed in D2384). I ended up getting alias cycles somehow in the generated data but I must have messed
up somewhere.

So I will simply update that diff to add the comment about the cycles detection in the strategy.

Update: There was no cycles generation between aliases issue after all, probably myself that messed up somewhere
when playing with hypothesis.

So modify that diff to simply add a comment in the strategy implementation to indicate the last post processing step
will remove any generated cycles if any.

Build is green
See https://jenkins.softwareheritage.org/job/DMOD/job/tox/159/ for more details.

Harbormaster completed remote builds in B9476: Diff 8401.Dec 2 2019, 8:01 PM

anlambert retitled this revision from hypothesis: Forbid branch alias cycles generation in snapshots strategy to hypothesis_strategies/snapshots: Explain last post-processing step.Dec 2 2019, 8:02 PM

anlambert edited the summary of this revision. (Show Details)

Thanks for the investigation!

This revision is now accepted and ready to land.Dec 3 2019, 10:35 AM

Closed by commit rDMOD7b6f474533cb: hypothesis_strategies/snapshots: Explain last post-processing step (authored by anlambert). · Explain WhyDec 3 2019, 10:38 AM

This revision was automatically updated to reflect the committed changes.

anlambert added a commit: rDMOD7b6f474533cb: hypothesis_strategies/snapshots: Explain last post-processing step.