[PATCH] contrib/emacs/vc-git.el: various improvements.

[PATCH] contrib/emacs/vc-git.el: various improvements.

[David Kastrup]

(vc-git-symbolic-commit): Simplify and make it return
something useful in almost all cases.
(vc-git-previous-version): Simplify.
(vc-git-next-version): Simplify and make more efficient.
(vc-git-annotate-command): heed REV argument.
(vc-annotate-extract-revision-at-line): Activate.

Signed-off-by: David Kastrup <dak@gnu.org>
---
 contrib/emacs/vc-git.el |  110 ++++++++++++++++++++---------------------------
 1 files changed, 47 insertions(+), 63 deletions(-)

diff --git a/contrib/emacs/vc-git.el b/contrib/emacs/vc-git.el
index b8f6be5..3637c8a 100644
--- a/contrib/emacs/vc-git.el
+++ b/contrib/emacs/vc-git.el
@@ -84,67 +84,51 @@
 (defun vc-git-symbolic-commit (commit)
   "Translate COMMIT string into symbolic form.
 Returns nil if not possible."
-  (and commit
-       (with-temp-buffer
-	 (and
-	  (zerop
-	   (call-process "git" nil '(t nil) nil "name-rev"
-			 "--name-only" "--tags"
-			 commit))
-	  (goto-char (point-min))
-	  (= (forward-line 2) 1)
-	  (bolp)
-	  (buffer-substring-no-properties (point-min) (1- (point-max)))))))
+  (with-temp-buffer
+    (and
+     (zerop
+      (call-process "git" nil '(t nil) nil "name-rev"
+		    commit))
+     (goto-char (point-max))
+     (bolp)
+     (zerop (forward-line -1))
+     (bobp)
+     (progn
+       (search-forward " " nil t)
+       (not (eolp)))
+     (buffer-substring-no-properties (point) (1- (point-max))))))
 
 (defun vc-git-previous-version (file rev)
   "git-specific version of `vc-previous-version'."
-  (let ((default-directory (file-name-directory (expand-file-name file)))
-	(file (file-name-nondirectory file)))
-    (vc-git-symbolic-commit
-     (with-temp-buffer
-       (and
-	(zerop
-	 (call-process "git" nil '(t nil) nil "rev-list"
-		       "-2" rev "--" file))
-	(goto-char (point-max))
-	(bolp)
-	(zerop (forward-line -1))
-	(not (bobp))
-	(buffer-substring-no-properties
-	   (point)
-	   (1- (point-max))))))))
+  (vc-git-symbolic-commit
+   (with-temp-buffer
+     (and
+      (zerop
+       (call-process "git" nil '(t nil) nil "rev-list" "--abbrev"
+		     "--abbrev-commit" "-2" rev "--" (file-relative-name file)))
+      (goto-char (point-max))
+      (bolp)
+      (zerop (forward-line -1))
+      (not (bobp))
+      (buffer-substring-no-properties
+       (point)
+       (1- (point-max)))))))
 
 (defun vc-git-next-version (file rev)
   "git-specific version of `vc-next-version'."
-  (let* ((default-directory (file-name-directory
-			     (expand-file-name file)))
-	(file (file-name-nondirectory file))
-	(current-rev
-	 (with-temp-buffer
-	   (and
-	    (zerop
-	     (call-process "git" nil '(t nil) nil "rev-list"
-			   "-1" rev "--" file))
-	    (goto-char (point-max))
-	    (bolp)
-	    (zerop (forward-line -1))
-	    (bobp)
-	    (buffer-substring-no-properties
-	     (point)
-	     (1- (point-max)))))))
-    (and current-rev
-	 (vc-git-symbolic-commit
-	  (with-temp-buffer
-	    (and
-	     (zerop
-	      (call-process "git" nil '(t nil) nil "rev-list"
-			    "HEAD" "--" file))
-	     (goto-char (point-min))
-	     (search-forward current-rev nil t)
-	     (zerop (forward-line -1))
-	     (buffer-substring-no-properties
-	      (point)
-	      (progn (forward-line 1) (1- (point))))))))))
+  (vc-git-symbolic-commit
+   (with-temp-buffer
+     (and
+      (zerop
+       (call-process "git" nil '(t nil) nil "rev-list" "--abbrev"
+		     "--abbrev-commit"
+		     "HEAD" "--not" rev "--" (file-relative-name file)))
+      (goto-char (point-max))
+      (bolp)
+      (zerop (forward-line -1))
+      (buffer-substring-no-properties
+       (point)
+       (1- (point-max)))))))
 
 (defun vc-git-revert (file &optional contents-done)
   "Revert FILE to the version stored in the git repository."
@@ -197,20 +181,20 @@ Returns nil if not possible."
     (vc-git--run-command file "checkout" (or rev "HEAD"))))
 
 (defun vc-git-annotate-command (file buf &optional rev)
-  ; FIXME: rev is ignored
   (let ((name (file-relative-name file)))
-    (call-process "git" nil buf nil "blame" name)))
+    (if rev
+	(call-process "git" nil buf nil "blame" rev "--" name)
+      (call-process "git" nil buf nil "blame" "--" name))))
 
 (defun vc-git-annotate-time ()
   (and (re-search-forward "[0-9a-f]+ (.* \\([0-9]+\\)-\\([0-9]+\\)-\\([0-9]+\\) \\([0-9]+\\):\\([0-9]+\\):\\([0-9]+\\) \\([-+0-9]+\\) +[0-9]+)" nil t)
        (vc-annotate-convert-time
         (apply #'encode-time (mapcar (lambda (match) (string-to-number (match-string match))) '(6 5 4 3 2 1 7))))))
 
-;; Not really useful since we can't do anything with the revision yet
-;;(defun vc-annotate-extract-revision-at-line ()
-;;  (save-excursion
-;;    (move-beginning-of-line 1)
-;;    (and (looking-at "[0-9a-f]+")
-;;         (buffer-substring (match-beginning 0) (match-end 0)))))
+(defun vc-annotate-extract-revision-at-line ()
+  (save-excursion
+    (move-beginning-of-line 1)
+    (and (looking-at "[0-9a-f]+")
+         (buffer-substring (match-beginning 0) (match-end 0)))))
 
 (provide 'vc-git)
-- 
1.4.4.2

[PATCH] contrib/emacs/Makefile: Also install .el files.

[David Kastrup]

Signed-off-by: David Kastrup <dak@gnu.org>
---
 contrib/emacs/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/emacs/Makefile b/contrib/emacs/Makefile
index 98aa0aa..92033f0 100644
--- a/contrib/emacs/Makefile
+++ b/contrib/emacs/Makefile
@@ -12,7 +12,7 @@ all: $(ELC)
 
 install: all
 	$(INSTALL) -d $(DESTDIR)$(emacsdir)
-	$(INSTALL_ELC) $(ELC) $(DESTDIR)$(emacsdir)
+	$(INSTALL_ELC) $(ELC:.elc=.el) $(ELC) $(DESTDIR)$(emacsdir)
 
 %.elc: %.el
 	$(EMACS) -batch -f batch-byte-compile $<
-- 
1.4.4.2

Re: [PATCH] contrib/emacs...

[David Kastrup]

I apologize for the somewhat messy state of the patches and commit
messages: I am trying to get a hang of the tools right now.

If people have suggestions how to read up on cleaning up and
reordering commits before submission, I'd be greatful.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] contrib/emacs/Makefile: Also install .el files.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Signed-off-by: David Kastrup <dak@gnu.org>
>  install: all
>  	$(INSTALL) -d $(DESTDIR)$(emacsdir)
> -	$(INSTALL_ELC) $(ELC) $(DESTDIR)$(emacsdir)
> +	$(INSTALL_ELC) $(ELC:.elc=.el) $(ELC) $(DESTDIR)$(emacsdir)

Ok, thanks.

[PATCH] Make several improvements and get annotations to work (Emacs support pending).

[David Kastrup]

(vc-git-symbolic-commit): Allow nil to pass through.
(vc-git-previous-version): Use explicit parent argument.
(vc-git-next-version): Simplify.
(vc-git-annotate-command): Use `vc-do-command'.
(vc-git-annotate-extract-revision-at-line): Rename from
`vc-annotate-extract-revision-at-line'.

Signed-off-by: David Kastrup <dak@gnu.org>
---
 contrib/emacs/vc-git.el |   76 +++++++++++++++++++++++++++--------------------
 1 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/contrib/emacs/vc-git.el b/contrib/emacs/vc-git.el
index 3637c8a..2a0a0fe 100644
--- a/contrib/emacs/vc-git.el
+++ b/contrib/emacs/vc-git.el
@@ -84,19 +84,20 @@
 (defun vc-git-symbolic-commit (commit)
   "Translate COMMIT string into symbolic form.
 Returns nil if not possible."
-  (with-temp-buffer
-    (and
-     (zerop
-      (call-process "git" nil '(t nil) nil "name-rev"
-		    commit))
-     (goto-char (point-max))
-     (bolp)
-     (zerop (forward-line -1))
-     (bobp)
-     (progn
-       (search-forward " " nil t)
-       (not (eolp)))
-     (buffer-substring-no-properties (point) (1- (point-max))))))
+  (and commit
+       (with-temp-buffer
+	 (and
+	  (zerop
+	   (call-process "git" nil '(t nil) nil "name-rev"
+			 commit))
+	  (goto-char (point-max))
+	  (bolp)
+	  (zerop (forward-line -1))
+	  (bobp)
+	  (progn
+	    (search-forward " " nil t)
+	    (not (eolp)))
+	  (buffer-substring-no-properties (point) (1- (point-max)))))))
 
 (defun vc-git-previous-version (file rev)
   "git-specific version of `vc-previous-version'."
@@ -104,15 +105,13 @@ Returns nil if not possible."
    (with-temp-buffer
      (and
       (zerop
-       (call-process "git" nil '(t nil) nil "rev-list" "--abbrev"
-		     "--abbrev-commit" "-2" rev "--" (file-relative-name file)))
-      (goto-char (point-max))
-      (bolp)
-      (zerop (forward-line -1))
-      (not (bobp))
-      (buffer-substring-no-properties
-       (point)
-       (1- (point-max)))))))
+       (call-process "git" nil '(t nil) nil "rev-list"
+		     "--abbrev"
+		     "--abbrev-commit" "-1" "--parents" rev "--"
+		     (file-relative-name file)))
+      (goto-char (point-min))
+      (search-forward-regexp " \\([^ \n]*\\)" nil t)
+      (match-string 1)))))
 
 (defun vc-git-next-version (file rev)
   "git-specific version of `vc-next-version'."
@@ -121,8 +120,9 @@ Returns nil if not possible."
      (and
       (zerop
        (call-process "git" nil '(t nil) nil "rev-list" "--abbrev"
-		     "--abbrev-commit"
+		     "--abbrev-commit"  "--remove-empty"
 		     "HEAD" "--not" rev "--" (file-relative-name file)))
+      (goto-char (point-min))
       (goto-char (point-max))
       (bolp)
       (zerop (forward-line -1))
@@ -174,24 +174,36 @@ Returns nil if not possible."
                        (vc-git--run-command-string file "ls-files" "-z" "--full-name" "--")
                        0 -1))
             (coding-system-for-read 'no-conversion)
-            (coding-system-for-write 'no-conversion))
+            coding-system-for-write)
         (with-temp-file destfile
-          (eq 0 (call-process "git" nil t nil "cat-file" "blob"
-                              (concat (or rev "HEAD") ":" fullname)))))
+	  (prog1
+	      (eq 0 (call-process "git" nil t nil "cat-file" "blob"
+				  (concat (or rev "HEAD") ":" fullname)))
+	    (setq coding-system-for-write 'no-conversion))))
     (vc-git--run-command file "checkout" (or rev "HEAD"))))
 
-(defun vc-git-annotate-command (file buf &optional rev)
-  (let ((name (file-relative-name file)))
-    (if rev
-	(call-process "git" nil buf nil "blame" rev "--" name)
-      (call-process "git" nil buf nil "blame" "--" name))))
+(defun vc-git-annotate-command (file buffer &optional version)
+  "Execute \"git blame\" on FILE, inserting the contents in BUFFER.
+Optional arg VERSION is a version to annotate from."
+  (vc-do-command buffer
+		 'async
+		 "git" file "blame"
+		 (or version "HEAD")
+		 "--"))
+
+;;(defun vc-git-annotate-command (file buf &optional rev)
+;;  (let ((name (file-relative-name file)))
+;;    (vc-setup-buffer buf)
+;;    (if rev
+;;	(call-process "git" nil buf nil "blame" rev "--" name)
+;;      (call-process "git" nil buf nil "blame" "--" name))))
 
 (defun vc-git-annotate-time ()
   (and (re-search-forward "[0-9a-f]+ (.* \\([0-9]+\\)-\\([0-9]+\\)-\\([0-9]+\\) \\([0-9]+\\):\\([0-9]+\\):\\([0-9]+\\) \\([-+0-9]+\\) +[0-9]+)" nil t)
        (vc-annotate-convert-time
         (apply #'encode-time (mapcar (lambda (match) (string-to-number (match-string match))) '(6 5 4 3 2 1 7))))))
 
-(defun vc-annotate-extract-revision-at-line ()
+(defun vc-git-annotate-extract-revision-at-line ()
   (save-excursion
     (move-beginning-of-line 1)
     (and (looking-at "[0-9a-f]+")
-- 
1.4.4.2

[PATCH] vc-git: support asynchronous annotations, and improve versioning.

[David Kastrup]

(vc-git-symbolic-commit): Allow nil to pass through.
(vc-git-previous-version): Use explicit parent argument.
(vc-git-next-version): Simplify.
(vc-git-annotate-command): Use `vc-do-command'.
(vc-git-annotate-extract-revision-at-line): Rename from
`vc-annotate-extract-revision-at-line'.
(vc-git-checkout): Make nicer way of ensuring encoding.

Signed-off-by: David Kastrup <dak@gnu.org>
---
 contrib/emacs/vc-git.el |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/contrib/emacs/vc-git.el b/contrib/emacs/vc-git.el
index 2a0a0fe..d7ad314 100644
--- a/contrib/emacs/vc-git.el
+++ b/contrib/emacs/vc-git.el
@@ -173,23 +173,22 @@ Returns nil if not possible."
       (let ((fullname (substring
                        (vc-git--run-command-string file "ls-files" "-z" "--full-name" "--")
                        0 -1))
-            (coding-system-for-read 'no-conversion)
-            coding-system-for-write)
+            (coding-system-for-read 'no-conversion))
         (with-temp-file destfile
-	  (prog1
-	      (eq 0 (call-process "git" nil t nil "cat-file" "blob"
-				  (concat (or rev "HEAD") ":" fullname)))
-	    (setq coding-system-for-write 'no-conversion))))
+	  (setq buffer-file-coding-system 'no-conversion)
+	  (eq 0 (call-process "git" nil t nil "cat-file" "blob"
+			      (concat (or rev "HEAD") ":" fullname)))))
     (vc-git--run-command file "checkout" (or rev "HEAD"))))
 
 (defun vc-git-annotate-command (file buffer &optional version)
   "Execute \"git blame\" on FILE, inserting the contents in BUFFER.
 Optional arg VERSION is a version to annotate from."
-  (vc-do-command buffer
-		 'async
-		 "git" file "blame"
-		 (or version "HEAD")
-		 "--"))
+  (let ((coding-system-for-read git-commits-coding-system))
+    (vc-do-command buffer
+		   'async
+		   "git" file "blame"
+		   (or version "HEAD")
+		   "--")))
 
 ;;(defun vc-git-annotate-command (file buf &optional rev)
 ;;  (let ((name (file-relative-name file)))
-- 
1.4.4.2

Re: [PATCH] vc-git: support asynchronous annotations, and improve versioning.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> (vc-git-symbolic-commit): Allow nil to pass through.
> (vc-git-previous-version): Use explicit parent argument.
> (vc-git-next-version): Simplify.
> (vc-git-annotate-command): Use `vc-do-command'.
> (vc-git-annotate-extract-revision-at-line): Rename from
> `vc-annotate-extract-revision-at-line'.
> (vc-git-checkout): Make nicer way of ensuring encoding.
>

These do not seem to match what the patch does at all.
I give up.

Will apply only the "install .el, too" change from the confusing
series for now.

> diff --git a/contrib/emacs/vc-git.el b/contrib/emacs/vc-git.el
> index 2a0a0fe..d7ad314 100644
> --- a/contrib/emacs/vc-git.el
> +++ b/contrib/emacs/vc-git.el
> @@ -173,23 +173,22 @@ Returns nil if not possible."
> ...
>  (defun vc-git-annotate-command (file buffer &optional version)
>    "Execute \"git blame\" on FILE, inserting the contents in BUFFER.
>  Optional arg VERSION is a version to annotate from."
> -  (vc-do-command buffer
> -		 'async
> -		 "git" file "blame"
> -		 (or version "HEAD")
> -		 "--"))
> +  (let ((coding-system-for-read git-commits-coding-system))
> +    (vc-do-command buffer
> +		   'async
> +		   "git" file "blame"
> +		   (or version "HEAD")
> +		   "--")))
>  
>  ;;(defun vc-git-annotate-command (file buf &optional rev)
>  ;;  (let ((name (file-relative-name file)))
> -- 
> 1.4.4.2

Re: [PATCH] vc-git: support asynchronous annotations, and improve versioning.

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> (vc-git-symbolic-commit): Allow nil to pass through.
>> (vc-git-previous-version): Use explicit parent argument.
>> (vc-git-next-version): Simplify.
>> (vc-git-annotate-command): Use `vc-do-command'.
>> (vc-git-annotate-extract-revision-at-line): Rename from
>> `vc-annotate-extract-revision-at-line'.
>> (vc-git-checkout): Make nicer way of ensuring encoding.
>>
>
> These do not seem to match what the patch does at all.
> I give up.

The previous changelog entry crept in as well.

> Will apply only the "install .el, too" change from the confusing
> series for now.

Fair enough.

How do I go about resubmitting with better comments?  Make an extra
branch and redo the part in artificial new commits?

I have no experience.  Any good pointers how one does this sort of
propose, rewind, propose stuff most conveniently?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] vc-git: support asynchronous annotations, and improve versioning.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> How do I go about resubmitting with better comments?  Make an extra
> branch and redo the part in artificial new commits?

The original was not even numbered so I cannot judge how the
development proceeded, but I think the series is somewhat ill
organized and the problem is not just commit log messages.

For example, "vc-git.el: various improvements." removes NIL
check for 'commit' and then later "Make several improvements"
reinstates that check, perhaps because you realized the earlier
change to remove it was a mistake (without the explanation in
the log I can only guess the reason).  Same for 'async to 0
change which is made by "Make synchronous for now".

Preserving a honest, true-to-reality, history is one thing
during the development, but when the overall improvements and
enhancements reached a usable state, it is much more pleasant to
review if the series is presented as if you knew what necessary
changes are and what pitfalls to avoid from the beginning, and
did your development in logical sequence, building one commit on
top of the previous enhancements without "oops, that was a
mistake so fix it while we are improving something else".

My cursory review suggests that:

* contrib/emacs/Makefile: Also install .el files.

This is a good patch, independent from anything else.

* contrib/emacs/vc-git.el: various improvements.
* Make several improvements and get annotations to work (Emacs support pending).

These two try to improve the same area, but the latter contains
bugfixes for the former, not necessarily a logical succession
(if it were a logical sequence to build and enhance, the first
should be usable by itself, without a later bugfix).

If these were to be made more than one logically independent
patches, then probably the first would be to simplify
vc-git-symbolic-commit implementation and update its callers
(without having to say "oops, I needed the check for NIL commit
after all" later), and without changing what the callers do.
The second one would be "prev and next can be made much
simpler".  The third one would be "annotate can take 'rev' and
extract-revision-at-line is now usable".  But judging from how
closely these changes are tied together (all of them depend on
the external interface to vc-git-symbolic-commit) and how small
each change is, I would probably make them a single commit that
you do not have to say "oops" anywhere.

* vc-git: support asynchronous annotations, and improve versioning.
* (vc-git-annotate-command):  Make synchronous for now.

The same discussion here.  The first one goes in the right
direction, but then the moment of "oops" comes --- async does
not necessarily work.  Probably the right separation, if these
two were to become two separate commits, would be to add async
(but leave it synchronous, with a big code comment at the
calling site of vc-do-command why you pass 0 instead of async),
and then make "and improve versioning" part (whatever that is --
I cannot figure out which part of the patch "improve"-ment
refers to) a separate commit.

I would end this message with a very useful URL:

    http://article.gmane.org/gmane.comp.version-control.git/10894

I am with Linus 100%, regarding "honest" vs "disgusting"
history.

[PATCH] (vc-git-annotate-command): Make synchronous for now.

[David Kastrup]

We need a test to figure out whether Emacs supports asynchronous
annotations (not yet available in CVS).

Signed-off-by: David Kastrup <dak@gnu.org>
---
 contrib/emacs/vc-git.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/emacs/vc-git.el b/contrib/emacs/vc-git.el
index d7ad314..c3f30ad 100644
--- a/contrib/emacs/vc-git.el
+++ b/contrib/emacs/vc-git.el
@@ -185,7 +185,7 @@ Returns nil if not possible."
 Optional arg VERSION is a version to annotate from."
   (let ((coding-system-for-read git-commits-coding-system))
     (vc-do-command buffer
-		   'async
+		   0
 		   "git" file "blame"
 		   (or version "HEAD")
 		   "--")))
-- 
1.4.4.2

Re: [PATCH] contrib/emacs/vc-git.el: various improvements.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> (vc-git-symbolic-commit): Simplify and make it return
> something useful in almost all cases.

It would be easier to review and understand if "something
useful" were more specific.

It appears that the previous version of this function tried not
to barf if it got nil or false as input but now it does not seem
to worry about that case.  Also the option given to name-rev is
different which would lead to different behaviour -- arguably a
better one, but that needs to be documented in the log message.

> (vc-git-previous-version): Simplify.
> (vc-git-next-version): Simplify and make more efficient.

If you make the result symbolic using vc-git-symbolic-commit, do
you need to add --abbrev/--abbrev-commit to these functions?
These options have very small but still non zero cost.

> (vc-git-annotate-command): heed REV argument.
> (vc-annotate-extract-revision-at-line): Activate.

Ok.

Re: [PATCH] contrib/emacs/vc-git.el: various improvements.

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> (vc-git-previous-version): Simplify.
>> (vc-git-next-version): Simplify and make more efficient.
>
> If you make the result symbolic using vc-git-symbolic-commit, do you
> need to add --abbrev/--abbrev-commit to these functions?  These
> options have very small but still non zero cost.

vc-git-symbolic-commit may not always find a name (at least it did not
while I still used --tags).  The abbrev provides a fallback in that
case.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum