André Colomb <acol...@schickhardt.org> writes:
>After some more testing, I have found another problem with the
>svn-status mode in psvn.el. The attached patches extend on the previous
>one to also make ediff-mode work again with psvn.el.
>
>[...]
>
>The previously proposed patch by Koji Nakamaru addressed an error when
>trying to invoke svn-status from a non-top-level working copy directory.
>In addition, I discovered some errors when trying to use
>svn-status-ediff-with-revision.
>
>The lisp function svn-status-get-specific-revision-internal included a
>short-cut for accessing the BASE revision of a specific working copy
>file. In working copies before SVN 1.7, the BASE revision could be
>retrieved from "./.svn/text-base/<filename>.svn-base". In SVN 1.7, it is
>much more complicated to find this data in the top-level .svn/pristine/
>directory.
>
>Therefore the attached patch psvn.el_svn1.7-BASE.diff adresses this by
>eliminating the shortcut. It treats BASE like any other revision and
>asks the subversion client for it via "svn cat". This still works even
>for offline operation, when the repository can not be accessed. The
>subversion CLI extracts the correct file content from its store for us.
>
>Also attached is the previous patch by Koji Nakamaru, renamed as
>psvn.el_svn1.7-dotsvn.diff, as well as a combination of the two named
>psvn.el_svn1.7.diff.
>
>Note that using svn cat on the BASE revision instead of opening it from
>text-base/ will trigger another bug in psvn.el, where the line endings
>might be misinterpreted. I have already identified the cause and sent a
>patch for this in another mail to this list.
>
>Feel free to ask for any feedback concerning the changes. I would be
>very happy if this could be committed to trunk and land in the next relea=
>se.
>
>If you accept my patches, I could take care of forwarding them to any
>distributions already shipping SVN 1.7 with psvn.el that I know of, so
>they can fix the problem in their subversion emacs packages.
Hi, André. This isn't a full review, and anyway in general these
changes look good to me!
But I'd like to request doc strings on new functions -- for example,
in psvn.el_svn1.7-dotsvn.diff:
>Index: contrib/client-side/emacs/psvn.el
>--- contrib/client-side/emacs/psvn.el (revision 1374368)
>+++ contrib/client-side/emacs/psvn.el (working copy)
>@@ -1137,7 +1137,7 @@ If there is no .svn directory, examine if there is
> (svn-wc-adm-dir-name)))
> (cvs-dir (format "%sCVS" (file-name-as-directory dir))))
> (cond
>- ((file-directory-p svn-dir)
>+ ((my-file-directory-p svn-dir)
> (setq arg (svn-status-possibly-negate-meaning-of-arg arg 'svn-status))
> (svn-status-1 dir arg))
> ((and (file-directory-p cvs-dir)
>@@ -6036,12 +6036,12 @@ Return nil, if not in a svn working copy."
> (let* ((base-dir start-dir)
> (repository-root (svn-status-repo-for-path base-dir))
> (dot-svn-dir (concat base-dir (svn-wc-adm-dir-name)))
>- (in-tree (and repository-root (file-exists-p dot-svn-dir)))
>+ (in-tree (and repository-root (my-file-exists-p dot-svn-dir)))
> (dir-below (expand-file-name base-dir)))
> ;; (message "repository-root: %s start-dir: %s" repository-root
> start-dir)
> (if (and (<= (car svn-client-version) 1) (< (cadr svn-client-version)
> 3))
> (setq base-dir (svn-status-base-dir-for-ancient-svn-client
> start-dir)) ;; svn version < 1.3
>- (while (when (and dir-below (file-exists-p dot-svn-dir))
>+ (while (when (and dir-below (my-file-exists-p dot-svn-dir))
> (setq base-dir (file-name-directory dot-svn-dir))
> (string-match "\\(.+/\\).+/" dir-below)
> (setq dir-below
>@@ -6423,6 +6423,19 @@ working directory."
> (setq svn-admin-last-repository-dir (read-string "Repository Url: ")))
> (svn-checkout svn-admin-last-repository-dir "."))
>
>+(defun my-file-directory-p (dir)
>+ (setq dir (expand-file-name dir))
>+ (if (file-directory-p dir)
>+ t
>+ (let* ((dir1 (directory-file-name (file-name-directory dir)))
>+ (dir2 (directory-file-name (file-name-directory dir1))))
>+ (if (equal dir1 dir2)
>+ nil
>+ (my-file-directory-p (concat (file-name-as-directory dir2)
>".svn"))))))
>+
>+(defun my-file-exists-p (dir)
>+ (my-file-directory-p dir))
There are two new helper functions here. If each had a doc string, then
the reader would understand why they exist. It's particularly necessary
here because `my-file-exists-p' is just a pass-through to
`my-file-directory-p', so a reader might wonder what `my-file-exists-p'
is for -- especially since its parameter is named "dir" :-).
I don't quite understand the code in `my-file-directory-p'. Are you
positive its correct? The names of the variables "dir1" and "dir2"
don't explain much -- it's not clear that they're always directories.
The reason I am skeptical about the code is this:
(file-name-directory "/home/kfogel/some-dir/")
==> "/home/kfogel/some-dir/"
(file-name-directory "/home/kfogel/some-dir")
==> "/home/kfogel/"
Note that when I tested the above, "some-dir" did not exist!
So I'm unsure about the logic in `my-file-directory-p', and I'm unsure
whether `my-file-exists-p' does what its name implies.
By the way, in the former, shouldn't it use `string='? As in:
(if (string= dir1 dir2)
...)
I haven't reviewed the other patches, but I spent some effort extracting
them from the mail archives, so I'm going to include them below to have
easier access to them later if I end up reviewing them :-).
Best,
-Karl
psvn.el_svn1.7-BASE.diff:
>Index: contrib/client-side/emacs/psvn.el
>--- contrib/client-side/emacs/psvn.el (revision 1377692)
>+++ contrib/client-side/emacs/psvn.el (working copy)
>@@ -4568,18 +4568,14 @@ names are relative to the directory where `svn-sta
> (expand-file-name(concat default-directory
> file-name-with-revision)))
> (let ((content
> (with-temp-buffer
>- (if (string= revision "BASE")
>- (insert-file-contents (concat
>(svn-wc-adm-dir-name)
>- "/text-base/"
>-
>(file-name-nondirectory file-name)
>- ".svn-base"))
>- (progn
>- (svn-run nil t 'cat "cat" "-r" revision
>- (concat default-directory
>(file-name-nondirectory file-name)))
>- ;;todo: error processing
>- ;;svn: Filesystem has no item
>- ;;svn: file not found: revision `15', path
>`/trunk/file.txt'
>- (insert-buffer-substring
>svn-process-buffer-name)))
>+ (progn
>+ (setq buffer-file-coding-system 'undefined)
>+ (svn-run nil t 'cat "cat" "-r" revision
>+ (concat default-directory
>(file-name-nondirectory file-name)))
>+ ;;todo: error processing
>+ ;;svn: Filesystem has no item
>+ ;;svn: file not found: revision `15', path
>`/trunk/file.txt'
>+ (insert-buffer-substring svn-process-buffer-name))
> (buffer-string))))
> (find-file file-name-with-revision)
> (setq buffer-read-only nil)
psvn.el_svn1.7.diff:
>Index: contrib/client-side/emacs/psvn.el
>--- contrib/client-side/emacs/psvn.el (revision 1377692)
>+++ contrib/client-side/emacs/psvn.el (working copy)
>@@ -1137,7 +1137,7 @@ If there is no .svn directory, examine if there is
> (svn-wc-adm-dir-name)))
> (cvs-dir (format "%sCVS" (file-name-as-directory dir))))
> (cond
>- ((file-directory-p svn-dir)
>+ ((my-file-directory-p svn-dir)
> (setq arg (svn-status-possibly-negate-meaning-of-arg arg 'svn-status))
> (svn-status-1 dir arg))
> ((and (file-directory-p cvs-dir)
>@@ -4568,18 +4568,14 @@ names are relative to the directory where `svn-sta
> (expand-file-name(concat default-directory
> file-name-with-revision)))
> (let ((content
> (with-temp-buffer
>- (if (string=3D revision "BASE")
>- (insert-file-contents (concat
>(svn-wc-adm-dir-name)
>- "/text-base/"
>-
>(file-name-nondirectory file-name)
>- ".svn-base"))
>- (progn
>- (svn-run nil t 'cat "cat" "-r" revision
>- (concat default-directory
>(file-name-nondirectory file-name)))
>- ;;todo: error processing
>- ;;svn: Filesystem has no item
>- ;;svn: file not found: revision `15', path
>`/trunk/file.txt'
>- (insert-buffer-substring
>svn-process-buffer-name)))
>+ (progn
>+ (setq buffer-file-coding-system 'undefined)
>+ (svn-run nil t 'cat "cat" "-r" revision
>+ (concat default-directory
>(file-name-nondirectory file-name)))
>+ ;;todo: error processing
>+ ;;svn: Filesystem has no item
>+ ;;svn: file not found: revision `15', path
>`/trunk/file.txt'
>+ (insert-buffer-substring svn-process-buffer-name))
> (buffer-string))))
> (find-file file-name-with-revision)
> (setq buffer-read-only nil)
>@@ -6036,12 +6032,12 @@ Return nil, if not in a svn working copy."
> (let* ((base-dir start-dir)
> (repository-root (svn-status-repo-for-path base-dir))
> (dot-svn-dir (concat base-dir (svn-wc-adm-dir-name)))
>- (in-tree (and repository-root (file-exists-p dot-svn-dir)))
>+ (in-tree (and repository-root (my-file-exists-p dot-svn-dir)))
> (dir-below (expand-file-name base-dir)))
> ;; (message "repository-root: %s start-dir: %s" repository-root
> start-dir)
> (if (and (<=3D (car svn-client-version) 1) (< (cadr
> svn-client-version) 3))
> (setq base-dir (svn-status-base-dir-for-ancient-svn-client
> start-dir)) ;; svn version < 1.3
>- (while (when (and dir-below (file-exists-p dot-svn-dir))
>+ (while (when (and dir-below (my-file-exists-p dot-svn-dir))
> (setq base-dir (file-name-directory dot-svn-dir))
> (string-match "\\(.+/\\).+/" dir-below)
> (setq dir-below
>@@ -6423,6 +6419,19 @@ working directory."
> (setq svn-admin-last-repository-dir (read-string "Repository Url: ")))
> (svn-checkout svn-admin-last-repository-dir "."))
>=20
>+(defun my-file-directory-p (dir)
>+ (setq dir (expand-file-name dir))
>+ (if (file-directory-p dir)
>+ t
>+ (let* ((dir1 (directory-file-name (file-name-directory dir)))
>+ (dir2 (directory-file-name (file-name-directory dir1))))
>+ (if (equal dir1 dir2)
>+ nil
>+ (my-file-directory-p (concat (file-name-as-directory dir2) ".svn=
>"))))))
>+
>+(defun my-file-exists-p (dir)
>+ (my-file-directory-p dir))
>+
> ;;
> --------------------------------------------------------------------------------
> ;; svn status profiling
> ;;
> --------------------------------------------------------------------------------
