wip: some improvement suggestions

[michael-heerdegen]

Hello,

I like the idea behind the wip mode stuff. Here are some thoughts and ideas about it.

1 The manual says that the wip modes will come with a performance penalty. I think especially "magit-wip-after-save-mode" will lead to many additional commits into refs/wip.

I think it would be good if the manual would be more precise about what kind of performance loss this will cause - take this also as a question! Of course will it slow down saving a bit (naturally), but are there other, durable costs? Will the logs take longer to pop up? Even more important, will the effect accumulate and slow down git as a whole, because the references tree will grow much faster than without the mode?

2 The implementation details should be hidden from the user. Consequently, to view a log of wip saves, the user should not have to type

lO refs/wip/index/refs/heads/master RET

as mentioned in the doc; instead, there should be an own binding in "magit-invoke-popup-action" IMHO, or something like that.

3 Apart from the automatic saves performed by the wip modes, use the refs tree to allow the user to make named explicit backups/snapshots of the current state (of the worktree), a bit like stash snapshots. This would be easy to do, the infrastructure is already there ("magit-wip-commit-worktree").

Example: I have implemented some new feature, but am far away from committing. So I would type M-x magit-wip-make-named-backup RET, then I'm prompted for a name, it could be "try-to-implement-foo", and then it calls something like (magit-wip-commit-worktree (magit-wip-get-ref) nil "try-to-implement-foo").

This would be a good stand-alone alternative to automatic saving via wip modes, but also a good complement, because the user will be able to find the individually named commits easier than automatically created ones with unmeaningful names.

Thanks!

[tarsius]

1. The performance penalty is mostly limited to creating the extra commit. But when there are massive amount of changes, then the cost can be quite high. I suppose I made this sound a bit scarier than it actually is, to avoid being asked for (impossible) performance improvements now.
2. I don't agree that the implementation details should be hidden. Users shouldn't have to type out the full ref, but they should know how the whole thing works. There should be commands that show the log for the current (or another) branch and its wip refs (going back >1 branch points).
3. There already exists a command magit-wip-commit, but it does not allow editing the commit message. But I would recommend you create such explicits snapshot commits on the actual branch instead of the wip refs.

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

1 The performance penalty is mostly limited to creating the extra commit. But when
there are massive amount of changes, then the cost can be quite high. I suppose I
made this sound a bit scarier than it actually is, to avoid being asked for
(impossible) performance improvements.

Ok, thanks, good to know.

2 I don't agree that the implementation details should be hidden. Users shouldn't have
to type out the full ref, but they should know how the whole thing works. There
should be commands that show the log for the current / another branch and it's wip
refs.

Ok.

3 There already exists a command magit-wip-commit, but it does not allow editing the
commit message. But I would recommend you create such explicits snapshot
commits on the actual branch instead of the wip refs.

I guess it depends on your working style. I tend to commit stuff in
related groups of changes, and when the thing is already stable and
finished. So I want to have a mean to make snapshots in between.

Anyway, this is just a suggestion, I have written my own command for my
purpose. Maybe I change my behavior later. If you think this
suggestion does not cover what the typical user wants, just ignore it.

Thanks,

Michael.

[tarsius]

So I want to have a mean to make snapshots in between.

Yes I understand - and suggest you do that on the actual branch. Later you can squash, reword, rearrange, or even just reset to the commit right before the "wip commits", whatever is suitable in any given situation. There's nothing wrong with creating "unfinished commits" on an actual branch. I do it all the time.

I have written my own command for my purpose

With a prefix argument magit-wip-commit now let's you write your own message.

Here is a very early draft for the wip log command:

(defun magit-wip-log-current (&optional args files)
  (interactive (magit-log-arguments))
  (let ((branch (magit-get-current-branch)))
    (magit-log (list branch
                     (format "%sindex/refs/heads/%s" magit-wip-namespace branch)
                     (format "%swtree/refs/heads/%s" magit-wip-namespace branch))
               args files)))

(magit-define-popup-action 'magit-log-popup ?w "Wip" 'magit-wip-log-current)

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

With a prefix argument magit-wip-commit now let's you write your own
message.

That's reasonable, thanks.

Here is a very early draft for the wip log command:

(defun magit-wip-log-current (&optional args files)
(interactive (magit-log-arguments))
(let ((branch (magit-get-current-branch)))
(magit-log (list branch
(format "%sindex/refs/heads/%s" magit-wip-namespace branch)
(format "%swtree/refs/heads/%s" magit-wip-namespace branch))
args files)))

(magit-define-popup-action 'magit-log-popup ?w "Wip" 'magit-wip-log-current)

Seems to work well here.

[tarsius]

I have added magit-wip-log and magit-wip-log-current. Try them with a prefix argument.

[michael-heerdegen]

Thanks, I will try them carefully. Looks fine at the first look.

Will these commands become an official binding in the "magit-log-popup"?

[tarsius]

Maybe after turning on the wip modes by default, but that probably won't happen until v2.4.0.

[michael-heerdegen]

I found that a linear autosave history would fit much better to my working habits than the current behavior of magit-wip-after-save-local-mode using the head commit in the current branch as a root.

In larger projects, one cannot assume that all file changes between two commits are related only to those two commits. You may work on different things you will commit later.

What is the rationale for the current behavior? Would you consider to allow to use a linear autosave commit tree as well?

The current behavior has also a disadvantage for logging: If you want to run git-log with -L:

"This is currently limited to a walk starting from a single revision,
i.e., you may only give zero or one positive revision arguments."

(from man git-log)

So AFAIU it is not possible to search autosaves prior to the last commit when you want to use -L. That's quite a restriction.

[michael-heerdegen]

I guess changing magit-wip-get-parent to something like

  (defun magit-wip-get-parent (ref wipref)
    (if (magit-rev-verify wipref)
        wipref
      ref))

more or less does what I want.

[michael-heerdegen]

This could also be a separated mode. If it doesn't fit into the current concept of magit's wip modes, I would even be willing to provide the functionality in a separate package, unless it is totally unwanted.

I'm not allowed to reopen this issue - does this read somebody? I would be thankful for a reply, even a negative one. Thanks.

[tarsius]

What is the rationale for the current behavior?

Well for one thing old wip commits get garbage collected after a while. And...

There are some uncommitted changes, you create a commit from some of that, and some other changes remain "uncommitted". I feel it makes sense to bring the two units of commits into a relation by "attaching" the "uncommitted" changes to the "committed" changes by creating a new "branch" of wip-development starting at what was just committed properly.

It seems you value the other relationship the "wip internal sequence" more. I feel that isn't so important and that only maintaining that sequence of "events" (wip commits) actually loses information, namely "what hasn't been committed yet (on the actual branch)".

In short: I value the "checkpoints". (And when I write that I can actually year the sound from old arcade games when you race below a banner :-)

That might be a bit vague, but that's vague too (you might want to go into some details):

I found that a linear autosave history would fit much better to my working habits

Okay you mention one thing: log -L. But that's more about a problem that arises from a non-linear wip-history than a description of why linear history makes more sense conceptually.

Anyways, you can have it both ways (but without the garbage collection) like this (untested):

diff --git a/lisp/magit-wip.el b/lisp/magit-wip.el
index e424892..7c5bcb7 100644
--- a/lisp/magit-wip.el
+++ b/lisp/magit-wip.el
@@ -210,6 +210,8 @@ (defun magit-wip-update-wipref (wipref tree parent files msg start-msg)
       (setq start-msg (concat "restart autosaving " start-msg))
       (magit-call-git "update-ref" wipref "-m" start-msg
                       (magit-git-string "commit-tree" "-p" parent
+                                        (and (magit-rev-verify wipref)
+                                             (list "-p" wipref))
                                         "-m" start-msg
                                         (concat parent "^{tree}")))
       (setq parent wipref))

[michael-heerdegen]

Thanks for reopening!

Jonas Bernoulli notifications@github.com writes:

Well for one thing old wip commits get garbage collected after a
while.

By whom - git? That's bad.

There are some uncommitted changes, you create a commit from some of that, and
some other changes remain "uncommitted". I feel it makes sense to bring the two
units of commits into a relation by "attaching" the "uncommitted" changes to the
"committed" changes by creating a new "branch" of wip-development starting at what
was just committed properly.

That's what I expected. It's reasonable, also for me, but it doesn't
always fit my needs, I think.

Okay you mention one thing: log -L. But that's more about a problem that arises from a
non-linear wip-history than a description of why linear history makes more sense
conceptually. You can have it both ways (but without the garbage collection) like this
(untested):

diff --git a/lisp/magit-wip.el b/lisp/magit-wip.el
index e424892..7c5bcb7 100644
--- a/lisp/magit-wip.el
+++ b/lisp/magit-wip.el
@@ -210,6 +210,8 @@ (defun magit-wip-update-wipref (wipref tree parent files msg start-msg)
(setq start-msg (concat "restart autosaving " start-msg))
(magit-call-git "update-ref" wipref "-m" start-msg
(magit-git-string "commit-tree" "-p" parent

•                                    (and (magit-rev-verify wipref)
  

•                                         (list "-p" wipref))
                                   "-m" start-msg
                                   (concat parent "^{tree}")))
  

  (setq parent wipref))

What effect will this have - how does this allow us to have it "both
ways"? Oh, AFAIU this will make a wip commit directly after a commit
refer to two parents. I guess that would fit my needs indeed, I think.
If you don't reply that it's not what I want after having read the rest
of the message, I'll give it a try.

You wanted me to give details: I had added some code to my (laaarge)
init file and discarded it. The file had been saved in between.

Then I had made some commits (totally unrelated to the code I had
discarded).

After a while, I found that the discarded code had not been a bad idea
and wanted it back. Because I had made some commits in between, I
didn't know where to search in those many wip saves, I even didn't know
at which root commit (of the wip branch) to search.

I thought "hey, you can find it with git-log -L", because I exactly knew
in which part of the file I had made the change. But I soon had to
discover that -L can only search one branch at once. No chance to find
the correct wip commit.

With a linear history, it would have been easy to git-log -L, as would
it have been to use git-grep.

I repeat: my working style is different from yours, with your working
style, what I want doesn't make any sense.

I develop a few little things in parallel most of the time, try them for
a while, and make a clean commit when I'm sure the thing is good.
That's not a complete description of my work with git, but an aspect.

Regards!

[michael-heerdegen]

Tried it out now. Does exactly what I suspected I think I like the idea.

However, I still can't use git-log -L like I want. The first autosave commit after any real commit has two parents, and two according diffs. It seems that git-log -L uses the diff against the real commit, not the diff against the preceding autosave commit, when searching. This diff includes all not-yet committed pending changes. This makes git-log -L completely useless to find the autosave commit that corresponds to a certain file change.

[michael-heerdegen]

I guess what I want would mean using something like

    (if (magit-rev-verify wipref)
        (concat wipref "^{tree}")
      (concat parent "^{tree}"))

in the git-commit-tree call in your patch. Dunno if that would be acceptable for you. But it solves my problem of git-log -L usage.

[tarsius]

What about this?

diff --git a/lisp/magit-wip.el b/lisp/magit-wip.el
index 431d805..ef899eb 100644
--- a/lisp/magit-wip.el
+++ b/lisp/magit-wip.el
@@ -195,6 +195,8 @@ (defun magit-wip-commit-worktree (ref files msg)
     (when (magit-git-failure "diff-tree" "--quiet" parent tree "--" files)
       (magit-wip-update-wipref wipref tree parent files msg "worktree"))))

+(defvar magit-wip-merge-ref t)
+
 (defun magit-wip-update-wipref (wipref tree parent files msg start-msg)
   (let ((len (length files)))
     (unless (and msg (not (= (aref msg 0) ?\s)))
@@ -206,7 +208,9 @@ (defun magit-wip-update-wipref (wipref tree parent files msg start-msg)
                                                       (magit-toplevel)))))
                  msg)))
     (magit-reflog-enable wipref)
-    (unless (equal parent wipref)
+    (unless (or (equal parent wipref)
+                (and magit-wip-merge-ref
+                     (magit-rev-verify wipref)))
       (setq start-msg (concat "restart autosaving " start-msg))
       (magit-call-git "update-ref" wipref "-m" start-msg
                       (magit-git-string "commit-tree" "-p" parent
@@ -215,6 +219,8 @@ (defun magit-wip-update-wipref (wipref tree parent files msg start-msg)
       (setq parent wipref))
     (magit-call-git "update-ref" wipref "-m" msg
                     (magit-git-string "commit-tree" tree
+                                      (and magit-wip-merge-ref
+                                           (list "-p" wipref))
                                       "-p" parent "-m" msg))))

 (defun magit-wip-get-ref ()

[tarsius]

But I am afraid you'll have to figure this out yourself and create a separate package from the result.

The fact that the wip refs are restarted after creating a real commit and that wip commits are eventually garbage collected are central ideas of this package (they are about the only things inherited from the git-wip script which inspired this package and was used in early implementations of this library). So I am not particularly motivated to change this. I consider it a feature that after creating a real commit, the next commit contains exactly "what hasn't been committed yet".

If you do write your own package, then you can probably get away with just implementing substitutes for magit-wip-update-wipref and possibly magit-wip-get-parent. You should then advise these functions and add a magit-wip-merge-ref-mode (or something like that) and only use the advise if that mode is on.

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

If you do write your own package, then you can probably get away with
just implementing substitutes for magit-wip-update-wipref and possibly
magit-wip-get-parent. You should then advise these functions and add a
magit-wip-merge-ref-mode (or something like that) and only use the
advise if that mode is on.

I think I just use a separate tree under e.g. "refs/backup/", and the
new mode won't at all interfere with the wip modes.

Question: if I do that and use a linear tree there (which doesn't
include "real" commits) - will I have garbage collection there (which I
obviously don't want?).

Regards,

Michael.

[michael-heerdegen]

For reference: here is the first try. Seems to do what I want.

;; -*- lexical-binding: t -*-


(require 'magit)
(require 'magit-wip)

(defvar magit-backup-namespace "refs/backup/")

(defun magit-backup-get-ref ()
  (let ((ref (or (magit-git-string "symbolic-ref" "HEAD") "HEAD")))
    (when (magit-rev-verify ref)
      ref)))

(define-minor-mode magit-backup-local-mode
  "$$$$FIXME"
  nil nil nil
  (if magit-backup-local-mode
      (if (and buffer-file-name (magit-inside-worktree-p))
          (add-hook 'after-save-hook 'magit-backup-commit-buffer-file t t)
        (setq magit-backup-local-mode nil)
        (user-error "Need a worktree and a file"))
    (remove-hook 'after-save-hook 'magit-backup-commit-buffer-file t)))

(defun magit-backup-after-save-local-mode-turn-on ()
  (and buffer-file-name
       (ignore-errors (magit-inside-worktree-p))
       (magit-file-tracked-p buffer-file-name)
       (magit-backup-local-mode)))

;;;###autoload
(define-globalized-minor-mode magit-backup-mode
  magit-backup-local-mode magit-backup-after-save-local-mode-turn-on)

(defun magit-backup-commit-buffer-file ()
  (interactive)
  (when-let ((ref (magit-backup-get-ref)))
    (let* ((default-directory (magit-toplevel))
           (file (file-relative-name buffer-file-name)))
      (magit-backup-commit-worktree ref (list file)
                                    (if (called-interactively-p 'any)
                                        (format "backup %s" file)
                                      (format "backup %s after save" file))))))

(defun magit-backup-commit (files msg)
  (interactive (list nil (read-string "Backup commit message: ")))
  (when-let ((ref (magit-backup-get-ref)))
    (magit-backup-commit-worktree ref files msg)))

(defun magit-backup-commit-worktree (ref files msg)
  (let* ((wipref (concat magit-backup-namespace ref))
         (parent (if (magit-rev-verify wipref) wipref ref))
         (tree (magit-with-temp-index parent
                 (if files
                     (magit-call-git "add" "--" files)
                   (let ((default-directory (magit-toplevel)))
                     (magit-call-git "add" "-u" ".")))
                 (magit-git-string "write-tree"))))
    (when (magit-git-failure "diff-tree" "--quiet" parent tree "--" files) ;i.e. there are changes
      (magit-backup-update-ref wipref tree parent msg "worktree"))))

(defun magit-backup-update-ref (wipref tree parent msg start-msg)
  (magit-reflog-enable wipref)
  (unless (equal parent wipref)
    (setq start-msg (concat "start autosaving " start-msg))
    (magit-call-git "update-ref" wipref "-m" start-msg
                    (magit-git-string "commit-tree" "-p" parent
                                      "-m" start-msg
                                      (concat parent "^{tree}")))
    (setq parent wipref))
  (magit-call-git "update-ref" wipref "-m" msg
                  (magit-git-string "commit-tree" tree
                                    "-p" parent "-m" msg)))

(defun magit-backup-log (branch args files _count)
  (interactive
   (nconc (list (magit-completing-read
                 "Log branch backups"
                 (nconc (magit-list-local-branch-names) (list "HEAD"))
                 nil t nil 'magit-revision-history
                 (or (magit-branch-at-point)
                     (magit-get-current-branch)
                     "HEAD")))
          (magit-log-arguments)
          (list (prefix-numeric-value current-prefix-arg))))
     (unless (equal branch "HEAD")
       (setq branch (concat "refs/heads/" branch)))
     (magit-log (list (concat magit-backup-namespace branch))
                args files))

(magit-define-popup-action 'magit-log-popup ?~ "Backup" #'magit-backup-log)


(provide 'magit-backup)

[tarsius]

I think I just use a separate tree under e.g. "refs/backup/", and the new mode won't at all interfere with the wip modes.

If you release this as a package, then note that on pre-2.1 next there existed a backup mode, which used the magit-backup- prefix and which worked by storing stashes in "refs/backup/".

Question: if I do that and use a linear tree there (which doesn't include "real" commits) - will I have garbage collection there (which I obviously don't want?).

They are reachable from a ref, so they aren't garbage and won't be collected.