diff-find-file-name and /dev/null

diff-find-file-name and /dev/null

[Nikolaj Schumacher]

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

Hello,

using diff files (from git) that create new files like this:

--- /dev/null
+++ b/foo/bar.c

I noticed that `diff-goto-source' opens /dev/null, which is usually not
desirable.  I use the attached small change to get the actual file.


regards,
Nikolaj Schumacher


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

--- lisp/diff-mode.el	2007-08-17 06:03:27.000000000 +0200
+++ lisp/diff-mode.el	2007-08-18 00:48:25.000000000 +0200
@@ -667,6 +667,7 @@
       (or (ignore-errors (diff-beginning-of-file))
 	  (re-search-forward diff-file-header-re nil t)))
     (let ((fs (diff-hunk-file-names old)))
+      (setq fs (delete "/dev/null" fs))
       (if prefix (setq fs (mapcar (lambda (f) (concat prefix f)) fs)))
       (or
        ;; use any previously used preference

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

_______________________________________________
bug-gnu-emacs mailing list
bug-gnu-emacs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-gnu-emacs

Re: diff-find-file-name and /dev/null

[Michaël Cadilhac]

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

Nikolaj Schumacher <n_schumacher@web.de> writes:

> Hello,
>
> using diff files (from git) that create new files like this:
>
> --- /dev/null
> +++ b/foo/bar.c
>
> I noticed that `diff-goto-source' opens /dev/null, which is usually not
> desirable.  I use the attached small change to get the actual file.

Just for my information, why do you think it's not a good thing?

-- 
 |   Michaël `Micha' Cadilhac       |  La culture c'est comme la confiture,  |
 |   http://michael.cadilhac.name   |      c'est meilleur avec du pain.      |
 |   JID/MSN:                       |           -- MOI59                     |
 `----  michael.cadilhac@gmail.com  |                                   -  --'

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

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

_______________________________________________
bug-gnu-emacs mailing list
bug-gnu-emacs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-gnu-emacs

Re: diff-find-file-name and /dev/null

[Nikolaj Schumacher]

michael@cadilhac.name (Michaël Cadilhac) wrote:

> Nikolaj Schumacher <n_schumacher@web.de> writes:
>
>> --- /dev/null
>> +++ b/foo/bar.c
>>
>> I noticed that `diff-goto-source' opens /dev/null, which is usually not
>> desirable.  I use the attached small change to get the actual file.
>
> Just for my information, why do you think it's not a good thing?

Since there's nothing to view or edit in /dev/null.

Also in a usual diff file

--- /dev/null	2007-08-17 06:03:27.000000000 +0200
+++ /foo/bar.c	2007-08-18 00:48:25.000000000 +0200

`diff-goto-source' would visit /foo/bar.c

git's peculiar a/ b/ prefixes are the problem here.  Usually it works,
because `diff-find-file-name' chops those off, if the file doesn't
exist.  It will try b/foo/bar.c first.  But then /dev/null, before
foo/bar.c.

I admit the actual fault could be the fact that it looks for first one
at all (or at least _before_ chopping off prefixes of the second one),
even when diff-jump-to-old-file is nil.  /dev/null is just the only
cause of such behavior.


regards,
Nikolaj Schumacher

Re: diff-find-file-name and /dev/null

[Glenn Morris]

Nikolaj Schumacher wrote:

> Since there's nothing to view or edit in /dev/null.

How about this patch then:

diff -c -c -w -r1.99.2.3 diff-mode.el
*** diff-mode.el	6 Aug 2007 20:08:07 -0000	1.99.2.3
--- diff-mode.el	25 Aug 2007 01:41:53 -0000
***************
*** 646,652 ****
  	   ((or (null files)
  		(setq file (do* ((files files (cdr files))
  				 (file (car files) (car files)))
! 			       ((or (null file) (file-exists-p file))
  				file))))
  	    file))
         ;; <foo>.rej patches implicitly apply to <foo>
--- 646,652 ----
  	   ((or (null files)
  		(setq file (do* ((files files (cdr files))
  				 (file (car files) (car files)))
! 			       ((or (null file) (file-regular-p file))
  				file))))
  	    file))
         ;; <foo>.rej patches implicitly apply to <foo>

> Also in a usual diff file
>
> --- /dev/null	2007-08-17 06:03:27.000000000 +0200
> +++ /foo/bar.c	2007-08-18 00:48:25.000000000 +0200
>
> `diff-goto-source' would visit /foo/bar.c

It's easy to produce a non-git diff that has the same behaviour. Just
do a diff between /dev/null and a relative filename:

diff -u /dev/null foo/somefile > foo/some.diff

Then open foo/some.diff in Emacs and try to jump to source. I'd never
make a hideous diff like that though...

> I admit the actual fault could be the fact that it looks for first
> one at all (or at least _before_ chopping off prefixes of the second
> one), even when diff-jump-to-old-file is nil. /dev/null is just the
> only cause of such behavior.

Yes, I don't see why diff-find-file-name does that, and I don't think
/dev/null is the only cause. Eg repeating the above example, but with
regular files:

diff -u /tmp/file1 foo/file2 > foo/file.diff

diff-goto-source will open file1 rather than file2.


Stefan, is there a reason why diff-find-file-name can return the old
file, even when OLD is nil? Why doesn't it just consider
(car (diff-hunk-file-names old))?

If it should consider both old and new in each case, should it not try
dropping the directory entries from the new/old file first, before
going on to check the old/new one in the same way only it it does not
find a match?

Re: diff-find-file-name and /dev/null

[Stefan Monnier]

>> Since there's nothing to view or edit in /dev/null.

> How about this patch then:

> diff -c -c -w -r1.99.2.3 diff-mode.el
> *** diff-mode.el	6 Aug 2007 20:08:07 -0000	1.99.2.3
> --- diff-mode.el	25 Aug 2007 01:41:53 -0000
> ***************
> *** 646,652 ****
>   	   ((or (null files)
>   		(setq file (do* ((files files (cdr files))
>   				 (file (car files) (car files)))
> ! 			       ((or (null file) (file-exists-p file))
>   				file))))
>   	    file))
>          ;; <foo>.rej patches implicitly apply to <foo>
> --- 646,652 ----
>   	   ((or (null files)
>   		(setq file (do* ((files files (cdr files))
>   				 (file (car files) (car files)))
> ! 			       ((or (null file) (file-regular-p file))
>   				file))))
>   	    file))
>          ;; <foo>.rej patches implicitly apply to <foo>

Looks very good.  This will also rule out directories.  Please install.

>> I admit the actual fault could be the fact that it looks for first
>> one at all (or at least _before_ chopping off prefixes of the second
>> one), even when diff-jump-to-old-file is nil. /dev/null is just the
>> only cause of such behavior.

> Yes, I don't see why diff-find-file-name does that, and I don't think
> /dev/null is the only cause. Eg repeating the above example, but with
> regular files:

> diff -u /tmp/file1 foo/file2 > foo/file.diff

> diff-goto-source will open file1 rather than file2.

> Stefan, is there a reason why diff-find-file-name can return the old
> file, even when OLD is nil? Why doesn't it just consider
> (car (diff-hunk-file-names old))?

diff-find-file-name tries pretty hard to find *A* file, so I think it's
OK for it to sometimes return an old file even when OLD is nil.
But maybe the order in which file names are tried could be improved.

> If it should consider both old and new in each case, should it not try
> dropping the directory entries from the new/old file first, before
> going on to check the old/new one in the same way only it it does not
> find a match?

Sounds right.  The current ordering is somewhat arbitrary.  Fell free to
change it as you suggest, but for any such change, please add a comment
explaining the situation for which the change makes sense.


        Stefan

Re: diff-find-file-name and /dev/null

[Glenn Morris]

Stefan Monnier wrote:

> Looks very good.  This will also rule out directories.  Please install.

OK, installed on release branch.

> Sounds right.  The current ordering is somewhat arbitrary.  Fell free to
> change it as you suggest, but for any such change, please add a comment
> explaining the situation for which the change makes sense.

OK, I may look at doing this on the trunk.