GNU bug report logs - #4587
23.1; sort-lines and sort-fields always set buffer modified

Previous Next

Full log

View this message in rfc822 format

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kevin Rodgers <kevin.d.rodgers <at> gmail.com>
Cc: 4587 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
Subject: bug#4587: Antwort: Re: bug#4587: 23.1; sort-lines and sort-fields always set buffer modified
Date: Sun, 25 Oct 2009 11:27:21 -0400

>> Maybe we could make it suitable, turn it into a macro and use it around
>> the various candidates.  AFAICT, here are the problems I see with it:
>> - the call to md5 should use as much as possible the internal encoding.
>> I.e. at least pass an `emacs-internal' arg, tho it would be even
>> better to let md5 work directly on the internal representation.
>> - it should only work on the afected region rather than the whole buffer
>> (i.e. it needs start..end arguments).
>> - should it fiddle with the undo list?  or even revert the whole
>> "without-effect" set of changes (the changes may result in the same
>> final text, but they may very well have moved markers and changed
>> text-properties, and it might be desirable to undo those changes, so
>> as to better pretend nothing happened).

> Is this what you have in mind?

That looks almost right.  Here are some nitpicks:

> (defmacro with-maybe-region-modified (beg end &rest body)
>   "Execute BODY, then `restore-buffer-modified-p' if the contents are unchanged.
> BODY should not change the current buffer or modify the contents outside the region
> between BEG and END."

The docstring is wider then our convention.

>   `(let* ((region-beg ,beg)
> 	  (region-end ,end)
> 	  (buffer-was-modified-p (buffer-modified-p))
> 	  (buffer-was-not-modified-md5 (if (not buffer-was-modified-p)
> 					   (md5 (current-buffer) region-beg region-end
> 						'emacs-mule)))

Use `emacs-internal' here (in Emacs-23, the internal encoding is not
emacs-mule any more).

Don't use hardcoded symbols, since `body' might actually refer to
identically-named variables.

Add a FIXME-comment indicating that md5 should be improved to compute
this result without actually performing the encoding.  Or better yet,
provide a patch to `md5' which does just that.

> 	  ;; (orig-buffer-undo-list buffer-undo-list)
> 	  (with-maybe-region-modified-result
> 	   (progn ,@body)))		; save-current-buffer?

Yes, save-current-buffer seems to be necessary here, otherwise the code
will misbehave.

>      (when (and (not buffer-was-modified-p)
> 		(buffer-modified-p)
> 		(not (equal buffer-was-not-modified-md5
> 			    (md5 (current-buffer) region-beg region-end
> 				 'emacs-mule))))
>        (restore-buffer-modified-p buffer-was-modified-p)
>        ;; (setq buffer-undo-list orig-buffer-undo-list)
>        )
>      with-maybe-region-modified-result))

I think region-end should be assisted by a marker so we can detect if
the size of the region has changed and skip the second md5 call in
that case.

More importantly, I think the "(not (equal ...))" should be "(equal ...)".

Also, please add a FIXME-comment about whether we should maybe use
`undo' to revert the "effectless" changes.


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.