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

[Philip Kaludercic]

"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:

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 ()

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