( )⚙ D4713 largefiles: automatically load largefiles extension when required (BC)

This is an archive of the discontinued Mercurial Phabricator instance.

largefiles: automatically load largefiles extension when required (BC)
ClosedPublic

Authored by indygreg on Sep 24 2018, 12:12 PM.

Details

Summary

This is very similar to what we just did for LFS but for largefiles.
See recent commit messages for the rationale here.

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

indygreg created this revision.Sep 24 2018, 12:12 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Sep 28 2018, 10:25 PM
  1. Map of requirements to list of extensions to load automatically when
  2. requirement is present. autoextensions = {

+ b'largefiles': [b'largefiles'],

    b'lfs': [b'lfs'],
}

Can we add some warnings here? The largefiles is IMHO one of the most buggy
extensions, and loading it has a side effect (e.g. fsmonitor is disabled.)
It shouldn't be silently loaded into the process.

I didn't realize largefiles is riddled with bugs. I'd be happy to drop this changeset if there are concerns about its behavior.

In D4713#72604, @yuja wrote:
  1. Map of requirements to list of extensions to load automatically when
  2. requirement is present. autoextensions = {

+ b'largefiles': [b'largefiles'],

    b'lfs': [b'lfs'],
}

Can we add some warnings here? The largefiles is IMHO one of the most buggy
extensions, and loading it has a side effect (e.g. fsmonitor is disabled.)
It shouldn't be silently loaded into the process.

I get what you’re thinking, but maybe a debug message is better than a warning, even though it’s less visible. I’m concerned about every invocation being unnecessarily noisy now that .hg/hgrc isn’t updated, and noise on stderr triggers that thgw.exe dialog when UAC is enabled. OTOH, updating the hgrc with this would make for a quick and dirty one shot warning.

yuja added a comment.Oct 2 2018, 10:59 AM
> Can we add some warnings here? The largefiles is IMHO one of the most buggy
>  extensions, and loading it has a side effect (e.g. fsmonitor is disabled.)
>  It shouldn't be silently loaded into the process.
I get what you’re thinking, but maybe a debug message is better than a warning, even though it’s less visible. I’m concerned about every invocation being unnecessarily noisy now that .hg/hgrc isn’t updated,

I don't think a debug message would work because ordinary users will never
see it. Is that common to work on largefiles repo without enabling the
extension globally?

and noise on stderr triggers that thgw.exe dialog when UAC is enabled.

Well, that's the bug of TortoiseHg if it doesn't redirect ui.ferr.

FWIW, I like the idea of enabling storage extensions automatically.

In D4713#73077, @yuja wrote:
> Can we add some warnings here? The largefiles is IMHO one of the most buggy
>  extensions, and loading it has a side effect (e.g. fsmonitor is disabled.)
>  It shouldn't be silently loaded into the process.
I get what you’re thinking, but maybe a debug message is better than a warning, even though it’s less visible. I’m concerned about every invocation being unnecessarily noisy now that .hg/hgrc isn’t updated,

I don't think a debug message would work because ordinary users will never
see it. Is that common to work on largefiles repo without enabling the
extension globally?

The only data point I have is what I did, which was to only enable it on the command line when cloning. The clone command modifying the local hgrc if necessary handled the rest. I only started to do that because I got annoyed at how simply having it loaded could alter certain commands. IDK if command line enabling was ever documented as the preferred alternative, but that's how I described it in e1dbe0b215ae.

At this point, I can't recall specific issues with globally enabling, and they might be mostly fixed. They definitely aren't all fixed. That said, this implicit loading on demand seems roughly equivalent to the previous behavior, so I'm not sure that this would be any more surprising. Repos on the command line would be isolated, but not with commandserver because of the global function wrapping. But I'm guessing most people don't understand that about commandserver.

yuja added a comment.Oct 3 2018, 10:26 AM
The only data point I have is what I did, which was to only enable it on the command line when cloning.  The clone command modifying the local hgrc if necessary handled the rest.  I only started to do that because I got annoyed at how simply having it loaded could alter certain commands.  IDK if command line enabling was ever documented as the preferred alternative, but that's how I described it in https://phab.mercurial-scm.org/rHGe1dbe0b215ae137eec53ceb12440536d570a83d2.
At this point, I can't recall specific issues with globally enabling, and they might be mostly fixed.  They definitely aren't all fixed.  That said, this implicit loading on demand seems roughly equivalent to the previous behavior, so I'm not sure that this would be any more surprising.  Repos on the command line would be isolated, but not with commandserver because of the global function wrapping.  But I'm guessing most people don't understand that about commandserver.

IIUC, we would have to enable the largefiles extension once to clone the
repo, but that's no longer needed. So you can enable the extension without
hearing its name at all.

Perhaps, a warning can be displayed when the non-core repository requirement
is added?

In D4713#73260, @yuja wrote:

IIUC, we would have to enable the largefiles extension once to clone the
repo, but that's no longer needed. So you can enable the extension without
hearing its name at all.
Perhaps, a warning can be displayed when the non-core repository requirement
is added?

I'll take a look and see what I can figure out, but it likely won't be until next week. I think it should maybe be limited to a clone/unbundle adding a requirement with an implicitly loaded extension.