Page MenuHomePhabricator

merge: replace calls to hg.updaterepo() by merge.update()
ClosedPublic

Authored by martinvonz on Sep 21 2020, 4:36 PM.

Details

Summary

The former no longer buys us anything.

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.Sep 21 2020, 4:36 PM
martinvonz updated this revision to Diff 22808.Sep 23 2020, 1:28 PM
pulkit added a subscriber: pulkit.Sep 24 2020, 3:08 AM

While I agree that resulting API is better than the previous one, however I think:

  • This is an API breakage we can prevent
  • merge.update() used to do a different thing before and now does something else before

Although, we don't provide any API's compatibility but preventing such breakages when we can sound a good thing to me. I think we can leave hg.updaterepo() as it is.

While I agree that resulting API is better than the previous one, however I think:

  • This is an API breakage we can prevent
  • merge.update() used to do a different thing before and now does something else before

Want me to add a new merge.plain_update() or something like that instead?

Although, we don't provide any API's compatibility but preventing such breakages when we can sound a good thing to me. I think we can leave hg.updaterepo() as it is.

Sure, I can do that. So just leave it without callers you mean? Or drop this patch completely?

While I agree that resulting API is better than the previous one, however I think:

  • This is an API breakage we can prevent
  • merge.update() used to do a different thing before and now does something else before

Want me to add a new merge.plain_update() or something like that instead?

Although, we don't provide any API's compatibility but preventing such breakages when we can sound a good thing to me. I think we can leave hg.updaterepo() as it is.

Sure, I can do that. So just leave it without callers you mean? Or drop this patch completely?

Let's leave hg.updaterepo() for now. If we want to remove, we can add a deprecation warning for few/one releases.

While I agree that resulting API is better than the previous one, however I think:

  • This is an API breakage we can prevent
  • merge.update() used to do a different thing before and now does something else before

Want me to add a new merge.plain_update() or something like that instead?

How about this question? Do you want me to call the new function merge.plain_update() instead? Another option is to do that while leaving merge.update() as a proxy to merge._update() with a call to ui.deprecwarn(). I'm fine with any of these 3 approaches (I'd prefer what I currently have -- it should be pretty simple for callers to check if _update() exists and prefer that for compatibility until they've finished a migration).

Although, we don't provide any API's compatibility but preventing such breakages when we can sound a good thing to me. I think we can leave hg.updaterepo() as it is.

Sure, I can do that. So just leave it without callers you mean? Or drop this patch completely?

Let's leave hg.updaterepo() for now. If we want to remove, we can add a deprecation warning for few/one releases.

Sure, will do. I'll assume you mean to leave it without callers (i.e. to not drop this entire patch).

martinvonz retitled this revision from merge: delete hg.updaterepo() and use merge.update() instead (API) to merge: replace calls to hg.updaterepo() by merge.update().Sep 24 2020, 11:23 AM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 22834.

While I agree that resulting API is better than the previous one, however I think:

  • This is an API breakage we can prevent
  • merge.update() used to do a different thing before and now does something else before

Want me to add a new merge.plain_update() or something like that instead?

How about this question? Do you want me to call the new function merge.plain_update() instead? Another option is to do that while leaving merge.update() as a proxy to merge._update() with a call to ui.deprecwarn(). I'm fine with any of these 3 approaches (I'd prefer what I currently have -- it should be pretty simple for callers to check if _update() exists and prefer that for compatibility until they've finished a migration).

I think a deprecation warning will be nice. However, I checked that (in all cases?) the old callers will end up with an error when trying to call a new function the old way, so we can document the replacement in the new merge.update() and that should be okay too I guess.

Although, we don't provide any API's compatibility but preventing such breakages when we can sound a good thing to me. I think we can leave hg.updaterepo() as it is.

Sure, I can do that. So just leave it without callers you mean? Or drop this patch completely?

Let's leave hg.updaterepo() for now. If we want to remove, we can add a deprecation warning for few/one releases.

Sure, will do. I'll assume you mean to leave it without callers (i.e. to not drop this entire patch).

Yes, function with no callers but some deprecation warning.

pulkit accepted this revision.Sep 26 2020, 6:27 AM

Do you plan to add a deprecation warning for updaterepo?

This revision is now accepted and ready to land.Sep 26 2020, 6:27 AM

Do you plan to add a deprecation warning for updaterepo?

Sorry, I just forgot about this review. I will send a patch.