indygreg added subscribers: yuja, indygreg.
indygreg added a comment.
I only looked at some of this code.
Overall, my threshold for adding to core is low. I think we should land then
iterate.
I would **really** like to see functionality related to modifying the
behavior of annotate live in core. I think it will be easier to comprehend when
implemented that way. I envision someday having arguments passed to
`annotate()` that control whether linelog is used, etc. We may end up inventing
something like `diffopts` but for annotate operations.
It might help to think of this extension as two features: a cache for linelog
data and user-facing UI to use it. The former is the most important to move to
core. Especially since I imagine we don't want an `hg fastannotate` command
lingering around forever (we would want to consolidate with `hg annotate` I
think).
The most concerning part of this extension is the wire protocol
functionality. It is not suitable to ship to end-users because of forward
compatibility concerns.
The wire protocol functionality is literally transferring files off disk with
little to no regard for the format of those files. The wire protocol doesn't
expose the version of the files on disk. And that's obviously bad for future
compatibility. We need some kind of compatibility checking that the client is
capable of understanding the data the server will send it. This could be as
simple as the server capabilities string advertising the on-disk format and
clients ensuring they can speak the server's format before attempting to fetch
that data.
Of course, there could be a mix of data versions on disk given the current
simple storage mechanism. We probably want to use separate directories for each
version of the cached data so we don't mix streams. Or at the very least the
server needs to validate that on-disk data it is serving matches the expected
version from the client. Otherwise e.g. a server could serve up an old file
version. Or a downgraded server could serve up a new file version.
I think the most robust way forward is to have version-specific capabilities,
wire protocol commands, and on-disk locations. This leaves the least margin for
error and yields the greatest flexibility for changing formats and exchange
semantics later without breaking BC.
INLINE COMMENTS
> __init__.py:27
> +# forward, usually it is "master" or "@".
> +mainbranch = master
> +
`branch` here is probably not the correct terminology.
> __init__.py:41
> +# (default: fastannotate)
> +modes = fastannotate
> +
I would prefer if there were a separate boolean option for each mode. That way,
individual .hg/hgrc files can override specific behavior without overriding
global defaults. It is a more flexible config model. We can, of course, have
some options imply others.
> __init__.py:44
> +# default format when no format flags are used (default: number)
> +defaultformat = changeset, user, date
> +
Should this be a template?
> __init__.py:57
> +# (default: True for remotefilelog repo, False otherwise)
> +client = True
> +
This should be a more description option, like `exchange-cache` or `cache-pull`.
> __init__.py:62-67
> +# share sshpeer with remotefilelog. this would allow fastannotate to peek
> +# into remotefilelog internals, and steal its sshpeer, or in the reversed
> +# direction: donate its sshpeer to remotefilelog. disable this if
> +# fastannotate and remotefilelog should not share a sshpeer when their
> +# endpoints are different and incompatible. (default: True)
> +clientsharepeer = True
Since remotefilelog is not part of core, we probably want to nuke this feature.
> __init__.py:176-177
> +
> +# fastannotate has its own locking, without depending on repo lock
> +localrepo.localrepository._wlockfreeprefix.add('fastannotate/')
> +
I think this wants to be in `extsetup()` since it mutates global module state.
Ideally it would be part of `reposetup()` since it only matters on repos that
have this extension enabled.
(This comes into play in e.g. hgweb environments, where some repos may not have
the extension enabled.)
> __init__.py:182
> +if client is None:
> +if util.safehasattr(repo, 'requirements'):
> +client = 'remotefilelog' in repo.requirements
Won't `repo.requirements` always be defined?
> context.py:759-772
> +@contextlib.contextmanager
> +def _lockflock(self):
> +"""the same as 'lock' but use flock instead of lockmod.lock, to avoid
> +creating temporary symlinks."""
> +import fcntl
> +lockpath = self.linelogpath
> +util.makedirs(os.path.dirname(lockpath))
This wants to move to a `utils` module somewhere.
> formatter.py:3
> +#
> +# format: defines the format used to output annotate result
> +#
I'm sure @yuja has opinions about this file.
> protocol.py:43
> +# output:
> +# FILE :=
