Re: git push/pull

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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