bug#17945: 24.4.50; vc-git-annotate-command is too slow

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[William Xu]

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

C-x v g on a git file is very slow, compare this:

    $ time git --no-pager  blame  -- vc-annotate.el 1>/dev/null

    real    0m1.432s
    user    0m1.364s
    sys    0m0.063s


    $ time git --no-pager  blame -C -C  -- vc-annotate.el 1>/dev/null

    real    0m23.477s
    user    0m22.058s
    sys    0m1.405s

It seems the -C -C options would slow down git hugely, making this
command almost useless.  Can we remove it from default option list?

-William

git version 1.9.3

In GNU Emacs 24.4.50.1 (i386-apple-darwin13.1.0, NS apple-appkit-1265.19)
 of 2014-04-16 on tokyolove.local
Windowing system distributor `Apple', version 10.3.1265
Configured using:
 `configure --with-ns CC=clang'

Configured features:
ACL GNUTLS LIBXML2 ZLIB

Important settings:
  locale-coding-system: utf-8-unix

[-- Attachment #2: Type: text/html, Size: 995 bytes --]

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry]

William Xu <william.xwl@gmail.com> writes:

> C-x v g on a git file is very slow, compare this:

What kind of repository are you doing this in?

For me, it always works quite fast (usually <1 s). 

> It seems the -C -C options would slow down git hugely, making this
> command almost useless.  Can we remove it from default option list?

These options provide a feature, so we shouldn't simply remove them.

Maybe we should provide a user option.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[William Xu]

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

Dmitry <dgutov@yandex.ru>于2014年7月5日星期六写道：

> William Xu <william.xwl@gmail.com <javascript:;>> writes:
>
> > C-x v g on a git file is very slow, compare this:
>
> What kind of repository are you doing this in?
>
> For me, it always works quite fast (usually <1 s)


As you can see, I ran it on the emacs git mirror(git://
git.savannah.org/emacs.git).  I can see this slowness on my several git
repos where there are many files.  I often fall back on running
shell-command with 'git blame'.


>
> It seems the -C -C options would slow down git hugely, making this
> > command almost useless.  Can we remove it from default option list?
>
> These options provide a feature, so we shouldn't simply remove them.


There are many features, the question is whether this is good for default
setup.


-William




-- 
-William

[-- Attachment #2: Type: text/html, Size: 1825 bytes --]

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Stefan Monnier]

>> It seems the -C -C options would slow down git hugely, making this
>> command almost useless.  Can we remove it from default option list?
> These options provide a feature, so we shouldn't simply remove them.

I think it makes sense to follow Git's defaults.

> Maybe we should provide a user option.

Indeed, vc-git-annotate-command should use (vc-switches 'git annotate).


        Stefan

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Glenn Morris]

I find vc-annotate _unusably_ slow on the Emacs repo thanks to these -"C"s.
Eg for rmailsum.el: 4 sec versus 5.5 minutes.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Óscar Fuentes]

So what about this patch?

	Modified   lisp/vc/vc-git.el
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index a31c121..07ff083 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -120,6 +120,16 @@ (defcustom vc-git-diff-switches t
   :version "23.1"
   :group 'vc-git)
 
+(defcustom vc-git-annotate-switches t
+  "String or list of strings specifying switches for Git blame under VC.
+If nil, use the value of `vc-annotate-switches'.  If t, use no switches."
+  :type '(choice (const :tag "Unspecified" nil)
+		 (const :tag "None" t)
+		 (string :tag "Argument String")
+		 (repeat :tag "Argument List" :value ("") string))
+  :version "25.1"
+  :group 'vc-git)
+
 (defcustom vc-git-program "git"
   "Name of the Git executable (excluding any arguments)."
   :version "24.1"
@@ -1013,7 +1023,9 @@ (defun vc-git-revision-completion-table (files)
 
 (defun vc-git-annotate-command (file buf &optional rev)
   (let ((name (file-relative-name file)))
-    (vc-git-command buf 'async nil "blame" "--date=iso" "-C" "-C" rev "--" name)))
+    (apply #'vc-git-command buf 'async nil "blame" "--date=iso"
+	   (append (vc-switches 'git 'annotate)
+		   (list rev "--" name)))))
 
 (declare-function vc-annotate-convert-time "vc-annotate" (time))

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 24 Feb 2015 16:31:45 +0100
> 
> So what about this patch?

Why not a prefix argument?  It's easier to set once when one needs it,
and is the usual way in Emacs to provide minor variations of commands.

Or maybe do both, for those who might want this optional behavior
always (although given the massive slow-down, and the general
dismissal of the issue among git users, I doubt that such a person
exists).

Thanks.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/24/2015 06:12 PM, Eli Zaretskii wrote:

> Why not a prefix argument?  It's easier to set once when one needs it,
> and is the usual way in Emacs to provide minor variations of commands.

I'm pretty sure `vc-switches' is the usual way to provide invocation 
arguments variations in VC, or else there would be no such function.

> Or maybe do both, for those who might want this optional behavior
> always (although given the massive slow-down, and the general
> dismissal of the issue among git users, I doubt that such a person
> exists).

I believe you should rather customize the new variable once and for all 
for your preference, or set it on by-project basis, in .dir-locals.el.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/24/2015 09:39 AM, Glenn Morris wrote:
>
> I find vc-annotate _unusably_ slow on the Emacs repo thanks to these -"C"s.
> Eg for rmailsum.el: 4 sec versus 5.5 minutes.

It takes ~40 sec with '-C -C' (as opposed to <1 sec) on my machine, with 
a fast SSD and Git 2.1.0. So it'd say it's usable.

The difference, I agree, is pretty striking, but rmailsum.el seems to be 
one of the worst cases.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Glenn Morris]

Dmitry Gutov wrote:

> On 02/24/2015 09:39 AM, Glenn Morris wrote:
>>
>> I find vc-annotate _unusably_ slow on the Emacs repo thanks to these -"C"s.
>> Eg for rmailsum.el: 4 sec versus 5.5 minutes.
>
> It takes ~40 sec with '-C -C' (as opposed to <1 sec) on my machine,
> with a fast SSD and Git 2.1.0. So it'd say it's usable.

Great, please send me your machine by soonest post. :)
It's unusable for me, and as Stefan said earlier, Emacs should follow
git's defaults.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Glenn Morris]

Óscar Fuentes wrote:

> So what about this patch?

Looks, right, except vc-annotate-switches does not exist and would also
need to be added. And for consistency, presumably all backends should
get the same treatment. Thanks.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/24/2015 07:56 PM, Glenn Morris wrote:

> Great, please send me your machine by soonest post. :)

I'll get right on that. :)

> It's unusable for me,

Maybe you could also benefit from upgrading your Git, you've never 
mentioned the version.

> and as Stefan said earlier, Emacs should follow
> git's defaults.

Anyway, no argument from me here.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/24/2015 07:57 PM, Glenn Morris wrote:

> except vc-annotate-switches does not exist and would also
> need to be added.

I think this is only justified if we expect different backends' annotate 
command to receive the same options.

So far, only '-w' looks a likely candidate, albeit its descriptions are 
a bit different between Git and Hg. And it's not in e.g. Bzr.

What if the user sets `vc-annotate-switches' to '-w', and then calls 
`vc-annotate' in a Bazaar repository, where it'll obviously lead to failure?

> And for consistency, presumably all backends should
> get the same treatment. Thanks.

Probably. But I'd rather the present patch gets installed without too 
much delay.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Óscar Fuentes]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02/24/2015 07:57 PM, Glenn Morris wrote:
>
>> except vc-annotate-switches does not exist and would also
>> need to be added.
>
> I think this is only justified if we expect different backends'
> annotate command to receive the same options.

Actually, I implemented vc-annotate-switches too, but at the end
excluded it from the patch for the reason you mentioned.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Óscar Fuentes]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02/24/2015 07:57 PM, Glenn Morris wrote:
>
>> except vc-annotate-switches does not exist and would also
>> need to be added.
>
> I think this is only justified if we expect different backends'
> annotate command to receive the same options.

As the docstrings of vc-BACKEND-annotate-switches mention
vc-annotate-switches *and* vc-switches checks vc-annotate-switches, I
changed my mind again and implemented vc-annotate-switches with a caveat
on the docstring. Please review and suggest a better wording, if
necessary:

(defcustom vc-annotate-switches nil
  "A string or list of strings specifying switches for annotate under VC.
When running annotate under a given BACKEND, VC uses the first
non-nil value of `vc-BACKEND-annotate-switches', `vc-annotate-switches',
and `annotate-switches', in that order.  Since nil means to check the
next variable in the sequence, either of the first two may use
the value t to mean no switches at all.  `vc-annotate-switches'
should contain switches that are specific to version control, but
not specific to any particular backend.

As very few switches (if any) are used across different VC tools,
please consider using the specific `vc-BACKEND-annotate-switches'
for the backend you use."
  :type '(choice (const :tag "Unspecified" nil)
		 (const :tag "None" t)
		 (string :tag "Argument String")
		 (repeat :tag "Argument List" :value ("") string))
  :group 'vc
  :version "25.1")


> So far, only '-w' looks a likely candidate, albeit its descriptions
> are a bit different between Git and Hg. And it's not in e.g. Bzr.
>
> What if the user sets `vc-annotate-switches' to '-w', and then calls
> `vc-annotate' in a Bazaar repository, where it'll obviously lead to
> failure?
>
>> And for consistency, presumably all backends should
>> get the same treatment. Thanks.
>
> Probably. But I'd rather the present patch gets installed without too
> much delay.

I extended the vc-switches support to the rest of backends that
implement `vc-BACKEND-annotate-command', except rcs, that does some
complicated things (and I have no easy way of testing it.)

Once we agree on the resolution of the vc-annotate-switches issue, I'll
commit the changes.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/25/2015 07:46 PM, Óscar Fuentes wrote:

> As the docstrings of vc-BACKEND-annotate-switches mention
> vc-annotate-switches *and* vc-switches checks vc-annotate-switches,

How about avoiding the mention of `vc-annotate-switches' in those 
docstrings instead?

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Óscar Fuentes]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02/25/2015 07:46 PM, Óscar Fuentes wrote:
>
>> As the docstrings of vc-BACKEND-annotate-switches mention
>> vc-annotate-switches *and* vc-switches checks vc-annotate-switches,
>
> How about avoiding the mention of `vc-annotate-switches' in those
> docstrings instead?

vc-switches does check `vc-annotate-switches' (as it checks
vc-OP-switches for any other OP and BACKEND), so we are hiding
information from the user.

The whole problem consists on a generic approach (vc-OP-switches) that
doesn't play nice for some (all?) OPs. So we either get rid of
`vc-OP-switches' entirely or warn the user about its pitfalls.

(Is there an OP where `vc-OP-switches' works well across more than two
supported backends?)

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Dmitry Gutov]

On 02/25/2015 08:06 PM, Óscar Fuentes wrote:

> vc-switches does check `vc-annotate-switches' (as it checks
> vc-OP-switches for any other OP and BACKEND), so we are hiding
> information from the user.

But there's no such user option. And if a variable with that name is 
created, well, the user stepped on something nobody asked them too.

> The whole problem consists on a generic approach (vc-OP-switches) that
> doesn't play nice for some (all?) OPs. So we either get rid of
> `vc-OP-switches' entirely or warn the user about its pitfalls.

I think in this case we can just as well avoid using `vc-switches' and 
reference `vc-git-annotate-switches' directly.

> (Is there an OP where `vc-OP-switches' works well across more than two
> supported backends?)

vc-diff-switches works pretty well.

The rest of -switches variables, though, look similarly suspicious to 
me. That's vc-checkin-switches, vc-checkout-switches and 
vc-register-switches.

bug#17945: 24.4.50; vc-git-annotate-command is too slow

[Óscar Fuentes]

Glenn Morris <rgm@gnu.org> writes:

> Óscar Fuentes wrote:
>
>> So what about this patch?
>
> Looks, right, except vc-annotate-switches does not exist and would also
> need to be added. And for consistency, presumably all backends should
> get the same treatment. Thanks.

Implemented on b5a0603eb41c7a350c16a1b3ec5c1b8d8c84a4eb.

I'm neutral about the existence of vc-annotate-switches, so if anyone
wants to remove it (including mentions on the docstrings of
vc-BACKEND-annotate-switches) it's fine with me.