[PATCH] git.el: Don't reset HEAD in git-amend-file.

[PATCH] git.el: Don't reset HEAD in git-amend-file.

[Nikolaj Schumacher]

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

Hello.

The current implementation of git-amend-file is a little dangerous.
While git --amend is atomic, git-amend-file is not.

If the user calls it, but doesn't go through with the commit (due to
error or choice), git --reset HEAD^ has been called anyway.

With this patch it doesn't reset the HEAD till the actual commit.


regards,
Nikolaj Schumacher

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

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index 4fa853f..1360cb0 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -400,16 +400,17 @@ and returns the process output as a string, or nil if the git failed."
   (git-get-string-sha1
    (git-call-process-env-string (and index-file `(("GIT_INDEX_FILE" . ,index-file))) "write-tree")))
 
-(defun git-commit-tree (buffer tree head)
+(defun git-commit-tree (buffer tree parent &optional head)
   "Call git-commit-tree with buffer as input and return the resulting commit SHA1."
+  (unless head (setq head parent))
   (let ((author-name (git-get-committer-name))
         (author-email (git-get-committer-email))
         (subject "commit (initial): ")
         author-date log-start log-end args coding-system-for-write)
-    (when head
+    (when parent
       (setq subject "commit: ")
       (push "-p" args)
-      (push head args))
+      (push parent args))
     (with-current-buffer buffer
       (goto-char (point-min))
       (if
@@ -425,7 +426,7 @@ and returns the process output as a string, or nil if the git failed."
               (setq author-date (match-string 1)))
             (goto-char (point-min))
             (while (re-search-forward "^Parent: +\\([0-9a-f]+\\)" nil t)
-              (unless (string-equal head (match-string 1))
+              (unless (string-equal parent (match-string 1))
                 (setq subject "commit (merge): ")
                 (push "-p" args)
                 (push (match-string 1) args))))
@@ -852,7 +853,7 @@ Return the list of files that haven't been handled."
               (git-run-hook "pre-commit" `(("GIT_INDEX_FILE" . ,index-file))))
           (delete-file index-file))))))
 
-(defun git-do-commit ()
+(defun git-do-commit (&optional amend)
   "Perform the actual commit using the current buffer as log message."
   (interactive)
   (let ((buffer (current-buffer))
@@ -862,10 +863,11 @@ Return the list of files that haven't been handled."
           (message "You cannot commit unmerged files, resolve them first.")
         (unwind-protect
             (let ((files (git-marked-files-state 'added 'deleted 'modified))
-                  head head-tree)
+                  head parent head-tree)
               (unless (git-empty-db-p)
                 (setq head (git-rev-parse "HEAD")
-                      head-tree (git-rev-parse "HEAD^{tree}")))
+                      parent (if amend (git-rev-parse "HEAD^") head)
+                      head-tree (git-rev-parse (concat "HEAD^{tree}"))))
               (if files
                   (progn
                     (message "Running git commit...")
@@ -875,7 +877,7 @@ Return the list of files that haven't been handled."
                     (let ((tree (git-write-tree index-file)))
                       (if (or (not (string-equal tree head-tree))
                               (yes-or-no-p "The tree was not modified, do you really want to perform an empty commit? "))
-                          (let ((commit (git-commit-tree buffer tree head)))
+                          (let ((commit (git-commit-tree buffer tree parent head)))
                             (when commit
                               (condition-case nil (delete-file ".git/MERGE_HEAD") (error nil))
                               (condition-case nil (delete-file ".git/MERGE_MSG") (error nil))
@@ -1263,13 +1265,22 @@ Return the list of files that haven't been handled."
       (when sign-off (git-append-sign-off committer-name committer-email)))
     buffer))
 
-(defun git-commit-file ()
-  "Commit the marked file(s), asking for a commit message."
-  (interactive)
+(defun git-commit-file (&optional amend)
+  "Commit the marked file(s), asking for a commit message.
+With optional argument, amend HEAD."
+  (interactive "P")
   (unless git-status (error "Not in git-status buffer."))
+  (and amend (git-empty-db-p) (error "No commit to amend."))
   (when (git-run-pre-commit-hook)
     (let ((buffer (get-buffer-create "*git-commit*"))
           (coding-system (git-get-commits-coding-system))
+          (action (if amend
+                      `(lambda () (interactive) (git-do-commit t))
+                    'git-do-commit))
+          (env (if (boundp 'log-edit-diff-function)
+                   '((log-edit-listfun . git-log-edit-files)
+                     (log-edit-diff-function . git-log-edit-diff))
+                 'git-log-edit-files))
           author-name author-email subject date)
       (when (eq 0 (buffer-size buffer))
         (when (file-readable-p ".dotest/info")
@@ -1286,10 +1297,8 @@ Return the list of files that haven't been handled."
             (when (re-search-forward "^Date: \\(.*\\)$" nil t)
               (setq date (match-string 1)))))
         (git-setup-log-buffer buffer author-name author-email subject date))
-      (if (boundp 'log-edit-diff-function)
-	  (log-edit 'git-do-commit nil '((log-edit-listfun . git-log-edit-files)
-					 (log-edit-diff-function . git-log-edit-diff)) buffer)
-	(log-edit 'git-do-commit nil 'git-log-edit-files buffer))
+      (when amend (git-setup-commit-buffer "HEAD"))
+      (log-edit action nil env buffer)
       (setq font-lock-keywords (font-lock-compile-keywords git-log-edit-font-lock-keywords))
       (setq buffer-file-coding-system coding-system)
       (re-search-forward (regexp-quote (concat git-log-msg-separator "\n")) nil t))))
@@ -1326,19 +1335,9 @@ Return the list of files that haven't been handled."
     files))
 
 (defun git-amend-commit ()
-  "Undo the last commit on HEAD, and set things up to commit an
-amended version of it."
+  "Call `git-commit-file' and have it amend HEAD."
   (interactive)
-  (unless git-status (error "Not in git-status buffer."))
-  (when (git-empty-db-p) (error "No commit to amend."))
-  (let* ((commit (git-rev-parse "HEAD"))
-         (files (git-get-commit-files commit)))
-    (when (git-call-process-display-error "reset" "--soft" "HEAD^")
-      (git-update-status-files (copy-sequence files) 'uptodate)
-      (git-mark-files git-status files)
-      (git-refresh-files)
-      (git-setup-commit-buffer commit)
-      (git-commit-file))))
+  (git-commit-file t))
 
 (defun git-find-file ()
   "Visit the current file in its own buffer."

Re: [PATCH] git.el: Don't reset HEAD in git-amend-file.

[Alexandre Julliard]

Nikolaj Schumacher <n_schumacher@web.de> writes:

> The current implementation of git-amend-file is a little dangerous.
> While git --amend is atomic, git-amend-file is not.
>
> If the user calls it, but doesn't go through with the commit (due to
> error or choice), git --reset HEAD^ has been called anyway.

That's a feature, even if it's a little dangerous. You have to reset
HEAD to be able to properly do diffs, refreshes, etc.  Maybe there should
be an easier way to redo the commit if you change your mind, but the
command would be much less useful without the reset.

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH] git.el: Don't reset HEAD in git-amend-file.

[Nikolaj Schumacher]

Alexandre Julliard <julliard@winehq.org> wrote:

> Nikolaj Schumacher <n_schumacher@web.de> writes:
>
>> The current implementation of git-amend-file is a little dangerous.
>> While git --amend is atomic, git-amend-file is not.
>>
>> If the user calls it, but doesn't go through with the commit (due to
>> error or choice), git --reset HEAD^ has been called anyway.
>
> That's a feature, even if it's a little dangerous. You have to reset
> HEAD to be able to properly do diffs, refreshes, etc.  Maybe there should
> be an easier way to redo the commit if you change your mind, but the
> command would be much less useful without the reset.

You're right, it would be less powerful.  But I still think it maps
better to what "git commit --amend" does, i.e. do a few changes and
amend them.  No need for diffing against the old head, or even showing
it in the summary.  For doing something more complicated I would reset
head, too.  So, yes, the command should exist as well.  But I'm not
so sure about the name...

As an aside, the current method doesn't work on the initial commit.


regards,
Nikolaj Schumacher