Page MenuHomePhabricator

rewriteutil: look up common predecessor on unfiltered repo
ClosedPublic

Authored by martinvonz on Jun 29 2021, 5:20 PM.

Details

Summary

Before this patch, the code looking for divergence could crash when
tried to look up a common predecessor in the filtered repo. This patch
fixes that by looking up the common predecessor in an unfiltered repo.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Jun 29 2021, 5:20 PM
marmoute requested changes to this revision.Jun 30 2021, 9:56 AM
marmoute added a subscriber: marmoute.

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

This revision now requires changes to proceed.Jun 30 2021, 9:56 AM

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

No, given the information returned by find_new_divergence_from() it seems unavoidable that filtered revision might be returned as the "common-predecessor" value. (sidenote: that the common-precessors should probably be a list, and the return of find_new_divergence_from and _find_new_divergence should probably be list too).

My point is that we are looking for divergence within the current filter and that we should not pass unfiltered repository to the function that looks for that divergence. Otherwise it might compute things on the wrong set of revisions.

However, function along the chain should be prepared for the "common-predecessor" nodes to be potentially filtered. In this case this means turning this line

return (repo[r], repo[div[0]], repo[div[1]])

to

return (repo[r], repo[div[0]], unfi[div[1]])

(sidenode: the previous if div is actually checking if div is not None, so we should update it too)

martinvonz retitled this revision from rewriteutil: check for divergence on unfiltered repo to rewriteutil: look up common predecessor on unfiltered repo.Jul 1 2021, 1:05 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 28720.

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

No, given the information returned by find_new_divergence_from() it seems unavoidable that filtered revision might be returned as the "common-predecessor" value. (sidenote: that the common-precessors should probably be a list, and the return of find_new_divergence_from and _find_new_divergence should probably be list too).
My point is that we are looking for divergence within the current filter and that we should not pass unfiltered repository to the function that looks for that divergence. Otherwise it might compute things on the wrong set of revisions.
However, function along the chain should be prepared for the "common-predecessor" nodes to be potentially filtered. In this case this means turning this line

return (repo[r], repo[div[0]], repo[div[1]])

to

return (repo[r], repo[div[0]], unfi[div[1]])

I see. I've updated the patch.

Alphare added a subscriber: Alphare.Jul 7 2021, 9:33 AM

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

marmoute accepted this revision.Jul 7 2021, 12:02 PM

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.

Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

(sorry for the delay, I kind of forgot about that patch)

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Alphare accepted this revision.Jul 7 2021, 12:07 PM

I see, that sounds reasonable!

This revision is now accepted and ready to land.Jul 7 2021, 12:07 PM

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

@Alphare, looks like this patch is not queued yet (which is fine). Do you want me to update the series to have a new test case in the first patch instead? That makes more sense (I can still add the current test case specifically for issue 6262), but maybe you've already queued it locally and it'll be more work for you...

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

@Alphare, looks like this patch is not queued yet (which is fine). Do you want me to update the series to have a new test case in the first patch instead? That makes more sense (I can still add the current test case specifically for issue 6262), but maybe you've already queued it locally and it'll be more work for you...

Go ahead, I hadn't queue it yet. You were saved by the making of food!

pulkit accepted this revision.Jul 8 2021, 4:06 AM