diff-apply-hunk documentation doesn't match implementation

diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

The doc string of diff-apply-hunk says:

  By default, the new source file is patched, but if the variable
  `diff-jump-to-old-file' is non-nil, then the old source file is
  patched instead

But the implementation then goes on to do the opposite:

      ;; If REVERSE go to the new file, otherwise go to the old.
      (diff-find-source-location (not reverse) reverse)

IMHO the implementation should be changed to match documentation, since
the current behaviour is quite counter-intuitive.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

> The doc string of diff-apply-hunk says:
>   By default, the new source file is patched, but if the variable
>   `diff-jump-to-old-file' is non-nil, then the old source file is
>   patched instead

> But the implementation then goes on to do the opposite:

>       ;; If REVERSE go to the new file, otherwise go to the old.
>       (diff-find-source-location (not reverse) reverse)

This is just one part of the implementation.  The behavior is actually
pretty subtle (too subtle, admittedly).  So I can't judge without knowing
which end-user-level behavior you're referring to.

I.e. give me a recipe.


        Stefan

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This is just one part of the implementation.  The behavior is actually
> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
> which end-user-level behavior you're referring to.

I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
applies it to the original instead of the file I look at.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

>> This is just one part of the implementation.  The behavior is actually
>> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
>> which end-user-level behavior you're referring to.

> I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
> applies it to the original instead of the file I look at.

Details please?


        Stefan

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> This is just one part of the implementation.  The behavior is actually
>>> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
>>> which end-user-level behavior you're referring to.
>
>> I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
>> applies it to the original instead of the file I look at.
>
> Details please?

What do you need?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

>>>> This is just one part of the implementation.  The behavior is actually
>>>> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
>>>> which end-user-level behavior you're referring to.
>> 
>>> I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
>>> applies it to the original instead of the file I look at.
>> 
>> Details please?

> What do you need?

Everything starting from "emacs -Q", including the patch file.
Ever heard of a "recipe"?  Hello?


        Stefan

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>>> This is just one part of the implementation.  The behavior is actually
>>>>> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
>>>>> which end-user-level behavior you're referring to.
>>> 
>>>> I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
>>>> applies it to the original instead of the file I look at.
>>> 
>>> Details please?
>
>> What do you need?
>
> Everything starting from "emacs -Q", including the patch file.

$ wget ftp://lwfinger.dynalias.org/patches/combined_2.6.20.2.patch
$ wget ftp://ftp.kernel.org/pub/linux/kernel/v2.6/testing/linux-2.6.21-rc3.tar.bz2
$ tar -xf linux-2.6.21-rc3.tar.bz2
$ cp -a linux-2.6.21-rc3 linux-2.6.21-rc3.orig
$ emacs combined_2.6.20.2.patch
C-c C-f
M-n M-4 M-f DEL 1-rc3 RET M-n M-n M-n
C-c C-a M-p

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

>>>>> This is just one part of the implementation.  The behavior is actually
>>>>> pretty subtle (too subtle, admittedly).  So I can't judge without knowing
>>>>> which end-user-level behavior you're referring to.
>>>>> I look at a hunk, it says "Hunk not yet applied", I want to apply it, it
>>>>> applies it to the original instead of the file I look at.
>>>> Details please?
>>> What do you need?
>> Everything starting from "emacs -Q", including the patch file.

> $ wget ftp://lwfinger.dynalias.org/patches/combined_2.6.20.2.patch
> $ wget ftp://ftp.kernel.org/pub/linux/kernel/v2.6/testing/linux-2.6.21-rc3.tar.bz2
> $ tar -xf linux-2.6.21-rc3.tar.bz2
> $ cp -a linux-2.6.21-rc3 linux-2.6.21-rc3.orig
> $ emacs combined_2.6.20.2.patch
> C-c C-f
> M-n M-4 M-f DEL 1-rc3 RET M-n M-n M-n
> C-c C-a M-p

Duh!  Now I see why I wasn't seeing your behavior.  Sorry for being a jerk.

Yes, indeed, there's a discrepency betwen the code and the doc.

The doc says "apply to new file, unless diff-jump-to-old-file says
otherwise".

The code does "apply forward to old file, or if prefix arg is given apply
backward to new file".

The code is as is it because it based on the assumption that either there
are 2 files and you're looking at the diff between the two (in which case
applying forward on the new file doesn't make sense and vice-versa), or
there's only one file so it doesn't matter.

Hmmm... I like the way the code works for my use, but it seems that it
doesn't work well in your case.

In your case, the patch specifies two files which diff-mode can both find,
yet, the patch is not between those two files (at least in your test case,
the two files are just the same, presumably one being a "working tree" and
the other being basically unrelated).
Is your test case very representative, or are there many other different
cases where you bump into the same problem?  If so, what does the "problem
in its most general description" look like?

Also, the fact that M-RET jumps to the exact opposite file from the one that
C-c C-a would apply to (i.e. with C-u it jumps to the new file and with it,
to the old), is ..hmm.. "unfortunate".


        Stefan

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> In your case, the patch specifies two files which diff-mode can both find,
> yet, the patch is not between those two files (at least in your test case,
> the two files are just the same, presumably one being a "working tree" and
> the other being basically unrelated).

The patch is relative to an older version of the file, my intention was to
find out which parts of it have already been applied, and apply those that
aren't (and some hunks need editing to apply, so I couldn't just ignore
the rejects).

> Is your test case very representative, or are there many other different
> cases where you bump into the same problem?

It's actually the first time I used that feature.  For me this behaviour
was very confusing, even before I read the doc string of diff-apply-hunk.
Especially more so because it also automatically moves forward so that the
effect is not immediately visible.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

>> In your case, the patch specifies two files which diff-mode can both find,
>> yet, the patch is not between those two files (at least in your test case,
>> the two files are just the same, presumably one being a "working tree" and
>> the other being basically unrelated).

> The patch is relative to an older version of the file, my intention was to
> find out which parts of it have already been applied, and apply those that
> aren't (and some hunks need editing to apply, so I couldn't just ignore
> the rejects).

So, there was fundamentally only 1 file, right?
It just so happened that diff-mode found 2 different matching files (one for
the "old" and one for the "new"), but it was unintended?

>> Is your test case very representative, or are there many other different
>> cases where you bump into the same problem?

> It's actually the first time I used that feature.  For me this behaviour
> was very confusing, even before I read the doc string of diff-apply-hunk.
> Especially more so because it also automatically moves forward so that the
> effect is not immediately visible.

I'm not sure what you mean by "this behaviour".  I guess the confusing
behavior is mostly the inconsistency between diff-goto-source and
diff-apply-hook, is that right?


        Stefan

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So, there was fundamentally only 1 file, right?
> It just so happened that diff-mode found 2 different matching files (one for
> the "old" and one for the "new"), but it was unintended?

Yes, that's one way to look at it.  In the real situation there were some
differences between the orig tree and to working tree, but those were
unrelated to the patch.

> I'm not sure what you mean by "this behaviour".  I guess the confusing
> behavior is mostly the inconsistency between diff-goto-source and
> diff-apply-hook, is that right?

Right, and it became worse after I read the doc string.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

>> So, there was fundamentally only 1 file, right?  It just so happened that
>> diff-mode found 2 different matching files (one for the "old" and one for
>> the "new"), but it was unintended?

> Yes, that's one way to look at it.  In the real situation there were some
> differences between the orig tree and to working tree, but those were
> unrelated to the patch.

I'm not concerned about the content of those two files, but rather I'm
trying to understand why they were there for diff-mode to find.

E.g. was it just that the patch happened to use the same name for the old
tree as a tree you happened to have lying around?  Or was the name identical
precisely because that was the old tree you passed to `diff' to generate
the patch?

>> I'm not sure what you mean by "this behaviour".  I guess the confusing
>> behavior is mostly the inconsistency between diff-goto-source and
>> diff-apply-hook, is that right?

> Right, and it became worse after I read the doc string.

The doc is easy to fix ;-)
More seriously, I think I'll just revert my change and make the code follow
the doc for now.  The current behavior is indeed too confusing and fixing it
right will require more changes than I'd like for Emacs-22.

Thanks for bringing it up.


        Stefan


--- diff-mode.el	13 Mar 2007 10:29:22 -0400	1.98
+++ diff-mode.el	14 Mar 2007 11:13:12 -0400	
@@ -72,7 +72,7 @@
   :group 'diff-mode)
 
 (defcustom diff-jump-to-old-file nil
-  "*Non-nil means `diff-goto-source' jumps to the old file.
+  "Non-nil means `diff-goto-source' jumps to the old file.
 Else, it jumps to the new file."
   :type 'boolean
   :group 'diff-mode)
@@ -1280,7 +1280,7 @@
 	(if (> (- (car forw) orig) (- orig (car back))) back forw)
       (or back forw))))
 
-(defsubst diff-xor (a b) (if a (not b) b))
+(defsubst diff-xor (a b) (if a (if (not b) a) b))
 
 (defun diff-find-source-location (&optional other-file reverse)
   "Find out (BUF LINE-OFFSET POS SRC DST SWITCHED).
@@ -1362,8 +1362,15 @@
 With a prefix argument, REVERSE the hunk."
   (interactive "P")
   (destructuring-bind (buf line-offset pos old new &optional switched)
-      ;; If REVERSE go to the new file, otherwise go to the old.
-      (diff-find-source-location (not reverse) reverse)
+      ;; Sometimes we'd like to have the following behavior: if REVERSE go
+      ;; to the new file, otherwise go to the old.  But that means that by
+      ;; default we use the old file, which is the opposite of the default
+      ;; for diff-goto-source, and is thus confusing.  Also when you don't
+      ;; know about it it's pretty surprising.
+      ;; TODO: make it possible to ask explicitly for this behavior.
+      ;; 
+      ;; This is duplicated in diff-test-hunk.
+      (diff-find-source-location nil reverse)
     (cond
      ((null line-offset)
       (error "Can't find the text to patch"))
@@ -1407,8 +1414,7 @@
 With a prefix argument, try to REVERSE the hunk."
   (interactive "P")
   (destructuring-bind (buf line-offset pos src dst &optional switched)
-      ;; If REVERSE go to the new file, otherwise go to the old.
-      (diff-find-source-location (not reverse) reverse)
+      (diff-find-source-location nil reverse)
     (set-window-point (display-buffer buf) (+ (car pos) (cdr src)))
     (diff-hunk-status-msg line-offset (diff-xor reverse switched) t)))

Re: diff-apply-hunk documentation doesn't match implementation

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> E.g. was it just that the patch happened to use the same name for the old
> tree as a tree you happened to have lying around?  Or was the name identical
> precisely because that was the old tree you passed to `diff' to generate
> the patch?

I did not generate the patch.  If you look closer you'll see that the
patch was generated against a different tree, and the naming convention
just happend to match the convention I use for my work.  Perhaps
diff-find-source-location is too smart of its own good? :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: diff-apply-hunk documentation doesn't match implementation

[Stefan Monnier]

> I did not generate the patch.  If you look closer you'll see that the
> patch was generated against a different tree, and the naming convention
> just happend to match the convention I use for my work.  Perhaps
> diff-find-source-location is too smart of its own good? :-)

If you move the patch into the tree you want to work on, then diff-mode
won't get confused.


        Stefan