subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Karl Fogel <kfo...@red-bean.com>
Subject Re: [PATCH] Support SVN 1.7 working copies in psvn.el
Date Thu, 30 Aug 2012 16:47:07 GMT
André Colomb <acolomb@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
> ;; --------------------------------------------------------------------------------

Mime
View raw message