Limitations of Emacs' vc when using modern backends

Limitations of Emacs' vc when using modern backends

[Juliusz Chroboczek]

Hi Andre,

In short, the problems with vc could be solved by making the following
three changes.  While I believe that these changes should be fully
backwards-compatible, I'm definitely not claiming I know enough about
the Emacs release schedule to claim they should be in Emacs 22.

1. It should be possible for a backend to override vc-previous-version.

2. vc-previous-version should take the file name in addition to the
   revision.

3. vc should call a backend-specific function (say
   vc-BACKEND-canonical-revision) to normalise a revision name (again,
   one that takes both a revision and a file name) before creating a
   buffer.

(1) CVS uses a very specific model for numbering revisions; not all
systems use sequences of integers that encode a path in an and/or
tree.  Subversion, for example, uses plain integers, while Darcs, Git
and Monotone use opaque tokens (actually SHA1 hashes of something or
something else, but that's irrelevant for our discussion).  Only the
backend can know what the ``previous version'' is.

(2) CVS versions files, while modern systems version trees; the effect
is that the correct ``previous version'' depends on the file name.

To give an example using Subversion, suppose that revision 1 creates
files A and B, revision 2 modifies file A, and revision 3 modifies
file B.  Then vc-darcs-previous-version(3, A) should return 2 (which
it already does), but vc-darcs-previous-version(3, B) should return 1.

(3) Darcs identifies revisions by a 65-character long hash of a bunch
of data, which is not something you want to type.  Because of that,
vc-darcs allows identifying a revision by a number of different means
(see vc-darcs-rev-to-hash if you want the gory details).  This means
that if you use vc-version-other-window and friends, you'll end up
with multiple buffers containing the same revision of a given file.

I believe that the functionality of vc-darcs-rev-to-hash (getting a
canonical revision identifier for a given revision identifier) should
be moved into vc itself, and called whenever vc gets a revision from
the user.  The default implementation would just be the identity, but
it should be possible for a backend to override it.

Because of point (2) above, vc-BACKEND-canonical-revision should be
able to access the file name in addition to the revision.

Thanks for your help,

                                        Juliusz Chroboczek

P.S. I'm not subscribed to the list -- please CC me with any follow-ups.

Re: Limitations of Emacs' vc when using modern backends

[Stefan Monnier]

> 1. It should be possible for a backend to override vc-previous-version.
> 2. vc-previous-version should take the file name in addition to the
>    revision.

Sure.

> 3. vc should call a backend-specific function (say
>    vc-BACKEND-canonical-revision) to normalise a revision name (again,
>    one that takes both a revision and a file name) before creating a
>    buffer.

I'm not sure I understand what it's supposed to do.

> (3) Darcs identifies revisions by a 65-character long hash of a bunch
> of data, which is not something you want to type.  Because of that,
> vc-darcs allows identifying a revision by a number of different means
> (see vc-darcs-rev-to-hash if you want the gory details).

Is this specific to vc-darcs or to darcs?  Could you show what it
does concretely?


        Stefan

Re: Limitations of Emacs' vc when using modern backends

[Juliusz Chroboczek]

>> (3) Darcs identifies revisions by a 65-character long hash of a bunch
>> of data, which is not something you want to type.  Because of that,
>> vc-darcs allows identifying a revision by a number of different means
>> (see vc-darcs-rev-to-hash if you want the gory details).

> Is this specific to vc-darcs or to darcs?  Could you show what it
> does concretely?

Right now, the penultimate revision of vc-darcs.el happens to be

  20051116215244-4cc09-4d8edc6c60cacc62a56bb4480ee9cd80be414c18

Obviously, we don't require the user to remember this; what we do is
that we allow the user to type:

 - any prefix of the revision hash;
 - any prefix of the log message.

If multiple revisions match, the latest one is chosen.

(I'm simplifying somewhat, but that will do for the discussion at hand.)

So I'd usually type ``Update version number to 1.6'', or ``Update
version'' or even just ``Update''.  With the current version of
vc-darcs, I end up with multiple buffers called

  vc-darcs.el~Update
  vc-darcs.el~Update version
  etc.

What I'm asking is the means to have vc-darcs.el normalise such
non-canonical revision identifiers to the canonical hash.

                                        Juliusz

Re: Limitations of Emacs' vc when using modern backends

[Stefan Monnier]

>> Is this specific to vc-darcs or to darcs?  Could you show what it
>> does concretely?

> Right now, the penultimate revision of vc-darcs.el happens to be

>   20051116215244-4cc09-4d8edc6c60cacc62a56bb4480ee9cd80be414c18

> Obviously, we don't require the user to remember this; what we do is
> that we allow the user to type:

>  - any prefix of the revision hash;
>  - any prefix of the log message.

> If multiple revisions match, the latest one is chosen.

I see.
And this is a feature of vc-darcs rather than of darcs itself, right?

> So I'd usually type ``Update version number to 1.6'', or ``Update
> version'' or even just ``Update''.  With the current version of
> vc-darcs, I end up with multiple buffers called

>   vc-darcs.el~Update
>   vc-darcs.el~Update version
>   etc.

Yes, there's a similar problem with VC where you can end up with
foo.el~emacs-unicode-2~.  The problem is not just that you have several
files for the same thing (which is just an inconvenience), but that
the `emacs-unicode-2' revision of foo.el is not fixed (it's not a revision,
it's a branch and implicitly refers to the branch), so if some newer version
is committed and you ask to see it, you get the old one because VC doesn't
realize that the file foo.el~emacs-unicode-2~ is out of date.  The same
thing proably happens for you: "Update" may stand for "Update
version number to 1.3" at some point and to "Update
version number to 1.6" at the other, so the "vc-darcs.el~Update" can become
out of date as well.

> What I'm asking is the means to have vc-darcs.el normalise such
> non-canonical revision identifiers to the canonical hash.

Yes, that makes total sense.
The problem mentioned above for CVS has been known for a while but nobody
has provided a fix for it yet.  `canonical-revision' wouldn't solve the
problem, but would provide a clean place where we could then put the fix.

The only problem I see with `canonical-revision' is that for some backends
it may be very costly (for CVS it requires a round trip to the repository),
and it may suffer from race conditions.

In the case of `vc-version-other-window' for CVS, there would be a better
way to do it: when fetching the file, we can get CVS to tell us which
revision it's returning, so we'd basically get the info for free.

Now `canonical-revision' is more general, but I'm wondering: where would it
be used other than in `vc-version-other-window'?  If not, maybe it'd be
better to change `find-version' so as to return the canonical revision that
it has fetched.


        Stefan


PS: Regarding completion, Arch revision names are also longish (tho
    they're not hashes), so I'm using the patch below, along with a new
    function vc-arch-revision-completion-table.


@@ -1700,6 +1813,8 @@
       (if (vc-workfile-unchanged-p buffer-file-name)
 	  (message "No changes to %s since latest version" file)
 	(vc-version-diff file nil nil)))))
+
+(defun vc-default-revision-completion-table (backend file) nil)
 
 (defun vc-version-diff (file rev1 rev2)
   "List the differences between FILE's versions REV1 and REV2.
@@ -1708,12 +1823,13 @@
 a directory, in that case, generate diffs between the correponding
 versions of all registered files in or below it."
   (interactive
-   (let ((file (expand-file-name
+   (let* ((file (expand-file-name
                 (read-file-name (if buffer-file-name
                                     "File or dir to diff (default visited file): "
                                   "File or dir to diff: ")
                                 default-directory buffer-file-name t)))
-         (rev1-default nil) (rev2-default nil))
+         (rev1-default nil) (rev2-default nil)
+	  (completion-table (vc-call revision-completion-table file)))
      ;; compute default versions based on the file state
      (cond
       ;; if it's a directory, don't supply any version default
@@ -1722,21 +1838,23 @@
       ;; if the file is not locked, use last and previous version as default
       (t
        (setq rev1-default (vc-call previous-version file
-                                   (vc-workfile-version file)))
+				   (vc-workfile-version file)))
        (if (string= rev1-default "") (setq rev1-default nil))
        (setq rev2-default (vc-workfile-version file))))
      ;; construct argument list
-     (list file
-           (read-string (if rev1-default
-			    (concat "Older version (default "
-				    rev1-default "): ")
-			  "Older version: ")
-			nil nil rev1-default)
-           (read-string (if rev2-default
-			    (concat "Newer version (default "
-				    rev2-default "): ")
-			  "Newer version (default current source): ")
-			nil nil rev2-default))))
+     (let* ((rev1-prompt (if rev1-default
+			     (concat "Older version (default "
+				     rev1-default "): ")
+			   "Older version: "))
+	    (rev2-prompt (concat "Newer version (default "
+				 (or rev2-default "current source") "): "))
+	    (rev1 (if completion-table
+		      (completing-read rev1-prompt completion-table nil nil nil nil rev1-default)
+		    (read-string rev1-prompt nil nil rev1-default)))
+	    (rev2 (if completion-table
+		      (completing-read rev2-prompt completion-table nil nil nil nil rev2-default)
+		    (read-string rev2-prompt nil nil rev2-default))))
+       (list file rev1 rev2))))
   (if (file-directory-p file)
       ;; recursive directory diff
       (progn

Re: Limitations of Emacs' vc when using modern backends

[Juliusz Chroboczek]

>> If multiple revisions match, the latest one is chosen.

> And this is a feature of vc-darcs rather than of darcs itself, right?

No, it's actually a feature of Darcs itself.

> The same thing proably happens for you: "Update" may stand for
> "Update version number to 1.3" at some point and to "Update version
> number to 1.6" at the other, so the "vc-darcs.el~Update" can become
> out of date as well.

Yes, exactly.

> The only problem I see with `canonical-revision' is that for some backends
> it may be very costly (for CVS it requires a round trip to the repository),
> and it may suffer from race conditions.

Good point about the cost (see below).  On the other hand, there's no
race condition if you do things in the right order: first normalise
the version, then fetch the file from the backend using the canonical
identifier.

> Now `canonical-revision' is more general, but I'm wondering: where would it
> be used other than in `vc-version-other-window'?

In the current vc-darcs, it's also used by vc-diff.  I haven't checked
exhaustively if there are other instances where it might be useful.

I suggest that for backends that need it vc-BACKEND-canonical-revision
should be compulsory, and we should optionally allow checkout and diff
to return the canonical identifier(s).  (What a pity elisp doesn't
have multiple return values).  I believe that the former should be
considered for 22, while the latter is something we might need more
time to experiment with.

And adding generic support for completion is definitely a good idea.
Bug again it might be premature to aim for having it in 22.

                                        Juliusz

Re: Limitations of Emacs' vc when using modern backends

[Stefan Monnier]

>>> If multiple revisions match, the latest one is chosen.
>> And this is a feature of vc-darcs rather than of darcs itself, right?
> No, it's actually a feature of Darcs itself.

OK, good.  So it's very much like CVS's tag names (from the point of view
of the problems it introduces).

>> The only problem I see with `canonical-revision' is that for some backends
>> it may be very costly (for CVS it requires a round trip to the repository),
>> and it may suffer from race conditions.

> Good point about the cost (see below).  On the other hand, there's no
> race condition if you do things in the right order: first normalise
> the version, then fetch the file from the backend using the canonical
> identifier.

True.  And this order is the natural one anyway, so the issue of the race
condition was a red herring.  Lucky someone's actually thinking here ;-)

>> Now `canonical-revision' is more general, but I'm wondering: where would it
>> be used other than in `vc-version-other-window'?

> In the current vc-darcs, it's also used by vc-diff.

Could you explain why you explicitly call vc-darcs-rev-to-hash in
vc-darcs-checkout and vc-darcs-diff ?  If it's a feature of darcs itself, it
seems like vc-darcs shouldn't need to do the mapping (except for things
like the file~rev~ names, but that's done outside vc-darcs.el so you're
stuck with it right now).

> I haven't checked exhaustively if there are other instances where it might
> be useful.

It seems like it's only really needed (from vc.el's point of view) at those
very few points where we store and/or compare revision names, and AFAICT
it's currently only done for the naming of "backup" files file~rev~.

> I suggest that for backends that need it vc-BACKEND-canonical-revision
> should be compulsory, and we should optionally allow checkout and diff
> to return the canonical identifier(s).

[ In Emacs-22, rather it's not `checkout' but `find-version'. ]
Why should `diff' return the canonical identifiers?

> (What a pity elisp doesn't have multiple return values).

But it does.  Not as efficient (syntax-wise or memory&cpu-wise), but that's
not an issue here since it's lost in the noise of the backend's execution.

> And adding generic support for completion is definitely a good idea.
> Bug again it might be premature to aim for having it in 22.

I haven't proposed it until now, indeed, although to tell you the truth,
I don't see what kind of controversy there might be about its shape.


        Stefan

Re: Limitations of Emacs' vc when using modern backends

[Andre Spiegel]

Juliusz, [ RMS, please see comments at the end ]

> 1. It should be possible for a backend to override
> vc-previous-version.

Agreed, it surely has to be backend-dependent.

> (2) CVS versions files, while modern systems version trees; the effect
> is that the correct ``previous version'' depends on the file name.

That's also reasonable.  Out of curiosity, I wonder whether that is
really the only place you came across where "versioning files" and
"versioning trees" conflict?  I would expect that there are more places,
and vc-previous-version sounds like a pretty inocuous, although no doubt
relevant spot.

> I believe that the functionality of vc-darcs-rev-to-hash (getting a
> canonical revision identifier for a given revision identifier) should
> be moved into vc itself, and called whenever vc gets a revision from
> the user.  The default implementation would just be the identity, but
> it should be possible for a backend to override it.

Although the hash identifier seems rather specific to Darcs, I can see
how this could be useful for other backends -- you can also refer to CVS
and RCS revisions symbolically (both branches and individual revisions),
and it would be nice to canonicalize those too (turning them into real
version numbers), if only optionally.  If this is done, auto-completion
for symbolic revision names would be a logical next step (but certainly
not now).

So your suggested changes seem very low-impact to me (they would be
trivial from the perspective of other backends).  I don't see a reason
not to make them right now, but perhaps RMS should have the final word
on this.  Richard?