bug#18824: split diff-check-labels off diff-no-select

bug#18824: split diff-check-labels off diff-no-select

[Ivan Shmakov]

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

Package:  emacs
Severity: wishlist
Tags:     patch

	Please provide a separate diff-check-labels function to allow
	for the code calling diff.el facilities to supply its own
	--label= arguments to diff in place of diff.el-generated ones.

	Possible patch, as well as the example usage, are MIMEd.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

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

--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -121,6 +121,15 @@ Possible values are:
   nil   -- no, it does not
   check -- try to probe whether it does")
 
+(defun diff-check-labels (&optional force)
+  (if (not (or force (eq 'check diff-use-labels)))
+      diff-use-labels
+    (setq diff-use-labels
+	  (with-temp-buffer
+	    (when (ignore-errors
+		    (call-process diff-command nil t nil "--help"))
+	      (if (search-backward "--label" nil t) t))))))
+
 (defun diff-no-select (old new &optional switches no-async buf)
   ;; Noninteractive helper for creating and reverting diff buffers
   (unless (bufferp new) (setq new (expand-file-name new)))
@@ -128,11 +137,7 @@ diff-no-select (old new &optional switches no-async buf)
   (or switches (setq switches diff-switches)) ; If not specified, use default.
   (unless (listp switches) (setq switches (list switches)))
   (or buf (setq buf (get-buffer-create "*Diff*")))
-  (when (eq 'check diff-use-labels)
-    (setq diff-use-labels
-	  (with-temp-buffer
-	    (when (ignore-errors (call-process diff-command nil t nil "--help"))
-	      (if (search-backward "--label" nil t) t)))))
+  (diff-check-labels)
   (let* ((old-alt (diff-file-local-copy old))
 	 (new-alt (diff-file-local-copy new))
 	 (command

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/emacs-lisp, Size: 682 bytes --]

(let ((old "old") (new "new")
      (target (get-buffer-create "*My Diff*"))
      (labels (and (diff-check-labels)
                   (mapcar 'shell-quote-argument
                           (list "--label"
                                 "*old revision*"
                                 "--label"
                                 "*new revision*")))))
  (if labels
      (let ((diff-use-labels nil)
            (sw (nconc labels
                       (if (listp diff-switches)
                           diff-switches
                         (list diff-switches)))))
        (diff-no-select old new switches nil target))
    (diff-no-select     old new nil      nil target)))

bug#18824: split diff-check-labels off diff-no-select

[Stefan Monnier]

> 	Please provide a separate diff-check-labels function to allow
> 	for the code calling diff.el facilities to supply its own
> 	--label= arguments to diff in place of diff.el-generated ones.

Sounds fine.  While you're at it, you might like to use this for
smerge-mode (which uses "-L" rather than "--label").


        Stefan

bug#18850: smerge-mode: use diff-check-labels

[Ivan Shmakov]

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

Package:  emacs
Severity: wishlist
Tags:     patch
Control:  block -1 by 18824

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

 >> Please provide a separate diff-check-labels function to allow for
 >> the code calling diff.el facilities to supply its own --label=
 >> arguments to diff in place of diff.el-generated ones.

 > Sounds fine.  While you're at it, you might like to use this for
 > smerge-mode (which uses "-L" rather than "--label").

	I assume you mean something along the lines of the (untested)
	patch MIMEd.

	Curiously, I don’t usually use the merge facility proper,
	resolving merge conflicts afterwards.  Instead, I save the diff
	against the current revision, switch to a newer one, apply the
	diff saved, and deal with the ‘.rej’ files, if any.  Like:

$ (umask 0222 && git diff > +patch-$(date +%s).diff) 
$ git reset origin/master \
      && patch -bVt -p1 -R < <(git diff) 
…
$ patch -bVt -p1 < +patch-1414405170.diff 
…

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

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

--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -44,6 +44,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(require 'diff)				; for diff-check-labels
 (require 'diff-mode)                    ;For diff-auto-refine-mode.
 (require 'newcomment)
 
@@ -1162,10 +1163,13 @@ smerge-diff (n1 n2)
 	    (erase-buffer)
 	    (let ((status
 		   (apply 'call-process diff-command nil t nil
-			  (append smerge-diff-switches
-				  (list "-L" (concat name1 "/" file)
-					"-L" (concat name2 "/" file)
-					file1 file2)))))
+			  (nconc (and (diff-check-labels)
+				      (list "--label"
+					    (concat name1 "/" file)
+					    "--label"
+					    (concat name2 "/" file)))
+				 smerge-diff-switches)
+			  file1 file2)))))
 	      (if (eq status 0) (insert "No differences found.\n"))))
 	  (goto-char (point-min))
 	  (diff-mode)

bug#18850: smerge-mode: use diff-check-labels

[Ivan Shmakov]

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

block 18850 by 18824
thanks

	Another try.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

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

--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -44,6 +44,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(require 'diff)				; for diff-check-labels
 (require 'diff-mode)                    ;For diff-auto-refine-mode.
 (require 'newcomment)
 
@@ -1163,9 +1164,12 @@ repeating the command will highlight other two parts."
 	    (let ((status
 		   (apply 'call-process diff-command nil t nil
 			  (append smerge-diff-switches
-				  (list "-L" (concat name1 "/" file)
-					"-L" (concat name2 "/" file)
-					file1 file2)))))
+				  (and (diff-check-labels)
+				       (list "--label"
+					     (concat name1 "/" file)
+					     "--label"
+					     (concat name2 "/" file)))
+				  (list file1 file2)))))
 	      (if (eq status 0) (insert "No differences found.\n"))))
 	  (goto-char (point-min))
 	  (diff-mode)

bug#18850: smerge-mode: use diff-check-labels

[Stefan Monnier]

> @@ -1163,9 +1164,12 @@ repeating the command will highlight other two parts."
>  	    (let ((status
>  		   (apply 'call-process diff-command nil t nil
>  			  (append smerge-diff-switches
> -				  (list "-L" (concat name1 "/" file)
> -					"-L" (concat name2 "/" file)
> -					file1 file2)))))
> +				  (and (diff-check-labels)
> +				       (list "--label"
> +					     (concat name1 "/" file)
> +					     "--label"
> +					     (concat name2 "/" file)))
> +				  (list file1 file2)))))
>  	      (if (eq status 0) (insert "No differences found.\n"))))
>  	  (goto-char (point-min))
>  	  (diff-mode)

That looks OK, yes.  Tho you don't need to change -L to --label.


        Stefan

bug#18850: smerge-mode: use diff-check-labels

[Ivan Shmakov]

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

[…]

 > That looks OK, yes.  Tho you don't need to change -L to --label.

	The point is that, strictly speaking, (diff-check-labels) only
	checks for the --label option proper, not for its aliases.

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#18850: smerge-mode: use diff-check-labels

[Stefan Monnier]

>> That looks OK, yes.  Tho you don't need to change -L to --label.
> 	The point is that, strictly speaking, (diff-check-labels) only
> 	checks for the --label option proper, not for its aliases.

Of course, part of the question, then is: are there diff commands that
handle one of the two but not the other.  If there are, then we should
use the one that's more often supported, and if there aren't then it
doesn't matter.
So far, smerge-mode hasn't even bothered to check, so maybe the check
isn't that important any more (IIRC it was added for Solaris's diff,
tho I'd suspect that current derivatives of Solaris woujld accept
either -L or --label or both).


        Stefan

bug#18850: smerge-mode: use diff-check-labels

[Ivan Shmakov]

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

 >>> That looks OK, yes.  Tho you don't need to change -L to --label.

 >> The point is that, strictly speaking, (diff-check-labels) only
 >> checks for the --label option proper, not for its aliases.

 > Of course, part of the question, then is: are there diff commands
 > that handle one of the two but not the other.  If there are, then we
 > should use the one that's more often supported, and if there aren't
 > then it doesn't matter.

	I have no reason to insist on doing it either way.  However, it
	still feels inconsistent to check for a specific option, and
	then use its alias.

	Then, however, there’s the following in diffutils/NEWS:

 > User-visible changes in version 2.8:

[…]

 > * The following diff options are still accepted, but are no longer documented.
 >   They may be withdrawn in future releases.
 >   -h (omit; it has no effect)
 >   -H (use --speed-large-files instead)
 >   -L (use --label instead)

	(Version 2.8 was apparently released March, 2002.)

 > So far, smerge-mode hasn't even bothered to check, so maybe the check
 > isn't that important any more (IIRC it was added for Solaris's diff,
 > tho I'd suspect that current derivatives of Solaris woujld accept
 > either -L or --label or both).

	Again, I have no reason to argue for retaining the check,
	especially given that none of my systems seem to have diff(1)
	versions other than the one from Diffutils, as of 3.0, or a
	later version.

	Also to note:

 > User-visible changes in version 2.0:

[…]

 > * diff options renamed:
 > --label renamed from --file-label

	Now, given that 2.7 – the oldest version available from
	http://ftp.gnu.org/gnu/diffutils/ – dates back to 1994…

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#18850: smerge-mode: use diff-check-labels

[Stefan Monnier]

>> -L (use --label instead)

OK, then let's switch to --label everywhere, thanks,


        Stefan

bug#18850: (ping) Emacs bugs with patches?

[Ivan Shmakov]

	Stefan, you’ve previously commented on several of the bug
	reports I’ve suggested patches for.  Could you please revisit
	these patches and either commit or explicitly reject them?
	(Any suggestion on how, if at all, do I improve them in the
	latter case will be appreciated.)

	The bug reports in question are as follows.

	TIA.

    http://debbugs.gnu.org/18175  Use (mapc 'switch-to-buffer …) in
	files.el (was: mapcar.)
    http://debbugs.gnu.org/18246  Use inhibit-point-motion-hooks
	(in addition to inhibit-read-only) in enriched-encode.
    http://debbugs.gnu.org/18824  New diff-check-labels function
	(split off diff-no-select)
    http://debbugs.gnu.org/18850  Use diff-check-labels and the --label
	option to diff (was: -L – deprecated) in smerge-mode.el

-- 
FSF associate member #7257  np. По полям, за рекой — Иллет   … B6A0 230E 334A

bug#18850: smerge-mode: use diff-check-labels

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> block 18850 by 18824
> thanks
>
> 	Another try.

The patch looks OK to me, and according to the discussion it sounds like
it was ready to be applied, but since that's more than a year ago, and
I'm not an smerge user, I wanted to ask whether the patch is still
something that should go in.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18824: split diff-check-labels off diff-no-select

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> Package:  emacs
> Severity: wishlist
> Tags:     patch
>
> 	Please provide a separate diff-check-labels function to allow
> 	for the code calling diff.el facilities to supply its own
> 	--label= arguments to diff in place of diff.el-generated ones.
>
> 	Possible patch, as well as the example usage, are MIMEd.

I think this patch looks fine.  Could you also provide a NEWS entry?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18824: split diff-check-labels off diff-no-select

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> 	Please provide a separate diff-check-labels function to allow
> 	for the code calling diff.el facilities to supply its own
> 	--label= arguments to diff in place of diff.el-generated ones.
>
> 	Possible patch, as well as the example usage, are MIMEd.

I've now applied the patch along with the smerge one.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18850: smerge-mode: use diff-check-labels

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

> block 18850 by 18824
> thanks
>
> 	Another try.

OK; applied.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no