[PATCH 1 of 6 path intent] ui: add an intent to expandpath

[Mads Kiilerich]

On 09/30/2014 12:53 AM, Gregory Szorc wrote:
> On 9/29/14 2:43 PM, Mads Kiilerich wrote:
>> On 09/29/2014 07:30 AM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1411967336 25200
>>> #      Sun Sep 28 22:08:56 2014 -0700
>>> # Node ID 2f06625fe4d502f43294e9087d9a83398a36fe9f
>>> # Parent  4109cc16279ef0e04dc70e7f4c9ab7415e6a22d3
>>> ui: add an intent to expandpath
>>>
>>> This patch adds an optional named argument to ui.expandpath() that
>>> defines the intended use of a repository.
>>>
>>> The impetus behind this patch is to allow extensions to provide
>>> more advanced and dynamic repository lookup functionality. For
>>> example, there exists an extension at Mozilla which automagically
>>> defines common repository URLs for the many Firefox repositories.
>>> We want read operations hitting read-only slaves over HTTP(S) and
>>> write operations hitting the master over SSH. Today, we have to
>>> use separate names for separate URLs, which increases cognitive
>>> burden and sometimes results in accidental use of the wrong repo.
>>> We desire built-in hg operations to automagically use the URL for
>>> the task at hand. This change and subsequent additions to define
>>> the intent parameter will enable this through monkeypatching
>>> ui.expandpath().
>>
>> Meh...
>>
>> First: It doesn't work to add "features" to Mercurial without any usage
>> or test in Mercurial - especially not without any reference to exactly
>> what it is that need this feature. That would be unmaintainable.
>>
>> I assume the intent parameter should be considered some enumeration of
>> some well-known path types - that do not seem like a nice API, but we
>> would at least need a definition of which values are valid and what they
>> mean.
>>
>> Also, Incoming should be like a preview of pull and outgoing like a
>> preview of push. They should thus be treated exactly the same and it
>> seems wrong that they have different "intents".
>>
>> The existing handling of push/pull paths is also a bit not-so-elegant.
>> The default and -push handling is duplicated in all the places it is 
>> used.
>>
>> Instead, I suggest giving the ui class a couple of extra methods:
>>
>>      def pushpath(self, loc):
>>          """Return path (which might be a URL) for pushing to loc.
>>          loc will be 'default' if not specified.
>>          If paths.loc is configured, use that instead of loc.
>>          However, if paths.loc-push is configured, prefer that.
>>          """
>>      def pullpath(loc):
>>          """Return path (which might be a URL) for pulling from loc.
>>          loc will be 'default' if not specified.
>>          If paths.loc is configured, use that instead of loc.
>>          """
>>
>> (This docstring also specify that the -push suffix should apply to other
>> paths than default. I find that an obvious extension that it would be
>> hard not to implement accidentally.)
>>
>> (I have always found the special -push suffix a bit odd. Having a
>> special -pull suffix seemed more obvious and safe to me. For symmetry
>> reasons I would like to have both so I could specify default-push and
>> default-pull and know that they "always" only would be used for their
>> intended purpose.)
>>
>> I assume your extensions could do their job by monkeypatching these
>> methods? Or why not? What do these extensions do?
>
> My primary case was documented in the commit message.

Anecdotal evidence is no evidence. Do you have a link to the extension 
so we can see what problem you are solving? How will it look when using 
the API you suggest?

> I think adding explicit APIs for pushpath() and pullpath() would be 
> sufficient for my needs. Although, outgoing could fall into either camp

How is that? Outgoing is the planning phase of a push. How can it be 
pull-ish?

> I don't feel comfortable bloating scope to include adding magical 
> "-push" for every path.

It sounds to me like that is exactly what you need to solve the problem 
you describe. That would make it possible to replace your extension with 
upstream functionality with trivial configuration.

/Mads