Looks good, but I find it isn't easy to parse the meaning of
getuipathfn(repo, forcevalue=True). Perhaps it can be spelled as
forcerelative=True.

+def getuipathfn(repo, legacyvalue=False, forcevalue=None):

+ if forcevalue is not None:
+ relative = forcevalue
+ else:
+ config = repo.ui.config('ui', 'relative-paths')
+ if config == 'legacy':
+ relative = legacyvalue
+ else:
+ relative = stringutil.parsebool(config)

If we want to report an invalid boolean value, we still need to use
ui.configbool().

In D5800#85128, @yuja wrote:

Looks good, but I find it isn't easy to parse the meaning of
getuipathfn(repo, forcevalue=True). Perhaps it can be spelled as
forcerelative=True.

The problem is that this an override value for a boolean value, so it's easy to mistake forcerelative=False to mean "don't force it to be relative". That's why I went for forcevalue (meaning "force to this value"). I agree that it can still easily be read as "force a value". Perhaps forceto works better, although it's a bit unconventional? I'll change it to that and you can tell me if it seems worse.

+def getuipathfn(repo, legacyvalue=False, forcevalue=None):
+ if forcevalue is not None:
+ relative = forcevalue
+ else:
+ config = repo.ui.config('ui', 'relative-paths')
+ if config == 'legacy':
+ relative = legacyvalue
+ else:
+ relative = stringutil.parsebool(config)

If we want to report an invalid boolean value, we still need to use
ui.configbool().

I had not realized that we errored out when attempting to use a bad config value. Anyway, I did what you suggested at first, but then I got the following from test-check-config.t:

+              relative = repo.ui.configbool('ui', 'relative-paths')
+  conflict on ui.relative-paths: ('bool', '') != ('str', '')
+  at mercurial/scmutil.py:748:

I just checked what we do for ui.color (which accepts e.g. "auto" and booleans). We don't report bad values there either. That doesn't mean it's right, of course. But I guess I'll have to duplicate the exception-raising rather than calling into ui.configbool() in order keep test-check-config.t happy.

> Looks good, but I find it isn't easy to parse the meaning of
>  `getuipathfn(repo, forcevalue=True)`. Perhaps it can be spelled as
>  `forcerelative=True`.
The problem is that this an override value for a boolean value, so it's easy to mistake `forcerelative=False` to mean "don't force it to be relative". That's why I went for `forcevalue` (meaning "force to this value"). I agree that it can still easily be read as "force a value". Perhaps `forceto` works better, although it's a bit unconventional? I'll change it to that and you can tell me if it seems worse.

What I thought confusing is scmutil.getuipathfn(ctx.repo(), legacyvalue=True)
in D5801. "What does the True mean? relative, absolute, or a complete
different stuff?"

I still think somehow including "relative" in the variable name is better than
"forceto" or "forcevalue".

In D5800#85220, @yuja wrote:

> Looks good, but I find it isn't easy to parse the meaning of
>  `getuipathfn(repo, forcevalue=True)`. Perhaps it can be spelled as
>  `forcerelative=True`.
The problem is that this an override value for a boolean value, so it's easy to mistake `forcerelative=False` to mean "don't force it to be relative". That's why I went for `forcevalue` (meaning "force to this value"). I agree that it can still easily be read as "force a value". Perhaps `forceto` works better, although it's a bit unconventional? I'll change it to that and you can tell me if it seems worse.

What I thought confusing is scmutil.getuipathfn(ctx.repo(), legacyvalue=True)
in D5801. "What does the True mean? relative, absolute, or a complete
different stuff?"

Same reason it's confusing, I believe: it's unclear if "legacyvalue=True" means "use the legacy value" (incorrect) or "for the legacy value, use the value True" (correct). I was hoping the "value" part would clarify that, but I agree that it's still not clear. I think you're also saying that the fact that the function deals with producing a cwd-relative or absolute (well, repo-relative) is also not clear and I agree with that too. Maybe "usedtoberelative=True"?

I still think somehow including "relative" in the variable name is better than
"forceto" or "forcevalue".

Sure, I can change it to "forcerelative". It will be used in a single place (because there's only one existing config variable about relative paths), so it doesn't matter much anyway.

> What I thought confusing is `scmutil.getuipathfn(ctx.repo(), legacyvalue=True)`
>  in https://phab.mercurial-scm.org/D5801. "What does the `True` mean? relative, absolute, or a complete
>  different stuff?"
Same reason it's confusing, I believe: it's unclear if "legacyvalue=True" means "use the legacy value" (incorrect) or "for the legacy value, use the value True" (correct). I was hoping the "value" part would clarify that, but I agree that it's still not clear.  I think you're also saying that the fact that the function deals with producing a cwd-relative or absolute (well, repo-relative) is also not clear and I agree with that too.

Yes. force/legacyvalue doesn't provide what the value means, which is my
point. I also get your point. Naming is hard.

Maybe "usedtoberelative=True"?

As a non-native speaker, a single word "legacy" is easier to parse than a
phrase "used to be".

In D5800#85228, @yuja wrote:

> What I thought confusing is `scmutil.getuipathfn(ctx.repo(), legacyvalue=True)`
>  in https://phab.mercurial-scm.org/D5801. "What does the `True` mean? relative, absolute, or a complete
>  different stuff?"
Same reason it's confusing, I believe: it's unclear if "legacyvalue=True" means "use the legacy value" (incorrect) or "for the legacy value, use the value True" (correct). I was hoping the "value" part would clarify that, but I agree that it's still not clear.  I think you're also saying that the fact that the function deals with producing a cwd-relative or absolute (well, repo-relative) is also not clear and I agree with that too.

Yes. force/legacyvalue doesn't provide what the value means, which is my
point. I also get your point. Naming is hard.

Maybe "usedtoberelative=True"?

As a non-native speaker, a single word "legacy" is easier to parse than a
phrase "used to be".

So do we prefer legacyrelativevalue then? Or legacywasrelative? Or legacyrelative? (I think the last one is least clear.)

So do we prefer `legacyrelativevalue` then? Or `legacywasrelative`? Or `legacyrelative`? (I think the last one is least clear.)

legacyrelativevalue sounds good to me.

In D5800#85624, @yuja wrote:

So do we prefer `legacyrelativevalue` then? Or `legacywasrelative`? Or `legacyrelative`? (I think the last one is least clear.)

legacyrelativevalue sounds good to me.

Okay, I will update the patch with that name.

Maybe a py3 thing?

$ py -3 run-tests.py --local test-http.t
running 1 tests using 1 parallel processes

--- c:/Users/Matt/hg/tests/test-http.t
+++ c:/Users/Matt/hg/tests/test-http.t.err
@@ -1,58 +1,262 @@
 #require serve

   $ hg init test
+  ** unknown exception encountered, please report by visiting
+  ** https://mercurial-scm.org/wiki/BugTracker
+  ** Python 3.7.1 (v3.7.1:260ec2c36a, Oct 20 2018, 14:57:15) [MSC v.1915 64 bit (AMD64)]
+  ** Mercurial Distributed SCM (version 4.9+253-acd3eda2aa5b)
+  ** Extensions loaded:
+  Traceback (most recent call last):\r (esc)
+    File "c:\\Users\\Matt\\hg\\hg", line 43, in <module>\r (esc)
+      dispatch.run()\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\dispatch.py", line 99, in run\r (esc)
+      status = dispatch(req)\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\dispatch.py", line 225, in dispatch\r (esc)
+      ret = _runcatch(req) or 0\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\dispatch.py", line 376, in _runcatch\r (esc)
+      return _callcatch(ui, _runcatchfunc)\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\dispatch.py", line 384, in _callcatch\r (esc)
+      return scmutil.callcatch(ui, func)\r (esc)
+    File "C:\\Program Files\\Python37\\Lib\\importlib\\util.py", line 245, in __getattribute__\r (esc)
+      self.__spec__.loader.exec_module(self)\r (esc)
+    File "<frozen importlib._bootstrap_external>", line 724, in exec_module\r (esc)
+    File "<frozen importlib._bootstrap_external>", line 860, in get_code\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\__init__.py", line 295, in source_to_code\r (esc)
+      return super(hgloader, self).source_to_code(data, path)\r (esc)
+    File "<frozen importlib._bootstrap_external>", line 791, in source_to_code\r (esc)
+    File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed\r (esc)
+    File "c:\\Users\\Matt\\hg\\mercurial\\scmutil.py", line 741\r (esc)
+          """""\r (esc)
+  SyntaxError: cannot mix bytes and nonbytes literals\r (esc)
+  [1]
   $ cd test