bug#70526: 29.2; package-vc-upgrade failed with error message "File is not under version control"

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

"Yi Yue" <include_yy@qq.com> writes:

>> (unrelated, but why do your messages include HTML entities?) 
>
> (Sorry, I forgot to forbid the default formatter of my email client.)

it appears more or less correctly when I view the HTML message, so don't
worry.

>> My main fear is that this might have some unintended consequences. 
>
> Of course. 
>
>> I wouldn't want to rely on my understanding of vc either. While it 
>> would be easy to add a dynamic variable to indicate this behaviour, I am 
>> careful not to overburden the abstractions that VC provides. 
>
> Understand. 
>
> I read the docstring of `vc-pull', which is used by `package-vc-upgrade'. 
> It says: 
>
> -------------------------------------------------------------------------- 
> "Update the current fileset or branch. 
> You must be visiting a version controlled file, or in a `vc-dir' buffer. 
> ..."
> -------------------------------------------------------------------------- 
>
> But `vc-dir' is a user command other than a function, it will open a new buffer and return nil. 
> In consideration of the fact that `vc-pull' is an async function, it is not easy for us to 
> kill the `vc-diff' buffer right after the pull operation. 

This actually gives us another possibility how to tackle the issue:


[-- Attachment #2: Type: text/plain, Size: 1111 bytes --]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index ef056c7909b..c29e8b5d738 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -774,6 +774,9 @@ package-vc-upgrade-all
         (package-vc-upgrade pkg-desc))))
   (message "Done upgrading packages."))
 
+(declare-function vc-dir-prepare-status-buffer "vc-dir"
+                  (bname dir backend &optional create-new))
+
 ;;;###autoload
 (defun package-vc-upgrade (pkg-desc)
   "Upgrade the package described by PKG-DESC from package's VC repository.
@@ -810,7 +813,10 @@ package-vc-upgrade
                   (remove-hook 'vc-post-command-functions post-upgrade))))))
     (add-hook 'vc-post-command-functions post-upgrade)
     (with-demoted-errors "Failed to fetch: %S"
-      (let ((default-directory pkg-dir))
+      (require 'vc-dir)
+      (with-current-buffer (vc-dir-prepare-status-buffer
+                            (format " *vc-dir: %s*" pkg-dir)
+                            pkg-dir (vc-responsible-backend pkg-dir))
         (vc-pull)))))
 
 (defun package-vc--archives-initialize ()

[-- Attachment #3: Type: text/plain, Size: 722 bytes --]


I am actually satisfied with this approach, and it seems reliable.

>
> As you suggested earlier, maybe we need to modify vc.el, making the restriction looser? 

If you can, try out the above patch and tell me if you end up having any
issues, otherwise I don't think we need to adjust vc.

> Also, I noticed that the maintainer bind `default-directory' in this commit: 
>
> https://github.com/emacs-mirror/emacs/commit/7ab556b57631cb28db86b89ba296bc0599d9a399 
> Improve robustness of 'package-vc-update' Regards

FYI that was my change, GitHub just doesn't display commit names by
default:

https://github.com/emacs-mirror/emacs/commit/7ab556b57631cb28db86b89ba296bc0599d9a399.patch

-- 
	Philip Kaludercic on peregrine