threads / patch / 9052

patchcontrib/emacs/Makefile: Also install .el files.

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

## tl;dr

12 messages between Jul 15, 2007 and Jul 16, 2007. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

David Kastrup· Jul 16, 2007, 01:24 UTC · lore

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

(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(-)

Show changes to contrib/emacs/vc-git.el +47 −63

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

David Kastrup· Jul 15, 2007, 09:46 UTC · re: David Kastrup · lore

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

Show changes to contrib/emacs/Makefile +1 −1

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

David Kastrup· Jul 16, 2007, 01:27 UTC · re: David Kastrup · lore

Re: [PATCH] contrib/emacs...

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

Junio C Hamano· Jul 16, 2007, 03:20 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:

Show 5 quoted lines

> 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.

David Kastrup· Jul 15, 2007, 23:42 UTC · re: David Kastrup · lore

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

(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(-)

Show changes to contrib/emacs/vc-git.el +44 −32

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

David Kastrup· Jul 15, 2007, 23:53 UTC · re: David Kastrup · lore

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

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

Show changes to contrib/emacs/vc-git.el +10 −11

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

Junio C Hamano· Jul 16, 2007, 03:20 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:

Show 8 quoted lines

> (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.

Show 25 quoted lines

> 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

David Kastrup· Jul 16, 2007, 05:29 UTC · re: Junio C Hamano · lore

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

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

Show 13 quoted lines

> 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

Junio C Hamano· Jul 16, 2007, 06:14 UTC · re: David Kastrup · lore

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

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.

David Kastrup· Jul 16, 2007, 00:05 UTC · re: David Kastrup · lore

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

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(-)

Show changes to contrib/emacs/vc-git.el +1 −1

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

Junio C Hamano· Jul 16, 2007, 03:20 UTC · re: David Kastrup · lore

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

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.

David Kastrup· Jul 16, 2007, 05:26 UTC · re: Junio C Hamano · lore

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

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

Show 8 quoted lines

> 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