git push/pull [was: support for bzr shelve/unshelve in vc-dir]

git push/pull [was: support for bzr shelve/unshelve in vc-dir]

[Fabian Ezequiel Gallina]

I love vc, the thing is I always wanted the hability to do git
push/pull from vc directly. So today I decided to implement it.

Below is the code, I sure there is a lot of room for improvement since
I consider an Emacs LISP newbie myself, but I guess is a good starting
point for a better implementation.

As you might deduce from the code, when calling vc-git-push and
vc-git-pull the user will be prompted for a repository and a refspec,
both prompts feature sane default values (I hope you will agree on
that) and nice minibuffer-completion.


(defvar vc-git-push-pull-perform 'pull
  "The git command `vc-git--do-push-pull' should execute")

(defun vc-git--do-push-pull (&optional repository refspec)
  "Calls git push/pull with REPOSITORY and REFSPEC as parameters.

The decision of using push or pull is beign made depending the
value of `vc-git-push-pull-perform'."
  (interactive
   (let ((default-refspec (vc-git-working-revision buffer-file-name))
         (default-repo)
         (available-repos)
         (available-refspecs)
         (current-repo)
         (current-refspec))
     (setq available-repos
           (let ((table (list ".")))
             (with-temp-buffer
               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
               (goto-char (point-min))
               (while (re-search-forward "^refs/remotes/\\([^/]+\\).*$" nil t)
                 (push (match-string 1) table)))
             table))
     (setq default-repo (nth 0 available-repos))
     (setq current-repo
           (completing-read (format "Repository (default %s): " default-repo)
                            available-repos nil 'confirm nil nil default-repo))
     (setq available-refspecs
           (let ((table (list "HEAD"))
                 (ref-regexp))
             (with-temp-buffer
               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
               (if (equal current-repo ".")
                   (setq ref-regexp
                         "^refs/\\(remotes\\|heads\\|tags\\)/\\([^\n]*\\)$")
                 (setq ref-regexp (concat "^refs/\\(remotes\\)/"
                                          (regexp-quote current-repo)
                                          "/\\([^\n]+\\).*$")))
               (goto-char (point-min))
               (while (re-search-forward
                       ref-regexp
                       nil t)
                 (push (match-string 2) table)))
             table))
     (setq current-refspec
           (completing-read (format "Branch (default %s): " default-refspec)
                            available-refspecs nil 'confirm nil nil
                            default-refspec))
     (list current-repo current-refspec)))
  (let ((buffer (current-buffer))
        (command-buffer-name "*vc-push-pull*"))
    (vc-git-command
     command-buffer-name 0 nil
     (prin1-to-string vc-git-push-pull-perform) repository refspec)
    (set-buffer command-buffer-name)
    (message (buffer-string))
    (set-buffer buffer)))

(defun vc-git-push (&optional repository refspec)
  "Update remote refs along with associated objects."
  (interactive)
  (setq vc-git-push-pull-perform 'push)
  (call-interactively 'vc-git--do-push-pull))

(defun vc-git-pull (&optional repository refspec)
  "Fetch from and merge with another repository or a local branch."
  (interactive)
  (setq vc-git-push-pull-perform 'pull)
  (call-interactively 'vc-git--do-push-pull))


Now, if this code is good enough. Can it be applied to vc-git.el? If
not can somebody provide a better alternative?

Also it will be really nice to define a menu/keybinding for these two
operations.



Best Regards,
-- 
Fabián E. Gallina
http://www.from-the-cloud.com

Re: git push/pull [was: support for bzr shelve/unshelve in vc-dir]

[Andreas Schwab]

Fabian Ezequiel Gallina <galli.87@gmail.com> writes:

>      (setq available-repos
>            (let ((table (list ".")))
>              (with-temp-buffer
>                (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
>                (goto-char (point-min))
>                (while (re-search-forward "^refs/remotes/\\([^/]+\\).*$" nil t)
>                  (push (match-string 1) table)))

A remote's name can contain / itself, and this would also miss remotes
that where never fetched from.  You can use git config to list the
defined remotes, see __git_remotes in /etc/bash_completion.d/git.sh.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: git push/pull

[Stefan Monnier]

> I love vc, the thing is I always wanted the hability to do git
> push/pull from vc directly. So today I decided to implement it.

That's great, thank you.

> Below is the code, I sure there is a lot of room for improvement since
> I consider an Emacs LISP newbie myself, but I guess is a good starting
> point for a better implementation.

I'll let git users decide whether they like your specific choices, but
what I'd want on my side is to add support for it directly in vc.el
(i.e. have `vc-push' and `vc-pull' commands which then delegate the
core of the work to the backends.  You don't have to implement any
other backend than Git).


        Stefan

Re: git push/pull

[Fabian Ezequiel Gallina]

2009/12/5 Stefan Monnier <monnier@iro.umontreal.ca>:
>> I love vc, the thing is I always wanted the hability to do git
>> push/pull from vc directly. So today I decided to implement it.
>
> That's great, thank you.
>
>> Below is the code, I sure there is a lot of room for improvement since
>> I consider an Emacs LISP newbie myself, but I guess is a good starting
>> point for a better implementation.
>
> I'll let git users decide whether they like your specific choices, but
> what I'd want on my side is to add support for it directly in vc.el
> (i.e. have `vc-push' and `vc-pull' commands which then delegate the
> core of the work to the backends.  You don't have to implement any
> other backend than Git).
>

I understand, while looking at the code of vc.el I found "deal with
push/pull operations" inside the TODO. I just added vc-git-pull and
vc-git-push based on the fact I saw vc-hg-pull and vc-hg-push already
implemented in vc-hg.el.

I'll study a little how vc works and see if I can implement a generic
vc-pull/push if nobody else can work on it.



Best Regards,
-- 
Fabián E. Gallina
http://www.from-the-cloud.com

Re: git push/pull

[Fabian Ezequiel Gallina]

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

2009/12/5 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Below is the code, I sure there is a lot of room for improvement since
>> I consider an Emacs LISP newbie myself, but I guess is a good starting
>> point for a better implementation.
>
> I'll let git users decide whether they like your specific choices, but
> what I'd want on my side is to add support for it directly in vc.el
> (i.e. have `vc-push' and `vc-pull' commands which then delegate the
> core of the work to the backends.  You don't have to implement any
> other backend than Git).
>

Stefan, two diffs are attached in this reply.

The first adds vc-pull and vc-push to vc.el. I'm not sure if I'm
missing something in their definitions since I looked at the vc
package very quickly.

The second diff adds vc-git-push and vc-git-pull to vc-git.el. I
modified the how the completions are computed so they render the same
as when you try the git autocompletion from the terminal.

I played around a bit with these changes and seems to work OK.


Regards,
-- 
Fabián E. Gallina
http://www.from-the-cloud.com

[-- Attachment #2: vc.el.diff --]
[-- Type: text/plain, Size: 1603 bytes --]

diff --git a/lisp/vc.el b/lisp/vc.el
index cd9f11b..5519ff6 100644
--- a/lisp/vc.el
+++ b/lisp/vc.el
@@ -884,7 +884,7 @@ Within directories, only files already under version control are noticed."
 				    state-model-only-files)
   "Deduce a set of files and a backend to which to apply an operation.
 
-Return (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL).
+Return (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL.
 If we're in VC-dir mode, the fileset is the list of marked files.
 Otherwise, if we're looking at a buffer visiting a version-controlled file,
 the fileset is a singleton containing this file.
@@ -1185,6 +1185,26 @@ merge in the changes into your working copy."
 (declare-function vc-dir-move-to-goal-column "vc-dir" ())
 
 ;;;###autoload
+(defun vc-pull ()
+  "Merges changes between branches."
+  (interactive)
+  (vc-ensure-vc-buffer)
+  (let ((backend (vc-backend buffer-file-name)))
+    (if (not (vc-find-backend-function backend 'pull))
+        (error "Sorry, pull is not implemented for %s" backend)
+      (vc-call-backend backend 'pull))))
+
+;;;###autoload
+(defun vc-push ()
+  "Pushes commits to some branch."
+  (interactive)
+  (vc-ensure-vc-buffer)
+  (let ((backend (vc-backend buffer-file-name)))
+    (if (not (vc-find-backend-function backend 'push))
+        (error "Sorry, push is not implemented for %s" backend)
+      (vc-call-backend backend 'push))))
+
+;;;###autoload
 (defun vc-register (&optional set-revision vc-fileset comment)
   "Register into a version control system.
 If VC-FILESET is given, register the files in that fileset.

[-- Attachment #3: vc-git.el.diff --]
[-- Type: text/plain, Size: 3494 bytes --]

diff --git a/lisp/vc-git.el b/lisp/vc-git.el
index a33a0f5..bb95b70 100644
--- a/lisp/vc-git.el
+++ b/lisp/vc-git.el
@@ -489,6 +489,80 @@ If nil, use the value of `vc-diff-switches'.  If t, use no switches."
 
 ;;; STATE-CHANGING FUNCTIONS
 
+(defvar vc-git-push-pull-perform 'pull
+  "The git command `vc-git--do-push-pull' should execute")
+
+(defun vc-git--do-push-pull (repository refspec)
+  "Calls git push/pull with REPOSITORY and REFSPEC as parameters.
+
+The decision of using push or pull is beign made depending the
+value of `vc-git-push-pull-perform'."
+  (interactive
+   (let ((default-refspec (vc-git-working-revision buffer-file-name))
+         (default-repo)
+         (available-repos)
+         (available-refspecs)
+         (current-repo)
+         (current-refspec))
+     (setq available-repos
+           (let ((table (list ".")))
+             (with-temp-buffer
+               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
+               (goto-char (point-min))
+               (while (re-search-forward "^refs/remotes/\\([^/]+\\).*$" nil t)
+                 (push (match-string 1) table)))
+             table))
+     (setq default-repo (nth 0 available-repos))
+     (setq current-repo
+           (completing-read (format "Repository (default %s): " default-repo)
+                            available-repos nil 'confirm nil nil default-repo))
+     (setq available-refspecs
+           (let ((table (list "HEAD"))
+                 (ref-regexp))
+             (with-temp-buffer
+               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
+               (if (and (not (equal "." current-repo))
+                        (equal vc-git-push-pull-perform 'pull))
+                   (setq ref-regexp (concat
+                                     "^refs/\\(remotes/"
+                                     (regexp-quote current-repo)
+                                     "\\|tags\\)/\\([^\n]+\\).*$"))
+                 (setq ref-regexp (concat
+                                   "^refs/\\(remotes\\|heads\\|tags\\)/\\("
+                                   (regexp-quote current-repo)
+                                   "/[^\n]+\\|[^\n]+\\).*$")))
+               (goto-char (point-min))
+               (while (re-search-forward
+                       ref-regexp
+                       nil t)
+                 (push (match-string 2) table)))
+             table))
+     (setq current-refspec
+           (completing-read (format "Branch (default %s): " default-refspec)
+                            available-refspecs nil 'confirm nil nil
+                            default-refspec))
+     (list current-repo current-refspec)))
+  (let ((buffer (current-buffer))
+        (command-buffer-name "*vc-push-pull*"))
+    (vc-git-command
+     command-buffer-name 0 nil
+     (prin1-to-string vc-git-push-pull-perform) repository refspec)
+    (set-buffer command-buffer-name)
+    (message (buffer-string))
+    (set-buffer buffer)))
+
+(defun vc-git-push ()
+  "Update remote refs along with associated objects."
+  (interactive)
+  (setq vc-git-push-pull-perform 'push)
+  (call-interactively 'vc-git--do-push-pull))
+
+(defun vc-git-pull ()
+  "Fetch from and merge with another repository or a local branch."
+  (interactive)
+  (setq vc-git-push-pull-perform 'pull)
+  (call-interactively 'vc-git--do-push-pull))
+
 (defun vc-git-create-repo ()
   "Create a new Git repository."
   (vc-git-command nil 0 nil "init"))

Re: git push/pull

[Andreas Schwab]

Fabian Ezequiel Gallina <galli.87@gmail.com> writes:

> +     (setq available-repos
> +           (let ((table (list ".")))
> +             (with-temp-buffer
> +               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
> +               (goto-char (point-min))
> +               (while (re-search-forward "^refs/remotes/\\([^/]+\\).*$" nil t)
> +                 (push (match-string 1) table)))
> +             table))

This is still wrong.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: git push/pull

[Fabian Ezequiel Gallina]

2009/12/6 Andreas Schwab <schwab@linux-m68k.org>:
> Fabian Ezequiel Gallina <galli.87@gmail.com> writes:
>
>> +     (setq available-repos
>> +           (let ((table (list ".")))
>> +             (with-temp-buffer
>> +               (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
>> +               (goto-char (point-min))
>> +               (while (re-search-forward "^refs/remotes/\\([^/]+\\).*$" nil t)
>> +                 (push (match-string 1) table)))
>> +             table))
>
> This is still wrong.
>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."
>

Andreas, I'm not sure about that.

That completion part is for the <repository> argument in a basic git
push/pull command. AFAIK repository could be remote name and a remote
name cannot contain "/"[0], this is different on the refspec part
which can be have the form <remote>/<branch>.

After I saw you previous email stating this was wrong I took a deeper
look at the completion and at the moment the completions you have
available in the minibuffer are the same you would have in the
terminal. Anyways with this I'm not saying it cannot be improved.


[0] http://repo.or.cz/w/git.git/blob/master:/remote.c#l671


Regards,
-- 
Fabián E. Gallina
http://www.from-the-cloud.com

Re: git push/pull

[Andreas Schwab]

Fabian Ezequiel Gallina <galli.87@gmail.com> writes:

> That completion part is for the <repository> argument in a basic git
> push/pull command. AFAIK repository could be remote name and a remote
> name cannot contain "/"[0],

git remote add foo/bar works fine here.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: git push/pull

[Stefan Monnier]

> Stefan, two diffs are attached in this reply.

Thanks.  See comments below.

>  ;;;###autoload
> +(defun vc-pull ()
> +  "Merges changes between branches."
> +  (interactive)
> +  (vc-ensure-vc-buffer)
> +  (let ((backend (vc-backend buffer-file-name)))
> +    (if (not (vc-find-backend-function backend 'pull))
> +        (error "Sorry, pull is not implemented for %s" backend)
> +      (vc-call-backend backend 'pull))))

Please don't (vc-find-backend-function backend 'pull) just to signal
such an error:  vc-call-backend should do it for you already.
More importantly, this has a major shortcoming: it lets the backend
handle all the updating of the vc-dir.  It should instead call the
backend operation in a similar way as what is done with dir-status
(i.e. passing a callback function that's called once per file/directory
that's changed by the pull).

> +(defun vc-git-push ()
> +  "Update remote refs along with associated objects."
> +  (interactive)
> +  (setq vc-git-push-pull-perform 'push)
> +  (call-interactively 'vc-git--do-push-pull))

Don't use call-interactively here.  And don't pass arguments implicitly
via global vars.  I.e. this should really be

> +(defun vc-git-push ()
> +  "Update remote refs along with associated objects."
> +  (vc-git--do-push-pull 'push))

Another thing: `setq' is bad.  It's often useful, but better avoid it
when it's not needed.  So rather than

  (let (var1 var2 var3)
    (setq var2 foo2)
    (setq var1 foo1)
    (setq var3 foo3)
    ...)

do

  (let ((var2 foo2)
        (var1 foo1)
        (var3 foo3))
    ...)

Oh, one more: eliminate common-subsexpressions, e.g.:

        (if (and (not (equal "." current-repo))
                 (equal vc-git-push-pull-perform 'pull))
            (setq ref-regexp (concat
                              "^refs/\\(remotes/"
                              (regexp-quote current-repo)
                              "\\|tags\\)/\\([^\n]+\\).*$"))
          (setq ref-regexp (concat
                            "^refs/\\(remotes\\|heads\\|tags\\)/\\("
                            (regexp-quote current-repo)
                            "/[^\n]+\\|[^\n]+\\).*$")))
=>
        (setq ref-regexp (if (and (not (equal "." current-repo))
                                  (equal vc-git-push-pull-perform 'pull))
                             (concat
                              "^refs/\\(remotes/"
                              (regexp-quote current-repo)
                              "\\|tags\\)/\\([^\n]+\\).*$")
                           (concat
                            "^refs/\\(remotes\\|heads\\|tags\\)/\\("
                            (regexp-quote current-repo)
                            "/[^\n]+\\|[^\n]+\\).*$")))

This change also happens to make it possible to fold this `setq'
directly into the `let' where ref-regexp is declared.

Of course, it could be CSE'd further using `format':

        (setq ref-regexp
              (format (if (and (not (equal "." current-repo))
                               (equal vc-git-push-pull-perform 'pull))
                          "^refs/\\(remotes/%s\\|tags\\)/\\([^\n]+\\).*$"
                        "^refs/\\(remotes\\|heads\\|tags\\)/\\(%s/[^\n]+\\|[^\n]+\\).*$")
                      (regexp-quote current-repo)))


-- Stefan

Re: git push/pull

[Fabian Ezequiel Gallina]

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

2009/12/8 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Stefan, two diffs are attached in this reply.
>
> Thanks.  See comments below.
>

Hi Stefan, thanks for your feedback.

Attached is the diff with the corrections you proposed. I also removed
the hacky call to `message' in favor of the standard vc behavior to
display command status only on errors.


Regards,
-- 
Fabián E. Gallina
http://www.from-the-cloud.com

[-- Attachment #2: vc.el.diff --]
[-- Type: text/plain, Size: 830 bytes --]

diff --git a/lisp/vc.el b/lisp/vc.el
index 9a71286..c940d1d 100644
--- a/lisp/vc.el
+++ b/lisp/vc.el
@@ -1188,6 +1188,22 @@ merge in the changes into your working copy."
 (declare-function vc-dir-move-to-goal-column "vc-dir" ())
 
 ;;;###autoload
+(defun vc-pull ()
+  "Merges changes between branches."
+  (interactive)
+  (vc-ensure-vc-buffer)
+  (let ((backend (vc-backend buffer-file-name)))    
+    (vc-call-backend backend 'pull)))
+
+;;;###autoload
+(defun vc-push ()
+  "Pushes commits to some branch."
+  (interactive)
+  (vc-ensure-vc-buffer)
+  (let ((backend (vc-backend buffer-file-name)))
+    (vc-call-backend backend 'push)))
+
+;;;###autoload
 (defun vc-register (&optional set-revision vc-fileset comment)
   "Register into a version control system.
 If VC-FILESET is given, register the files in that fileset.

[-- Attachment #3: vc-git.el.diff --]
[-- Type: text/plain, Size: 2496 bytes --]

diff --git a/lisp/vc-git.el b/lisp/vc-git.el
index 69e8614..6f60cb2 100644
--- a/lisp/vc-git.el
+++ b/lisp/vc-git.el
@@ -490,6 +490,51 @@ If nil, use the value of `vc-diff-switches'.  If t, use no switches."
 
 ;;; STATE-CHANGING FUNCTIONS
 
+(defun vc-git--do-push-pull (operation)
+  "Calls the command git OPERATION. OPERATION could be 'push or 'pull."
+  (let* ((default-refspec (vc-git-working-revision buffer-file-name))
+         (available-repos
+          (let ((table (list ".")))
+            (with-temp-buffer
+              (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
+              (goto-char (point-min))
+              (while (re-search-forward "^refs/remotes/\\(.*\\)/.*$" nil t)
+                (push (match-string 1) table)))
+            table))
+         (default-repo (nth 0 available-repos))
+         (current-repo
+          (completing-read (format "Repository (default %s): " default-repo)
+                           available-repos nil 'confirm nil nil default-repo))
+         (available-refspecs
+          (let ((table (list "HEAD"))
+                (ref-regexp))
+            (with-temp-buffer
+              (vc-git-command t nil nil "for-each-ref" "--format=%(refname)")
+              (setq ref-regexp
+                    (format (if (and (not (equal "." current-repo))
+                                     (equal vc-git-push-pull-perform 'pull))
+                                "^refs/\\(remotes/%s\\|tags\\)/\\([^\n]+\\).*$"
+                              "^refs/\\(remotes\\|heads\\|tags\\)/\\(%s/[^\n]+\\|[^\n]+\\).*$")
+                            (regexp-quote current-repo)))
+              (goto-char (point-min))
+              (while (re-search-forward ref-regexp nil t)
+                (push (match-string 2) table)))
+            table))
+         (current-refspec
+          (completing-read (format "Branch (default %s): " default-refspec)
+                           available-refspecs nil 'confirm nil nil 
+                           default-refspec))
+         (command (prin1-to-string operation))) 
+    (vc-git-command nil 0 nil command current-repo current-refspec)))
+
+(defun vc-git-push ()
+  "Update remote refs along with associated objects."
+  (vc-git--do-push-pull 'push))
+
+(defun vc-git-pull ()
+  "Fetch from and merge with another repository or a local branch."
+  (vc-git--do-push-pull 'pull))
+
 (defun vc-git-create-repo ()
   "Create a new Git repository."
   (vc-git-command nil 0 nil "init"))