bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

When a directory is viewed via VC (‘vc-dir’), when that directory is a
CVS directory, all files that are edited, whether in that directory or
in its subdirectories, are listed as if they were in the toplevel
directory of the repo.

Example (25.1):

,----
| VC backend : CVS
| Working dir: [deduced]
| Repository : [deduced]
| Module     : emacs.d
| 
| 
|                          ./
|     edited               eval-sexp-fu.el
|     edited               gk-org.el
|     edited               gk-utils.el
|     edited               gk.el
`----

Same directory, in 24.5:

,----
| VC backend : CVS
| Working dir: [deduced]
| Repository : [deduced]
| Module     : emacs.d
| 
| 
|                          ./
|                          gk/
|     edited               gk/gk-org.el
|     edited               gk/gk-utils.el
|     edited               gk/gk.el
|                          site/
|     edited               site/eval-sexp-fu.el
`----

Commands like vc-diff from this buffer (via ‘=’) fail, complaining it
can't find the VC backend.

Reproduction:

- Modify a tracked file in a submodule of a toplevel CVS module
- Call ‘vc-dir’ on the toplevel module
- Run a command on the modified file.

This is the stacktrace I get from ‘vc-diff’ (‘=’ in vc-dir) with
‘debug-on-error’ t:

,----
| Debugger entered--Lisp error: (file-error "Cannot open load file" "No such file or directory" "vc-nil")
|   require(vc-nil)
|   vc-find-backend-function(nil make-version-backups-p)
|   vc-call-backend(nil make-version-backups-p "[deduced]/emacs.d/gk-org.el")
|   vc-version-backup-file("[deduced]/emacs.d/gk-org.el" nil)
|   vc-cvs-diff(("[deduced]/emacs.d/gk-org.el") nil nil "*vc-diff*" t)
|   apply(vc-cvs-diff (("[deduced]/emacs.d/gk-org.el") nil nil "*vc-diff*" t))
|   vc-call-backend(CVS diff ("[deduced]/emacs.d/gk-org.el") nil nil "*vc-diff*" t)
|   vc-diff-internal(t (CVS ("[deduced]/emacs.d/gk-org.el") nil nil nil) nil nil t)
|   vc-diff(nil t)
|   funcall-interactively(vc-diff nil t)
|   call-interactively(vc-diff nil nil)
|   command-execute(vc-diff)
`----

If I try to see the version log, via ‘l’ in the *vc-dir* buffer,  I get
the ‘*vc-change-log*’ buffer with these contents:

,----
| cvs log: nothing known about gk-org.el
`----




In GNU Emacs 25.1.1 (x86_64-unknown-freebsd10.3, X toolkit, Xaw scroll bars)
 of 2016-07-26 built on xi.bootis
Windowing system distributor 'The X.Org Foundation', version 11.0.11704000
Configured using:
 'configure -C --enable-silent-rules --disable-dependency-tracking
 --without-pop --with-x-toolkit=athena --without-gpm --without-dbus
 --without-gconf --without-gsettings --without-selinux --with-modules
 --with-file-notification=yes --with-x
 --prefix=[deduced]'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND NOTIFY ACL GNUTLS LIBXML2
FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 MODULES

Important settings:
  value of $LC_MESSAGES: en_US.UTF-8
  value of $LANG: tr_TR.UTF-8
  locale-coding-system: utf-8-unix

Major mode: VC dir

Minor modes in effect:
  diff-auto-refine-mode: t
  show-paren-mode: t
  auto-insert-mode: t
  change-cursor-mode: t
  winner-mode: t
  auto-image-file-mode: t
  which-key-mode: t
  smooth-scroll-mode: t
  save-place-mode: t
  savehist-mode: t
  global-paren-face-mode: t
  eval-sexp-fu-flash-mode: t
  global-gk-minor-mode: t
  gk-minor-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  gk-utf8-entry-mode: t
  pdf-occur-global-minor-mode: t
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  size-indication-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Traversing directory [deduced]/emacs.d/lisp...
Traversing directory [deduced]/emacs.d/packages...
Traversing directory [deduced]/emacs.d/packages/bbdb-20150523.1239...
Traversing directory [deduced]/emacs.d/packages/elfeed-git...
Traversing directory [deduced]/emacs.d/packages/pdf-tools-0.70...
Traversing directory [deduced]/emacs.d/site...
Traversing directory [deduced]/emacs.d/themes...
Traversing directory [deduced]/emacs.d/...done
Finding changes in [deduced]/emacs.d/eval-sexp-fu.el...
Entering debugger...

Load-path shadows:
[deduced]/co/Exwm/gk-wm hides [deduced]/.emacs.d/gk/gk-wm
[deduced]/co/Exwm/cl-generic hides [deduced]/Local/emacs25.1/share/emacs/25.1/lisp/emacs-lisp/cl-generic
[deduced]/.emacs.d/site/let-alist hides [deduced]/Local/emacs25.1/share/emacs/25.1/lisp/emacs-lisp/let-alist

Features:
(conf-mode shadow mailalias mail-extr emacsbug make-mode vc-git
diff-mode vc-bzr vc-src vc-sccs vc-svn eieio-opt speedbar sb-image
ezimage dframe debug dabbrev sort tabify two-column iso-transl dired-aux
org-colview shr-color color qp goto-addr rmailmm exim xcb-xim xcb-xlib
exwm-systemtray xcb-systemtray xcb-xembed exwm-config ido exwm
exwm-input xcb-keysyms exwm-manage exwm-floating xcb-cursor xcb-render
exwm-layout exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb xcb-xproto
xcb-types tls gnutls ibuf-macs supercite regi rmailsum mh-e mh-compat
mh-acros mh-buffers mh-loaddefs ispell bbdb-mua diary-lib diary-loaddefs
misearch multi-isearch grep executable pcmpl-cvs vc-dir ewoc pcmpl-unix
parse-time disp-table org-rmail org-mhe org-irc org-info org-gnus
org-docview org-bibtex bibtex org-bbdb org-w3m paren autoinsert init
server gk thinks analog-clock artist picture reporter boxquote gk-www
twittering-mode gk-feeds gk-urls elfeed-show elfeed-db elfeed-lib
avl-tree elfeed-search elfeed xml-query xml gk-vc vc-rcs vc-cvs vc
vc-dispatcher log-edit pcvs-util add-log gk-ui cursor-chg winner
gk-system man gk-servers simple-httpd gk-multimedia image+ image-file
gk-mail sendmail rmail message idna rfc822 mml mml-sec mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums
gmm-utils mailheader f dash epa-mail bbdb-vcard bbdb-com crm mailabbrev
vcard mairix gk-ibuffer ibuffer-vc tramp-cache tramp tramp-compat
tramp-loaddefs trampver bbdb bbdb-site timezone gk-globals forecast
which-key smooth-scroll saveplace savehist paren-face gk-keys windmove
gk-programming sass-mode haml-mode css-mode ruby-mode smie js sgml-mode
thingatpt cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align
cc-engine cc-vars cc-defs gk-outline eval-sexp-fu warnings rx highlight
inf-lisp highlight-parentheses paredit edmacro kmacro gk-org ob-sh
latexenc ox-odt rng-loc rng-uri rng-parse rng-match rng-dt rng-util
rng-pttrn nxml-parse nxml-ns nxml-enc xmltok nxml-util ox-beamer
ox-latex ox-icalendar ox-html ox-ascii ox-publish ox org-element
org-mobile org-location-google-maps org-capture org-agenda interleave
doc-view google-maps google-maps-static google-maps-geocode
google-maps-base json map gk-org-cite gk-fonts gk-alist gk-lingua
gk-minor-mode ace-jump-mode face-remap apropos gk-file gk-editing
zencoding-mode cl writeroom-mode visual-fill-column undo-tree diff typo
lorem-ipsum gk-utf8 gk-input-methods gk-unilat gk-greek gk-armenian
quail gk-global-modes diminish gk-edit rect gk-documents gk-utils epa
derived whole-line-or-region s ucs-normalize org org-macro org-footnote
org-pcomplete org-list org-faces org-entities noutline outline
easy-mmode org-version ob-emacs-lisp ob ob-tangle ob-ref ob-lob ob-table
ob-exp org-src ob-keys ob-comint ob-core ob-eval org-compat org-macs
org-loaddefs cal-menu calendar cal-loaddefs etags xref project eww
mm-url gnus gnus-ems nnheader mail-utils url-queue url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util url-parse auth-source cl-seq gnus-util mm-util help-fns
mail-prsvr password-cache url-vars mailcap shr seq dom subr-x browse-url
pdf-links pdf-occur ibuf-ext ibuffer tablist tablist-filter
semantic/wisent/comp semantic/wisent semantic/wisent/wisent
semantic/util-modes semantic/util semantic semantic/tag semantic/lex
semantic/fw eieio byte-opt bytecomp byte-compile cl-extra help-mode
cconv eieio-core cl-macs gv mode-local find-func cedet pdf-isearch
let-alist pdf-misc imenu pdf-tools compile cus-edit cus-start cus-load
wid-edit pdf-view bookmark pp jka-compr pdf-cache pdf-info tq pdf-util
advice image-mode gk-dired ls-lisp wdired image-dired format-spec
dired-x dired gk-crypt gk-mac epg epg-config gk-comint shell pcomplete
comint ansi-color ring info easymenu cl-loaddefs pcase cl-lib time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel x-win term/common-win x-dnd tool-bar dnd fontset
image regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote kqueue dynamic-setting
font-render-setting x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 723191 648769)
 (symbols 48 77111 0)
 (miscs 40 2748 5673)
 (strings 32 158878 311981)
 (string-bytes 1 5053215)
 (vectors 16 96985)
 (vector-slots 8 2335188 485593)
 (floats 8 2069 3651)
 (intervals 56 21909 2441)
 (buffers 976 119))

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/

bug#24082: vc-dir changes working directory (git backend)

[Steve Revilak]

[-- Attachment #1.1: Type: text/plain, Size: 1774 bytes --]

I'd like to submit some additional information.  I seem similar (but
not identical behavior) when git is used as the vc backend.

I am using emacs version

   GNU Emacs 25.1.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.14.15) of 2016-07-29

I've attached a small shell script to assist in reproducing the
behavior.  The script creates a git repository and commits two files
./my-repo/file1.txt and ./my-repo/file2.txt.  The script also creates
(an unregistered) file ./my-repo/new-file.txt.

STEPS TO REPRODUCE:

  sh mk-repository.sh
  emacs -Q
  C-x f my-repo/subdir RET
  C-x v d

At this point emacs prompts "VC status for directory: ~/my-repo",
although the buffer's working directory was "~/my-repo/subdir".

I press RET at the "VC status for directory: ~/my-repo".  Here's what
I see

---------------------------------
VC backend : Git
Working dir: ~/my-repo/
Branch     : master
Remote     : 
Stash      : Nothing stashed

                          ./
      unregistered        new-file.txt
---------------------------------

When performing these steps with emacs-24.5:

  - After pressing C-x v d, emacs prompts
    "VC status for directory: ~/my-repo/subdir/"

  - I press RET and see

---------------------------------
VC backend : Git
Working dir: ~/my-repo/subdir/
Branch     : master
Remote     : 
Stash      : Nothing stashed

                          ./
---------------------------------

I agree with the reporter: the VC status is being reported from the
top-level of the repository, as opposed to the buffer's working
directory.

emacs-25.0.90 behaves the same way as emacs 25.1-rc1

Unlike the CVS report, '=' (in vc-dir) behaves as I would expect: it
shows me a diff of the file at point.


[-- Attachment #1.2: mk-repository.sh --]
[-- Type: application/x-sh, Size: 222 bytes --]

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#24082: vc-dir changes working directory (git backend)

[Dmitry Gutov]

On 30.07.2016 03:35, Steve Revilak wrote:

> At this point emacs prompts "VC status for directory: ~/my-repo",
> although the buffer's working directory was "~/my-repo/subdir".
>
> I press RET at the "VC status for directory: ~/my-repo".  Here's what
> I see

What if you don't just press RET, and pick the directory you wanted first?

I don't see a problem here: offering the repository root as default is a 
reasonable (if new-ish) behavior. We've changed it in commit f30247547, 
nearly two years ago, and haven't seen any other complaints so far.

> I agree with the reporter: the VC status is being reported from the
> top-level of the repository, as opposed to the buffer's working
> directory.

It's reported from the directory you pick, as far as I can see.

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Göktuğ Kayaalp]

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

Hi, attached is a patch that fixes bug#24082.

The reason of the bug was that, in function ‘vc-cvs-after-dir-status’,
the CVS status line ‘cvs status: Examining <dir>’ was excluded when the
function narrows to match, and when it tries to set the local variable
‘subdir’, as it does not find this line, it skips setting it.  As
‘subdir’ defaults to ‘default-directory’, which is previously set to
repo root (i.e. the argument to function ‘vc-dir’, when ‘subdir’ is used
to construct the relative path to file, concatenating it with the
already-known file base name, it returns the basename, i.e. in
the form ‘name.ext’, with no directory path.  This because it constructs
the relative path like ‘(file-relative-name basename subdir)’.

The patch uses ‘cvs update’ command instead.  The implementation was
already there, commented out.  I enabled and corrected it.  The result
is more correct than the ‘cvs status’ approach, i.e. includes
unregistered and missing, and is faster compared to ‘cvs status’ way.

PS.: Please have me in CC when updating, I'm not subscribed to
bug-gnu-emacs.

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch for #24082 --]
[-- Type: text/x-diff, Size: 6019 bytes --]

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 5019871..fa385ae 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -944,103 +944,32 @@ state."
 	  (t 'edited))))))))
 
 (defun vc-cvs-after-dir-status (update-function)
-  ;; Heavily inspired by vc-cvs-parse-status. AKA a quick hack.
-  ;; This needs a lot of testing.
-  (let ((status nil)
-	(status-str nil)
-	(file nil)
-	(result nil)
-	(missing nil)
-	(ignore-next nil)
-	(subdir default-directory))
+  (let ((result nil)
+  	(translation '((?? . unregistered)
+  		       (?A . added)
+  		       (?C . conflict)
+  		       (?M . edited)
+  		       (?P . needs-merge)
+  		       (?R . removed)
+  		       (?U . needs-update))))
     (goto-char (point-min))
-    (while
-	;; Look for either a file entry, an unregistered file, or a
-	;; directory change.
-	(re-search-forward
-	 "\\(^=+\n\\([^=c?\n].*\n\\|\n\\)+\\)\\|\\(\\(^?? .*\n\\)+\\)\\|\\(^cvs status: \\(Examining\\|nothing\\) .*\n\\)"
-	 nil t)
-      ;; FIXME: get rid of narrowing here.
-      (narrow-to-region (match-beginning 0) (match-end 0))
-      (goto-char (point-min))
-      ;; The subdir
-      (when (looking-at "cvs status: Examining \\(.+\\)")
-	(setq subdir (expand-file-name (match-string 1))))
-      ;; Unregistered files
-      (while (looking-at "? \\(.*\\)")
-	(setq file (file-relative-name
-		    (expand-file-name (match-string 1) subdir)))
-	(push (list file 'unregistered) result)
-	(forward-line 1))
-      (when (looking-at "cvs status: nothing known about")
-	;; We asked about a non existent file.  The output looks like this:
-
-	;; cvs status: nothing known about `lisp/v.diff'
-	;; ===================================================================
-	;; File: no file v.diff            Status: Unknown
-	;;
-	;;    Working revision:    No entry for v.diff
-	;;    Repository revision: No revision control file
-	;;
-
-	;; Due to narrowing in this iteration we only see the "cvs
-	;; status:" line, so just set a flag so that we can ignore the
-	;; file in the next iteration.
-	(setq ignore-next t))
-      ;; A file entry.
-      (when (re-search-forward "^File: \\(no file \\)?\\(.*[^ \t]\\)[ \t]+Status: \\(.*\\)" nil t)
-	(setq missing (match-string 1))
-	(setq file (file-relative-name
-		    (expand-file-name (match-string 2) subdir)))
-	(setq status-str (match-string 3))
-	(setq status
-	      (cond
-	       ((string-match "Up-to-date" status-str) 'up-to-date)
-	       ((string-match "Locally Modified" status-str) 'edited)
-	       ((string-match "Needs Merge" status-str) 'needs-merge)
-	       ((string-match "Needs \\(Checkout\\|Patch\\)" status-str)
-		(if missing 'missing 'needs-update))
-	       ((string-match "Locally Added" status-str) 'added)
-	       ((string-match "Locally Removed" status-str) 'removed)
-	       ((string-match "File had conflicts " status-str) 'conflict)
-	       ((string-match "Unknown" status-str) 'unregistered)
-	       (t 'edited)))
-	(if ignore-next
-	    (setq ignore-next nil)
-	  (unless (eq status 'up-to-date)
-	    (push (list file status) result))))
-      (goto-char (point-max))
-      (widen))
-    (funcall update-function result))
-  ;; Alternative implementation: use the "update" command instead of
-  ;; the "status" command.
-  ;; (let ((result nil)
-  ;; 	(translation '((?? . unregistered)
-  ;; 		       (?A . added)
-  ;; 		       (?C . conflict)
-  ;; 		       (?M . edited)
-  ;; 		       (?P . needs-merge)
-  ;; 		       (?R . removed)
-  ;; 		       (?U . needs-update))))
-  ;;   (goto-char (point-min))
-  ;;   (while (not (eobp))
-  ;;     (if (looking-at "^[ACMPRU?] \\(.*\\)$")
-  ;; 	  (push (list (match-string 1)
-  ;; 		      (cdr (assoc (char-after) translation)))
-  ;; 		result)
-  ;; 	(cond
-  ;; 	 ((looking-at "cvs update: warning: \\(.*\\) was lost")
-  ;; 	  ;; Format is:
-  ;; 	  ;; cvs update: warning: FILENAME was lost
-  ;; 	  ;; U FILENAME
-  ;; 	  (push (list (match-string 1) 'missing) result)
-  ;; 	  ;; Skip the "U" line
-  ;; 	  (forward-line 1))
-  ;; 	 ((looking-at "cvs update: New directory `\\(.*\\)' -- ignored")
-  ;; 	  (push (list (match-string 1) 'unregistered) result))))
-  ;;     (forward-line 1))
-  ;;   (funcall update-function result)))
-  )
+    (while (not (eobp))
+      (if (looking-at "^[ACMPRU?] \\(.*\\)$")
+  	  (push (list (match-string 1)
+  		      (cdr (assoc (char-after) translation)))
+  		result)
+  	(cond
+  	 ((looking-at "cvs update: warning: \\(.*\\) was lost")
+  	  ;; Format is:
+  	  ;; cvs update: warning: FILENAME was lost
+  	  ;; U FILENAME
+  	  (push (list (match-string 1) 'missing) result)
+  	  ;; Skip the "U" line
+  	  (forward-line 1))
+  	 ((looking-at "cvs update: New directory `\\(.*\\)' -- ignored")
+  	  (push (list (match-string 1) 'unregistered) result))))
+      (forward-line 1))
+    (funcall update-function result)))
 
 ;; Based on vc-cvs-dir-state-heuristic from Emacs 22.
 ;; FIXME does not mention unregistered files.
@@ -1078,15 +1007,12 @@ Query all files in DIR if files is nil."
   (let ((local (vc-cvs-stay-local-p dir)))
     (if (and (not files) local (not (eq local 'only-file)))
 	(vc-cvs-dir-status-heuristic dir update-function)
-      (if (not files) (setq files (vc-expand-dirs (list dir) 'CVS)))
-      (vc-cvs-command (current-buffer) 'async files "-f" "status")
-      ;; Alternative implementation: use the "update" command instead of
-      ;; the "status" command.
-      ;; (vc-cvs-command (current-buffer) 'async
-      ;; 		  (file-relative-name dir)
-      ;; 		  "-f" "-n" "update" "-d" "-P")
-      (vc-run-delayed
-       (vc-cvs-after-dir-status update-function)))))
+      (if (not files) (setq files (vc-expand-dirs (list dir) 'CVS)))) 
+    (vc-cvs-command (current-buffer) 'async
+                    (file-relative-name dir)
+                    "-f" "-n" "-q" "update")
+    (vc-run-delayed
+      (vc-cvs-after-dir-status update-function))))
 
 (defun vc-cvs-file-to-string (file)
   "Read the content of FILE and return it as a string."

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dmitry Gutov]

Hi! Sorry for the long wait.

On 28.08.2016 23:17, Göktuğ Kayaalp wrote:
> Hi, attached is a patch that fixes bug#24082.

Could you help me reproduce the issue locally first?

Either by pointing to a publicly available CVS repo with submodules, or 
by providing a sequence of commands that would create such repo locally 
(we do something like that in vc-tests.el).

> The reason of the bug was that, in function ‘vc-cvs-after-dir-status’,
> the CVS status line ‘cvs status: Examining <dir>’ was excluded when the
> function narrows to match, and when it tries to set the local variable
> ‘subdir’, as it does not find this line, it skips setting it.  As
> ‘subdir’ defaults to ‘default-directory’, which is previously set to
> repo root (i.e. the argument to function ‘vc-dir’, when ‘subdir’ is used
> to construct the relative path to file, concatenating it with the
> already-known file base name, it returns the basename, i.e. in
> the form ‘name.ext’, with no directory path.  This because it constructs
> the relative path like ‘(file-relative-name basename subdir)’.

Could we just fix that, to address this specific bug?

> The patch uses ‘cvs update’ command instead.  The implementation was
> already there, commented out.  I enabled and corrected it.  The result
> is more correct than the ‘cvs status’ approach, i.e. includes
> unregistered and missing, and is faster compared to ‘cvs status’ way.

If that change is mostly unrelated to the reported bug, could you send 
it as two separate patches?

Further, do you have any inkling why the 'cvs update' implementation was 
commented out? Did it require a particular recent version of CVS, maybe?

We can ask Dan if you don't.

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Göktuğ Kayaalp]

On 2016-10-07 02:25:24 AM +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Hi! Sorry for the long wait.

No problem, though actually I'm the one who caused the long delay b/c I
was too late to send in copyright papers, sorry :)

>
> On 28.08.2016 23:17, Göktuğ Kayaalp wrote:
>> Hi, attached is a patch that fixes bug#24082.
>
> Could you help me reproduce the issue locally first?
>
> Either by pointing to a publicly available CVS repo with submodules, or 
> by providing a sequence of commands that would create such repo locally 
> (we do something like that in vc-tests.el).

On a computer with CVS installed, these commands should create a working
directory with which the bug can be reproduced:

    $ cd /tmp
    $ mkdir cvsroot
    $ cvs -d /tmp/cvsroot init
    $ mkdir -p test/subdir
    $ touch test/testfil test/subdir/subfil
    $ cd test/
    $ cvs -d /tmp/cvsroot/ import $(basename $(pwd)) $(whoami) start
    $ cd ../
    $ mv test test.bkp
    $ cvs -d /tmp/cvsroot/ co test
    $ echo test > test/testfil 
    $ echo test > test/subdir/subfil 

Then, in Emacs, run [ C-x v d /tmp/test RET ], and on ‘subfil’, hit ‘=’
(i.e. ask for a diff).

>> The reason of the bug was that, in function ‘vc-cvs-after-dir-status’,
>> the CVS status line ‘cvs status: Examining <dir>’ was excluded when the
>> function narrows to match, and when it tries to set the local variable
>> ‘subdir’, as it does not find this line, it skips setting it.  As
>> ‘subdir’ defaults to ‘default-directory’, which is previously set to
>> repo root (i.e. the argument to function ‘vc-dir’, when ‘subdir’ is used
>> to construct the relative path to file, concatenating it with the
>> already-known file base name, it returns the basename, i.e. in
>> the form ‘name.ext’, with no directory path.  This because it constructs
>> the relative path like ‘(file-relative-name basename subdir)’.
>
> Could we just fix that, to address this specific bug?

The output for the ‘cvs status’ command does not provide adequate
information to reliably and simply construct the full path to each
file.  The output from the abovementioned command in the checkout the
commands I listed created is like follows:

,----
| cvs status: Examining .
| ===================================================================
| File: testfil          	Status: Locally Modified
| 
|    Working revision:	1.1.1.1	Fri Oct  7 18:46:53 2016
|    Repository revision:	1.1.1.1	/tmp/cvsroot/test/testfil,v
|    Sticky Tag:		(none)
|    Sticky Date:		(none)
|    Sticky Options:	(none)
| 
| cvs status: Examining subdir
| ===================================================================
| File: subfil           	Status: Locally Modified
| 
|    Working revision:	1.1.1.1	Fri Oct  7 18:46:53 2016
|    Repository revision:	1.1.1.1	/tmp/cvsroot/test/subdir/subfil,v
|    Sticky Tag:		(none)
|    Sticky Date:		(none)
|    Sticky Options:	(none)
`----

The code was trying to rememeber the ‘cvs status: Examining <directory>’
line in order to reconstruct the path, a method that's fragile and
which requires a lot of regexp magic.  ‘cvs update’, instead has a very
filter-friendly and determinable output syntax also imitated by other
version control software (output for the same checkout):

,----
| $ cvs -fn update -dP
| cvs update: Updating .
| M testfil
| cvs update: Updating subdir
| M subdir/subfil
`----

The ‘-n’ argument makes sure the ‘cvs update’ command does not alter in
any way the working directory (i.e. the checkout), and -f surpresses the
user CVS config (~/.cvsrc).

I actually sort-a-kind-a fixed the ‘cvs status’ version initially, but
it's waay slower than using update.  I don't have that fix anymore, but
as I said, using ‘cvs update’ is more robust, simpler, and faster.  In
order to fix the ‘cvs status’ method, I read the relevant files from the
CVS/ subdir of the checkout, reconstruct the absolute module path using
info from files therewithin, than subtract that from the third column of
the ‘ Repository version:...’ lines for each file to find its relative
path to the checkout's root directory, ending up with a complex and
fragile hack, that's also slow and incomplete.

>> The patch uses ‘cvs update’ command instead.  The implementation was
>> already there, commented out.  I enabled and corrected it.  The result
>> is more correct than the ‘cvs status’ approach, i.e. includes
>> unregistered and missing, and is faster compared to ‘cvs status’ way.
>
> If that change is mostly unrelated to the reported bug, could you send 
> it as two separate patches?

That actually is the fix.  ‘cvs status’ output does not lend itself to
being computed in a simple, reliable manner.  The patched function's
first half was the implementation of an incomplete and buggy parser for
the abovementioned command, and the other half was commented out code to
parse the output of ‘cvs update’.

> Further, do you have any inkling why the 'cvs update' implementation was 
> commented out? Did it require a particular recent version of CVS, maybe?
>
> We can ask Dan if you don't.
>

Well, I'm unaware of both the history of the command and why the related
Elisp was commented out, though I do not think that the output format
changed in a long time, or that anything else really changed in CVS
recently :)  I recommend asking someone more knowledgeable than me as I
can't see why it was commented out.  The related commits are: 798dafb4
and 769303ae, both don't say anything about why.

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dmitry Gutov]

On 07.10.2016 22:29, Göktuğ Kayaalp wrote:
> On 2016-10-07 02:25:24 AM +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> Hi! Sorry for the long wait.
>
> No problem, though actually I'm the one who caused the long delay b/c I
> was too late to send in copyright papers, sorry :)

We could have done the review earlier. But anyway...

> On a computer with CVS installed, these commands should create a working
> directory with which the bug can be reproduced:

Thank you, that works.

> The output for the ‘cvs status’ command does not provide adequate
> information to reliably and simply construct the full path to each
> file.  The output from the abovementioned command in the checkout the
> commands I listed created is like follows:
>
> ,----
> | cvs status: Examining .
> | ===================================================================
> | File: testfil          	Status: Locally Modified
> |
> |    Working revision:	1.1.1.1	Fri Oct  7 18:46:53 2016
> |    Repository revision:	1.1.1.1	/tmp/cvsroot/test/testfil,v
> |    Sticky Tag:		(none)
> |    Sticky Date:		(none)
> |    Sticky Options:	(none)
> |
> | cvs status: Examining subdir
> | ===================================================================
> | File: subfil           	Status: Locally Modified
> |
> |    Working revision:	1.1.1.1	Fri Oct  7 18:46:53 2016
> |    Repository revision:	1.1.1.1	/tmp/cvsroot/test/subdir/subfil,v
> |    Sticky Tag:		(none)
> |    Sticky Date:		(none)
> |    Sticky Options:	(none)
> `----
>
> The code was trying to rememeber the ‘cvs status: Examining <directory>’
> line in order to reconstruct the path, a method that's fragile and
> which requires a lot of regexp magic.

Maybe the problem here is that the output we have to deal with does not 
contain "Examining subdir" because the current implementation of 
vc-cvs-dir-status-files passes each individual files name to 'cvs -f 
status'.

And 'cvs -f status testfil subdir/subfil' does not output that line. I 
think the most reasonable approach for that solution would be to parse 
the "Repository revision" lines.

Apparently, vc-cvs-dir-status-files was always broken for this usage, 
and the problem has surfaced now when we've reimplemented dir-status on 
top of dir-status-files.

   ‘cvs update’, instead has a very
> filter-friendly and determinable output syntax also imitated by other
> version control software (output for the same checkout):

That looks reasonable. However, it looks like a significant change, so 
I'm not sure it's appropriate for Emacs 25.2. We'd like to get *a* fix 
into 25.2, however.

> I actually sort-a-kind-a fixed the ‘cvs status’ version initially, but
> it's waay slower than using update.  I don't have that fix anymore, but
> as I said, using ‘cvs update’ is more robust, simpler, and faster.  In
> order to fix the ‘cvs status’ method, I read the relevant files from the
> CVS/ subdir of the checkout, reconstruct the absolute module path using
> info from files therewithin, than subtract that from the third column of
> the ‘ Repository version:...’ lines for each file to find its relative
> path to the checkout's root directory, ending up with a complex and
> fragile hack, that's also slow and incomplete.

Couldn't you subtract those values from default-directory instead?

> Well, I'm unaware of both the history of the command and why the related
> Elisp was commented out, though I do not think that the output format
> changed in a long time, or that anything else really changed in CVS
> recently :)  I recommend asking someone more knowledgeable than me

OK, let's ask Dan.

In the meantime, here's a simple fix we can consider. This would still 
adhere to the dir-status-files protocol, but it's probably slower than 
ideal:

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index a2499a2..e949f30 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -1073,7 +1073,7 @@ vc-cvs-dir-status-files
      (if (and (not files) local (not (eq local 'only-file)))
  	(vc-cvs-dir-status-heuristic dir update-function)
        (if (not files) (setq files (vc-expand-dirs (list dir) 'CVS)))
-      (vc-cvs-command (current-buffer) 'async files "-f" "status")
+      (vc-cvs-command (current-buffer) 'async dir "-f" "status")
        ;; Alternative implementation: use the "update" command instead of
        ;; the "status" command.
        ;; (vc-cvs-command (current-buffer) 'async

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dmitry Gutov]

Hi Dan,

Do you remember why you added 'cvs update' based implementation of 
vc-cvs-dir-status, but decided to leave it commented out, in commit 
769303ae?

Thanks,
Dmitry.

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dan Nicolaescu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Dan,
>
> Do you remember why you added 'cvs update' based implementation of
> vc-cvs-dir-status, but decided to leave it commented out, in commit
> 769303ae?

I think it was getting closer to a release and didn't want to take any
risks. 

       --Dan

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Göktuğ Kayaalp]

Hello,

What do you think about the patch?  Should it be merged?

Also, on this thread [1, 2], there's Jérémie's issue which I couldn't
reproduce (I tried various local and remote repos, including the OpenBSD
‘src/’ repo).  Do you have any ideas regarding this?

I'll be able to work on the patch in the weekend (GMT+3) if any
modifications are needed.

-gk.

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#45
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#54

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: [PATCH] Use ‘cvs update’ instead ‘cvs status’ for CVS *vc-dir* buffers

[Dan Nicolaescu]

Göktuğ Kayaalp <self@gkayaalp.com> writes:

> Hello,
>
> What do you think about the patch?  Should it be merged?

Sorry, I haven't used CVS in many years, so I don't know the
implications of the proposed changes...

             --Dan


> Also, on this thread [1, 2], there's Jérémie's issue which I couldn't
> reproduce (I tried various local and remote repos, including the OpenBSD
> ‘src/’ repo).  Do you have any ideas regarding this?
>
> I'll be able to work on the patch in the weekend (GMT+3) if any
> modifications are needed.


>
> -gk.
>
> [1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#45
> [2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#54
>
> -- 
> İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
>                         PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: Update

[Göktuğ Kayaalp]

Hi, I just wanted to check in for an update on whether or not this patch
will be accepted or not.  I have my copyright assignment stuff sorted
(confirmed by FSF today).

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/

bug#24082: Update

[Eli Zaretskii]

> From: Göktuğ Kayaalp <self@gkayaalp.com>
> Date: Wed, 05 Oct 2016 21:31:54 +0300
> 
> Hi, I just wanted to check in for an update on whether or not this patch
> will be accepted or not.  I have my copyright assignment stuff sorted
> (confirmed by FSF today).

Dmitry, could you please look into this?

Thanks.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

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


I see this regression using 25.1 (tarball) and emacs-26.0.50 (git).

Reverting

  http://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/vc/vc-cvs.el?id=fa7f79e8234c60ae425f7c3cf1b9486765a7111e

fixes the problem here.

-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
OpenBSD emacs port maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

Hi!

On 07.10.2016 17:45, Jérémie Courrèges-Anglas wrote:
>
> I see this regression using 25.1 (tarball) and emacs-26.0.50 (git).
>
> Reverting
>
>   http://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/vc/vc-cvs.el?id=fa7f79e8234c60ae425f7c3cf1b9486765a7111e
>
> fixes the problem here.

I wonder how that could be possible: reverting it gives me a characterp 
error. But if you mean just replacing `files' with `dir', that seems to 
work, at the cost of possible performance overhead with larger repositories.

Could you try the more complex patch that has been posted here? You can 
view it at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#15.

In particular, I'd like to know whether it fixes this bug for you, and 
whether it works well with all CVS repositories you work with.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi!

Hi,

> On 07.10.2016 17:45, Jérémie Courrèges-Anglas wrote:
>>
>> I see this regression using 25.1 (tarball) and emacs-26.0.50 (git).
>>
>> Reverting
>>
>>   http://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/vc/vc-cvs.el?id=fa7f79e8234c60ae425f7c3cf1b9486765a7111e
>>
>> fixes the problem here.
>
> I wonder how that could be possible: reverting it gives me a characterp
> error. But if you mean just replacing `files' with `dir', that seems to
> work, at the cost of possible performance overhead with larger
> repositories.

Oops, sorry.  Yes indeed, I only replaced `files' with `dir'.

> Could you try the more complex patch that has been posted here? You can
> view it at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#15.
>
> In particular, I'd like to know whether it fixes this bug for you,

It does.

> and
> whether it works well with all CVS repositories you work with.

Works fine, there are some differences.

With just `files' replaced with `dir':

--8<--
VC backend : CVS
Working dir: /usr/ports/editors/
Repository : /cvs
Module     : ports/editors


                         ./
                         emacs/
    edited               emacs/Makefile
    edited               emacs/distinfo
                         emacs/patches/
    edited               emacs/patches/patch-Makefile_in
    edited               emacs/patches/patch-configure
    removed              emacs/patches/patch-src_unexelf_c
                         emacs/pkg/
    edited               emacs/pkg/PFRAG.no-no_x11
    edited               emacs/pkg/PLIST
-->8--

With the patch in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#15:

--8<--
VC backend : CVS
Working dir: /usr/ports/editors/
Repository : /cvs
Module     : ports/editors


                         ./
    unregistered         .//emacs-wip
                         emacs/
    edited               .//emacs/Makefile
    edited               .//emacs/distinfo
                         emacs/patches/
    edited               .//emacs/patches/patch-Makefile_in
    edited               .//emacs/patches/patch-configure
    unregistered         .//emacs/patches/patch-lisp_vc_vc-cvs_el
    unregistered         .//emacs/patches/patch-lisp_vc_vc_el
    removed              .//emacs/patches/patch-src_unexelf_c
                         emacs/pkg/
    edited               .//emacs/pkg/PFRAG.no-no_x11
    edited               .//emacs/pkg/PLIST
-->8--

So the patched version shows unregistered files and directories, which
is good, but has formatting artifacts (`.//' above).  Also, pressing `='
on `.//emacs-wip', I get the following in *Messages*:

...
Traversing directory /usr/ports/editors/...done
Finding changes in /usr/ports/editors/emacs-wip...
vc-find-backend-function: Cannot open load file: No such file or directory, vc-nil

even though the directory contains a CVS dir, and cvs(1) works fine in
it.  I don't think it's a big problem though.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 08.10.2016 18:13, Jérémie Courrèges-Anglas wrote:

> Oops, sorry.  Yes indeed, I only replaced `files' with `dir'.

OK, good. Any obvious problems with that solution? I don't work with CVS 
at all, and if you (or anyone else) interact with any particularly big 
repositories, I'd love to hear how it fares there.

>> and
>> whether it works well with all CVS repositories you work with.
>
> Works fine, there are some differences.

That's too bad. I've tried the patch myself, and I didn't see those, 
neither with the temporary repository created using Göktuğ's 
instructions, nor with a random GNU CVS repo I cloned afterwards.

Might there be something special with the repo you're trying it on? 
Maybe the fact that the working dir and the repository root only have 
the root directory as the common ancestor? Just a random guess.

> So the patched version shows unregistered files and directories, which
> is good, but has formatting artifacts (`.//' above).  Also, pressing `='
> on `.//emacs-wip', I get the following in *Messages*:
>
> ...
> Traversing directory /usr/ports/editors/...done
> Finding changes in /usr/ports/editors/emacs-wip...
> vc-find-backend-function: Cannot open load file: No such file or directory, vc-nil
>
> even though the directory contains a CVS dir, and cvs(1) works fine in
> it.  I don't think it's a big problem though.

Agree, that doesn't sound like too big of a problem.

And seeing unregistered files is pretty much working as intended. Alas, 
I see them in both versions of the code (the one using `dir', and the 
one using 'cvs update').

Any ideas why you're seeing differently? Do you have a non-default value 
of vc-cvs-stay-local, maybe?

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-08 11:06:10 PM +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> On 08.10.2016 18:13, Jérémie Courrèges-Anglas wrote:
>> Oops, sorry.  Yes indeed, I only replaced `files' with `dir'.
> OK, good. Any obvious problems with that solution? [...]

Does not show unregistered files at all, so need to add them from shell.
In fact, ‘cvs status’ command itself does not know anything about
unregistered files at all, it only deals with registered files.

>> [... Göktuğ's patch works] fine, there are some differences.
>
> That's too bad. I've tried the patch myself, and I didn't see those, 
> neither [...]
> Might there be something special with the repo you're trying it on? 

I've been using the patch since I submitted it, first on 25.1 release
candidates and then on 25.1 release itself, and have not encountered any
of these problems.  And most of my source-controlled stuff is in CVS.
I'd say the local configuration or the patch application may have caused
this problems.

I'd be glad if Jérémie Courrèges-Anglas could post output from ‘cvs -fnq
update’ in that checkout, I could maybe use it to understand the
situation (BTW the ‘cvs -fn update -dP’ command I gave as an example in
my previous message was mistaken, I took it from the ‘-’ lines of the
patch, sorry).

>> So the patched version shows unregistered files and directories, which
>> is good, but has formatting artifacts (`.//' above).  Also, pressing `='
>> on `.//emacs-wip', I get the following in *Messages*:
>>
>> ...
>> Traversing directory /usr/ports/editors/...done
>> Finding changes in /usr/ports/editors/emacs-wip...
>> vc-find-backend-function: Cannot open load file: No such file or directory, vc-nil
>>
>> even though the directory contains a CVS dir, and cvs(1) works fine in
>> it.  I don't think it's a big problem though.
>
> Agree, that doesn't sound like too big of a problem.
>
> And seeing unregistered files is pretty much working as intended. Alas, 
> I see them in both versions of the code (the one using `dir', and the 
> one using 'cvs update').

I don't see unregistered files with the dir version.

> Any ideas why you're seeing differently? Do you have a non-default value 
> of vc-cvs-stay-local, maybe?

If he's using the port from the repo in his message, the ‘lisp/vc/vc.el’
file seems to be patched and that may be conflicting.  If possible, I
ask Jérémie to test my patch on master or 25.1 release without all those
other patches, (applying it on package root w/ ‘patch -p 0’), and
running emacs w/ -Q flag.

Also, ‘vc-cvs-stay-local’ does not cause any problem, I just set it to
‘.*’ and tested, the output is as expected for me:

,----
| VC backend : CVS
| Working dir: /tmp/emacs.d/
| Repository : /igk/cvsroot
| Module     : emacs.d
| 
| 
|                          ./
|                          site/
|     unregistered         site/test.el
`----

-- 
İ. Göktuğ Kayaalp.
http://gkayaalp.com/

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 09.10.2016 15:18, Göktuğ Kayaalp wrote:

> Does not show unregistered files at all, so need to add them from shell.
> In fact, ‘cvs status’ command itself does not know anything about
> unregistered files at all, it only deals with registered files.

For some reason, I get 'cvs -f status' include the unregistered files in 
the output with the remote repo, but not with the one I've created from 
your test scenario. With the remote one, the output looks like:

cvs -f status
? asdasd
? tests/foo
cvs status: Examining .
===================================================================
File: AUTHORS          	Status: Up-to-date

    Working revision:	1.1
    Repository revision:	1.1	/sources/public/recut/AUTHORS,v
    Commit Identifier:	6fOuIAy2oJtoJf6y
    Sticky Tag:		(none)
    Sticky Date:		(none)
    Sticky Options:	(none)

===================================================================
File: COPYING          	Status: Locally Modified

    Working revision:	1.1
    Repository revision:	1.1	/sources/public/recut/COPYING,v
    Commit Identifier:	6fOuIAy2oJtoJf6y
    Sticky Tag:		(none)
    Sticky Date:		(none)
    Sticky Options:	(none)

...

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

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

On 2016-10-11 02:55 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> On 09.10.2016 15:18, Göktuğ Kayaalp wrote:
>
>> Does not show unregistered files at all, so need to add them from shell.
>> In fact, ‘cvs status’ command itself does not know anything about
>> unregistered files at all, it only deals with registered files.
>
> For some reason, I get 'cvs -f status' include the unregistered files in 
> the output with the remote repo, but not with the one I've created from 
> your test scenario. With the remote one, the output looks like:
>
> cvs -f status
> ? asdasd
> ? tests/foo
> [... snip ...]
>     Commit Identifier:	6fOuIAy2oJtoJf6y
> [... snip ...]

This looks like a CVSNT server[1] (given the ‘commit identifier’), so
that may be the cause.  I don't have a remote repo now, I'll try with
one as soon as I'll have free time (I'll try with OpenBSD src/).  Or it
may be the artifact of the missing ‘-q’ flag to cvs, or some config on
the remote.  I'm mostly a user of CVS so I don't know these detail,
sorry.

Would you mind trying ‘cvs -fnq update’ and ‘cvs -fq status’ with that
server too?

The GNU CVS manpage describes the ‘status’ comment thus:

,---- cvs(5)
| status Show current status of files: latest version, version in working
|        directory,  whether working version has been edited and, option-
|        ally, symbolic tags in the RCS file.  (Does not  change  reposi-
|        tory or working directory.)
`----

Which sort-a indicates that its scope is registered files, as
non-registered files would not have latest versions or modification
statuses.


[1] It is a fork of CVS in 2004, backwards compatible with many new
    features ap per the wiki says: https://en.wikipedia.org/wiki/CVSNT

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Andreas Schwab]

On Okt 11 2016, Göktuğ Kayaalp <self@gkayaalp.com> wrote:

> On 2016-10-11 02:55 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> On 09.10.2016 15:18, Göktuğ Kayaalp wrote:
>>
>>> Does not show unregistered files at all, so need to add them from shell.
>>> In fact, ‘cvs status’ command itself does not know anything about
>>> unregistered files at all, it only deals with registered files.
>>
>> For some reason, I get 'cvs -f status' include the unregistered files in 
>> the output with the remote repo, but not with the one I've created from 
>> your test scenario. With the remote one, the output looks like:
>>
>> cvs -f status
>> ? asdasd
>> ? tests/foo
>> [... snip ...]
>>     Commit Identifier:	6fOuIAy2oJtoJf6y
>> [... snip ...]
>
> This looks like a CVSNT server[1] (given the ‘commit identifier’), so

Commit identifiers are also implemented in CVS.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 11.10.2016 05:09, Göktuğ Kayaalp wrote:

> This looks like a CVSNT server[1] (given the ‘commit identifier’), so
> that may be the cause.  I don't have a remote repo now, I'll try with
> one as soon as I'll have free time (I'll try with OpenBSD src/).  Or it
> may be the artifact of the missing ‘-q’ flag to cvs,

I'm getting the same output from 'cvs -fq status'.

> or some config on
> the remote.  I'm mostly a user of CVS so I don't know these detail,
> sorry.

I'm not even a user, so unfortunately it seems like it falls on you to 
make sense of it. But if the feature doesn't work with all popular CVS 
servers, then it would be half-broken anyway, and we'd be better off 
relying on your solution.

Anyway, to clone the repo I've tried, you can:

CVSROOT=:pserver:anonymous@dev.w3.org:/sources/public cvs login
(enter 'anonymous')
CVSROOT=:pserver:anonymous@dev.w3.org:/sources/public cvs checkout recut

> Would you mind trying ‘cvs -fnq update’ and ‘cvs -fq status’ with that
> server too?

'cvs -fnq update' gives:

$ cvs -fnq update
? asdasd
? tests/foo
M COPYING
M tests/bytes2.sh

(So it doesn't mention up-to-date files; which can be a boon 
performance-wise, starting with repos of certain size).

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-11 11:51 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> [... snip ...] But if the feature doesn't work with all popular CVS 
> servers, then it would be half-broken anyway, and we'd be better off 
> relying on your solution.

Turns out my cvs was a year behind the latest, and the commitid was
ported in a later release (see also Andreas Schwab's message).  Sorry.

> 'cvs -fnq update' gives:
> [... snip ...]
> (So it doesn't mention up-to-date files; which can be a boon 
> performance-wise, starting with repos of certain size).

‘cvs status’ output is mostly garbage (i.e. not needed) in this context,
whereas every byte of ‘cvs -fnq update’ is used.  Also, the amount of
the output from the latter is a fraction of that from the former.  With
‘status’, reporting even a completely pristine repo will be slow if it
has more than a dozen files, and the results will depend on the CVS
version, regarding unregistered files, commitid, etc.

I'm CC'ing Andreas too, maybe he can comment about Jérémie's issue?
Also, AFAIK we havent heard from Dan Nicolescu yet.

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

Göktuğ Kayaalp <self@gkayaalp.com> writes:

> On 2016-10-08 11:06:10 PM +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> On 08.10.2016 18:13, Jérémie Courrèges-Anglas wrote:
>>> Oops, sorry.  Yes indeed, I only replaced `files' with `dir'.
>> OK, good. Any obvious problems with that solution? [...]
>
> Does not show unregistered files at all, so need to add them from shell.
> In fact, ‘cvs status’ command itself does not know anything about
> unregistered files at all, it only deals with registered files.
>
>>> [... Göktuğ's patch works] fine, there are some differences.
>>
>> That's too bad. I've tried the patch myself, and I didn't see those, 
>> neither [...]
>> Might there be something special with the repo you're trying it on? 

Well, the repo is a local mirror of the OpenBSD cvs repo, stored in /cvs
(/d/cvs today, after I had to do partitioning changes).  There may be
special settings in that repo, but the /tmp/test repo set up by the
script in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#33 shows
the same artifacts (see below).

> I've been using the patch since I submitted it, first on 25.1 release
> candidates and then on 25.1 release itself, and have not encountered any
> of these problems.  And most of my source-controlled stuff is in CVS.
> I'd say the local configuration or the patch application may have caused
> this problems.
>
> I'd be glad if Jérémie Courrèges-Anglas could post output from ‘cvs -fnq
> update’ in that checkout, I could maybe use it to understand the
> situation (BTW the ‘cvs -fn update -dP’ command I gave as an example in
> my previous message was mistaken, I took it from the ‘-’ lines of the
> patch, sorry).

/usr/ports/editors$ cvs -fnq update
? emacs-wip
? unregistered-file
M emacs/Makefile
M emacs/distinfo
M emacs/patches/patch-Makefile_in
M emacs/patches/patch-configure
R emacs/patches/patch-src_unexelf_c
? emacs/patches/patch-lisp_vc_vc-cvs_el
? emacs/patches/patch-lisp_vc_vc_el
M emacs/pkg/PFRAG.no-no_x11
M emacs/pkg/PLIST

The output is the same if I rename my ~/.cvsrc, which contains:
--8<--
cvs -q
diff -uNp
update -Pd
checkout -P
-->8--
probably irrelevant, since you specify -f.

Note that this is cvs from the base OpenBSD system:
--8<--
$ cvs --version

Concurrent Versions System (CVS) 1.11.1p1 (client/server)
...
-->8--

which AFAIK doesn't contain differences wrt cvs update from upstream
cvs-1.11.1p1.

>>> So the patched version shows unregistered files and directories, which
>>> is good, but has formatting artifacts (`.//' above).  Also, pressing `='
>>> on `.//emacs-wip', I get the following in *Messages*:
>>>
>>> ...
>>> Traversing directory /usr/ports/editors/...done
>>> Finding changes in /usr/ports/editors/emacs-wip...
>>> vc-find-backend-function: Cannot open load file: No such file or directory, vc-nil
>>>
>>> even though the directory contains a CVS dir, and cvs(1) works fine in
>>> it.  I don't think it's a big problem though.
>>
>> Agree, that doesn't sound like too big of a problem.
>>
>> And seeing unregistered files is pretty much working as intended. Alas, 
>> I see them in both versions of the code (the one using `dir', and the 
>> one using 'cvs update').
>
> I don't see unregistered files with the dir version.
>
>> Any ideas why you're seeing differently? Do you have a non-default value 
>> of vc-cvs-stay-local, maybe?

I had it in my ~/.emacs.d/init.el (I since removed it), but I ran my
tests with emacs -q, so it shouldn't affect the results anyway.  Also
note that the OpenBSD emacs package doesn't ship with a start file,
so -q and -Q should be equivalent:

/usr/local/share/emacs$ find . -name site-start.el
/usr/local/share/emacs$

> If he's using the port from the repo in his message, the ‘lisp/vc/vc.el’
> file seems to be patched and that may be conflicting.  If possible, I
> ask Jérémie to test my patch on master or 25.1 release without all those
> other patches, (applying it on package root w/ ‘patch -p 0’), and
> running emacs w/ -Q flag.

Here's the patch you spotted, it suppresses messages that slow down
vc-dir processes, but shouldn't affect the inner workings of vc-cvs.
Note that I first tested your patch using the master branch of the git
repo, which doesn't contain the patch below.

;$OpenBSD$
;
;- kill messages that slow down emacs at startup with many vc-dir opened.
;
;--- lisp/vc/vc.el.orig	Mon Oct  3 23:42:15 2016
;+++ lisp/vc/vc.el	Mon Oct  3 23:43:16 2016
;@@ -2897,13 +2897,11 @@ to provide the `find-revision' operation instead."
; (defun vc-file-tree-walk (dirname func &rest args)
;   "Walk recursively through DIRNAME.
; Invoke FUNC f ARGS on each VC-managed file f underneath it."
;-  (vc-file-tree-walk-internal (expand-file-name dirname) func args)
;-  (message "Traversing directory %s...done" dirname))
;+  (vc-file-tree-walk-internal (expand-file-name dirname) func args))
; 
; (defun vc-file-tree-walk-internal (file func args)
;   (if (not (file-directory-p file))
;       (when (vc-backend file) (apply func file args))
;-    (message "Traversing directory %s..." (abbreviate-file-name file))
;     (let ((dir (file-name-as-directory file)))
;       (mapcar
;        (lambda (f) (or

For completeness, here are the results with:

1. /usr/ports/editors, using emacs-25.1 and your patch

--8<--
VC backend : CVS
Working dir: /usr/ports/editors/
Repository : /d/cvs
Module     : ports/editors


                         ./
    unregistered         .//emacs-wip
    unregistered         .//unregistered-file
                         emacs/
    edited               .//emacs/Makefile
    edited               .//emacs/distinfo
                         emacs/patches/
    edited               .//emacs/patches/patch-Makefile_in
    edited               .//emacs/patches/patch-configure
    unregistered         .//emacs/patches/patch-lisp_vc_vc-cvs_el
    removed              .//emacs/patches/patch-src_unexelf_c
                         emacs/pkg/
    edited               .//emacs/pkg/PFRAG.no-no_x11
    edited               .//emacs/pkg/PLIST
-->8--

2. your /tmp/test testcase, from the cli and from vc, using emacs-master:

/tmp/test$ cvs -fnq update
M testfil
M subdir/subfil
/tmp/test$ emacs -Q .
--8<--
VC backend : CVS
Working dir: /tmp/test/
Repository : /tmp/cvsroot/
Module     : test


                         ./
    edited               .//testfil
                         subdir/
    edited               .//subdir/subfil
-->8--


3. your /tmp/test testcase, from vc, using emacs-master and your patch:

/tmp/test$ $HOME/src/emacs-master/src/emacs -Q .
--8<--
VC backend : CVS
Working dir: /tmp/test/
Repository : /tmp/cvsroot/
Module     : test


                         ./
    edited               .//testfil
                         subdir/
    edited               .//subdir/subfil
-->8--

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

Hi, I've tried it out with a checkout of OpenBSD src/, and with the
`recut' repo that Dmitry pointed, was unable to reproduce your issue.
The output from ‘cvs update’ that you shared is just as expected.  I'm
on FreeBSD myself, cvs 1.11.22.1-20080310-FreeBSD.  The NEWS file for
CVS suggests nothing happened to ‘update’ between at least 1.11.1 and
1.11.4.

I believe something happens when applying the patch, it should be
applied w/ ‘patch -p 1’ in emacs source root, and the resulting md5 hash
for the ‘vc-cvs.el’ file is this:

  MD5 (lisp/vc/vc-cvs.el) = 4e0f51335e8de5957eef2d92606146f8

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

Hi,

Göktuğ Kayaalp <self@gkayaalp.com> writes:

> Hi, I've tried it out with a checkout of OpenBSD src/, and with the
> `recut' repo that Dmitry pointed, was unable to reproduce your issue.
> The output from ‘cvs update’ that you shared is just as expected.  I'm
> on FreeBSD myself, cvs 1.11.22.1-20080310-FreeBSD.  The NEWS file for
> CVS suggests nothing happened to ‘update’ between at least 1.11.1 and
> 1.11.4.
>
> I believe something happens when applying the patch, it should be
> applied w/ ‘patch -p 1’ in emacs source root, and the resulting md5 hash
> for the ‘vc-cvs.el’ file is this:
>
>   MD5 (lisp/vc/vc-cvs.el) = 4e0f51335e8de5957eef2d92606146f8

ritchie ~/src/emacs-master$ md5 lisp/vc/vc-cvs.el
MD5 (lisp/vc/vc-cvs.el) = 4e0f51335e8de5957eef2d92606146f8

The problem is elsewhere, I'll see if I can debug this in the next days.
In the meantime, maybe a conservative approach would be better?

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 14.10.2016 23:33, Jérémie Courrèges-Anglas wrote:

> The problem is elsewhere, I'll see if I can debug this in the next days.

Thanks.

> In the meantime, maybe a conservative approach would be better?

Considering it introduces a feature regression (no unregistered and 
ignored files in the listing) at least with some clients, I'm not sure 
it's best.

And since it your case the problem is only of visual nature (diffs and 
navigation work all right, IIUC), the proposed more complex patch seems 
desirable.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-15 15:36 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> On 14.10.2016 23:33, Jérémie Courrèges-Anglas wrote:
>> In the meantime, maybe a conservative approach would be better?
>
> Considering it introduces a feature regression (no unregistered and 
> ignored files in the listing) at least with some clients, I'm not sure 
> it's best.
>
> And since it your case the problem is only of visual nature (diffs and 
> navigation work all right, IIUC), the proposed more complex patch seems 
> desirable.

As I noted before I couldn't reproduce his issues and can't spot
anything that might cause it in the patch (the code does not change the
strings in any way, maybe apart from the call to ‘file-relative-name’ in
the second hunk).  Hope Jérémie will be able to debug the issue.

-gk.

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

Göktuğ Kayaalp <self@gkayaalp.com> writes:

> On 2016-10-15 15:36 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> On 14.10.2016 23:33, Jérémie Courrèges-Anglas wrote:
>>> In the meantime, maybe a conservative approach would be better?
>>
>> Considering it introduces a feature regression (no unregistered and 
>> ignored files in the listing) at least with some clients, I'm not sure 
>> it's best.
>>
>> And since it your case the problem is only of visual nature (diffs and 
>> navigation work all right, IIUC), the proposed more complex patch seems 
>> desirable.
>
> As I noted before I couldn't reproduce his issues and can't spot
> anything that might cause it in the patch (the code does not change the
> strings in any way, maybe apart from the call to ‘file-relative-name’ in
> the second hunk).  Hope Jérémie will be able to debug the issue.

So...,

You asked for the output of `cvs -fnq update', but in my tests the
actual command that is run seems to be `cvs -fnq update ./'.  Re-using
your instructions in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#33

--8<--
ritchie /tmp/test$ cvs -fnq update ./
M .//testfil
M .//subdir/subfil
-->8--

The result looks like this in vc-dir:

--8<--
VC backend : CVS
Working dir: /tmp/test/
Repository : /tmp/cvsroot/
Module     : test


                         ./
    edited               .//testfil
                         subdir/
    edited               .//subdir/subfil
-->8--

This happens for all the *local repos* I have on this machine (including
my mirrors of the OpenBSD source tree).

Adding a few file-relative-name calls around `(match-string 1)' in
vc-cvs-after-dir-status helps getting a nicer formatting.


However, there could be a nastier problem with remote repos.

--8<--
ritchie /tmp/test$ cvs -d jca@localhost:/tmp/cvsroot/ -fnq up
M testfil
M subdir/subfil
-->8--

All is well... note how the file names are not prefixed with `./'.

--8<--
ritchie /tmp/test$ cvs -d jca@localhost:/tmp/cvsroot/ -fnq up ./
cvs server: conflict: testfil is modified but no longer in the repository
C testfil
M subdir/subfil
-->8--

Oops... cvs thinks what we want to run update from the cvs root
directory (which doesn't contain `testfil').

Here I modified the Root entries to point to localhost:
--8<--
ritchie /tmp/test$ cat CVS/Root
jca@localhost:/tmp/cvsroot/
ritchie /tmp/test$ cat subdir/CVS/Root
jca@localhost:/tmp/cvsroot/
ritchie /tmp/test$ cvs -fnq up
M testfil
M subdir/subfil
ritchie /tmp/test$ cvs -fnq up ./
cvs server: conflict: testfil is modified but no longer in the repository
C testfil
M subdir/subfil
-->8--

vc-dir shows:
--8<--
VC backend : CVS
Working dir: /tmp/test/
Repository : jca@localhost:/tmp/cvsroot/
Module     : test


                         ./
    conflict             testfil
                         subdir/
    edited               subdir/subfil
-->8--

So with `./', files in the current directory seem to be considered as if
we were at the root of the cvs repo root directory.  Subdirectories seem
to be fine, vc-diff works fine on them.

I can't see how the code would manage to pass `./' to cvs on my setup
and not on others.  I don't know if more recent cvs(1) versions handle
`./' and remote repos in a nicer way.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-15 16:06 +0200, jca@wxcvbn.org (Jérémie Courrèges-Anglas) wrote:
[... snip ...]
> You asked for the output of `cvs -fnq update', but in my tests the
> actual command that is run seems to be `cvs -fnq update ./'.  Re-using
> your instructions in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#33
>
> --8<--
> ritchie /tmp/test$ cvs -fnq update ./
> M .//testfil
> M .//subdir/subfil
> -->8--

I see the expected output, i.e. without the leading `.//'.

> The result looks like this in vc-dir:
[... snip ...]
>                          ./
>     edited               .//testfil
>                          subdir/
>     edited               .//subdir/subfil
> -->8--

Again I have no problems.

> This happens for all the *local repos* I have on this machine (including
> my mirrors of the OpenBSD source tree).
>
> Adding a few file-relative-name calls around `(match-string 1)' in
> vc-cvs-after-dir-status helps getting a nicer formatting.

See below.

> However, there could be a nastier problem with remote repos.
>
> --8<--
> ritchie /tmp/test$ cvs -d jca@localhost:/tmp/cvsroot/ -fnq up
> M testfil
> M subdir/subfil
> -->8--
>
> All is well... note how the file names are not prefixed with `./'.
>
> --8<--
> ritchie /tmp/test$ cvs -d jca@localhost:/tmp/cvsroot/ -fnq up ./
> cvs server: conflict: testfil is modified but no longer in the repository
> C testfil
> M subdir/subfil
> -->8--
>
> Oops... cvs thinks what we want to run update from the cvs root
> directory (which doesn't contain `testfil').

It does contain `testfil', when you `cvs import', it registers and
commits all the files in the current tree (‘ls -R /tmp/cvsroot/test/’).
I do not experience these problems, so maybe they've to do with the
OpenBSD version of CVS (they have their own copy under
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/gnu/usr.bin/cvs/ which
doesn't seem to be up-to-date, and seems that they cherry-pick patches
as NEWS and ChangeLog are 20 years old there).  I think vc.el is
supposed to display the state of the repo, not interpret it, though the
its would know better than me.  This is (and the initial ‘./’) probably
an issue of OpenBSD's fork of GNU CVS.

> Here I modified the Root entries to point to localhost:
> --8<--
> ritchie /tmp/test$ cat CVS/Root
> jca@localhost:/tmp/cvsroot/
> ritchie /tmp/test$ cat subdir/CVS/Root
> jca@localhost:/tmp/cvsroot/
> ritchie /tmp/test$ cvs -fnq up
> M testfil
> M subdir/subfil
> ritchie /tmp/test$ cvs -fnq up ./
> cvs server: conflict: testfil is modified but no longer in the repository
> C testfil
> M subdir/subfil
> -->8--
>
> vc-dir shows:
> --8<--
> VC backend : CVS
> Working dir: /tmp/test/
> Repository : jca@localhost:/tmp/cvsroot/
> Module     : test
>
>
>                          ./
>     conflict             testfil
>                          subdir/
>     edited               subdir/subfil
> -->8--
>
> So with `./', files in the current directory seem to be considered as if
> we were at the root of the cvs repo root directory.  Subdirectories seem
> to be fine, vc-diff works fine on them.
>
> I can't see how the code would manage to pass `./' to cvs on my setup
> and not on others.  I don't know if more recent cvs(1) versions handle
> `./' and remote repos in a nicer way.

Probably the code passes ‘./’ on all setups but they just handle it
fine.  Your OpenBSD CVS (not OpenCVS, it's their re-implementation of
CVS) is probably a bit divergent from mainstream CVS and thus these
problems.  I guess if you used mainstream CVS, it'd work OK.

Maybe these need to be reported on OpenBSD bugtracker.  I'm reluctant to
trying to guess what CVS wants to way WRT file names, but I can work on
it if that's considered a better approach.

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

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

On 15.10.2016 17:06, Jérémie Courrèges-Anglas wrote:

> You asked for the output of `cvs -fnq update', but in my tests the
> actual command that is run seems to be `cvs -fnq update ./'.

Could you both please try the attached modified patch.

It removes the vc-expand-dirs call from vc-cvs-dir-status-files (its 
return value was unused anyway), and passes FILES unmodified to 
vc-cvs-command.

We can be fairly sure that DIR is default-directory, so passing it in 
explicitly is unnecessary. Whenever that ceases to be the case, we'll 
have to update other backends as well.

[-- Attachment #2: bug-24082-2.patch --]
[-- Type: text/x-diff, Size: 6018 bytes --]

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index a2499a2..2134793 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -938,103 +938,32 @@ vc-cvs-parse-status
 	  (t 'edited))))))))
 
 (defun vc-cvs-after-dir-status (update-function)
-  ;; Heavily inspired by vc-cvs-parse-status. AKA a quick hack.
-  ;; This needs a lot of testing.
-  (let ((status nil)
-	(status-str nil)
-	(file nil)
-	(result nil)
-	(missing nil)
-	(ignore-next nil)
-	(subdir default-directory))
+  (let ((result nil)
+  	(translation '((?? . unregistered)
+  		       (?A . added)
+  		       (?C . conflict)
+  		       (?M . edited)
+  		       (?P . needs-merge)
+  		       (?R . removed)
+  		       (?U . needs-update))))
     (goto-char (point-min))
-    (while
-	;; Look for either a file entry, an unregistered file, or a
-	;; directory change.
-	(re-search-forward
-	 "\\(^=+\n\\([^=c?\n].*\n\\|\n\\)+\\)\\|\\(\\(^?? .*\n\\)+\\)\\|\\(^cvs status: \\(Examining\\|nothing\\) .*\n\\)"
-	 nil t)
-      ;; FIXME: get rid of narrowing here.
-      (narrow-to-region (match-beginning 0) (match-end 0))
-      (goto-char (point-min))
-      ;; The subdir
-      (when (looking-at "cvs status: Examining \\(.+\\)")
-	(setq subdir (expand-file-name (match-string 1))))
-      ;; Unregistered files
-      (while (looking-at "? \\(.*\\)")
-	(setq file (file-relative-name
-		    (expand-file-name (match-string 1) subdir)))
-	(push (list file 'unregistered) result)
-	(forward-line 1))
-      (when (looking-at "cvs status: nothing known about")
-	;; We asked about a non existent file.  The output looks like this:
-
-	;; cvs status: nothing known about `lisp/v.diff'
-	;; ===================================================================
-	;; File: no file v.diff            Status: Unknown
-	;;
-	;;    Working revision:    No entry for v.diff
-	;;    Repository revision: No revision control file
-	;;
-
-	;; Due to narrowing in this iteration we only see the "cvs
-	;; status:" line, so just set a flag so that we can ignore the
-	;; file in the next iteration.
-	(setq ignore-next t))
-      ;; A file entry.
-      (when (re-search-forward "^File: \\(no file \\)?\\(.*[^ \t]\\)[ \t]+Status: \\(.*\\)" nil t)
-	(setq missing (match-string 1))
-	(setq file (file-relative-name
-		    (expand-file-name (match-string 2) subdir)))
-	(setq status-str (match-string 3))
-	(setq status
-	      (cond
-	       ((string-match "Up-to-date" status-str) 'up-to-date)
-	       ((string-match "Locally Modified" status-str) 'edited)
-	       ((string-match "Needs Merge" status-str) 'needs-merge)
-	       ((string-match "Needs \\(Checkout\\|Patch\\)" status-str)
-		(if missing 'missing 'needs-update))
-	       ((string-match "Locally Added" status-str) 'added)
-	       ((string-match "Locally Removed" status-str) 'removed)
-	       ((string-match "File had conflicts " status-str) 'conflict)
-	       ((string-match "Unknown" status-str) 'unregistered)
-	       (t 'edited)))
-	(if ignore-next
-	    (setq ignore-next nil)
-	  (unless (eq status 'up-to-date)
-	    (push (list file status) result))))
-      (goto-char (point-max))
-      (widen))
-    (funcall update-function result))
-  ;; Alternative implementation: use the "update" command instead of
-  ;; the "status" command.
-  ;; (let ((result nil)
-  ;; 	(translation '((?? . unregistered)
-  ;; 		       (?A . added)
-  ;; 		       (?C . conflict)
-  ;; 		       (?M . edited)
-  ;; 		       (?P . needs-merge)
-  ;; 		       (?R . removed)
-  ;; 		       (?U . needs-update))))
-  ;;   (goto-char (point-min))
-  ;;   (while (not (eobp))
-  ;;     (if (looking-at "^[ACMPRU?] \\(.*\\)$")
-  ;; 	  (push (list (match-string 1)
-  ;; 		      (cdr (assoc (char-after) translation)))
-  ;; 		result)
-  ;; 	(cond
-  ;; 	 ((looking-at "cvs update: warning: \\(.*\\) was lost")
-  ;; 	  ;; Format is:
-  ;; 	  ;; cvs update: warning: FILENAME was lost
-  ;; 	  ;; U FILENAME
-  ;; 	  (push (list (match-string 1) 'missing) result)
-  ;; 	  ;; Skip the "U" line
-  ;; 	  (forward-line 1))
-  ;; 	 ((looking-at "cvs update: New directory `\\(.*\\)' -- ignored")
-  ;; 	  (push (list (match-string 1) 'unregistered) result))))
-  ;;     (forward-line 1))
-  ;;   (funcall update-function result)))
-  )
+    (while (not (eobp))
+      (if (looking-at "^[ACMPRU?] \\(.*\\)$")
+  	  (push (list (match-string 1)
+  		      (cdr (assoc (char-after) translation)))
+  		result)
+  	(cond
+  	 ((looking-at "cvs update: warning: \\(.*\\) was lost")
+  	  ;; Format is:
+  	  ;; cvs update: warning: FILENAME was lost
+  	  ;; U FILENAME
+  	  (push (list (match-string 1) 'missing) result)
+  	  ;; Skip the "U" line
+  	  (forward-line 1))
+  	 ((looking-at "cvs update: New directory `\\(.*\\)' -- ignored")
+  	  (push (list (match-string 1) 'unregistered) result))))
+      (forward-line 1))
+    (funcall update-function result)))
 
 ;; Based on vc-cvs-dir-state-heuristic from Emacs 22.
 ;; FIXME does not mention unregistered files.
@@ -1071,16 +1000,12 @@ vc-cvs-dir-status-files
 Query all files in DIR if files is nil."
   (let ((local (vc-cvs-stay-local-p dir)))
     (if (and (not files) local (not (eq local 'only-file)))
-	(vc-cvs-dir-status-heuristic dir update-function)
-      (if (not files) (setq files (vc-expand-dirs (list dir) 'CVS)))
-      (vc-cvs-command (current-buffer) 'async files "-f" "status")
-      ;; Alternative implementation: use the "update" command instead of
-      ;; the "status" command.
-      ;; (vc-cvs-command (current-buffer) 'async
-      ;; 		  (file-relative-name dir)
-      ;; 		  "-f" "-n" "update" "-d" "-P")
-      (vc-run-delayed
-       (vc-cvs-after-dir-status update-function)))))
+	(vc-cvs-dir-status-heuristic dir update-function))
+    (vc-cvs-command (current-buffer) 'async
+                    files
+                    "-f" "-n" "-q" "update")
+    (vc-run-delayed
+      (vc-cvs-after-dir-status update-function))))
 
 (defun vc-cvs-file-to-string (file)
   "Read the content of FILE and return it as a string."

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-16 00:36 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> [1:text/plain Hide]
>
> On 15.10.2016 17:06, Jérémie Courrèges-Anglas wrote:
>
>> You asked for the output of `cvs -fnq update', but in my tests the
>> actual command that is run seems to be `cvs -fnq update ./'.
>
> Could you both please try the attached modified patch.

Works fine for me, both local and remote.

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Jérémie Courrèges-Anglas]

Göktuğ Kayaalp <self@gkayaalp.com> writes:

> On 2016-10-16 00:36 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>> [1:text/plain Hide]
>>
>> On 15.10.2016 17:06, Jérémie Courrèges-Anglas wrote:
>>
>>> You asked for the output of `cvs -fnq update', but in my tests the
>>> actual command that is run seems to be `cvs -fnq update ./'.
>>
>> Could you both please try the attached modified patch.
>
> Works fine for me, both local and remote.

Same here, thanks.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 16.10.2016 15:38, Jérémie Courrèges-Anglas wrote:
> Göktuğ Kayaalp <self@gkayaalp.com> writes:
>
>> On 2016-10-16 00:36 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
>>> Could you both please try the attached modified patch.
>>
>> Works fine for me, both local and remote.
>
> Same here, thanks.

That's great.

Eli, do we have your blessing to push the patch to emacs-25? It seems 
non-trivial, but there's no much new code there actually, and it seems 
to work well.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Eli Zaretskii]

> Cc: Jérémie Courrèges-Anglas <jca@wxcvbn.org>,
>  Göktuğ Kayaalp <self@gkayaalp.com>,
>  24082@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 16 Oct 2016 17:07:44 +0300
> 
> Eli, do we have your blessing to push the patch to emacs-25? It seems 
> non-trivial, but there's no much new code there actually, and it seems 
> to work well.

Can you show me the patch again?  I'm afraid I got confused between
the alternatives.

Thanks.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 16.10.2016 17:23, Eli Zaretskii wrote:

> Can you show me the patch again?  I'm afraid I got confused between
> the alternatives.
>
> Thanks.

It's the one in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#96.

Direct link: 
https://debbugs.gnu.org/cgi/bugreport.cgi?filename=bug-24082-2.patch;bug=24082;att=1;msg=96

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Eli Zaretskii]

> Cc: self@gkayaalp.com, 24082@debbugs.gnu.org, jca@wxcvbn.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 16 Oct 2016 18:58:36 +0300
> 
> It's the one in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24082#96.
> 
> Direct link: 
> https://debbugs.gnu.org/cgi/bugreport.cgi?filename=bug-24082-2.patch;bug=24082;att=1;msg=96

This is okay for emacs-25, thanks.

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Dmitry Gutov]

On 16.10.2016 20:58, Eli Zaretskii wrote:

>> Direct link:
>> https://debbugs.gnu.org/cgi/bugreport.cgi?filename=bug-24082-2.patch;bug=24082;att=1;msg=96
>
> This is okay for emacs-25, thanks.

Pushed. Thanks everyone!

bug#24082: 25.1; vc-dir for CVS repositories list all files as if from toplevel directory

[Göktuğ Kayaalp]

On 2016-10-18 03:04 +0300, Dmitry Gutov <dgutov@yandex.ru> wrote:
> Pushed. Thanks everyone!

My pleasure, and thanks to everybody for their help!  I'm ecstatic for
having gotten my first patch into Emacs!

-- 
İ. V. Göktuğ Kayaalp                           <http://www.gkayaalp.com>
                        PGP pubkey: <http://www.gkayaalp.com/pubkey.asc>