bug#13882: 24.2; saveplace.el limit drop least recently used

bug#13882: 24.2; saveplace.el limit drop least recently used

[Kevin Ryde]

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

When saveplace.el reaches save-place-limit, the file positions retained
via ~/.emacs-places are the first limit-many in alphabetical order.
I hoped instead it would be the first limit-many most recently used.
Ie. drop the least recently visited files in order to enforce the limit.

I struck this when I reached my save-place-limit with lots of files I
had visited long ago but which happened to be alphabetically before ones
I was visiting now.  The save-place feature no longer saved places
across sessions for files I now visited.

The effect can be seen by setting a small limit per init.el below.  Then
foo.el to visit three files successively

    HOME=`pwd` emacs -Q -l ./init.el -l ./foo.el

This leaves .emacs-places (in the current directory due to faked $HOME)
containing

    (("/tmp/aa" . 4)
     ("/tmp/bb" . 4)
     ("/tmp/cc" . 4))

Notice /tmp/cc was the most recently visited file but it's at the end of
the list and will be truncated when load-save-place-alist-from-file
enforces save-place-limit of 2.

    HOME=`pwd` emacs -Q -l ./init.el /tmp/cc
    =>
    point is at start of buffer /tmp/cc
    I hoped it would be at the end from the last visit

Note that you must exit and restart emacs to see the effect, because
save-place-limit is only enforced by load-save-place-alist-from-file.
Within a session there's no limit, only in reading the .emacs-places
file on restarting emacs.

I get some joy from not sorting save-place-alist when saving per change
below.

I believe save-place-to-alist keeps save-place-alist in "most recent
first" order (by delq and re-push to move an existing entry to the
start), and that that order should be preserved when saving.

2013-03-04  Kevin Ryde  <user42@zip.com.au>

	* saveplace.el (save-place-alist-to-file): Don't `sort'
	save-place-alist alphabetically, keep it in "most recent first" order.
	This ensures save-place-limit drops the least recently visited files,
	not the alphabetically last files.  Dropping alphabetically last files
	had meant save-place stopped working across sessions after
	.emacs-places filled with alphabetically early names.


[-- Attachment #2: init.el --]
[-- Type: application/emacs-lisp, Size: 78 bytes --]

[-- Attachment #3: foo.el --]
[-- Type: application/emacs-lisp, Size: 265 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: saveplace.el.nosort.diff --]
[-- Type: text/x-diff, Size: 507 bytes --]

--- saveplace.el.orig	2013-03-04 17:06:20.000000000 +1100
+++ saveplace.el	2013-03-04 17:07:42.000000000 +1100
@@ -224,8 +224,7 @@
                       (symbol-name coding-system-for-write)))
       (let ((print-length nil)
             (print-level nil))
-        (pp (sort save-place-alist
-                  (lambda (a b) (string< (car a) (car b))))
+        (pp save-place-alist ;; saved in order of most recently used
             (current-buffer)))
       (let ((version-control
              (cond

[-- Attachment #5: Type: text/plain, Size: 1159 bytes --]




In GNU Emacs 24.2.1 (i486-pc-linux-gnu, GTK+ Version 2.24.10)
 of 2012-09-10 on biber, modified by Debian
Configured using:
 `configure '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var/lib' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.2/site-lisp:/usr/share/emacs/site-lisp'
 '--with-crt-dir=/usr/lib/i386-linux-gnu' '--with-x=yes'
 '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
 'build_alias=i486-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default enable-multibyte-characters: t

bug#13882: 24.2; saveplace.el limit drop least recently used

[Dmitry Gutov]

Kevin Ryde <user42@zip.com.au> writes:
> When saveplace.el reaches save-place-limit, the file positions retained
> via ~/.emacs-places are the first limit-many in alphabetical order.
> I hoped instead it would be the first limit-many most recently used.
> Ie. drop the least recently visited files in order to enforce the limit.
>
> I struck this when I reached my save-place-limit with lots of files I
> had visited long ago but which happened to be alphabetically before ones
> I was visiting now.  The save-place feature no longer saved places
> across sessions for files I now visited.
> <example>

I can confirm that this is a problem, and it doesn't look "minor" in the
context of this package.

> ...
> I get some joy from not sorting save-place-alist when saving per change
> below.
>
> I believe save-place-to-alist keeps save-place-alist in "most recent
> first" order (by delq and re-push to move an existing entry to the
> start), and that that order should be preserved when saving.

I like the solution, but according to the ChangeLog the decision to sort
the list was made at the request of a user, who apparently has to merge
saveplace history files from time to time:

2010-12-29  Karl Fogel  <kfogel@red-bean.com>

	* saveplace.el (save-place-alist-to-file): Save list sorted and
	pretty-printed, so that it is mergeable by line-based text merging,
	as suggested by Iain Dalton <iain.dalton {_AT_} gmail.com>.

Paging the author.

Karl, do you think this consideration is still important? I don't see a
reasonable way to keep the list easy to merge and still retain the
"most-recently used" information.

You either keep the list unsorted (and continually shuffle the
elements), or store some timestamps, which will also be a source of
merge conflicts.

> 2013-03-04  Kevin Ryde  <user42@zip.com.au>
>
> 	* saveplace.el (save-place-alist-to-file): Don't `sort'
> 	save-place-alist alphabetically, keep it in "most recent first" order.
> 	This ensures save-place-limit drops the least recently visited files,
> 	not the alphabetically last files.  Dropping alphabetically last files
> 	had meant save-place stopped working across sessions after
> 	.emacs-places filled with alphabetically early names.

--Dmitry

bug#13882: 24.2; saveplace.el limit drop least recently used

[Karl Fogel]

Dmitry Gutov <dgutov@yandex.ru> writes:
>Kevin Ryde <user42@zip.com.au> writes:
>> When saveplace.el reaches save-place-limit, the file positions retained
>> via ~/.emacs-places are the first limit-many in alphabetical order.
>> I hoped instead it would be the first limit-many most recently used.
>> Ie. drop the least recently visited files in order to enforce the limit.
>>
>> I struck this when I reached my save-place-limit with lots of files I
>> had visited long ago but which happened to be alphabetically before ones
>> I was visiting now.  The save-place feature no longer saved places
>> across sessions for files I now visited.
>> <example>
>
>I can confirm that this is a problem, and it doesn't look "minor" in the
>context of this package.
>
>> ...
>> I get some joy from not sorting save-place-alist when saving per change
>> below.
>>
>> I believe save-place-to-alist keeps save-place-alist in "most recent
>> first" order (by delq and re-push to move an existing entry to the
>> start), and that that order should be preserved when saving.
>
>I like the solution, but according to the ChangeLog the decision to sort
>the list was made at the request of a user, who apparently has to merge
>saveplace history files from time to time:
>
>2010-12-29  Karl Fogel  <kfogel@red-bean.com>
>
>	* saveplace.el (save-place-alist-to-file): Save list sorted and
>	pretty-printed, so that it is mergeable by line-based text merging,
>	as suggested by Iain Dalton <iain.dalton {_AT_} gmail.com>.
>
>Paging the author.
>
>Karl, do you think this consideration is still important? I don't see a
>reasonable way to keep the list easy to merge and still retain the
>"most-recently used" information.
>
>You either keep the list unsorted (and continually shuffle the
>elements), or store some timestamps, which will also be a source of
>merge conflicts.

Hmmm.  I see your point, and, of course, there are arguments both ways.

We could supply an user-option to control the behavior, but that still
leaves the question of which default we should choose...

However, I think the answer to that is also clear: unsorted should be
the default (or rather, chronologically sorted should be the default),
and if a user wants the list alphabetized (for merge purposes), they can
configure it so.  And this would be documented not only in the new
variable, but also in the doc string of `save-place-limit'.

What do you think?

-Karl

>> 2013-03-04  Kevin Ryde  <user42@zip.com.au>
>>
>> 	* saveplace.el (save-place-alist-to-file): Don't `sort'
>> 	save-place-alist alphabetically, keep it in "most recent first" order.
>> 	This ensures save-place-limit drops the least recently visited files,
>> 	not the alphabetically last files.  Dropping alphabetically last files
>> 	had meant save-place stopped working across sessions after
>> 	.emacs-places filled with alphabetically early names.
>
>--Dmitry

bug#13882: 24.2; saveplace.el limit drop least recently used

[Dmitry Gutov]

On 08.03.2013 1:20, Karl Fogel wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>> Karl, do you think this consideration is still important? I don't see a
>> reasonable way to keep the list easy to merge and still retain the
>> "most-recently used" information.
>>
>> You either keep the list unsorted (and continually shuffle the
>> elements), or store some timestamps, which will also be a source of
>> merge conflicts.
>
> Hmmm.  I see your point, and, of course, there are arguments both ways.
>
> We could supply an user-option to control the behavior, but that still
> leaves the question of which default we should choose...
>
> However, I think the answer to that is also clear: unsorted should be
> the default (or rather, chronologically sorted should be the default),
> and if a user wants the list alphabetized (for merge purposes), they can
> configure it so.  And this would be documented not only in the new
> variable, but also in the doc string of `save-place-limit'.
>
> What do you think?

We can do it this way, of course.

But I imagine that merging of place files can only be a once-in-a-while 
occurrence, and at all other times the user would prefer the 
chronological sorting (if they were aware of this issue, at all).

So maybe we should provide an export command instead, that would save 
the sorted places list to a file specified by the user, to do as they 
please.

Let's also ask the original reporter. Iain, any opinion?

bug#13882: 24.2; saveplace.el limit drop least recently used

[Kevin Ryde]

Karl Fogel <kfogel@red-bean.com> writes:
>
> However, I think the answer to that is also clear: unsorted should be
> the default (or rather, chronologically sorted should be the default),
> and if a user wants the list alphabetized (for merge purposes), they can
> configure it so.

You'd be very tempted to let them put it through the sort program or
sort func themselves, not have any option at all.

In a merge you presumably still want the most-recent 400 visits, or
whatever limit, which would require per-entry timestamps to do properly.
And if you're not limiting it then I imagine there's no need to sort,
just concat the lot.

I wondered how well the simple save-place-loaded bit works when you've
got two running copies of emacs.  I suppose the save places of the last
one to exit will overwrite anything the others saved.  That wasn't the
aim of the "merge" was it?

bug#13882: 24.2; saveplace.el limit drop least recently used

[Karl Fogel]

Kevin Ryde <user42@zip.com.au> writes:
>Karl Fogel <kfogel@red-bean.com> writes:
>> However, I think the answer to that is also clear: unsorted should be
>> the default (or rather, chronologically sorted should be the default),
>> and if a user wants the list alphabetized (for merge purposes), they can
>> configure it so.
>
>You'd be very tempted to let them put it through the sort program or
>sort func themselves, not have any option at all.

?  Sorry; I didn't understand the above.  Are you saying I personally
would be tempted to do that, or that in the abstract one would be
tempted to do that?

How would the user hook in to run the sort, unless we provide some
option in saveplace.el?  (We don't generally expect users to modify the
source code of core Emacs packages.)

>In a merge you presumably still want the most-recent 400 visits, or
>whatever limit, which would require per-entry timestamps to do properly.
>And if you're not limiting it then I imagine there's no need to sort,
>just concat the lot.
>
>I wondered how well the simple save-place-loaded bit works when you've
>got two running copies of emacs.  I suppose the save places of the last
>one to exit will overwrite anything the others saved.  That wasn't the
>aim of the "merge" was it?

Um.  Can you say the above more verbosely?  I'm not really understanding
how many different topics you're addressing, nor what changes you're
proposing... I'm happy to address a concrete proposal, I'm just having
trouble understanding exactly what you're saying above.  (It could be
because I'm a bit sick right now; apologies if my brain isn't working
right...)

-Karl

bug#13882: 24.2; saveplace.el limit drop least recently used

[Kevin Ryde]

Karl Fogel <kfogel@red-bean.com> writes:
>
> or that in the abstract one would be
> tempted to do that?

Yes, in the abstract :-).

> I'm happy to address a concrete proposal,

I would say take away the `sort', and don't have an option for it.

> How would the user hook in to run the sort, unless we provide some
> option in saveplace.el?

I presume a user has two .emacs-places files and wants to merge them.
Was that the motivation for the sort?

If yes then I would say the files can be sorted easily enough at that
time, no need to always save sorted.  In fact I would say keeping
most-recent-first order might be desirable when merging two files
anyway.

> I'm just having
> trouble understanding exactly what you're saying above.

The second part I wondered what happens if I have two running copies of
emacs, both with save-place enabled.  When the first one exits it writes
.emacs-places.  When the second one exits it too writes .emacs-places.
I suspect the second overwrites the first.  Was such a situation the
motivation for the sort to "merge"?

bug#13882: 24.2; saveplace.el limit drop least recently used

[Karl Fogel]

Kevin Ryde <user42@zip.com.au> writes:
>> I'm happy to address a concrete proposal,
>I would say take away the `sort', and don't have an option for it.

Ah, okay.  So you're saying the merge use case is not worth supporting,
it sounds like?

>> How would the user hook in to run the sort, unless we provide some
>> option in saveplace.el?
>
>I presume a user has two .emacs-places files and wants to merge them.
>Was that the motivation for the sort?

Right, I think so.  I think it's typically that they keep their
.emacs-places under version control, and they do an update ("pull") and
get changes that need to be merged.  So they want the file to work well
with the usual merge algorithms.

>If yes then I would say the files can be sorted easily enough at that
>time, no need to always save sorted.  In fact I would say keeping
>most-recent-first order might be desirable when merging two files
>anyway.

Yes, now that you mention it, it makes me wonder why there was a merge
problem in the first place...

>The second part I wondered what happens if I have two running copies of
>emacs, both with save-place enabled.  When the first one exits it writes
>.emacs-places.  When the second one exits it too writes .emacs-places.
>I suspect the second overwrites the first.  Was such a situation the
>motivation for the sort to "merge"?

No; I think it was the above (version control) scenario.

Best,
-Karl

bug#13882: 24.2; saveplace.el limit drop least recently used

[Ian Dalton]

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

For the record, my original bug report read:

> I sync my Emacs profile across several computers with Unison, and it
> can't merge changes in .places because it's one long line.

I no longer use this setup, so I don't know how useful a change it was. I
think that the Unison UI had some kind of option to do guided file merges,
but it only worked for the usual simple line-oriented diff.

[-- Attachment #2: Type: text/html, Size: 458 bytes --]

bug#13882: 24.2; saveplace.el limit drop least recently used

[Kevin Ryde]

Ian Dalton <iain.dalton@gmail.com> writes:
>
> I no longer use this setup, so I don't know how useful a change it was. I
> think that the Unison UI had some kind of option to do guided file merges,
> but it only worked for the usual simple line-oriented diff.

Hmm.  Making the file line-wise probably helped, but I imagine there's
no file format which could give the perfect result from the usual
diff/patch contextual merges.

How about keeping the line-by-line pretty print for everyone's
readability, but drop the sort since it's actively harmful, and leave
anything more for future thought.

bug#13882: 24.2; saveplace.el limit drop least recently used

[Karl Fogel]

Kevin Ryde <user42@zip.com.au> writes:
>Ian Dalton <iain.dalton@gmail.com> writes:
>> I no longer use this setup, so I don't know how useful a change it was. I
>> think that the Unison UI had some kind of option to do guided file merges,
>> but it only worked for the usual simple line-oriented diff.
>
>Hmm.  Making the file line-wise probably helped, but I imagine there's
>no file format which could give the perfect result from the usual
>diff/patch contextual merges.
>
>How about keeping the line-by-line pretty print for everyone's
>readability, but drop the sort since it's actively harmful, and leave
>anything more for future thought.

Done; see revision below.  Thanks, Dmitry, Kevin, and Ian, for your
thorough & patient explanations.

  ------------------------------------------------------------
  revno: 112040
  revision-id: kfogel@red-bean.com-20130313185405-ibq2um8vj55d4x0a
  parent: eggert@cs.ucla.edu-20130313184222-yq69wjnyhzd6o40d
  committer: Karl Fogel <kfogel@red-bean.com>
  branch nick: trunk
  timestamp: Wed 2013-03-13 13:54:05 -0500
  message:
    * saveplace.el (save-place-alist-to-file): Don't sort
    `save-place-alist', just pretty-print it (bug#13882).
  modified:
    lisp/ChangeLog      changelog-20091113204419-o5vbwnq5f7feedwu-1432
    lisp/saveplace.el   saveplace.el-20091113204419-o5vbwnq5f7feedwu-622
  ------------------------------------------------------------

-Karl