diff-apply-hunk broken

diff-apply-hunk broken

[Sam Steingold]

GNU Emacs 21.3.50.1 (i386-msvc-nt5.0.2195)
 of 2004-03-19 on WINSTEINGOLDLAP
--with-msvc (12.00)

Stefan, it appears that your recent patch broke `diff-apply-hunk':

in a modified CVS file, I do C-x v = (vc-diff) and in the resulting
*vc-diff* buffer I do C-c C-a (diff-apply-hunk) to reverse the section
of the diff:

Debugger entered--Lisp error: (buffer-read-only #<buffer io.d.~1.204.~>)
  delete-region(223236 223574)
  (save-current-buffer (set-buffer buf) (goto-char (car pos)) (delete-region (car pos) (cdr pos)) (insert (car new)))
  (with-current-buffer buf (goto-char (car pos)) (delete-region (car pos) (cdr pos)) (insert (car new)))
  (cond ((null line-offset) (error "Can't find the text to patch")) ((and switched ...) (message "(Nothing done)")) (t (with-current-buffer buf ... ... ...) (set-window-point ... ...) (diff-hunk-status-msg line-offset ... nil) (when diff-advance-after-apply-hunk ...)))
  (let* ((--rest--45507 ...) (buf ...) (line-offset ...) (pos ...) (old ...) (new ...) (switched ...)) (if --rest--45507 (signal ... ...)) (cond (... ...) (... ...) (t ... ... ... ...)))
  (progn (let* (... ... ... ... ... ... ...) (if --rest--45507 ...) (cond ... ... ...)))
  (destructuring-bind (buf line-offset pos old new &optional switched) (diff-find-source-location (not reverse) reverse) (cond (... ...) (... ...) (t ... ... ... ...)))
  diff-apply-hunk(nil)
* call-interactively(diff-apply-hunk)


-- 
Sam Steingold (http://www.podval.org/~sds) running w2k
<http://www.camera.org> <http://www.iris.org.il> <http://www.memri.org/>
<http://www.mideasttruth.com/> <http://www.honestreporting.com>
Incorrect time syncronization.

Re: diff-apply-hunk broken

[Stefan Monnier]

> Stefan, it appears that your recent patch broke `diff-apply-hunk':

Hmm...

> in a modified CVS file, I do C-x v = (vc-diff) and in the resulting
> *vc-diff* buffer I do C-c C-a (diff-apply-hunk) to reverse the section
> of the diff:

To undo a hunk you should use C-u C-c C-a rather than C-c C-a.
It is true that when C-c C-a finds that the requested direction cannot be
done on the buffer but the reverse can, it tries to reverse the direction
for you, but it's always better to say explicitly what you want.

> Debugger entered--Lisp error: (buffer-read-only #<buffer io.d.~1.204.~>)

The reason why things have changed is that it used to be that C-c C-a
looked for the "dest file" no matter what, whereas now the file to which
the hunk is applied depends on the direction you've chosen (via C-u C-c
C-a).  This way if you `diff a b' it used to be that both C-c C-a and C-u
C-c C-a tried to apply (or reverse) the current hunk on file `b' whereas
now C-c C-a will try to apply the hunk in file `a' and C-u C-c C-a will
try to undo the hunk in file `b'.

In your case, clearly the new behavior is undesirable, so I think we need
to fix VC so that the diff doesn't mention io.d.~1.204.~.


        Stefan

Re: diff-apply-hunk broken

[Stefan Monnier]

> The reason why things have changed is that it used to be that C-c C-a
> looked for the "dest file" no matter what, whereas now the file to which
> the hunk is applied depends on the direction you've chosen (via C-u C-c
> C-a).  This way if you `diff a b' it used to be that both C-c C-a and C-u
> C-c C-a tried to apply (or reverse) the current hunk on file `b' whereas
> now C-c C-a will try to apply the hunk in file `a' and C-u C-c C-a will
> try to undo the hunk in file `b'.

> In your case, clearly the new behavior is undesirable, so I think we need
> to fix VC so that the diff doesn't mention io.d.~1.204.~.

I've just installed such a change in vc.el.
Please try it and tell me how you like it,


        Stefan

Re: diff-apply-hunk broken

[Andre Spiegel]

On Mon, 2004-03-29 at 00:46, Stefan Monnier wrote:

> > In your case, clearly the new behavior is undesirable, so I think we need
> > to fix VC so that the diff doesn't mention io.d.~1.204.~.
> 
> I've just installed such a change in vc.el.
> Please try it and tell me how you like it,

Looks good to me.  Two questions, though: a local diff and a remote diff
now look exactly the same in the buffer.  Perhaps it would be good to
add something like "(local)" after the version number, in a way that
wouldn't confuse diff-mode (or patch, for that matter)?  Just a thought.

Second, I couldn't find documentation for the "-L" option in the GNU
diff manual, although it is quite happy with that option (--label, which
seems equivalent, is documented).  Do you have any idea about the
general availability of -L in other diff implementations?

Re: diff-apply-hunk broken

[Stefan Monnier]

> Looks good to me.  Two questions, though: a local diff and a remote diff
> now look exactly the same in the buffer.

Actually, no: the remote diff has some extra lines before the diff itself.
At least with CVS it does.  But: why do you care?

> Perhaps it would be good to add something like "(local)" after the version
> number, in a way that wouldn't confuse diff-mode (or patch, for that
> matter)?  Just a thought.

I don't see the need, but anything after the first TAB can be changed at
will (i.e. you can remove the date or replace it with something else, for
example).

> Second, I couldn't find documentation for the "-L" option in the GNU
> diff manual, although it is quite happy with that option (--label, which
> seems equivalent, is documented).

The manpage definitely mentions -L as does `diff --help'.

> Do you have any idea about the general availability of -L in other
> diff implementations?

It's also there in Mac OS X (which is using a version derived from BSD,
which is itself derived from an older GNU code it seems) which is the only
non-GNU system to which I have access.

Actually, the -L args are used in smerge-mode since Emacs-21.1 and I haven't
heard any complaint about it.  Admittedly, I'd expect much fewer people use
smerge-mode's diff than VC's diff, so maybe it's not so simple.


        Stefan

Re: diff-apply-hunk broken

[Andre Spiegel]

On Mon, 2004-03-29 at 16:19, Stefan Monnier wrote:

> Actually, no: the remote diff has some extra lines before the diff itself.
> At least with CVS it does.  But: why do you care?

I find it valuable feedback when I can see at a glance whether a diff
was computed locally or remotely.  A simple example where it might help
is this: A user makes a mistake setting up his vc-cvs-stay-local
variable.  As a result, he gets remote diffs when he actually wants them
to be local.  If he can't tell from the output what happened, it might
be difficult for him to realize the problem.

But if the disply is still different anyway, that's enough of a visual
clue I think.

> > Second, I couldn't find documentation for the "-L" option in the GNU
> > diff manual, although it is quite happy with that option (--label, which
> > seems equivalent, is documented).
> 
> The manpage definitely mentions -L as does `diff --help'.

What version are you referring to?  In GNU diffutils 2.8.1-6 (Debian
unstable), diff -L is neither mentioned in the manpage, the info manual,
nor diff --help.  The -L option is mentioned, however, for diff3 (in all
three places).  As far as I can tell, 2.8.1 is the most recent version. 
Please confirm whether you have a version where diff -L is documented. 
Otherwise, I'll file a bug report to the diffutil guys.

Re: diff-apply-hunk broken

[Stefan Monnier]

> I find it valuable feedback when I can see at a glance whether a diff
> was computed locally or remotely.  A simple example where it might help
> is this: A user makes a mistake setting up his vc-cvs-stay-local
> variable.  As a result, he gets remote diffs when he actually wants them
> to be local.  If he can't tell from the output what happened, it might
> be difficult for him to realize the problem.

If he can't tell from the time it took, then does the difference matter ?

> But if the disply is still different anyway, that's enough of a visual
> clue I think.

I'm not wedded to the current format, so feel free to change it.
I just chose an existing format to minimize people's surprise.

>> > Second, I couldn't find documentation for the "-L" option in the GNU
>> > diff manual, although it is quite happy with that option (--label, which
>> > seems equivalent, is documented).
>> The manpage definitely mentions -L as does `diff --help'.

> What version are you referring to?  In GNU diffutils 2.8.1-6 (Debian
> unstable), diff -L is neither mentioned in the manpage, the info manual,
> nor diff --help.  The -L option is mentioned, however, for diff3 (in all
> three places).  As far as I can tell, 2.8.1 is the most recent version. 
> Please confirm whether you have a version where diff -L is documented. 
> Otherwise, I'll file a bug report to the diffutil guys.

On a RedHat 9 system:

   ~-0% uname -a
   Linux asado.iro.umontreal.ca 2.4.20-30.9 #1 Wed Feb 4 20:44:26 EST 2004 i686 athlon i386 GNU/Linux
   ~-0% diff --version
   diff (GNU diffutils) 2.8.1
   Copyright (C) 2002 Free Software Foundation, Inc.
   
   Ce programme est fourni sans AUCUNE GARANTIE, tel que permis par la loi.
   Vous pouvez le redistribuer selon les termes de « GNU General Public License »,
   lire le texte du fichier COPYING pour plus de détails.
   
   Écrit par Paul Eggert, Mike Haertel, David Hayes,
   Richard Stallman et Len Tower.
   ~-0% man diff | grep .-L 
          -L étiquette
   ~-0% LANG=C man diff | grep .-L
          -L label
   ~-0% 


On a Mac:

   ~-0% uname -a
   Darwin empanada.local 7.3.0 Darwin Kernel Version 7.3.0: Fri Mar  5 14:22:55 PST 2004; root:xnu/xnu-517.3.15.obj~4/RELEASE_PPC  Power Macintosh powerpc
   ~-0% diff --version       
   diff - GNU diffutils version 2.7
   ~-0% man diff | grep ".-L"
          -L label
   ~-0% 


-- Stefan

Re: diff-apply-hunk broken

[Miles Bader]

On Tue, Mar 30, 2004 at 06:28:22PM -0500, Stefan Monnier wrote:
> >> The manpage definitely mentions -L as does `diff --help'.
> 
> > What version are you referring to?  In GNU diffutils 2.8.1-6 (Debian
> > unstable), diff -L is neither mentioned in the manpage, the info manual,
> > nor diff --help.
>
>    ~-0% diff --version
>    diff (GNU diffutils) 2.8.1

FWIW, fencepost has diff 2.7, which supports -L, and --help mentions it.

[This is a great option, BTW -- I used to do the same thing with an
awk-script that parsed the diff-output, and munged the names, but man what a
pain, -L is soooo much better.]

-Miles
-- 
We have met the enemy, and he is us.  -- Pogo

Re: diff-apply-hunk broken

[Eli Zaretskii]

> Date: Wed, 31 Mar 2004 18:08:18 -0500
> From: Miles Bader <miles@gnu.org>
> 
> On Tue, Mar 30, 2004 at 06:28:22PM -0500, Stefan Monnier wrote:
> > >> The manpage definitely mentions -L as does `diff --help'.
> > 
> > > What version are you referring to?  In GNU diffutils 2.8.1-6 (Debian
> > > unstable), diff -L is neither mentioned in the manpage, the info manual,
> > > nor diff --help.
> >
> >    ~-0% diff --version
> >    diff (GNU diffutils) 2.8.1
> 
> FWIW, fencepost has diff 2.7, which supports -L, and --help mentions it.

Actually, -L is supported in Diffutils 2.8.x as well, it's just
undocumented.

The file NEWS in the Diffutils distro tells the story:

    * The following diff options are still accepted, but are no longer
      documented. They may be withdrawn in future releases.
      [...]
      -L (use --label instead)

"diff --help" and the node "diff Options" in the Info manual have -L
removed in 2.8.x, but -L is still supported (and mentioned elsewhere
in the Info manual, perhaps inadvertently).

So it sounds like we shouldn't use -L in Emacs, lest it stops working
with future releases of Diffutils.  We should use --label instead.

Re: diff-apply-hunk broken

[Miles Bader]

On Thu, Apr 01, 2004 at 08:12:16AM +0200, Eli Zaretskii wrote:
> So it sounds like we shouldn't use -L in Emacs, lest it stops working
> with future releases of Diffutils.  We should use --label instead.

Aaaah, _that's_ what's going on .... I was a bit confused that they would
delete such a useful option, but if it's merely a move to using the
long-option instead, no problem.

-Miles
-- 
Occam's razor split hairs so well, I bought the whole argument!

Re: diff-apply-hunk broken

[Paul Eggert]

At 01 Apr 2004 08:12:16 +0200, Eli Zaretskii <eliz@elta.co.il> writes:

>     * The following diff options are still accepted, but are no longer
>       documented. They may be withdrawn in future releases.
>       [...]
>       -L (use --label instead)

Yes; the basic motivation here is a plan to support the -H, -L, -P,
and -R options that are used by many programs that traverse directory
structures (e.g., "cp").  It won't be in the next official diffutils
version, but perhaps the one after that.

Re: diff-apply-hunk broken

[Eli Zaretskii]

> From: Paul Eggert <eggert@twinsun.com>
> Date: Thu, 01 Apr 2004 15:41:09 -0800
> 
> Yes; the basic motivation here is a plan to support the -H, -L, -P,
> and -R options that are used by many programs that traverse directory
> structures (e.g., "cp").  It won't be in the next official diffutils
> version, but perhaps the one after that.

Then I think the manual should be fixed to not mention -L at all;
currently, it does.

Re: diff-apply-hunk broken

[Paul Eggert]

At Fri, 02 Apr 2004 09:32:34 +0200, "Eli Zaretskii" <eliz@gnu.org> writes:

> Then I think the manual should be fixed to not mention [diff] -L at all;

Thanks for the suggestion; I'll do that in the next version.

Re: diff-apply-hunk broken

[Stefan Monnier]

>> Then I think the manual should be fixed to not mention [diff] -L at all;
> Thanks for the suggestion; I'll do that in the next version.

How about the problem if packages that use diff and want to work with old
and new version just as well?  Is --label accepted by all known non-ancient
diff implementations?


        Stefan

Re: diff-apply-hunk broken

[Paul Eggert]

At 02 Apr 2004 19:45:08 -0500, Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Is --label accepted by all known non-ancient diff implementations?

Yes.  --label has been supported ever since diffutils 2.0, released in
1992.  These days it's safe to assume --label, if it's GNU diff.

Re: diff-apply-hunk broken

[Stefan Monnier]

>> Is --label accepted by all known non-ancient diff implementations?
> Yes.  --label has been supported ever since diffutils 2.0, released in
> 1992.  These days it's safe to assume --label, if it's GNU diff.

I can't assume GNU diff.  So how about other system's `diff'?


        Stefan

Re: diff-apply-hunk broken

[Paul Eggert]

At 02 Apr 2004 20:48:04 -0500, Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Is --label accepted by all known non-ancient diff implementations?
>> Yes.  --label has been supported ever since diffutils 2.0, released in
>> 1992.  These days it's safe to assume --label, if it's GNU diff.
>
> I can't assume GNU diff.  So how about other system's `diff'?

Some "diff" implementations lack -L, so portable applications cannot
use "diff -L".  For example, Solaris 9 "diff" lacks -L.  -L is not
standardized by POSIX.

I recall that this is not a problem with GNU Emacs, as it tests
whether diff supports -L (or --label) before using that option.

Re: diff-apply-hunk broken

[Juri Linkov]

Paul Eggert <eggert@twinsun.com> writes:
> At 02 Apr 2004 20:48:04 -0500, Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> Is --label accepted by all known non-ancient diff implementations?
>>> Yes.  --label has been supported ever since diffutils 2.0, released in
>>> 1992.  These days it's safe to assume --label, if it's GNU diff.
>>
>> I can't assume GNU diff.  So how about other system's `diff'?
>
> Some "diff" implementations lack -L, so portable applications cannot
> use "diff -L".  For example, Solaris 9 "diff" lacks -L.  -L is not
> standardized by POSIX.
>
> I recall that this is not a problem with GNU Emacs, as it tests
> whether diff supports -L (or --label) before using that option.

It seems that GNU Emacs have no such a test, but fortunately there
are only three files (diff.el, smerge-mode.el and vc.el) that use
a "-L" string, so it's not a problem to replace "-L" by "--label"
in these files.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: diff-apply-hunk broken

[Juri Linkov]

Sam Steingold <sds@gnu.org> writes:
> in a modified CVS file, I do C-x v = (vc-diff) and in the resulting
> *vc-diff* buffer I do C-c C-a (diff-apply-hunk) to reverse the section

BTW, what do you think about the following problem?

The problem is that the function `log-view-diff' can correctly work
when her arguments `beg' and `end' are located on the same point.
In this case it makes diffs with the next revision.  But calling
the function is impossible when region is not active.  Due to the "r"
interactive code letter it reports an error about mark-inactive.
This is too inconvenient.

This patch makes it possible to call this function even if region
is not active.

Maybe, a better solution could be to add a new code letter e.g. "R" to
the `interactive' which will work like "r", but will be less restrictive
and will allow a function to be called when region is inactive and
will set both its arguments `beg' and `end' to the current point.
This might be useful for some other functions too, where it makes sense
to call a function either on the selected region or the current point.

Index: emacs/lisp/log-view.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/log-view.el,v
retrieving revision 1.17
diff -c -r1.17 log-view.el
*** emacs/lisp/log-view.el	1 Sep 2003 15:45:13 -0000	1.17
--- emacs/lisp/log-view.el	25 Mar 2004 21:13:11 -0000
***************
*** 192,198 ****
  If the point is the same as the mark, get the diff for this revision.
  Otherwise, get the diff between the revisions
   were the region starts and ends."
!   (interactive "r")
    (let ((fr (log-view-current-tag beg))
          (to (log-view-current-tag end)))
      (when (string-equal fr to)
--- 192,200 ----
  If the point is the same as the mark, get the diff for this revision.
  Otherwise, get the diff between the revisions
   were the region starts and ends."
!   (interactive
!    (list (if mark-active (region-beginning) (point))
!          (if mark-active (region-end) (point))))
    (let ((fr (log-view-current-tag beg))
          (to (log-view-current-tag end)))
      (when (string-equal fr to)

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: diff-apply-hunk broken

[Sam Steingold]

> * Juri Linkov <whev@whegn.bet> [2004-03-25 23:53:17 +0200]:
>
> Maybe, a better solution could be to add a new code letter e.g. "R" to
> the `interactive' which will work like "r", but will be less
> restrictive and will allow a function to be called when region is
> inactive and will set both its arguments `beg' and `end' to the
> current point.

I think this is a great idea.

-- 
Sam Steingold (http://www.podval.org/~sds) running w2k
<http://www.camera.org> <http://www.iris.org.il> <http://www.memri.org/>
<http://www.mideasttruth.com/> <http://www.honestreporting.com>
"A pint of sweat will save a gallon of blood."          -- George S. Patton

Re: diff-apply-hunk broken

[Richard Stallman]

    > Maybe, a better solution could be to add a new code letter e.g. "R" to
    > the `interactive' which will work like "r", but will be less
    > restrictive and will allow a function to be called when region is
    > inactive and will set both its arguments `beg' and `end' to the
    > current point.

Do we want this feature often enough to justify a new interactive code?
If it is wanted only rarely, implementing it in Lisp is probably better.

Another reason that might make a new interactive code desirable is if
we want to specifically encourage programmers to use this interface
for their new commands.  I don't see that we want to.

Re: diff-apply-hunk broken

[Kim F. Storm]

Juri Linkov <juri@jurta.org> writes:

> Maybe, a better solution could be to add a new code letter e.g. "R" to
> the `interactive' which will work like "r", but will be less restrictive
> and will allow a function to be called when region is inactive and
> will set both its arguments `beg' and `end' to the current point.
> This might be useful for some other functions too, where it makes sense
> to call a function either on the selected region or the current point.

That seems to make good sense.

--
Kim F. Storm <storm@cua.dk> http://www.cua.dk