bug#52172: Fix vc-git--program-version to support Git for macOS version string

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Justin Schell]

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

The current `vc-git.el` `vc-git--program-version` function doesn't
support the version string returned by Git on macOS. `git version` on
macOS returns e.g., "git version 2.30.1 (Apple Git-130)" and
`vc-git--program-version` currently returns "0" instead of "2.30.1".

I've attached patches for two options for a fix.

`topic.fix-vc-git--program-version.ignore-trailing.patch` updates the
regexp used to match numeric version strings that are a combination of
digits and dots (starting with a digit), ignoring anything else that
comes after. Any trailing dots are then stripped. This is more general
than the current approach of supporting a numeric version string only,
or a numeric version string followed by the specific additional
strings that some Git versions append (currently only the additional
string appended by Git for Windows is supported) and supports
reasonable version strings that I can think of that potential other
versions of Git might return. I'm unaware of why this approach wasn't
used in the previous fix used for Git for Windows support. As such,
I've also attached `topic.fix-vc-git--program-version.support-macos`,
which adds to the current approach, adding support for the specific
additional string that Git for macOS returns, if that is preferred.

Also included are unit tests for each approach.

Thanks,
Justin Schell

[-- Attachment #2: topic.fix-vc-git--program-version.ignore-trailing.patch --]
[-- Type: application/octet-stream, Size: 3680 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2d35061b26..3aaf995d1b 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -298,12 +298,15 @@ included in the completions."
              (vc-git--run-command-string nil "version")))
         (setq vc-git--program-version
               (if (and version-string
-                       ;; Git for Windows appends ".windows.N" to the
-                       ;; numerical version reported by Git.
+                       ;; Some Git versions append additional strings
+                       ;; to the numerical version string. E.g., Git
+                       ;; for Windows appends ".windows.N", while Git
+                       ;; for Mac appends " (Apple Git-N)". Capture
+                       ;; numerical version and ignore the rest.
                        (string-match
-                        "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
+                        "git version \\([0-9][0-9.]+\\)"
                         version-string))
-                  (match-string 1 version-string)
+                  (string-trim-right (match-string 1 version-string) "\\.")
                 "0")))))
 
 (defun vc-git--git-status-to-vc-state (code-list)
diff --git a/test/lisp/vc/vc-git-tests.el b/test/lisp/vc/vc-git-tests.el
new file mode 100644
index 0000000000..997ab3c4b5
--- /dev/null
+++ b/test/lisp/vc/vc-git-tests.el
@@ -0,0 +1,67 @@
+;;; vc-git-tests.el --- tests for vc/vc-git.el  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2016-2021 Free Software Foundation, Inc.
+
+;; Author: Justin Schell <justinmschell@gmail.com>
+;; Maintainer: emacs-devel@gnu.org
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'vc-git)
+
+(ert-deftest vc-git-test-program-version-general ()
+  (vc-git-test--run-program-version-test
+   "git version 2.30.1.0"
+   "2.30.1.0"))
+
+(ert-deftest vc-git-test-program-version-windows ()
+  (vc-git-test--run-program-version-test
+   "git version 2.30.1.1.windows.1"
+   "2.30.1.1"))
+
+(ert-deftest vc-git-test-program-version-apple ()
+  (vc-git-test--run-program-version-test
+   "git version 2.30.1.2 (Apple Git-130)"
+   "2.30.1.2"))
+
+(ert-deftest vc-git-test-program-version-other ()
+  (vc-git-test--run-program-version-test
+   "git version 2.30.1.3.foo.bar"
+   "2.30.1.3"))
+
+(ert-deftest vc-git-test-program-version-invalid-leading-string ()
+  (vc-git-test--run-program-version-test
+   "git version foo.bar.2.30.1.4"
+   "0"))
+
+(ert-deftest vc-git-test-program-version-invalid-leading-dot ()
+  (vc-git-test--run-program-version-test
+   "git version .2.30.1.5"
+   "0"))
+
+(defun vc-git-test--run-program-version-test
+    (mock-version-string expected-output)
+  (cl-letf* (((symbol-function 'vc-git--run-command-string)
+              (lambda (_file _args) mock-version-string))
+             (vc-git--program-version nil)
+             (actual-output (vc-git--program-version)))
+    (should (equal actual-output expected-output))))
+
+;;; vc-git-tests.el ends here

[-- Attachment #3: topic.fix-vc-git--program-version.support-macos --]
[-- Type: application/octet-stream, Size: 2997 bytes --]

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2d35061b26..eab1450a41 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -299,9 +299,10 @@ included in the completions."
         (setq vc-git--program-version
               (if (and version-string
                        ;; Git for Windows appends ".windows.N" to the
-                       ;; numerical version reported by Git.
+                       ;; numerical version reported by Git. Git for
+                       ;; macOS appends " (Apple Git-N)".
                        (string-match
-                        "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?$"
+                        "git version \\([0-9.]+\\)\\(\\.windows\\.[0-9]+\\)?\\( (Apple Git\\-[0-9]+)\\)?$"
                         version-string))
                   (match-string 1 version-string)
                 "0")))))
diff --git a/test/lisp/vc/vc-git-tests.el b/test/lisp/vc/vc-git-tests.el
new file mode 100644
index 0000000000..abcd9d42be
--- /dev/null
+++ b/test/lisp/vc/vc-git-tests.el
@@ -0,0 +1,57 @@
+;;; vc-git-tests.el --- tests for vc/vc-git.el  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2016-2021 Free Software Foundation, Inc.
+
+;; Author: Justin Schell <justinmschell@gmail.com>
+;; Maintainer: emacs-devel@gnu.org
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'vc-git)
+
+(ert-deftest vc-git-test-program-version-general ()
+  (vc-git-test--run-program-version-test
+   "git version 2.29.0"
+   "2.29.0"))
+
+(ert-deftest vc-git-test-program-version-windows ()
+  (vc-git-test--run-program-version-test
+   "git version 2.29.1.windows.1"
+   "2.29.1"))
+
+(ert-deftest vc-git-test-program-version-apple ()
+  (vc-git-test--run-program-version-test
+   "git version 2.29.2 (Apple Git-130)"
+   "2.29.2"))
+
+(ert-deftest vc-git-test-program-version-other ()
+  (vc-git-test--run-program-version-test
+   "git version 2.29.3.foo.bar"
+   "0"))
+
+(defun vc-git-test--run-program-version-test
+    (mock-version-string expected-output)
+  (cl-letf* (((symbol-function 'vc-git--run-command-string)
+              (lambda (_file _args) mock-version-string))
+             (vc-git--program-version nil)
+             (actual-output (vc-git--program-version)))
+    (should (equal actual-output expected-output))))
+
+;;; vc-git-tests.el ends here

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Lars Ingebrigtsen]

Justin Schell <justinmschell@gmail.com> writes:

> `topic.fix-vc-git--program-version.ignore-trailing.patch` updates the
> regexp used to match numeric version strings that are a combination of
> digits and dots (starting with a digit), ignoring anything else that
> comes after. Any trailing dots are then stripped. This is more general
> than the current approach of supporting a numeric version string only,

I think this approach makes the most sense, but perhaps Dmitry has an
opinion here (added to the CCs).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Dmitry Gutov]

Hi Lars!

On 29.11.2021 17:47, Lars Ingebrigtsen wrote:
> Justin Schell<justinmschell@gmail.com>  writes:
> 
>> `topic.fix-vc-git--program-version.ignore-trailing.patch` updates the
>> regexp used to match numeric version strings that are a combination of
>> digits and dots (starting with a digit), ignoring anything else that
>> comes after. Any trailing dots are then stripped. This is more general
>> than the current approach of supporting a numeric version string only,
> I think this approach makes the most sense, but perhaps Dmitry has an
> opinion here (added to the CCs).

Looks good to me (especially with included tests). The 
trailing-dot-stripping approach, that is.

I would even suggest to try to get it into emacs-28 (given that 
apparently that functionality has been broken in macOS for a long time).

But then again, if it's only been reported now, maybe it's not hugely 
important and Emacs 28 can live without it.

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Dmitry Gutov]

On 29.11.2021 17:47, Lars Ingebrigtsen wrote:
> perhaps Dmitry has an
> opinion here

BTW, that was the email of a different Dmitry. ;-)

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 29.11.2021 17:47, Lars Ingebrigtsen wrote:
>> perhaps Dmitry has an
>> opinion here
>
> BTW, that was the email of a different Dmitry. ;-)

Oops!  I blame ecomplete!

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Looks good to me (especially with included tests). The
> trailing-dot-stripping approach, that is.

OK; I've now pushed it to Emacs 29.

> I would even suggest to try to get it into emacs-28 (given that
> apparently that functionality has been broken in macOS for a long
> time).
>
> But then again, if it's only been reported now, maybe it's not hugely
> important and Emacs 28 can live without it.

This sort of change can be somewhat ticklish, as it depends on output
from external programs, and it's hard to get full test coverage of that.
So I'd rather just put it on master.

Justin, this change was small enough to apply without assigning
copyright to the FSF, but for future patches you want to submit, it
might make sense to get the paperwork started now, so that subsequent
patches can be applied speedily. Would you be willing to sign such
paperwork?

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Justin Schell]

> Justin, this change was small enough to apply without assigning
> copyright to the FSF, but for future patches you want to submit, it
> might make sense to get the paperwork started now, so that subsequent
> patches can be applied speedily. Would you be willing to sign such
> paperwork?

Sure thing. Will you email them to me or is there a URL I should visit?

Justin

bug#52172: Fix vc-git--program-version to support Git for macOS version string

[Lars Ingebrigtsen]

Justin Schell <justinmschell@gmail.com> writes:

> Sure thing. Will you email them to me or is there a URL I should visit?

Great; here's the form to get started:


Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]