Patchwork D8223: merge: respect ui.relative-paths for some warning messages

login
register
mail settings
Submitter phabricator
Date March 5, 2020, 7:58 a.m.
Message ID <differential-rev-PHID-DREV-yib5i3pf2mfud7sezeeb-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45488/
State Superseded
Headers show

Comments

phabricator - March 5, 2020, 7:58 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I didn't bother adding tests. It doesn't seem worth having tests for
  every place we use getuipathfn(). I've tested it manually.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

AFFECTED FILES
  mercurial/merge.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - March 5, 2020, 8:32 a.m.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  Adding test for this would be better. It clarify the intend and prevent regression.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 1:56 p.m.
martinvonz added a comment.


  In D8223#122355 <https://phab.mercurial-scm.org/D8223#122355>, @marmoute wrote:
  
  > Adding test for this would be better. It clarify the intend and prevent regression.
  
  Tests for each of the strings?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 2:37 p.m.
marmoute added a comment.


  I think we are talking about a handful of different string. Are we not?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 2:54 p.m.
martinvonz added a comment.


  In D8223#122398 <https://phab.mercurial-scm.org/D8223#122398>, @marmoute wrote:
  
  > I think we are talking about a handful of different string. Are we not?
  
  Yes, but I trust the uipathfn to work as it does elsewhere. I'll let reviewers decide to take this as is or not. Otherwise I'll just abandon it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 2:55 p.m.
marmoute added a comment.


  I am confused. Why can't we get a handful of case added to the test ? I assume these message are alredy tested and would be easy to retest.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 3:18 p.m.
marmoute added a comment.


  Chatted with Raphaël to try to understand where a misunderstanding could lie here.
  
  So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the `uipathfn` call would silently regress your fix.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 3:20 p.m.
martinvonz added a comment.


  In D8223#122410 <https://phab.mercurial-scm.org/D8223#122410>, @marmoute wrote:
  
  > Chatted with Raphaël to try to understand where a misunderstanding could lie here.
  > So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the `uipathfn` call would silently regress your fix.
  
  Yes, that's correct.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 3:21 p.m.
marmoute added a comment.


  Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 4:14 p.m.
martinvonz added a comment.


  In D8223#122412 <https://phab.mercurial-scm.org/D8223#122412>, @marmoute wrote:
  
  > Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?
  
  Ah, I think I now understand where the confusion comes from. I initially said "I trust the uipathfn to work as it does elsewhere.", but that was only half the truth -- the other half is that I spotted this little bug and decided to fix it, but I don't really care much about it. Sorry I wasn't clear about that part. So I don't care enough to write tests with relative paths for theses cases. (I also don't care enough to add retroactive tests for D5916 <https://phab.mercurial-scm.org/D5916>, D5936 <https://phab.mercurial-scm.org/D5936>, D5937 <https://phab.mercurial-scm.org/D5937>, D5939 <https://phab.mercurial-scm.org/D5939>, D6264 <https://phab.mercurial-scm.org/D6264>, and probably many more. And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 4:20 p.m.
marmoute added a comment.


  > And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.
  
  I don't understand that part, is there some negation missing?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 5, 2020, 4:27 p.m.
martinvonz added a comment.


  In D8223#122425 <https://phab.mercurial-scm.org/D8223#122425>, @marmoute wrote:
  
  >> And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.
  >
  > I don't understand that part, is there some negation missing?
  
  Oops, should have been "And I *don't* definitely care enough" :P

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 6, 2020, 4:34 p.m.
marmoute added a comment.


  Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.
  
  I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 6, 2020, 4:35 p.m.
martinvonz added a comment.


  In D8223#122728 <https://phab.mercurial-scm.org/D8223#122728>, @marmoute wrote:
  
  > Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.
  > I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.
  
  nit: s/twice/10 times/

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 6, 2020, 4:39 p.m.
marmoute added a comment.


  Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 6, 2020, 4:46 p.m.
martinvonz added a comment.


  In D8223#122733 <https://phab.mercurial-scm.org/D8223#122733>, @marmoute wrote:
  
  > Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)
  
  I've said before that I'll let other reviewers decide if they want the patch or if I should abandon it. Talking about it more just makes me tired.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8223/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8223

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -2420,6 +2420,8 @@ 
                     hint = _(b"commit or update --clean to discard changes")
                     raise error.Abort(msg, hint=hint)
 
+        uipathfn = scmutil.getuipathfn(repo)
+
         # Prompt and create actions. Most of this is in the resolve phase
         # already, but we can't handle .hgsubstate in filemerge or
         # subrepoutil.submerge yet so we have to keep prompting for it.
@@ -2427,7 +2429,7 @@ 
             f = b'.hgsubstate'
             m, args, msg = actionbyfile[f]
             prompts = filemerge.partextras(labels)
-            prompts[b'f'] = f
+            prompts[b'f'] = uipathfn(f)
             if m == ACTION_CHANGED_DELETED:
                 if repo.ui.promptchoice(
                     _(
@@ -2493,7 +2495,7 @@ 
                     b"note: possible conflict - %s was renamed "
                     b"multiple times to:\n"
                 )
-                % f
+                % uipathfn(f)
             )
             for nf in sorted(fl):
                 repo.ui.warn(b" %s\n" % nf)
@@ -2505,10 +2507,10 @@ 
                     b"note: possible conflict - %s was deleted "
                     b"and renamed to:\n"
                 )
-                % f
+                % uipathfn(f)
             )
             for nf in sorted(fl):
-                repo.ui.warn(b" %s\n" % nf)
+                repo.ui.warn(b" %s\n" % uipathfn(nf))
 
         ### apply phase
         if not branchmerge:  # just jump to the new rev