bug#19548: VC changes under-documented, needlessly incompatible

bug#19548: VC changes under-documented, needlessly incompatible

[Glenn Morris]

Package: emacs
Version: 25.0.50

There have been extensive changes to VC in master, but there is zero
mention of any of it in NEWS. Normally one lists every changed
user-option and non-internal function.

Also, the changes are more incompatible than they need to be.
For example, in Emacs 24.4 there is a user option vc-cvs-stay-local.
In master, it was deleted in favour of vc-stay-local.
Use of define-obsolete-variable-alias would ease the transition.
Probably there are other such cases, as well as cases where setting
advertised-calling-convention rather than simply deleting function
arguments would help.

bug#19548: VC changes under-documented, needlessly incompatible

[Dmitry Gutov]

On 01/09/2015 07:41 PM, Glenn Morris wrote:

Sorry about saying it this late, but:

> Also, the changes are more incompatible than they need to be.
> For example, in Emacs 24.4 there is a user option vc-cvs-stay-local.
> In master, it was deleted in favour of vc-stay-local.

Looking at the commit that made this (185320a5), I see no reason for 
there to be vc-stay-local. Quite the opposite, the commit message says:

"The CVS back end retaiin this machibery and the vc-stay-local 
configuration variable now only affects it."

Why don't we remove vc-stay-local instead, and keep the appropriately 
named vc-cvs-stay-local?

Do we expect vc-stay-local to have been a lot more popular to customize? 
Then indeed it can become an alias.

> Probably there are other such cases, as well as cases where setting
> advertised-calling-convention rather than simply deleting function
> arguments would help.

If you could point out the specific ones, that would be helpful.

bug#19548: VC changes under-documented, needlessly incompatible

[Glenn Morris]

Dmitry Gutov wrote:

>> Probably there are other such cases, as well as cases where setting
>> advertised-calling-convention rather than simply deleting function
>> arguments would help.
>
> If you could point out the specific ones, that would be helpful.

I can't be bothered to check. If no-one else can either, then obv.
there's no point keeping this report open - feel free to close it.

bug#19548: VC changes under-documented, needlessly incompatible

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Fri, 13 May 2016 17:05:26 -0400
> 
> Dmitry Gutov wrote:
> 
> >> Probably there are other such cases, as well as cases where setting
> >> advertised-calling-convention rather than simply deleting function
> >> arguments would help.
> >
> > If you could point out the specific ones, that would be helpful.
> 
> I can't be bothered to check. If no-one else can either, then obv.
> there's no point keeping this report open - feel free to close it.

Can you list the issues that prompted you to give the bug report its
subject line?  I'm sure you bumped into more than one of them.  In
addition to the few mentioned explicitly in the OP, are there any
others?

Thanks.

bug#19548: VC changes under-documented, needlessly incompatible

[Dmitry Gutov]

On 05/09/2016 02:42 AM, Dmitry Gutov wrote:

> Why don't we remove vc-stay-local instead, and keep the appropriately
> named vc-cvs-stay-local?
>
> Do we expect vc-stay-local to have been a lot more popular to customize?
> Then indeed it can become an alias.

On the other hand, vc-cvs-stay-local-p contains this bit of code:

(let* ((sym (vc-make-backend-sym 'CVS 'stay-local))
        (stay-local (if (boundp sym) (symbol-value sym) vc-stay-local)))

which seems like it will ensure that if the user has customized both 
vc-stay-local and vc-cvs-stay-local, the latter will win out (which 
won't necessarily happen if we just declare the former to be an obsolete 
alias of the latter).

So it seems we're actually buying some extra compatibility here at the 
cost of some complexity. Do we care about the above detail?

If yes, vc-stay-local should remain as it is now. If not, I'll gladly 
rename it back to vc-cvs-stay-local, and create an obsolete alias.

bug#19548: VC changes under-documented, needlessly incompatible

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 16 May 2016 02:37:33 +0300
> Cc: "Eric S. Raymond" <esr@thyrsus.com>
> 
> On the other hand, vc-cvs-stay-local-p contains this bit of code:
> 
> (let* ((sym (vc-make-backend-sym 'CVS 'stay-local))
>         (stay-local (if (boundp sym) (symbol-value sym) vc-stay-local)))
> 
> which seems like it will ensure that if the user has customized both 
> vc-stay-local and vc-cvs-stay-local, the latter will win out (which 
> won't necessarily happen if we just declare the former to be an obsolete 
> alias of the latter).
> 
> So it seems we're actually buying some extra compatibility here at the 
> cost of some complexity. Do we care about the above detail?

I'm not sure I see the gain, given that (AFAIU) CVS is the only
back-end for which this option is relevant.

bug#19548: VC changes under-documented, needlessly incompatible

[Dmitry Gutov]

On 05/23/2016 08:37 PM, Eli Zaretskii wrote:

> I'm not sure I see the gain, given that (AFAIU) CVS is the only
> back-end for which this option is relevant.

For the benefit of someone who customized vc-stay-local 10 years ago? 
Not very compelling, I agree.

Does this patch have your blessing?

Someone should test it out, to be safe; the only CVS repo I have to 
experiment on doesn't seem to work great with either version of the code 
(but then, it's an old public checkout of the Samba repository).

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 2dca708..a2499a2 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -121,7 +121,7 @@ vc-cvs-use-edit
    :version "21.1"
    :group 'vc-cvs)

-(defcustom vc-stay-local 'only-file
+(defcustom vc-cvs-stay-local 'only-file
    "Non-nil means use local operations when possible for remote 
repositories.
  This avoids slow queries over the network and instead uses heuristics
  and past information to determine the current status of a file.
@@ -131,11 +131,11 @@ vc-stay-local
  all other VC operations.

  The value can also be a regular expression or list of regular
-expressions to match against the host name of a repository; then VC
-only stays local for hosts that match it.  Alternatively, the value
-can be a list of regular expressions where the first element is the
-symbol `except'; then VC always stays local except for hosts matched
-by these regular expressions."
+expressions to match against the host name of a repository; then
+vc-cvs only stays local for hosts that match it.  Alternatively,
+the value can be a list of regular expressions where the first
+element is the symbol `except'; then vc-cvs always stays local
+except for hosts matched by these regular expressions."
    :type '(choice (const :tag "Always stay local" t)
  		 (const :tag "Only for file operations" only-file)
  		 (const :tag "Don't stay local" nil)
@@ -789,8 +789,7 @@ vc-cvs-stay-local-p
  individually should stay local."
    (if (listp file)
        (delq nil (mapcar (lambda (arg) (vc-cvs-stay-local-p arg)) file))
-    (let* ((sym (vc-make-backend-sym 'CVS 'stay-local))
-          (stay-local (if (boundp sym) (symbol-value sym) vc-stay-local)))
+    (let ((stay-local vc-cvs-stay-local))
        (if (symbolp stay-local) stay-local
         (let ((dirname (if (file-directory-p file)
                            (directory-file-name file)

bug#19548: VC changes under-documented, needlessly incompatible

[Eli Zaretskii]

> Cc: rgm@gnu.org, 19548@debbugs.gnu.org, esr@thyrsus.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 24 May 2016 02:07:35 +0300
> 
> Does this patch have your blessing?

Yes, it looks good to me.

> Someone should test it out, to be safe; the only CVS repo I have to 
> experiment on doesn't seem to work great with either version of the code 
> (but then, it's an old public checkout of the Samba repository).

You can try this one:

  http://www.delorie.com/djgpp/cvs.html

Thanks.

bug#19548: VC changes under-documented, needlessly incompatible

[Dmitry Gutov]

On 05/24/2016 06:34 PM, Eli Zaretskii wrote:

> Yes, it looks good to me.

Pushed, thanks. If someone would like to point out what else we're 
missing in this bug, that would be great.

I've glanced through the "Changes from the pre-25.1 API" list, and one 
change we could make to improve backward compatibility there, is to 
catch wrong-number-of-arguments when calling the `diff' method, and 
retry with one fewer argument.

Doesn't really seem worth the trouble, however. The list of changes is 
longer than that, and third-party backends will have to adapt anyway.

Similarly, the change to dir-status-files could be smoothed over by 
continuing to pass some dummy fourth argument, but it's unclear when 
we'd be allowed to stop doing that. So maybe not doing that at all is 
just as valid an option. IIRC, nobody has really complained about their 
backend having stopped working.

> You can try this one:
>
>   http://www.delorie.com/djgpp/cvs.html

Thanks. revert-buffer works *very* slowly with it using the default 
settings, but at least the results look correct.

bug#19548: VC changes under-documented, needlessly incompatible

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 9 May 2016 02:42:09 +0300
> Cc: "Eric S. Raymond" <esr@thyrsus.com>
> 
> Looking at the commit that made this (185320a5), I see no reason for 
> there to be vc-stay-local. Quite the opposite, the commit message says:
> 
> "The CVS back end retaiin this machibery and the vc-stay-local 
> configuration variable now only affects it."
> 
> Why don't we remove vc-stay-local instead, and keep the appropriately 
> named vc-cvs-stay-local?

Will that also fix the typos in the log message? ;-)

Anyway, I tend to agree to go back to vc-cvs-stay-local.

bug#19548: VC changes under-documented, needlessly incompatible

[Dmitry Gutov]

On 05/23/2016 08:36 PM, Eli Zaretskii wrote:

> Will that also fix the typos in the log message? ;-)

I'm sure the next version of Reposurgeon will do that, as well as hunt 
down and correct all the respective messages in the mailing list archives.

> Anyway, I tend to agree to go back to vc-cvs-stay-local.

Cool.