From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: git push/pull Date: Mon, 07 Dec 2009 23:50:20 -0500 Message-ID: References: <9de1a5ef0912050009i59707986gb177505e04975e1b@mail.gmail.com> <9de1a5ef0912061121l2e4645c7q6ff334d57e4ec3bf@mail.gmail.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1260247839 5902 80.91.229.12 (8 Dec 2009 04:50:39 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 8 Dec 2009 04:50:39 +0000 (UTC) Cc: Emacs-Devel devel To: Fabian Ezequiel Gallina Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Dec 08 05:50:32 2009 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1NHs1w-0002Ve-2F for ged-emacs-devel@m.gmane.org; Tue, 08 Dec 2009 05:50:32 +0100 Original-Received: from localhost ([127.0.0.1]:38187 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NHs1v-0002gs-JK for ged-emacs-devel@m.gmane.org; Mon, 07 Dec 2009 23:50:31 -0500 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NHs1q-0002g0-Fd for emacs-devel@gnu.org; Mon, 07 Dec 2009 23:50:26 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NHs1m-0002f8-Uz for emacs-devel@gnu.org; Mon, 07 Dec 2009 23:50:26 -0500 Original-Received: from [199.232.76.173] (port=53316 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NHs1m-0002f4-KS for emacs-devel@gnu.org; Mon, 07 Dec 2009 23:50:22 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.183]:2522 helo=ironport2-out.pppoe.ca) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NHs1m-0005ia-87 for emacs-devel@gnu.org; Mon, 07 Dec 2009 23:50:22 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqIEAIppHUvO+INN/2dsb2JhbACBTNcShDIEgWeINg X-IronPort-AV: E=Sophos;i="4.47,359,1257138000"; d="scan'208";a="50966782" Original-Received: from 206-248-131-77.dsl.teksavvy.com (HELO ceviche.home) ([206.248.131.77]) by ironport2-out.pppoe.ca with ESMTP; 07 Dec 2009 23:50:21 -0500 Original-Received: by ceviche.home (Postfix, from userid 20848) id D5899B41E0; Mon, 7 Dec 2009 23:50:20 -0500 (EST) In-Reply-To: <9de1a5ef0912061121l2e4645c7q6ff334d57e4ec3bf@mail.gmail.com> (Fabian Ezequiel Gallina's message of "Sun, 6 Dec 2009 16:21:25 -0300") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:118384 Archived-At: > 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