vc-find-revision-no-save?

vc-find-revision-no-save?

[John Yates]

Towards the end of Oct 2022, each of you contributed to the above named
emacs-devel email thread.

With issue #61071 on debbugs.gnu.org, I have attempted to address some
of the points mentioned in the email thread via:

    [PATCH 1/3] Refactor and document vc-find-revision caching

I append the cover letter and commit message below.

This is my very first attempt to contribute to Emacs.  So far I have
received no feedback.  If I am doing something wrong, please let me know.

/john

== [PATCH 0/3] Cover letter ==========================================

This is a series of three bisectable (I hope :-) patches, culminating
in support of a new Emacs backup scheme:

* [PATCH 1/3] Refactor and document vc-find-revision caching
* [PATCH 2/3] Introduce VC timemachine capability
* [PATCH 3/3] Introduce vc-bos: backup on save (to an RCS file)

This Backup-On-Save scheme exploits a file system mirror scheme
introduced in the first patch.  By exploiting a little known aspect
of RCS's algorithm for locating a master file, backups are stored
completely removed from the work file (i.e. no local RCS directories)
and under exactly the same filename (i.e. no ',v' suffix or similar).

Accessing backed-ups exploits a new vc-timemachine capability,
introduced in the second patch.  Both the design and code owe much
to Peter Stiernström's original git-timemachine.el.  To sidestep any
copyright issues, Peter has graciously assigned git-timemachine.el's
copyright to the FSF.  With the submission timemachine functionality
is available in both vc-git and vc-rcs.

This backup scheme works equaly well with files already under some
VCS as well as with files that are not currently version controlled.

For me (primarily a C++ programmer) this is:
  * My first significant bit of elisp
  * My first exposure to the VC codebase
  * My first Emacs / FSF submission

I welcome all nature of feedback:
  * Code criticism
  * Violations of pertinent standards
  * Bug reports
  * Suggested improvement
  * . . .

== [PATCH 1/3] Refactor and document vc-find-revision caching ==========

Previously there existed two helper functions for vc--revision-other-window:

* vc--revision-other-window-save
* vc--revision-other-window-no-save

The expectation seems to have been that when materializing a revision is
deemed costly (slow backend? remote? ...) it should be saved.  I believe
that, even though the word 'cache' is never used, this was intended to
be a caching mechanism.  That said, the logic provided only a single
save/no-save global toggle.  Aspects of this mechanism were discussed
in this email thread:

  https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01794.html

I have tried to address some of the concerns raised therein and to
provide some clearer abstractions:

* When a revision gets saved it is deemed a cache.  Thus it is imperative
  that the cached revision be protected and adequately validated before
  being reused.

* A cached revision may be saved as a sibling of the file that triggered its
  materialization or in may be saved in a mirror directory tree rooted at
  `vc-cache-root'.  The latter choice avoids cluttering work trees with with
  historic revisions and enables caching across work trees.  `vc-cache-root'
  will also provide a location for the forthcoming vc-bos's backups.

* I have defined the concept of a revision buffer.  This is the form
  of buffer returned by vc's find-revision operation.  It is bound to
  a specific revision, it is read-only and it has a nil buffer-file-name.
  Thus it visits no saved nor cached file.  The rationale is twofold:

  - A revision is a materialization of immutable history

  - The only potential value for a revision buffer's buffer-file-name is a
    cache file which should likewise be regarded as immutable.  Futher, if
    materializing revisions is not deemed costly, even that file may not
    exist.  So, in the interest of consistency, revision buffers do not
    visit files.

========================================================================

* lisp/vc/vc.el (vc-find-revision-no-save, vc-find-revision-cache):
Rename defcustoms to be more descriptive.
(vc-find-revision, vc-find-revision-save, vc-find-revision-no-save):
Reimplement the enssence of these three function as a single
`vc-find-revision' function.  Clarify that the result is a revision
buffer, unattached to any file.  Support optional caching, either
alongside the original file or within a mirror directory structure
beneath `vc-cache-root'.

Re: vc-find-revision-no-save?

[Dmitry Gutov]

Hi John,

On 31/01/2023 13:57, John Yates wrote:
> Towards the end of Oct 2022, each of you contributed to the above named
> emacs-devel email thread.
> 
> With issue #61071 on debbugs.gnu.org, I have attempted to address some
> of the points mentioned in the email thread via:
> 
>      [PATCH 1/3] Refactor and document vc-find-revision caching
> 
> I append the cover letter and commit message below.
> 
> This is my very first attempt to contribute to Emacs.  So far I have
> received no feedback.  If I am doing something wrong, please let me know.

You've done everything correctly regarding the submission, but it seems 
like nobody highly interested in this particular addition has found the 
time to review it so far.

Myself, I just saw the title (vc-timemachine) and skipped it. It wasn't 
obvious that some existing VC code was altered and needed reviewing. Sorry.

We're also close to releasing Emacs 29, so it seemed like higher priority.

Otherwise, though, it can be a good idea to ping a bug report a week or 
two after receiving to response. If nobody else does, the head 
maintainer usually ends up responding.

I will also use this opportunity to complain about the lack of code 
review tools here. Some of the reviewers here might be able to quote 
attached patches inline, but I don't use Gnus, so that's not a real 
option. Just a lot of opening the patches externally and copying and 
pasting. We could move the review to EMBA, though: 
https://emba.gnu.org/emacs/emacs/-/merge_requests

Now, to go back to the original thread you referred to, I mentioned 
vc-annotate, and you agreed that it has similar features but misses some 
stuff. Such as syntax highlighting. And editing support (is that 
necessary?).

Overall, I think it might be better to add features to vc-annotate than 
add a very similar but different feature. Especially since it has unique 
features of its own, such as showing and being able to jump to a 
revision that last modified a given line. Or the one before it, etc. 
IME, that's usually more useful than going through a file history 
linearly. But that's my opinion.

If that idea holds your interest, Stefan might also give a couple of 
recommendations, since as I recall he suggested something like that in 
the past. E.g. one of the ideas was to remove the sidebar text from the 
buffer text when processing, and instead store it in overlays or some 
such. Then put the buffer into a corresponding major mode which will 
apply font-lock and etc. But keep the (hidable) sidebar using overlays, 
for example. Or put it in the margin (also using overlays). And keep the 
navigation commands. The latter conflicts with having the buffer 
editable (if we're going to keep the one-char bindings). I haven't 
looked yet how it's solved with timemachine. The bindings might depend 
on whether the buffer is in read-only mode or not, though.

Finally, some nits about the first patch:
- It moves from the cache-by-default behavior to dont-cache-by-default.
- It removes an existing user option without a deprecation period.
- It adds a timemachine-related variable to vc.el (vc-tm--revision ?). 
Timemachine will be a separate package, right?

The overall idea seems sound. But if we choose the route of improving 
vc-annotate, a revision cache will probably not help because we would be 
caching the 'git annotate' output instead. Thus making it specific to 
that feature only.

Re: vc-find-revision-no-save?

[John Yates]

On Fri, Feb 10, 2023 at 6:47 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> I will also use this opportunity to complain about the lack of code
> review tools here. Some of the reviewers here might be able to quote
> attached patches inline, but I don't use Gnus, so that's not a real
> option. Just a lot of opening the patches externally and copying and
> pasting. We could move the review to EMBA, though:
> https://emba.gnu.org/emacs/emacs/-/merge_requests

Happy to try any approach that might generate more feedback.
grepping, I see lots of mentions of EMBA in an emacs sandbox,
but nothing that describes how to set up a merge request.

/john

Re: vc-find-revision-no-save?

[John Yates]

On Fri, Feb 10, 2023 at 6:47 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> Now, to go back to the original thread you referred to, I mentioned
> vc-annotate, and you agreed that it has similar features but misses some
> stuff. Such as syntax highlighting. And editing support (is that
> necessary?).

Actually, what I wrote was:
|  That said, vc-annotate is a jarring presentation.  As I browse through
|  different revisions I want the major mode to remain that of the original
|  (non-annotated) file.  I want syntax-based highlighting, lsp support, etc.

I do not wantdestructive editing.  A revision buffer presents a view
of an immutable revision.  I should never be allowed to alter history
(unless, of course, I do choose to rewrite history, but then, as we all
know, I am actually creating new revisions with new revision IDs).
What I would like is to be able to operate within a revision buffer using
all non-destructive commands and modes.

> Overall, I think it might be better to add features to vc-annotate than
> add a very similar but different feature. Especially since it has unique
> features of its own, such as showing and being able to jump to a
> revision that last modified a given line. Or the one before it, etc.
> IME, that's usually more useful than going through a file history
> linearly. But that's my opinion.

I agree that stepping linearly through the revisions on a branch often
is not the interface one would want.  But that is beside the point.
vc-annotate is a gussied-up presentation of the output of a backend
database join.  To me it shoves in my face how smart it is because
it can tell me, for every line, the revision, the author and the date when
that line was last modified.  Further, to present a color coded sense
of time, it presents my source in a jarring, and utterly unfamiliar style.

I view the vc-timemachine framework as one that simply knows, given
a file, how to display arbitrary revisions on its branch.  How the next
revision to display is chosen is orthogonal.  Today, vc-timemachine
supports the analogs of vc-annotate's 'n' and 'p', plus "search the
revision subject lines" and "jump to the n'th revision on the branch".
I am working on date based navigation.  There is no fundamental
reason why, over time, other vc-annotate -like navigation functions
cannot be added.

> Finally, some nits about the first patch:
> - It moves from the cache-by-default behavior to dont-cache-by-default.

Correct.  Stefan endorsed that change.

> - It removes an existing user option without a deprecation period.

The option did not go away.  Originally, I changed its name to reflect
the new default.  Stefan had much the same objection that you did.
I have since restored the original name and changed only the default.

> - It adds a timemachine-related variable to vc.el (vc-tm--revision ?).
> Timemachine will be a separate package, right?

No.  vc-timemachine is a properly integrated vc concept.  (RMS explicity
requested that it be so.)  Any VCS backend can support vc-timemachine
functionality by implementing one (optionally two) methods.

> The overall idea seems sound. But if we choose the route of improving
> vc-annotate, a revision cache will probably not help because we would be
> caching the 'git annotate' output instead. Thus making it specific to
> that feature only.

That would be a different cache.  This cache addresses cases where the
cost simply to retrieve a revision is high, independent of whether or not
that revision is then annotated.

vc-find-revision-no-save?

[John Yates]

(I am working on incorporating git-timemachine-like functionality into
vc.  If I should post questions such as the following in
help-gnu-emacs, just let me know.)

vc.el contains the following defcustom:

    (defcustom vc-find-revision-no-save nil
      "If non-nil, `vc-find-revision' doesn't write
the created buffer to file."
      :type 'boolean
      :version "27.1")

Notice that the default is nil.  What is the logic in allowing an
historic revision to be associated with a file?  Why is it not merely
a readonly buffer with no backing file?

Perhaps I have been too influenced by git...  My mental model is that
revisions are immutable _values_.  Is that not vc's model?

Re: vc-find-revision-no-save?

[Dmitry Gutov]

On 24.10.2022 03:27, John Yates wrote:
> Notice that the default is nil.  What is the logic in allowing an
> historic revision to be associated with a file?  Why is it not merely
> a readonly buffer with no backing file?
> 
> Perhaps I have been too influenced by git...  My mental model is that
> revisions are immutable_values_.  Is that not vc's model?

Caching? Easy of copying to replace the original?

VC's old, and a lot of it prefers convenience over purity. Including the 
convenience of implementation.

Re: vc-find-revision-no-save?

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 24.10.2022 03:27, John Yates wrote:
>> Notice that the default is nil.  What is the logic in allowing an
>> historic revision to be associated with a file?  Why is it not merely
>> a readonly buffer with no backing file?
>> Perhaps I have been too influenced by git...  My mental model is
>> that
>> revisions are immutable_values_.  Is that not vc's model?
>
> Caching? Easy of copying to replace the original?
>
> VC's old, and a lot of it prefers convenience over purity. Including
> the convenience of implementation.

If that is the motivation, is there a reason the files are stored on the
current working directory, instead of using /tmp or ~/.cache?  I have
set that option to t which is a great improvement IMO, as I don't want
to fill up my working directory with temporary files.

Re: vc-find-revision-no-save?

[Dmitry Gutov]

On 24.10.2022 19:02, Philip Kaludercic wrote:
> If that is the motivation, is there a reason the files are stored on the
> current working directory, instead of using /tmp or ~/.cache?  I have
> set that option to t which is a great improvement IMO, as I don't want
> to fill up my working directory with temporary files.

I don't really have a strong opinion. But I suppose if one wanted to 
rename the newly created file to its base name (removing the rev 
suffix), it's easier to do if the file is in the same dir.

The variable was added by Juri just recently, and all the work before 
that was done by Dan Nicolaescu and ESR, neither of who are active on 
the development lists anymore. You could shoot them a message, though.

Re: vc-find-revision-no-save?

[Stefan Monnier]

Philip Kaludercic [2022-10-24 16:02:15] wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>> On 24.10.2022 03:27, John Yates wrote:
>>> Notice that the default is nil.  What is the logic in allowing an
>>> historic revision to be associated with a file?  Why is it not merely
>>> a readonly buffer with no backing file?

I can answer this one: because back when we wrote this code, the "Git"
of the day was called "CVS" and getting a historic revision required
fetching it from the centralized server, so you were usually quite happy
to keep a local copy, even if you had to erase them manually every once
in a while.

> If that is the motivation, is there a reason the files are stored on the
> current working directory, instead of using /tmp or ~/.cache?  I have
> set that option to t which is a great improvement IMO, as I don't want
> to fill up my working directory with temporary files.

I suspect the default should be changed, indeed.


        Stefan

Re: vc-find-revision-no-save?

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  John Yates <john@yates-sheets.org>,
>  Emacs developers <emacs-devel@gnu.org>
> Date: Mon, 24 Oct 2022 17:10:49 -0400
> 
> Philip Kaludercic [2022-10-24 16:02:15] wrote:
> > Dmitry Gutov <dgutov@yandex.ru> writes:
> >> On 24.10.2022 03:27, John Yates wrote:
> >>> Notice that the default is nil.  What is the logic in allowing an
> >>> historic revision to be associated with a file?  Why is it not merely
> >>> a readonly buffer with no backing file?
> 
> I can answer this one: because back when we wrote this code, the "Git"
> of the day was called "CVS" and getting a historic revision required
> fetching it from the centralized server, so you were usually quite happy
> to keep a local copy, even if you had to erase them manually every once
> in a while.
> 
> > If that is the motivation, is there a reason the files are stored on the
> > current working directory, instead of using /tmp or ~/.cache?  I have
> > set that option to t which is a great improvement IMO, as I don't want
> > to fill up my working directory with temporary files.
> 
> I suspect the default should be changed, indeed.

Could the default be backend-dependent?  Then people who use the VCSes
where you are "quite happy to keep a local copy" will still have their
cake.  That'd include CVS and SVN, I think.

Re: vc-find-revision-no-save?

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I can answer this one: because back when we wrote this code, the "Git"
  > of the day was called "CVS" and getting a historic revision required
  > fetching it from the centralized server, so you were usually quite happy
  > to keep a local copy, even if you had to erase them manually every once
  > in a while.

I still sometimes use CVS, and this feature is still useful with it.

  > > If that is the motivation, is there a reason the files are stored on the
  > > current working directory, instead of using /tmp or ~/.cache?

I think it is natural to put old-version files in the same directory
with the work files.  That's where one can find them again.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: vc-find-revision-no-save?

[Philip Kaludercic]

Richard Stallman <rms@gnu.org> writes:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > I can answer this one: because back when we wrote this code, the "Git"
>   > of the day was called "CVS" and getting a historic revision required
>   > fetching it from the centralized server, so you were usually quite happy
>   > to keep a local copy, even if you had to erase them manually every once
>   > in a while.
>
> I still sometimes use CVS, and this feature is still useful with it.

In that case the suggestion to change the behaviour depending on the VCS
should be fine.  If a VCS requires a network connection, storing the
files locally can be helpful.

Adding a third value for `vc-find-revision-no-save', something like
`if-remove', could be a nice default.

>   > > If that is the motivation, is there a reason the files are stored on the
>   > > current working directory, instead of using /tmp or ~/.cache?
>
> I think it is natural to put old-version files in the same directory
> with the work files.  That's where one can find them again.

Natural out of habit or natural out of necessity?  I remember being
surprised that after inspecting a few old versions of a file that my
directory was littered with the these files.

Re: vc-find-revision-no-save?

[Stefan Kangas]

Philip Kaludercic <philipk@posteo.net> writes:

> Adding a third value for `vc-find-revision-no-save', something like
> `if-remove', could be a nice default.

What would be the semantic of `if-remove'?

Re: vc-find-revision-no-save?

[Philip Kaludercic]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Adding a third value for `vc-find-revision-no-save', something like
>> `if-remove', could be a nice default.
           *t
     
> What would be the semantic of `if-remove'?

The semantics of *`if-remote'* would create a local file if the VCS
relies on connecting to a remote server to work properly.

Re: vc-find-revision-no-save?

[Andreas Schwab]

On Okt 29 2022, Philip Kaludercic wrote:

> Natural out of habit or natural out of necessity?  I remember being
> surprised that after inspecting a few old versions of a file that my
> directory was littered with the these files.

They are like numbered backups.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: vc-find-revision-no-save?

[Philip Kaludercic]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Okt 29 2022, Philip Kaludercic wrote:
>
>> Natural out of habit or natural out of necessity?  I remember being
>> surprised that after inspecting a few old versions of a file that my
>> directory was littered with the these files.
>
> They are like numbered backups.

Fair enough, but a popular configuration is

     (setq backup-directory-alist '((".*" . "~/.local/share/backup")))

that avoids littering the working directory with backup files.

Re: vc-find-revision-no-save?

[Stefan Monnier]

Philip Kaludercic [2022-10-29 06:46:00] wrote:
> Richard Stallman <rms@gnu.org> writes:
>> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
>> [[[ whether defending the US Constitution against all enemies,     ]]]
>> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>>
>>   > I can answer this one: because back when we wrote this code, the "Git"
>>   > of the day was called "CVS" and getting a historic revision required
>>   > fetching it from the centralized server, so you were usually quite happy
>>   > to keep a local copy, even if you had to erase them manually every once
>>   > in a while.
>>
>> I still sometimes use CVS, and this feature is still useful with it.
>
> In that case the suggestion to change the behaviour depending on the VCS
> should be fine.  If a VCS requires a network connection, storing the
> files locally can be helpful.

Note that, IIRC, the way the code works, if `vc-find-revision-no-save`
is non-nil you get an unsaved "file buffer", so it just takes a `C-x
C-s` to write the file to its "usual" place (i.e. `FILENAME.~REV~`,
IIRC) if you think it's worthwhile.

For that reason, I'm not convinced we need a special per-VCS handling
of this.


        Stefan

Re: vc-find-revision-no-save?

[Philip Kaludercic]

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

> Philip Kaludercic [2022-10-29 06:46:00] wrote:
>> Richard Stallman <rms@gnu.org> writes:
>>> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
>>> [[[ whether defending the US Constitution against all enemies,     ]]]
>>> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>>>
>>>   > I can answer this one: because back when we wrote this code, the "Git"
>>>   > of the day was called "CVS" and getting a historic revision required
>>>   > fetching it from the centralized server, so you were usually quite happy
>>>   > to keep a local copy, even if you had to erase them manually every once
>>>   > in a while.
>>>
>>> I still sometimes use CVS, and this feature is still useful with it.
>>
>> In that case the suggestion to change the behaviour depending on the VCS
>> should be fine.  If a VCS requires a network connection, storing the
>> files locally can be helpful.
>
> Note that, IIRC, the way the code works, if `vc-find-revision-no-save`
> is non-nil you get an unsaved "file buffer", so it just takes a `C-x
> C-s` to write the file to its "usual" place (i.e. `FILENAME.~REV~`,
> IIRC) if you think it's worthwhile.
>
> For that reason, I'm not convinced we need a special per-VCS handling
> of this.

Right, I didn't think of that.

I am just wondering if there are people who would rely on the previous
behaviour and would be surprised if this were to change all of a sudden?

>
>         Stefan

Re: vc-find-revision-no-save?

[Juri Linkov]

> vc.el contains the following defcustom:
>
>     (defcustom vc-find-revision-no-save nil
>       "If non-nil, `vc-find-revision' doesn't write
> the created buffer to file."
>       :type 'boolean
>       :version "27.1")
>
> Notice that the default is nil.  What is the logic in allowing an
> historic revision to be associated with a file?  Why is it not merely
> a readonly buffer with no backing file?

The default value is nil just to keep its previous default behaviour.
I don't know why previously it saved every visited revision to a file,
thus littering the source directories with useless files.

> Perhaps I have been too influenced by git...  My mental model is that
> revisions are immutable _values_.  Is that not vc's model?

I don't know a reason why anyone might want to edit an old revision.
But anyway when one needs to save a revision to file, it's easy to do
with 'C-x C-w RET', when the default value will be changed to t.

Re: vc-find-revision-no-save?

[John Yates]

On Mon, Oct 24, 2022 at 3:47 PM Juri Linkov <juri@linkov.net> wrote:
>
> I don't know a reason why anyone might want to edit an old revision.
> But anyway when one needs to save a revision to file, it's easy to do
> with 'C-x C-w RET', when the default value will be changed to t.

Thanks for the clarification.  It sounds like you intend to change
this default at some point in the future.  That sounds great.  For now
I will leave well enough alone.