Page MenuHomePhabricator

error: normalize "unresolved conflicts" error messages with a custom class
ClosedPublic

Authored by dploch on Jul 8 2020, 6:50 PM.

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

dploch created this revision.Jul 8 2020, 6:50 PM
mjacob added a subscriber: mjacob.Jul 8 2020, 8:20 PM

Can you separate the change from "unresolved conflicts (see hg resolve, then hg rebase --continue)" to "unresolved conflicts (see 'hg resolve', then 'hg rebase --continue')", to make the diff less noisy?

Of course, it feels stupid to change code that’s going to be replaced in the next patch. Still, I think it makes things easier to review (at least that’s my opinion, I don’t know if more experienced reviewers agree).

Can you separate the change from "unresolved conflicts (see hg resolve, then hg rebase --continue)" to "unresolved conflicts (see 'hg resolve', then 'hg rebase --continue')", to make the diff less noisy?

Done. Added a temporary legacy hook which is removed in D8730; all the .t file updates are moved to that revision as well.

Of course, it feels stupid to change code that’s going to be replaced in the next patch. Still, I think it makes things easier to review (at least that’s my opinion, I don’t know if more experienced reviewers agree).

I suppose it's nice in this case because it confirms that it's only the rebase message that's changing (whereas otherwise, you'd need to check every .t file to see this).

mjacob added inline comments.Mon, Jul 13, 12:34 AM
mercurial/error.py
117

I think it would have been easier to have

  • the first patch changing the message in rebase.py and tests, and
  • the second patch introducing the ConflictResolutionRequired class.

Generally, patches should not depend on other patches applied later. "Depend" is a bit vague. If the tests fail in between, it’s of course a forbidden dependency. If the tests pass in between, it might be acceptable. Still, in this case, probably both patches should be committed at the same time. If you organize like I proposed above, the text change can be committed independently. Also the total length of both patches would be shorter.

dploch added inline comments.Mon, Jul 13, 7:47 PM
mercurial/error.py
117

Done.

pulkit added a subscriber: pulkit.Tue, Jul 14, 2:50 PM
pulkit added inline comments.
hgext/rebase.py
26

unrelated change

mercurial/error.py
118

Seems like this if is not required anymore after D8730.

mercurial/shelve.py
59

unrelated change

dploch updated this revision to Diff 21903.Tue, Jul 14, 4:38 PM
dploch added inline comments.Tue, Jul 14, 4:42 PM
hgext/rebase.py
26

Removed. Seems like this is a difference between black 19.3b and 19.10b

mercurial/error.py
118

Fixed - I had made the change earlier but due to a bug in Phabricator the upload failed. Martinvonz fixed it so this should be up to date now.

mercurial/shelve.py
59

Removed.

marmoute accepted this revision.Wed, Jul 15, 1:34 PM
marmoute added a subscriber: marmoute.

Looks like a step in the right direction.

pulkit accepted this revision.Fri, Jul 17, 1:06 PM
This revision is now accepted and ready to land.Fri, Jul 17, 1:06 PM