excerpts from tomorrow's "What's cooking" draft

excerpts from tomorrow's "What's cooking" draft

[Junio C Hamano]

I'll do the full-list at the end of my git Wednesday, but here are the
current highlights I am sending out to hear opinions from people.

Please note that I haven't caught up with the patches from past 12 hours
or so.

----------------------------------------------------------------

[New Topics]

* jn/help-everywhere (2009-11-09) 21 commits
 - diff --no-index: make the usage string less scary
 - ...
 - Show usage string for 'git grep -h'
 (this branch uses jn/maint-http-fetch-mingw and jn/remove-fetch--tool.)

There were unrelated but still worthy fixes, so I reordered some of them;
also the "usage()" change is different from the one that was posted (see
my comment in $gmane/132592).

* jn/maint-http-fetch-mingw (2009-11-09) 1 commit.
 - http-fetch: add missing initialization of argv0_path
 (this branch is used by jn/help-everywhere.)

* jn/remove-fetch--tool (2009-11-09) 1 commit
 - Retire fetch--tool helper to contrib/examples
 (this branch is used by jn/help-everywhere.)

These two were originally part of the "help-everywhere" topic but
they can stand on their own.

* jc/log-stdin (2009-11-03) 1 commit
 - Teach --stdin option to "log" family

This is not signed-off (see $gmane/131971 for list of things you can do to
help advancing this topic).

--------------------------------------------------
[Stalled]

* sc/protocol-doc (2009-10-29) 1 commit
 - Update packfile transfer protocol documentation

* jl/submodule-add-noname (2009-09-22) 1 commit.
 - git submodule add: make the <path> parameter optional

Dscho started an interesting discussion regarding the larger workflow in
which the "submodule add" is used.  I think the patch itself makes sense
but at the same time it probably makes sense to also take the <path> and
infer the <repository> as Dscho suggested, probably in "git submodule
add", not in "git add" proper, at least initially.

Any objections against merging this to 'next'?

* jc/fix-tree-walk (2009-10-22) 11 commits.
  (merged to 'next' on 2009-10-22 at 10c0c8f)
 + Revert failed attempt since 353c5ee
 + read-tree --debug-unpack
  (merged to 'next' on 2009-10-11 at 0b058e2)
 + ...
 + diff-lib.c: fix misleading comments on oneway_diff()

This has some stupid bugs and temporarily reverted from 'next' until I can
fix it, but the "temporarily" turned out to be very loooong.  Sigh...

* jh/notes (2009-10-09) 22 commits.
 - fast-import: Proper notes tree manipulation using the notes API
 - ...
 - Notes API: get_commit_notes() -> format_note() + remove the commit restriction
  (merged to 'next' on 2009-11-01 at 948327a)
 + Add selftests verifying concatenation of multiple notes for the same commit
 + ...
 + Introduce commit notes

I somehow thought that the later API part was waiting for updates but
nothing seems to be happening.

* jn/gitweb-blame (2009-09-01) 5 commits.
 - gitweb: Minify gitweb.js if JSMIN is defined
 - gitweb: Create links leading to 'blame_incremental' using JavaScript
  (merged to 'next' on 2009-10-11 at 73c4a83)
 + gitweb: Colorize 'blame_incremental' view during processing
 + ...

Ajax-y blame.  Any progress or RFH?

* sr/gfi-options (2009-09-06) 6 commits.
 - fast-import: test the new option command
 - ...
 - fast-import: put option parsing code in separate functions

It seemed to be moving again soon, but nothing has happened yet...

* je/send-email-no-subject (2009-08-05) 1 commit.
  (merged to 'next' on 2009-10-11 at 1b99c56)
 + send-email: confirm on empty mail subjects

The existing tests cover the positive case (i.e. as long as the user says
"yes" to the "do you really want to send this message that lacks subject",
the message is sent) of this feature, but the feature itself needs its own
test to verify the negative case (i.e. does it correctly stop if the user
says "no"?)

--------------------------------------------------
[Cooking]

* bw/autoconf-more (2009-11-04) 2 commits
 - configure: add settings for gitconfig, editor and pager
 - configure: add macro to set arbitrary make variables

Looked sensible.  Any comments?

* bg/format-patch-doc-update (2009-11-07) 4 commits
 - format-patch: Add "--no-stat" as a synonym for "-p"
 - format-patch documentation: Fix formatting
 - format-patch documentation: Remove diff options that are not useful
 - format-patch: Always generate a patch

Looked sensible, even though this may want to wait for 1.7.0.  We'll see
when we merge this to 'next'.

* rj/maint-simplify-cygwin-makefile (2009-10-27) 1 commit.
 - Makefile: merge two Cygwin configuration sections into one
 (this branch is used by rj/cygwin-msvc.)

This is one of the most obviously correct bit from "Compiling on Cygwin
using MSVC fails" topic.

* rj/cygwin-msvc (2009-11-09) 3 commits.
 - Add explicit Cygwin check to guard WIN32 header inclusion
 - MSVC: Add support for building with NO_MMAP
 - Makefile: keep MSVC and Cygwin configuration separate
 (this branch uses rj/maint-simplify-cygwin-makefile.)

I think J6t was not happy with the tip one.

* cc/bisect-doc (2009-11-08) 1 commit
 - Documentation: add "Fighting regressions with git bisect" article

Any comments?  Should it go to Documentation/technical instead?

* jn/editor-pager (2009-10-30) 8 commits
 - Provide a build time default-pager setting
 - Provide a build time default-editor setting
 - am -i, git-svn: use "git var GIT_PAGER"
 - add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"
 - Teach git var about GIT_PAGER
 - Teach git var about GIT_EDITOR
 - Do not use VISUAL editor on dumb terminals
 - Handle more shell metacharacters in editor names

Any comments?

* sc/difftool-p4merge (2009-10-28) 1 commit
  (merged to 'next' on 2009-10-31 at 194b5c5)
 + mergetool--lib: add p4merge as a pre-configured mergetool option

I do not do p4 nor use difftool, so it's much easier for me to merge this
to 'master' and wait for anybody to scream if there is breakage.  I'll do
so, unless I hear objections in a few days.

* sr/vcs-helper (2009-11-06) 12 commits
 - Add Python support library for remote helpers
 - ...
 - Fix memory leak in helper method for disconnect

Re-rolled series that contains Daniel's and Johan's.
Any comments?  Is everybody happy?

* fc/doc-fast-forward (2009-10-24) 1 commit.
  (merged to 'next' on 2009-11-01 at faaad90)
 + Use 'fast-forward' all over the place

Soon in 'master'; carrying this in 'next' for too long is turning out to
be quite painful.

* ks/precompute-completion (2009-10-26) 3 commits.
  (merged to 'next' on 2009-10-28 at cd5177f)
 + completion: ignore custom merge strategies when pre-generating
  (merged to 'next' on 2009-10-22 at f46a28a)
 + bug: precomputed completion includes scripts sources
  (merged to 'next' on 2009-10-14 at adf722a)
 + Speedup bash completion loading

What's the status of this thing?  Last time I polled the list I had an
impression that it was not quite ready...

* sp/smart-http (2009-11-09) 34 commits
 - t5551-http-fetch: Work around broken Accept header in libcurl
 - t5551-http-fetch: Work around some libcurl versions
 - http-backend: Protect GIT_PROJECT_ROOT from /../ requests
 - Git-aware CGI to provide dumb HTTP transport
  (merged to 'next' on 2009-11-06 at 666837c)
 + http-backend: Test configuration options
 + ...
 + http-push: fix check condition on http.c::finish_http_pack_request()

The tip ones are workarounds to the issues identified by somebody without
a real name (I do not remember---I am bad at people's names even when they
are real).

* ef/msys-imap (2009-10-22) 9 commits.
  (merged to 'next' on 2009-10-31 at 8630603)
 + Windows: use BLK_SHA1 again
 + MSVC: Enable OpenSSL, and translate -lcrypto
 + mingw: enable OpenSSL
 + mingw: wrap SSL_set_(w|r)fd to call _get_osfhandle
 + imap-send: build imap-send on Windows
 + imap-send: fix compilation-error on Windows
 + imap-send: use run-command API for tunneling
 + imap-send: use separate read and write fds
 + imap-send: remove useless uid code

Soon in 'master', unless I hear objections from msys folks or imap-send
users.

* nd/sparse (2009-08-20) 19 commits.
 - sparse checkout: inhibit empty worktree
 - ...
 - update-index: refactor mark_valid() in preparation for new options

The latest update I didn't look at very closely but I had an impression
that it was touching very generic codepath that would affect non sparse
cases, iow the patch looked very scary (the entire series already is).

Re: excerpts from tomorrow's "What's cooking" draft

[Ben Walton]

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

Excerpts from Junio C Hamano's message of Wed Nov 11 04:34:07 -0500 2009:

> * bw/autoconf-more (2009-11-04) 2 commits
>  - configure: add settings for gitconfig, editor and pager
>  - configure: add macro to set arbitrary make variables

The only note here is that these patches are dependent on the ones in
jn/editor-pager, so if that doesn't get merged, this shouldn't
either.  From my POV, jn/editor-pager does what I was after, so I'm
happy.

Thanks
-Ben
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: excerpts from tomorrow's "What's cooking" draft

[Johan Herland]

On Wednesday 11 November 2009, Junio C Hamano wrote:
> * jh/notes (2009-10-09) 22 commits.
>  - fast-import: Proper notes tree manipulation using the notes API
>  - ...
>  - Notes API: get_commit_notes() -> format_note() + remove the commit
> restriction (merged to 'next' on 2009-11-01 at 948327a)
>  + Add selftests verifying concatenation of multiple notes for the
> same commit + ...
>  + Introduce commit notes
>
> I somehow thought that the later API part was waiting for updates but
> nothing seems to be happening.

Sorry for the silence, I've been out with the flu, and been caught up 
with meatspace issues in general. Will try to send another iteration of 
the later API part in a few days.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: excerpts from tomorrow's "What's cooking" draft

[Sverre Rabbelier]

Heya,

On Wed, Nov 11, 2009 at 10:34, Junio C Hamano <junio@pobox.com> wrote:
> * sr/gfi-options (2009-09-06) 6 commits.
>  - fast-import: test the new option command
>  - ...
>  - fast-import: put option parsing code in separate functions
>
> It seemed to be moving again soon, but nothing has happened yet...

I ran out of git time due to the start of my uni's next quarter, this
is next on my to-do list though, as my remote helper depends on it.

> * sr/vcs-helper (2009-11-06) 12 commits
>  - Add Python support library for remote helpers
>  - ...
>  - Fix memory leak in helper method for disconnect
>
> Re-rolled series that contains Daniel's and Johan's.
> Any comments?  Is everybody happy?

Daniel, are you going to send a follow-up to the memory-leaking patch?
If not, this needs to stay out of next until I have time to do so. My
gitdir patch might need to be evicted as it is connected to
sr/gfi-options which is not yet done. Also, we need to update the
documentation, but I think we can at least start cooking it in next
without these documentation updates?

> * ks/precompute-completion (2009-10-26) 3 commits.
>  (merged to 'next' on 2009-10-28 at cd5177f)
>  + completion: ignore custom merge strategies when pre-generating
>  (merged to 'next' on 2009-10-22 at f46a28a)
>  + bug: precomputed completion includes scripts sources
>  (merged to 'next' on 2009-10-14 at adf722a)
>  + Speedup bash completion loading
>
> What's the status of this thing?  Last time I polled the list I had an
> impression that it was not quite ready...

Does the current version require me to 'cd contrib/completion && make'
each time we update completion? If so, that's a (very annoying)
regression that needs to be fixed before it's merged to master IMHO.

-- 
Cheers,

Sverre Rabbelier

Re: excerpts from tomorrow's "What's cooking" draft

[Nicolas Sebrecht]

The 11/11/09, Junio C Hamano wrote:

> I'll do the full-list at the end of my git Wednesday, but here are the
> current highlights I am sending out to hear opinions from people.

A bit OT, I've noticed the following output today:

  % git clone git://repo.or.cz/girocco.git
  Initialized empty Git repository in /home/nicolas/dev/official_packages/girocco/.git/
  remote: Counting objects: 3017, done.
  g objects: 100% (994/994), done.
  remote: Total 3017 (delta 1911), reused 2988 (delta 1896)
  Receiving objects: 100% (3017/3017), 403.99 KiB | 309 KiB/s, done.
  Resolving deltas: 100% (1911/1911), done.
  %

Notice the "g " at the begining at the 3th line. This is reproducible.

  % git --version
  git version 1.6.5.2.352.g8b8744


I can provide more test or bissect it if wanted.

-- 
Nicolas Sebrecht

Re: excerpts from tomorrow's "What's cooking" draft

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5088 bytes --]

On Wed, 11 Nov 2009, Sverre Rabbelier wrote:

> Heya,
> 
> On Wed, Nov 11, 2009 at 10:34, Junio C Hamano <junio@pobox.com> wrote:
> > * sr/gfi-options (2009-09-06) 6 commits.
> >  - fast-import: test the new option command
> >  - ...
> >  - fast-import: put option parsing code in separate functions
> >
> > It seemed to be moving again soon, but nothing has happened yet...
> 
> I ran out of git time due to the start of my uni's next quarter, this
> is next on my to-do list though, as my remote helper depends on it.
> 
> > * sr/vcs-helper (2009-11-06) 12 commits
> >  - Add Python support library for remote helpers
> >  - ...
> >  - Fix memory leak in helper method for disconnect
> >
> > Re-rolled series that contains Daniel's and Johan's.
> > Any comments?  Is everybody happy?
> 
> Daniel, are you going to send a follow-up to the memory-leaking patch?
> If not, this needs to stay out of next until I have time to do so. My
> gitdir patch might need to be evicted as it is connected to
> sr/gfi-options which is not yet done. Also, we need to update the
> documentation, but I think we can at least start cooking it in next
> without these documentation updates?

Okay, finally got to it just now. This is entirely untested (because I 
don't have anything that uses it), but it compiles and should be correct. 
It would be nice to get a clean bill of health on it from valgrind.

If it works, please squash it into my other patch, keep that commit 
message, and add my sign-off.

commit 5fdebd88a8e5c2714470c86a2c13ee2c795210cb
Author: Daniel Barkalow <barkalow@iabervon.org>
Date:   Wed Nov 11 13:36:36 2009 -0500

    Free memory used by remote helper importing refspecs, document
    
    "Allow helper to map private ref names into normal names" was just an
    RFD which turned out to be surprisingly correct. This adds the necessary
    memory-management and documentation, and generally makes it a suitable
    patch.
    
    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 2c5130f..f4b6a5a 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -46,7 +46,11 @@ Supported if the helper has the "fetch" capability.
 'import' <name>::
 	Produces a fast-import stream which imports the current value
 	of the named ref. It may additionally import other refs as
-	needed to construct the history efficiently.
+	needed to construct the history efficiently. The script writes
+	to a helper-specific private namespace. The value of the named
+	ref should be written to a location in this namespace derived
+	by applying the refspecs from the "refspec" capability to the
+	name of the ref.
 +
 Supported if the helper has the "import" capability.
 
@@ -67,6 +71,16 @@ CAPABILITIES
 'import'::
 	This helper supports the 'import' command.
 
+'refspec' 'spec'::
+	When using the import command, expect the source ref to have
+	been written to the destination ref. The earliest applicable
+	refspec takes precedence. For example
+	"refs/heads/*:refs/svn/origin/branches/*" means that, after an
+	"import refs/heads/name", the script has written to
+	refs/svn/origin/branches/name. If this capability is used at
+	all, it must cover all refs reported by the list command; if
+	it is not used, it is effectively "*:*"
+
 REF LIST ATTRIBUTES
 -------------------
 
diff --git a/remote.c b/remote.c
index 09e14a8..1f7870d 100644
--- a/remote.c
+++ b/remote.c
@@ -673,6 +673,16 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
 }
 
+void free_refspec(int nr_refspec, struct refspec *refspec)
+{
+	int i;
+	for (i = 0; i < nr_refspec; i++) {
+		free(refspec[i].src);
+		free(refspec[i].dst);
+	}
+	free(refspec);
+}
+
 static int valid_remote_nick(const char *name)
 {
 	if (!name[0] || is_dot_or_dotdot(name))
diff --git a/remote.h b/remote.h
index c2f920b..f89cb0e 100644
--- a/remote.h
+++ b/remote.h
@@ -91,6 +91,8 @@ void ref_remove_duplicates(struct ref *ref_map);
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
 
+void free_refspec(int nr_refspec, struct refspec *refspecs);
+
 char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
 		     const char *name);
 
diff --git a/transport-helper.c b/transport-helper.c
index 7ea76fd..cea646c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -72,8 +72,13 @@ static struct child_process *get_helper(struct transport *transport)
 		}
 	}
 	if (refspecs) {
+		int i;
 		data->refspec_nr = refspec_nr;
 		data->refspecs = parse_fetch_refspec(refspec_nr, refspecs);
+		for (i = 0; i < refspec_nr; i++) {
+			free((char *)refspecs[i]);
+		}
+		free(refspecs);
 	}
 	return data->helper;
 }
@@ -90,6 +95,8 @@ static int disconnect_helper(struct transport *transport)
 		free(data->helper);
 		data->helper = NULL;
 	}
+	free_refspec(data->refspec_nr, data->refspecs);
+	data->refspecs = NULL;
 	return 0;
 }
 

Re: excerpts from tomorrow's "What's cooking" draft

[Jakub Narebski]

Junio C Hamano <junio@pobox.com> writes:

> I'll do the full-list at the end of my git Wednesday, but here are the
> current highlights I am sending out to hear opinions from people.
> 
> Please note that I haven't caught up with the patches from past 12 hours
> or so.
> 
> ----------------------------------------------------------------

> * jn/gitweb-blame (2009-09-01) 5 commits.
>  - gitweb: Minify gitweb.js if JSMIN is defined
>  - gitweb: Create links leading to 'blame_incremental' using JavaScript
>   (merged to 'next' on 2009-10-11 at 73c4a83)
>  + gitweb: Colorize 'blame_incremental' view during processing
>  + ...
> 
> Ajax-y blame.  Any progress or RFH?

I have reordered two top commits (those that are only in 'pu'), but I
didn't improve "Create links..." commit.  Current version works, but
is suboptimal.
 
I wonder if Packy has any comments about 'blame_incremental' series...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: excerpts from tomorrow's "What's cooking" draft

[Nicolas Pitre]

On Wed, 11 Nov 2009, Nicolas Sebrecht wrote:

> A bit OT, I've noticed the following output today:
> 
>   % git clone git://repo.or.cz/girocco.git
>   Initialized empty Git repository in /home/nicolas/dev/official_packages/girocco/.git/
>   remote: Counting objects: 3017, done.
>   g objects: 100% (994/994), done.
>   remote: Total 3017 (delta 1911), reused 2988 (delta 1896)
>   Receiving objects: 100% (3017/3017), 403.99 KiB | 309 KiB/s, done.
>   Resolving deltas: 100% (1911/1911), done.
>   %
> 
> Notice the "g " at the begining at the 3th line. This is reproducible.

I get much worse:

Initialized empty Git repository in /home/nico/git/girocco/.git/
remote: Counting objects: 3017, done.
remote: Compressing objects:   5% (50/994)   Receiving objects:   3% (91/3017)
remote: Compressing objects:  15% (150/994) Receiving objects:   7% (212/3017)
remote: Compressing oReceiving objects:  14% (423/3017), 76.00 KiB | 135 KiB/s
remote: Compressing objects:  35Receiving objects:  16% (483/3017), 76.00 KiB |
remote: Compressing objects:  38% (378/994)Receiving objects:  17% (513/3017), 7
remote: Compressing objectReceiving objects:  20% (604/3017), 76.00 KiB | 135 Ki
remote: Compressing objects:  48% (47Receiving objects:  22% (664/3017), 76.00 K
remote: Compressing objects:  5Receiving objects:  23% (694/3017), 76.00 KiB | 1
remote: Compressing objects:  Receiving objects:  25% (755/3017), 76.00 KiB | 13
remote: Compressing objects:  84% (835/99Receiving objects:  26% (785/3017), 76.
remote: Compressing objeReceiving objects:  33% (996/3017), 76.00 KiB | 135 KiB/
remote: Compressing objects:  94% (Receiving objects:  36% (1087/3017), 76.00 Ki
remote: Compressing objects:  97% (965/994)   Receiving objects:  39% (1177/3017
g objects: 100% (994/994), done.
remote: Total 3017 (delta 1911), reused 2988 (delta 1896)
Receiving objects: 100% (3017/3017), 403.99 KiB | 335 KiB/s, done.
Resolving deltas: 100% (1911/1911), done.

According to strace, data from sideband channel #2 (prefixed with 
"remote: ") pertaining to object compression is printed way after pack 
data has already started to arrive locally.  This is really weird.

And this occurs only when fetching from repo.or.cz and not from 
git.kernel.org for example.  So there is something to investigate on the 
server side.  Pasky: anything you changed in your git installation 
lately?


Nicolas

Re: excerpts from tomorrow's "What's cooking" draft

[Petr Baudis]

On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre wrote:
> On Wed, 11 Nov 2009, Nicolas Sebrecht wrote:
> 
> > A bit OT, I've noticed the following output today:
> > 
> >   % git clone git://repo.or.cz/girocco.git
> >   Initialized empty Git repository in /home/nicolas/dev/official_packages/girocco/.git/
> >   remote: Counting objects: 3017, done.
> >   g objects: 100% (994/994), done.
> >   remote: Total 3017 (delta 1911), reused 2988 (delta 1896)
> >   Receiving objects: 100% (3017/3017), 403.99 KiB | 309 KiB/s, done.
> >   Resolving deltas: 100% (1911/1911), done.
> >   %
> > 
> > Notice the "g " at the begining at the 3th line. This is reproducible.
> 
> I get much worse:
> 
> Initialized empty Git repository in /home/nico/git/girocco/.git/
> remote: Counting objects: 3017, done.
> remote: Compressing objects:   5% (50/994)   Receiving objects:   3% (91/3017)
> remote: Compressing objects:  15% (150/994) Receiving objects:   7% (212/3017)
> remote: Compressing oReceiving objects:  14% (423/3017), 76.00 KiB | 135 KiB/s
> remote: Compressing objects:  35Receiving objects:  16% (483/3017), 76.00 KiB |
> remote: Compressing objects:  38% (378/994)Receiving objects:  17% (513/3017), 7
> remote: Compressing objectReceiving objects:  20% (604/3017), 76.00 KiB | 135 Ki
> remote: Compressing objects:  48% (47Receiving objects:  22% (664/3017), 76.00 K
> remote: Compressing objects:  5Receiving objects:  23% (694/3017), 76.00 KiB | 1
> remote: Compressing objects:  Receiving objects:  25% (755/3017), 76.00 KiB | 13
> remote: Compressing objects:  84% (835/99Receiving objects:  26% (785/3017), 76.
> remote: Compressing objeReceiving objects:  33% (996/3017), 76.00 KiB | 135 KiB/
> remote: Compressing objects:  94% (Receiving objects:  36% (1087/3017), 76.00 Ki
> remote: Compressing objects:  97% (965/994)   Receiving objects:  39% (1177/3017
> g objects: 100% (994/994), done.
> remote: Total 3017 (delta 1911), reused 2988 (delta 1896)
> Receiving objects: 100% (3017/3017), 403.99 KiB | 335 KiB/s, done.
> Resolving deltas: 100% (1911/1911), done.
> 
> According to strace, data from sideband channel #2 (prefixed with 
> "remote: ") pertaining to object compression is printed way after pack 
> data has already started to arrive locally.  This is really weird.
> 
> And this occurs only when fetching from repo.or.cz and not from 
> git.kernel.org for example.  So there is something to investigate on the 
> server side.  Pasky: anything you changed in your git installation 
> lately?

Yes, but nothing should have changed in git-daemon, that's the only part
of the infrastructure that uses system-wide git (which it perhaps
shouldn't). I cannot reproduce this problem, though. I have changed
git-daemon to use my local git version (about one week old master), does
this still happen for you?

-- 
				Petr "Pasky" Baudis
A lot of people have my books on their bookshelves.
That's the problem, they need to read them. -- Don Knuth

Re: excerpts from tomorrow's "What's cooking" draft

[Nicolas Pitre]

On Wed, 11 Nov 2009, Petr Baudis wrote:

> On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre wrote:
> > According to strace, data from sideband channel #2 (prefixed with 
> > "remote: ") pertaining to object compression is printed way after pack 
> > data has already started to arrive locally.  This is really weird.
> > 
> > And this occurs only when fetching from repo.or.cz and not from 
> > git.kernel.org for example.  So there is something to investigate on the 
> > server side.  Pasky: anything you changed in your git installation 
> > lately?
> 
> Yes, but nothing should have changed in git-daemon, that's the only part
> of the infrastructure that uses system-wide git (which it perhaps
> shouldn't). I cannot reproduce this problem, though. I have changed
> git-daemon to use my local git version (about one week old master), does
> this still happen for you?

No, it doesn't happen anymore.

What was the git-daemon version before?


Nicolas

Re: excerpts from tomorrow's "What's cooking" draft

[Nicolas Sebrecht]

The 11/11/09, Nicolas Pitre wrote:
> On Wed, 11 Nov 2009, Petr Baudis wrote:
> 
> > Yes, but nothing should have changed in git-daemon, that's the only part
> > of the infrastructure that uses system-wide git (which it perhaps
> > shouldn't). I cannot reproduce this problem, though. I have changed
> > git-daemon to use my local git version (about one week old master), does
> > this still happen for you?
> 
> No, it doesn't happen anymore.

I couldn't reproduce it, me neither.

> What was the git-daemon version before?

-- 
Nicolas Sebrecht

Re: excerpts from tomorrow's "What's cooking" draft

[Petr Baudis]

On Wed, Nov 11, 2009 at 04:19:44PM -0500, Nicolas Pitre wrote:
> On Wed, 11 Nov 2009, Petr Baudis wrote:
> 
> > On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre wrote:
> > > According to strace, data from sideband channel #2 (prefixed with 
> > > "remote: ") pertaining to object compression is printed way after pack 
> > > data has already started to arrive locally.  This is really weird.
> > > 
> > > And this occurs only when fetching from repo.or.cz and not from 
> > > git.kernel.org for example.  So there is something to investigate on the 
> > > server side.  Pasky: anything you changed in your git installation 
> > > lately?
> > 
> > Yes, but nothing should have changed in git-daemon, that's the only part
> > of the infrastructure that uses system-wide git (which it perhaps
> > shouldn't). I cannot reproduce this problem, though. I have changed
> > git-daemon to use my local git version (about one week old master), does
> > this still happen for you?
> 
> No, it doesn't happen anymore.
> 
> What was the git-daemon version before?

1.5.6.5, the default version in debian lenny

-- 
				Petr "Pasky" Baudis
A lot of people have my books on their bookshelves.
That's the problem, they need to read them. -- Don Knuth

Re: excerpts from tomorrow's "What's cooking" draft

[Nicolas Pitre]

On Wed, 11 Nov 2009, Petr Baudis wrote:

> On Wed, Nov 11, 2009 at 04:19:44PM -0500, Nicolas Pitre wrote:
> > On Wed, 11 Nov 2009, Petr Baudis wrote:
> > 
> > > Yes, but nothing should have changed in git-daemon, that's the only part
> > > of the infrastructure that uses system-wide git (which it perhaps
> > > shouldn't). I cannot reproduce this problem, though. I have changed
> > > git-daemon to use my local git version (about one week old master), does
> > > this still happen for you?
> > 
> > No, it doesn't happen anymore.
> > 
> > What was the git-daemon version before?
> 
> 1.5.6.5, the default version in debian lenny

Go figure.  I don't see anything that would explain the difference in 
behavior with latest git from a quick look.


Nicolas

ks/precompute-completion

[Jonathan Nieder]

Hi,

>> * ks/precompute-completion (2009-10-26) 3 commits.
>>  (merged to 'next' on 2009-10-28 at cd5177f)
>>  + completion: ignore custom merge strategies when pre-generating
>>  (merged to 'next' on 2009-10-22 at f46a28a)
>>  + bug: precomputed completion includes scripts sources
>>  (merged to 'next' on 2009-10-14 at adf722a)
>>  + Speedup bash completion loading
>>
>> What's the status of this thing?  Last time I polled the list I had an
>> impression that it was not quite ready...

As a distro user, I don’t think I would be able to use it until there
is a command to update the installed completion, to call after adding
a new git command to my $PATH.  This could mean:

 - git-completion.bash.generate learns to read the .in file and
   write the completion script to arbitrary paths (or just always
   uses stdin and stdout?)

 - distros install git-completion.bash.{generate,in} to /usr/share/git-core

 - distros install a simple completion script to /etc/bash_completion.d
   that passes the buck, e.g.

-- %< --
# bash completion support for core Git.
#
# Run update-git-completion to generate these files.
#
__git_user_completion=~/.cache/git-core/git-completion.bash
__git_system_completion=/var/cache/git-core/git-completion.bash

if test -r "$__git_user_completion"
then
	. "$__git_user_completion"
elif test -r "$__git_system_completion"
then
	. "$__git_system_completion"
fi
-- >% --

 - new update-git-completion script, something like this:

-- %< --
#!/bin/sh
USAGE="update-git-completion {--system | --user | <filename>}"
datadir=/usr/share/git-core
die() {
	echo >&2 "$*"
	exit 1
}

if ! test $# -eq 1
then
	die "usage: $USAGE"
fi

if test "$1" = "--system"
then
	output=/var/cache/git-core/git-completion.bash
elif test "$1" = "--user"
then
	output=$HOME/.cache/git-core/git-completion.bash
else
	output=$1
fi

rm -f "$output+" || die "cannot remove $output+"
sh "$datadir"/git-completion.bash.generate \
	< "$datadir"/git-completion.bash.in \
	> "$output+" || die "failed to generate completion script"
bash -n "$output+" || {
	rm -f "$output+"
	die "generated script fails syntax check"
}
mv -f "$output+" "$output" || {
	rm -f "$output+"
	die "failed to install completion script"
}
-- >% --

Thoughts?

[PATCH] give priority to progress messages

[Nicolas Pitre]

In theory it is possible for sideband channel #2 to be delayed if
pack data is quick to come up for sideband channel #1.  And because
data for channel #2 is read only 128 bytes at a time while pack data
is read 8192 bytes at a time, it is possible for many pack blocks to
be sent to the client before the progress message fifo is emptied,
making the situation even worse.  This would result in totally garbled
progress display on the client's console as local progress gets mixed
with partial remote progress lines.

Let's prevent such situations by giving transmission priority to 
progress messages over pack data at all times.

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

On Wed, 11 Nov 2009, Petr Baudis wrote:

> On Wed, Nov 11, 2009 at 04:19:44PM -0500, Nicolas Pitre wrote:
> > On Wed, 11 Nov 2009, Petr Baudis wrote:
> > 
> > > On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre wrote:
> > > > According to strace, data from sideband channel #2 (prefixed with 
> > > > "remote: ") pertaining to object compression is printed way after pack 
> > > > data has already started to arrive locally.  This is really weird.
> > > > 
> > > > And this occurs only when fetching from repo.or.cz and not from 
> > > > git.kernel.org for example.  So there is something to investigate on the 
> > > > server side.  Pasky: anything you changed in your git installation 
> > > > lately?
> > > 
> > > Yes, but nothing should have changed in git-daemon, that's the only part
> > > of the infrastructure that uses system-wide git (which it perhaps
> > > shouldn't). I cannot reproduce this problem, though. I have changed
> > > git-daemon to use my local git version (about one week old master), does
> > > this still happen for you?
> > 
> > No, it doesn't happen anymore.
> > 
> > What was the git-daemon version before?
> 
> 1.5.6.5, the default version in debian lenny

I don't see why the issue couldn't happen with latest git as well.
This patch however would plug any possibilities for this to happen again 
though.

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index c4cd1e1..29446e8 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -132,7 +132,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 
 	while (1) {
 		struct pollfd pfd[2];
-		ssize_t processed[2] = { 0, 0 };
 		int status;
 
 		pfd[0].fd = fd1[0];
@@ -147,15 +146,14 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 			}
 			continue;
 		}
-		if (pfd[0].revents & POLLIN)
-			/* Data stream ready */
-			processed[0] = process_input(pfd[0].fd, 1);
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			processed[1] = process_input(pfd[1].fd, 2);
-		/* Always finish to read data when available */
-		if (processed[0] || processed[1])
-			continue;
+			if (process_input(pfd[1].fd, 2))
+				continue;
+		if (pfd[0].revents & POLLIN)
+			/* Data stream ready */
+			if (process_input(pfd[0].fd, 1))
+				continue;
 
 		if (waitpid(writer, &status, 0) < 0)
 			error_clnt("%s", lostchild);
diff --git a/upload-pack.c b/upload-pack.c
index 70badcf..6bfb500 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -310,6 +310,23 @@ static void create_pack_file(void)
 			}
 			continue;
 		}
+		if (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {
+			/* Status ready; we ship that in the side-band
+			 * or dump to the standard error.
+			 */
+			sz = xread(pack_objects.err, progress,
+				  sizeof(progress));
+			if (0 < sz)
+				send_client_data(2, progress, sz);
+			else if (sz == 0) {
+				close(pack_objects.err);
+				pack_objects.err = -1;
+			}
+			else
+				goto fail;
+			/* give priority to status messages */
+			continue;
+		}
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
 			/* Data ready; we keep the last byte to ourselves
 			 * in case we detect broken rev-list, so that we
@@ -347,21 +364,6 @@ static void create_pack_file(void)
 			if (sz < 0)
 				goto fail;
 		}
-		if (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {
-			/* Status ready; we ship that in the side-band
-			 * or dump to the standard error.
-			 */
-			sz = xread(pack_objects.err, progress,
-				  sizeof(progress));
-			if (0 < sz)
-				send_client_data(2, progress, sz);
-			else if (sz == 0) {
-				close(pack_objects.err);
-				pack_objects.err = -1;
-			}
-			else
-				goto fail;
-		}
 	}
 
 	if (finish_command(&pack_objects)) {

Re: ks/precompute-completion

[Stephen Boyd]

Jonathan Nieder wrote:
>
> As a distro user, I don't think I would be able to use it until there
> is a command to update the installed completion, to call after adding
> a new git command to my $PATH.  This could mean:
>
>  - git-completion.bash.generate learns to read the .in file and
>    write the completion script to arbitrary paths (or just always
>    uses stdin and stdout?)
>
>  - distros install git-completion.bash.{generate,in} to /usr/share/git-core
>
>  - distros install a simple completion script to /etc/bash_completion.d
>    that passes the buck, e.g.
>
> <snip scripts>
>
> Thoughts?

I'm confused. Shouldn't your distro take care of updating the completion 
for you? And wouldn't update-git-completion be more suited as part of a 
makefile if it was needed at all?

Re: ks/precompute-completion

[Jonathan Nieder]

Stephen Boyd wrote:
> Jonathan Nieder wrote:
>>
>>As a distro user, I don't think I would be able to use it until there
>>is a command to update the installed completion, to call after adding
>>a new git command to my $PATH.  This could mean:
[...]
> I'm confused. Shouldn't your distro take care of updating the
> completion for you? And wouldn't update-git-completion be more
> suited as part of a makefile if it was needed at all?

The problem is that I have git commands the distro did not install in
my $PATH.  For example, I currently have git-new-workdir in ~/bin, and
once I bzr-fastexport works a little better, I will install git-bzr.

Even without such commands, in many distributions the completion
should not be one size fits all, since git-svn (for example) belongs
to a different package.

I would expect the distribution to take care of populating the initial
completion list and updating it on upgrades.

Jonathan

Re: ks/precompute-completion

[Stephen Boyd]

Jonathan Nieder wrote:
> Stephen Boyd wrote:
>> I'm confused. Shouldn't your distro take care of updating the
>> completion for you? And wouldn't update-git-completion be more
>> suited as part of a makefile if it was needed at all?
>
> The problem is that I have git commands the distro did not install in
> my $PATH.  For example, I currently have git-new-workdir in ~/bin, and
> once I bzr-fastexport works a little better, I will install git-bzr.
>
> Even without such commands, in many distributions the completion
> should not be one size fits all, since git-svn (for example) belongs
> to a different package

Ah ok. I think this proves even more that pregenerating the completion 
is a bad idea. With dynamic population we don't have these problems and 
it only takes 250ms more to load on a P3 700Mhz.

Maybe we should try and speedup 'git help -a' and 'git merge -s help' 
instead? Perhaps options for the command/porcelain lists and available 
strategies formatted for script consumption? I doubt it will be as fast 
as compiling, but every bit helps apparently.

Side Note: Running git merge -s help outside a git repository fails, so 
caching of the merge strategies isn't very effective.

[PATCH] Speed up bash completion loading

[Jonathan Nieder]

On my slow laptop (P3 600MHz), system-wide bash completions take
too much time to load (> 2s), and a significant fraction of this
time is spent loading git-completion.bash:

$ time bash -c ". ./git-completion.bash"  # hot cache, before this patch

real    0m0.509s
user    0m0.310s
sys     0m0.180s

Kirill tracked down that the most time is spent warming up
merge_strategy, all_command & porcelain_command caches.

Since git is not used in each and every interactive xterm, it
seems best to load completion support with cold caches, and then
load each needed thing lazily.  This has most of the speed
advantage of pre-generating everything at build time, without the
complication of figuring out at build time what commands will be
available at run time.

The result is that loading completion is significantly faster
now:

$ time bash -c ". ./git-completion.bash"  # cold cache, after this patch

real    0m0.171s
user    0m0.060s
sys     0m0.040s

Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
Cc: Shawn O. Pearce <spearce@spearce.org>
Cc: Stephen Boyd <bebarino@gmail.com>
Cc: Sverre Rabbelier <srabbelier@gmail.com>
Cc: Junio C Hamano <junio@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Stephen Boyd wrote:

> Ah ok. I think this proves even more that pregenerating the
> completion is a bad idea. With dynamic population we don't have
> these problems and it only takes 250ms more to load on a P3 700Mhz.

Hmm, 250 ms is a lot.

Strictly speaking, even with the existing completion script, it might
be nice to provide a "hash -r"-like command to bring the caches up to
date.  Such a function could be:

git-completion-rehash ()
{
	__git_compute_all_commands
	__git_compute_merge_strategies
	__git_compute_porcelain_commands
}

But that is a separate topic.

 contrib/completion/git-completion.bash |   35 +++++++++++++++++--------------
 1 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9e8b607..7088ec7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -330,10 +330,9 @@ __git_list_merge_strategies ()
 	}'
 }
 
-__git_merge_strategies ()
+__git_compute_merge_strategies ()
 {
-	: ${__git_merge_strategylist:=$(__git_list_merge_strategies)}
-	echo "$__git_merge_strategylist"
+	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
 }
 
 __git_complete_file ()
@@ -468,15 +467,16 @@ __git_complete_remote_or_refspec ()
 
 __git_complete_strategy ()
 {
+	__git_compute_merge_strategies
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategies"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategies" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
@@ -495,16 +495,16 @@ __git_list_all_commands ()
 	done
 }
 
-__git_all_commands ()
+__git_compute_all_commands ()
 {
-	: ${__git_all_commandlist:=$(__git_list_all_commands)}
-	echo "$__git_all_commandlist"
+	: ${__git_all_commands=$(__git_list_all_commands)}
 }
 
 __git_list_porcelain_commands ()
 {
 	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
+	__git_compute_all_commands
+	for i in "help" $__git_all_commands
 	do
 		case $i in
 		*--*)             : helper pattern;;
@@ -586,10 +586,9 @@ __git_list_porcelain_commands ()
 	done
 }
 
-__git_porcelain_commands ()
+__git_compute_porcelain_commands ()
 {
-	: ${__git_porcelain_commandlist:=$(__git_list_porcelain_commands)}
-	echo "$__git_porcelain_commandlist"
+	: ${__git_porcelain_commands=$(__git_list_porcelain_commands)}
 }
 
 __git_aliases ()
@@ -1082,7 +1081,8 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__git_compute_all_commands
+	__gitcomp "__git_all_commands
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1438,7 +1438,8 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__git_compute_merge_strategies
+		__gitcomp "$__git_merge_strategies"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1539,7 +1540,8 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__git_compute_all_commands
+		__gitcomp "__git_all_commands" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2136,7 +2138,8 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __git_compute_porcelain_commands
+		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.2

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

On my slow laptop (P3 600MHz), system-wide bash completions take
too much time to load (> 2s), and a significant fraction of this
time is spent loading git-completion.bash:

$ time bash -c ". ./git-completion.bash"  # hot cache, before this patch

real    0m0.509s
user    0m0.310s
sys     0m0.180s

Kirill noticed that most of the time is spent warming up
merge_strategy, all_command & porcelain_command caches.

Since git is not used in each and every interactive xterm, it
seems best to load completion support with cold caches, and then
load each needed thing lazily.  This has most of the speed
advantage of pre-generating everything at build time, without the
complication of figuring out at build time what commands will be
available at run time.

The result is that loading completion is significantly faster
now:

$ time bash -c ". ./git-completion.bash"  # cold cache, after this patch

real    0m0.171s
user    0m0.060s
sys     0m0.040s

Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
Cc: Shawn O. Pearce <spearce@spearce.org>
Cc: Stephen Boyd <bebarino@gmail.com>
Cc: Sverre Rabbelier <srabbelier@gmail.com>
Cc: Junio C Hamano <junio@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
> Stephen Boyd wrote:

>> Ah ok. I think this proves even more that pregenerating the
>> completion is a bad idea. With dynamic population we don't have
>> these problems and it only takes 250ms more to load on a P3
>> 700Mhz.
>
> Hmm, 250 ms is a lot.

Here’s the real patch.  Sorry about that.

Jonathan

 contrib/completion/git-completion.bash |   67 +++++++++++++++----------------
 1 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e3ddecc..7088ec7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -324,12 +318,8 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
+__git_list_merge_strategies ()
 {
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
 	git merge -s help 2>&1 |
 	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
 		s/\.$//
@@ -339,8 +329,11 @@ __git_merge_strategies ()
 		p
 	}'
 }
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
+
+__git_compute_merge_strategies ()
+{
+	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
+}
 
 __git_complete_file ()
 {
@@ -474,27 +467,24 @@ __git_complete_remote_or_refspec ()
 
 __git_complete_strategy ()
 {
+	__git_compute_merge_strategies
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategies"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategies" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
+__git_list_all_commands ()
 {
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
 	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
 	do
@@ -504,17 +494,17 @@ __git_all_commands ()
 		esac
 	done
 }
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
-__git_porcelain_commands ()
+__git_compute_all_commands ()
+{
+	: ${__git_all_commands=$(__git_list_all_commands)}
+}
+
+__git_list_porcelain_commands ()
 {
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
+	__git_compute_all_commands
+	for i in "help" $__git_all_commands
 	do
 		case $i in
 		*--*)             : helper pattern;;
@@ -595,8 +585,11 @@ __git_porcelain_commands ()
 		esac
 	done
 }
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
+
+__git_compute_porcelain_commands ()
+{
+	: ${__git_porcelain_commands=$(__git_list_porcelain_commands)}
+}
 
 __git_aliases ()
 {
@@ -1088,7 +1081,8 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__git_compute_all_commands
+	__gitcomp "__git_all_commands
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1444,7 +1438,8 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__git_compute_merge_strategies
+		__gitcomp "$__git_merge_strategies"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1545,7 +1540,8 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__git_compute_all_commands
+		__gitcomp "__git_all_commands" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2142,7 +2138,8 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __git_compute_porcelain_commands
+		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.2

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

Jonathan Nieder wrote:
> @@ -1545,7 +1540,8 @@ _git_config ()
>  	pager.*)
>  		local pfx="${cur%.*}."
>  		cur="${cur#*.}"
> -		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
> +		__git_compute_all_commands
> +		__gitcomp "__git_all_commands" "$pfx" "$cur"
>  		return
>  		;;
>  	remote.*.*)

s/__git_all_commands/\$__git_all_commands/

Yikes.  Next patch I’ll let sit for a day.

Good night,
Jonathan

Re: [PATCH] Speed up bash completion loading

[Stephen Boyd]

Jonathan Nieder wrote:
> On my slow laptop (P3 600MHz), system-wide bash completions take
> too much time to load (> 2s), and a significant fraction of this
> time is spent loading git-completion.bash:
>   
[...]
> Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
> Cc: Shawn O. Pearce <spearce@spearce.org>
> Cc: Stephen Boyd <bebarino@gmail.com>
> Cc: Sverre Rabbelier <srabbelier@gmail.com>
> Cc: Junio C Hamano <junio@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

I was under the impression that setting variables during completion 
meant they were lost at the end of the completion cycle. So to be safe I 
put a 'sleep 5' in __git_list_porcelain_commands() and it only stalled 
me once :-)

I see a small problem, but it can be fixed in another patch. git merge 
-s <TAB><TAB> the first time when you're not in a git directory will 
make git merge -s <TAB><TAB> after never complete correctly even in a 
git directory. I guess this become more serious if git isn't in your 
path initially and you do git <TAB><TAB> and then git becomes part of 
your path and you try again. Here you lose porcelain completion. This is 
probably never going to happen though, right?

Plus it seems that __git_all_commands is computed twice if I git 
<TAB><TAB> and then git help <TAB><TAB> after. Can that be avoided?

Squashing this on top fixes the two typos you mentioned.

---->8----

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7088ec7..6817953 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1082,7 +1082,7 @@ _git_help ()
                ;;
        esac
        __git_compute_all_commands
-       __gitcomp "__git_all_commands
+       __gitcomp "$__git_all_commands
                attributes cli core-tutorial cvs-migration
                diffcore gitk glossary hooks ignore modules
                repository-layout tutorial tutorial-2
@@ -1541,7 +1541,7 @@ _git_config ()
                local pfx="${cur%.*}."
                cur="${cur#*.}"
                __git_compute_all_commands
-               __gitcomp "__git_all_commands" "$pfx" "$cur"
+               __gitcomp "$__git_all_commands" "$pfx" "$cur"
                return
                ;;
        remote.*.*)

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

Stephen Boyd wrote:

> I was under the impression that setting variables during completion
> meant they were lost at the end of the completion cycle. So to be
> safe I put a 'sleep 5' in __git_list_porcelain_commands() and it
> only stalled me once :-)

Clever. :)

> Plus it seems that __git_all_commands is computed twice if I git
> <TAB><TAB> and then git help <TAB><TAB> after. Can that be avoided?

Sounds like a bug; thanks.  I’ll squash in something like the following
for the next iteration.

-- %< --
Subject: completion: avoid computing command list twice

__git_all_commands is being computed twice on git <TAB><TAB> with
git help <TAB><TAB> after.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6817953..748d4f9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -588,6 +588,7 @@ __git_list_porcelain_commands ()
 
 __git_compute_porcelain_commands ()
 {
+	__git_compute_all_commands
 	: ${__git_porcelain_commands=$(__git_list_porcelain_commands)}
 }
 
-- 
1.6.5.2

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

Stephen Boyd wrote:

> I see a small problem, but it can be fixed in another patch. git
> merge -s <TAB><TAB> the first time when you're not in a git
> directory will make git merge -s <TAB><TAB> after never complete
> correctly even in a git directory.

Not good.  I think the sanest thing to do is avoid caching the merge
strategy list entirely.  Listing merge strategies takes about 100 ms
here, which is short enough not to be annoying.

> I guess this become more serious
> if git isn't in your path initially and you do git <TAB><TAB> and
> then git becomes part of your path and you try again. Here you lose
> porcelain completion. This is probably never going to happen though,
> right?

Right, if this happened to me I would not be too surprised.  A similar
problem occurs when someone installs a new git subcommand in the
middle of a session.  Starting a new session fixes the completion, but
should the completion script do something to help people before then?

It could provide a shell function with a slightly friendlier name than
"__git_compute_porcelain_commands" for the user to invoke explicitly.

It could retry "git help -a" each time completion was needed if it
gave no results last time (i.e. use "${__git_all_commands:=" instead
of "${__git_all_commands=").  This would help with the missing git
problem (which seems unusual), but not the missing git-svn.

Maybe it could detect signs of user frustration (repeated attempts to
complete the same thing?) and add to the frustration by silently
fixing the cache.

-- %< --
Subject: completion: do not cache merge strategy list

If "git merge -s help" fails the first time we try it (e.g. if you're
not in a git directory), git merge -s <TAB><TAB> never completes
correctly within the same session.

Reported-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 748d4f9..634941f 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -332,7 +332,7 @@ __git_list_merge_strategies ()
 
 __git_compute_merge_strategies ()
 {
-	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
+	__git_merge_strategies=$(__git_list_merge_strategies)
 }
 
 __git_complete_file ()
-- 
1.6.5.2

Re: [PATCH] Speed up bash completion loading

[SZEDER Gábor]

Hi,


On Sat, Nov 14, 2009 at 05:01:41AM -0600, Jonathan Nieder wrote:
> Stephen Boyd wrote:
> 
> > I see a small problem, but it can be fixed in another patch. git
> > merge -s <TAB><TAB> the first time when you're not in a git
> > directory will make git merge -s <TAB><TAB> after never complete
> > correctly even in a git directory.
> 
> Not good.  I think the sanest thing to do is avoid caching the merge
> strategy list entirely.  Listing merge strategies takes about 100 ms
> here, which is short enough not to be annoying.
> 
> > I guess this become more serious
> > if git isn't in your path initially and you do git <TAB><TAB> and
> > then git becomes part of your path and you try again. Here you lose
> > porcelain completion. This is probably never going to happen though,
> > right?
> 
> Right, if this happened to me I would not be too surprised.  A similar
> problem occurs when someone installs a new git subcommand in the
> middle of a session.  Starting a new session fixes the completion, but
> should the completion script do something to help people before then?
> 
> It could provide a shell function with a slightly friendlier name than
> "__git_compute_porcelain_commands" for the user to invoke explicitly.
> 
> It could retry "git help -a" each time completion was needed if it
> gave no results last time (i.e. use "${__git_all_commands:=" instead
> of "${__git_all_commands=").  This would help with the missing git
> problem (which seems unusual), but not the missing git-svn.
> 
> Maybe it could detect signs of user frustration (repeated attempts to
> complete the same thing?) and add to the frustration by silently
> fixing the cache.

Why?  Don't get overly creative here, the command

  . /path/to/git-completion.bash

already does that, plus it fixes the merge strategy completion issue,
and it's friendly enough for the users.


Best,
Gábor

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

Hi Gábor,

SZEDER Gábor wrote:

> Why?  Don't get overly creative here, the command
> 
>   . /path/to/git-completion.bash
> 
> already does that, plus it fixes the merge strategy completion issue,
> and it's friendly enough for the users.

Sounds like a good approach.  Squashing this in should get that
working again.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
In this patch, I assume the merge strategy list is not being cached
any more.  Something like this would allow recovering from the merge
strategy completion issue, but the victim would have to notice what
went wrong first.

 contrib/completion/git-completion.bash |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 634941f..ae39373 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -495,6 +495,7 @@ __git_list_all_commands ()
 	done
 }
 
+unset __git_all_commands
 __git_compute_all_commands ()
 {
 	: ${__git_all_commands=$(__git_list_all_commands)}
@@ -586,6 +587,7 @@ __git_list_porcelain_commands ()
 	done
 }
 
+unset __git_porcelain_commands
 __git_compute_porcelain_commands ()
 {
 	__git_compute_all_commands
-- 
1.6.5.2

Re: [PATCH] Speed up bash completion loading

[Stephen Boyd]

On Sat, 2009-11-14 at 05:01 -0600, Jonathan Nieder wrote:
> Stephen Boyd wrote:
> 
> > I see a small problem, but it can be fixed in another patch. git
> > merge -s <TAB><TAB> the first time when you're not in a git
> > directory will make git merge -s <TAB><TAB> after never complete
> > correctly even in a git directory.
> 
> Not good.  I think the sanest thing to do is avoid caching the merge
> strategy list entirely.  Listing merge strategies takes about 100 ms
> here, which is short enough not to be annoying.
> 

I was thinking of adding an option to git merge (like --strategies) to
list the strategies without requiring you to be in a git directory. It
doesn't look that easy at first glance so it might not be worth the
trouble.

Re: excerpts from tomorrow's "What's cooking" draft

[Sverre Rabbelier]

Heya,

On Wed, Nov 11, 2009 at 19:45, Daniel Barkalow <barkalow@iabervon.org> wrote:
> Okay, finally got to it just now. This is entirely untested (because I
> don't have anything that uses it), but it compiles and should be correct.
> It would be nice to get a clean bill of health on it from valgrind.

After a lot of manual comparison of valgrind traces (comparing cloning
a local git repo and a local hg repo), I have found the following
before applying your patch (I removed the ==proc== and "by 0xDEADBEEF"
noise):

  20 bytes in 1 blocks are definitely lost in loss record 24 of 95
    at : calloc (vg_replace_malloc.c:418)
    by : xcalloc (wrapper.c:75)
    by : get_importer (transport-helper.c:132)
    by : fetch_with_import (transport-helper.c:150)
    by : fetch (transport-helper.c:198)
    by : transport_fetch_refs (transport.c:977)
    by : cmd_clone (builtin-clone.c:538)
    by : run_builtin (git.c:251)
    by : handle_internal_command (git.c:396)
    by : run_argv (git.c:438)
    by : main (git.c:509)

  60 bytes in 1 blocks are definitely lost in loss record 43 of 95
    at : realloc (vg_replace_malloc.c:476)
    by : xrealloc (wrapper.c:59)
    by : strbuf_grow (strbuf.c:61)
    by : strbuf_getwholeline (strbuf.c:335)
    by : strbuf_getline (strbuf.c:349)
    by : get_helper (transport-helper.c:52)
    by : get_refs_list (transport-helper.c:228)
    by : transport_get_remote_refs (transport.c:943)
    by : cmd_clone (builtin-clone.c:535)
    by : run_builtin (git.c:251)
    by : handle_internal_command (git.c:396)
    by : run_argv (git.c:438)

  65 bytes in 1 blocks are definitely lost in loss record 47 of 95
    at : malloc (vg_replace_malloc.c:195)
    by : realloc (vg_replace_malloc.c:476)
    by : xrealloc (wrapper.c:59)
    by : strbuf_grow (strbuf.c:61)
    by : strbuf_addf (strbuf.c:199)
    by : get_helper (transport-helper.c:69)
    by : get_refs_list (transport-helper.c:228)
    by : transport_get_remote_refs (transport.c:943)
    by : cmd_clone (builtin-clone.c:535)
    by : run_builtin (git.c:251)
    by : handle_internal_command (git.c:396)
    by : run_argv (git.c:438)

  65 bytes in 1 blocks are definitely lost in loss record 50 of 95
    at : malloc (vg_replace_malloc.c:195)
    by : realloc (vg_replace_malloc.c:476)
    by : xrealloc (wrapper.c:59)
    by : strbuf_grow (strbuf.c:61)
    by : strbuf_addf (strbuf.c:199)
    by : fetch_with_import (transport-helper.c:158)
    by : fetch (transport-helper.c:198)
    by : transport_fetch_refs (transport.c:977)
    by : cmd_clone (builtin-clone.c:538)
    by : run_builtin (git.c:251)
    by : handle_internal_command (git.c:396)
    by : run_argv (git.c:438)

  123 (96 direct, 27 indirect) bytes in 1 blocks are definitely lost
in loss record 71 of 95
    at : malloc (vg_replace_malloc.c:195)
    by : realloc (vg_replace_malloc.c:476)
    by : xrealloc (wrapper.c:59)
    by : get_helper (transport-helper.c:62)
    by : get_refs_list (transport-helper.c:228)
    by : transport_get_remote_refs (transport.c:943)
    by : cmd_clone (builtin-clone.c:535)
    by : run_builtin (git.c:251)
    by : handle_internal_command (git.c:396)
    by : run_argv (git.c:438)
    by : main (git.c:509)

Annotated with the relevant source and my comments:
    by : get_importer (transport-helper.c:132)
	fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));

You never free fastimport->argv.

    by : get_helper (transport-helper.c:52)
		if (strbuf_getline(&buf, file, '\n') == EOF)

You never strbuf_release(&buf).

    by : get_helper (transport-helper.c:69)
			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());

I do strbuf_reset(&gitdir), which should of course be
strbuf_release(&gitdir), oops.

    by : fetch_with_import (transport-helper.c:158)
		strbuf_addf(&buf, "import %s\n", posn->name);

Same here, you never strbuf_release(&buf).

    by : get_helper (transport-helper.c:62)
			ALLOC_GROW(refspecs,
				   refspec_nr + 1,
				   refspec_alloc);

This would of course be fixed by your patch.

However, applying your patch causes:
error: Trying to write ref refs/heads/tip with nonexistant object
0000000000000000000000000000000000000000
fatal: Cannot update the ref 'HEAD'.

If I comment out both new lines in disonnect_helper like so fixes that:
	//free_refspec(data->refspec_nr, data->refspecs);
	//data->refspecs = NULL;

Commenting out only one of the two makes the error return, any idea's?

-- 
Cheers,

Sverre Rabbelier

Re: excerpts from tomorrow's "What's cooking" draft

[Daniel Barkalow]

On Sun, 15 Nov 2009, Sverre Rabbelier wrote:

> Heya,
> 
> On Wed, Nov 11, 2009 at 19:45, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > Okay, finally got to it just now. This is entirely untested (because I
> > don't have anything that uses it), but it compiles and should be correct.
> > It would be nice to get a clean bill of health on it from valgrind.
> 
> After a lot of manual comparison of valgrind traces (comparing cloning
> a local git repo and a local hg repo), I have found the following
> before applying your patch (I removed the ==proc== and "by 0xDEADBEEF"
> noise):
> 
>   20 bytes in 1 blocks are definitely lost in loss record 24 of 95
>     at : calloc (vg_replace_malloc.c:418)
>     by : xcalloc (wrapper.c:75)
>     by : get_importer (transport-helper.c:132)
>     by : fetch_with_import (transport-helper.c:150)
>     by : fetch (transport-helper.c:198)
>     by : transport_fetch_refs (transport.c:977)
>     by : cmd_clone (builtin-clone.c:538)
>     by : run_builtin (git.c:251)
>     by : handle_internal_command (git.c:396)
>     by : run_argv (git.c:438)
>     by : main (git.c:509)
> 
>   60 bytes in 1 blocks are definitely lost in loss record 43 of 95
>     at : realloc (vg_replace_malloc.c:476)
>     by : xrealloc (wrapper.c:59)
>     by : strbuf_grow (strbuf.c:61)
>     by : strbuf_getwholeline (strbuf.c:335)
>     by : strbuf_getline (strbuf.c:349)
>     by : get_helper (transport-helper.c:52)
>     by : get_refs_list (transport-helper.c:228)
>     by : transport_get_remote_refs (transport.c:943)
>     by : cmd_clone (builtin-clone.c:535)
>     by : run_builtin (git.c:251)
>     by : handle_internal_command (git.c:396)
>     by : run_argv (git.c:438)
> 
>   65 bytes in 1 blocks are definitely lost in loss record 47 of 95
>     at : malloc (vg_replace_malloc.c:195)
>     by : realloc (vg_replace_malloc.c:476)
>     by : xrealloc (wrapper.c:59)
>     by : strbuf_grow (strbuf.c:61)
>     by : strbuf_addf (strbuf.c:199)
>     by : get_helper (transport-helper.c:69)
>     by : get_refs_list (transport-helper.c:228)
>     by : transport_get_remote_refs (transport.c:943)
>     by : cmd_clone (builtin-clone.c:535)
>     by : run_builtin (git.c:251)
>     by : handle_internal_command (git.c:396)
>     by : run_argv (git.c:438)
> 
>   65 bytes in 1 blocks are definitely lost in loss record 50 of 95
>     at : malloc (vg_replace_malloc.c:195)
>     by : realloc (vg_replace_malloc.c:476)
>     by : xrealloc (wrapper.c:59)
>     by : strbuf_grow (strbuf.c:61)
>     by : strbuf_addf (strbuf.c:199)
>     by : fetch_with_import (transport-helper.c:158)
>     by : fetch (transport-helper.c:198)
>     by : transport_fetch_refs (transport.c:977)
>     by : cmd_clone (builtin-clone.c:538)
>     by : run_builtin (git.c:251)
>     by : handle_internal_command (git.c:396)
>     by : run_argv (git.c:438)
> 
>   123 (96 direct, 27 indirect) bytes in 1 blocks are definitely lost
> in loss record 71 of 95
>     at : malloc (vg_replace_malloc.c:195)
>     by : realloc (vg_replace_malloc.c:476)
>     by : xrealloc (wrapper.c:59)
>     by : get_helper (transport-helper.c:62)
>     by : get_refs_list (transport-helper.c:228)
>     by : transport_get_remote_refs (transport.c:943)
>     by : cmd_clone (builtin-clone.c:535)
>     by : run_builtin (git.c:251)
>     by : handle_internal_command (git.c:396)
>     by : run_argv (git.c:438)
>     by : main (git.c:509)
> 
> Annotated with the relevant source and my comments:
>     by : get_importer (transport-helper.c:132)
> 	fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));
> 
> You never free fastimport->argv.
> 
>     by : get_helper (transport-helper.c:52)
> 		if (strbuf_getline(&buf, file, '\n') == EOF)
> 
> You never strbuf_release(&buf).
> 
>     by : get_helper (transport-helper.c:69)
> 			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());
> 
> I do strbuf_reset(&gitdir), which should of course be
> strbuf_release(&gitdir), oops.
> 
>     by : fetch_with_import (transport-helper.c:158)
> 		strbuf_addf(&buf, "import %s\n", posn->name);
> 
> Same here, you never strbuf_release(&buf).
> 
>     by : get_helper (transport-helper.c:62)
> 			ALLOC_GROW(refspecs,
> 				   refspec_nr + 1,
> 				   refspec_alloc);
> 
> This would of course be fixed by your patch.
> 
> However, applying your patch causes:
> error: Trying to write ref refs/heads/tip with nonexistant object
> 0000000000000000000000000000000000000000
> fatal: Cannot update the ref 'HEAD'.
> 
> If I comment out both new lines in disonnect_helper like so fixes that:
> 	//free_refspec(data->refspec_nr, data->refspecs);
> 	//data->refspecs = NULL;
> 
> Commenting out only one of the two makes the error return, any idea's?

Looks like I'm discarding the information before I use it, instead of 
after. Try moving those lines to release_helper(), and a "!data->refspecs 
&&" to the !prefixcmp(... "refspec ") if condition. (That is, keep the 
refspecs from the first call to the helper until the whole thing is 
released, and ignore those capabilities in later calls.)

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Speed up bash completion loading

[Jonathan Nieder]

Stephen Boyd wrote:

> I was thinking of adding an option to git merge (like --strategies) to
> list the strategies without requiring you to be in a git directory. It
> doesn't look that easy at first glance so it might not be worth the
> trouble.

Right, that would require support from run_builtin().

Re: [PATCH] Speed up bash completion loading

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>  __git_compute_merge_strategies ()
>  {
> -	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
> +	__git_merge_strategies=$(__git_list_merge_strategies)

Wouldn't

	: ${__gms:=$(command)}

run command only until it gives a non-empty string?

[PATCH v2] Speed up bash completion loading

[Jonathan Nieder]

On my slow laptop (P3 600MHz), system-wide bash completions take
too much time to load (> 2s), and a significant fraction of this
time is spent loading git-completion.bash:

$ time bash -c ". ./git-completion.bash"  # hot cache, before this patch

real    0m0.509s
user    0m0.310s
sys     0m0.180s

Kirill noticed that most of the time is spent warming up the
merge_strategy, all_command and porcelain_command caches.

Since git is not used in each and every interactive xterm, it
seems best to load completion support with cold caches and then
load each needed thing lazily.  This has most of the speed
advantage of pre-generating everything at build time, without the
complication of figuring out at build time what commands will be
available at run time.

The result is that loading completion is significantly faster
now:

$ time bash -c ". ./git-completion.bash"  # cold cache, after this patch

real    0m0.171s
user    0m0.060s
sys     0m0.040s

Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
Cc: Shawn O. Pearce <spearce@spearce.org>
Cc: Stephen Boyd <bebarino@gmail.com>
Cc: SZEDER Gábor <szeder@ira.uka.de>
Cc: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This version incorporates the small improvements previously sent, plus the
following nice one (which makes caching merge strategies safe again).
Thanks for the advice, everyone.

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >  __git_compute_merge_strategies ()
> >  {
> > -	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
> > +	__git_merge_strategies=$(__git_list_merge_strategies)
> 
> Wouldn't
> 
> 	: ${__gms:=$(command)}
> 
> run command only until it gives a non-empty string?

Yes. :)

 contrib/completion/git-completion.bash |   76 +++++++++++++++++---------------
 1 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e3ddecc..43d76b7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -324,12 +318,8 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
+__git_list_merge_strategies ()
 {
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
 	git merge -s help 2>&1 |
 	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
 		s/\.$//
@@ -339,8 +329,17 @@ __git_merge_strategies ()
 		p
 	}'
 }
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
+
+unset __git_merge_strategies
+# 'git merge -s help' (and thus detection of the merge strategy
+# list) fails, unfortunately, if run outside of any git working
+# tree.  __git_merge_strategies is set to the empty string in
+# that case, and the detection will be repeated the next time it
+# is needed.
+__git_compute_merge_strategies ()
+{
+	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+}
 
 __git_complete_file ()
 {
@@ -474,27 +473,24 @@ __git_complete_remote_or_refspec ()
 
 __git_complete_strategy ()
 {
+	__git_compute_merge_strategies
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategies"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategies" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
+__git_list_all_commands ()
 {
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
 	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
 	do
@@ -504,17 +500,18 @@ __git_all_commands ()
 		esac
 	done
 }
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
-__git_porcelain_commands ()
+unset __git_all_commands
+__git_compute_all_commands ()
+{
+	: ${__git_all_commands=$(__git_list_all_commands)}
+}
+
+__git_list_porcelain_commands ()
 {
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
+	__git_compute_all_commands
+	for i in "help" $__git_all_commands
 	do
 		case $i in
 		*--*)             : helper pattern;;
@@ -595,8 +592,13 @@ __git_porcelain_commands ()
 		esac
 	done
 }
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
+
+unset __git_porcelain_commands
+__git_compute_porcelain_commands ()
+{
+	__git_compute_all_commands
+	: ${__git_porcelain_commands=$(__git_list_porcelain_commands)}
+}
 
 __git_aliases ()
 {
@@ -1088,7 +1090,8 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__git_compute_all_commands
+	__gitcomp "$__git_all_commands
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1444,7 +1447,8 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__git_compute_merge_strategies
+		__gitcomp "$__git_merge_strategies"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1545,7 +1549,8 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__git_compute_all_commands
+		__gitcomp "$__git_all_commands" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2142,7 +2147,8 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __git_compute_porcelain_commands
+		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.2

Re: [PATCH v2] Speed up bash completion loading

[Shawn O. Pearce]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Since git is not used in each and every interactive xterm, it
> seems best to load completion support with cold caches and then
> load each needed thing lazily.  This has most of the speed
> advantage of pre-generating everything at build time, without the
> complication of figuring out at build time what commands will be
> available at run time.
> 
> Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
> Cc: Shawn O. Pearce <spearce@spearce.org>

Acked-By: Shawn O. Pearce <spearce@spearce.org>

-- 
Shawn.

Re: [PATCH v2] Speed up bash completion loading

[Stephen Boyd]

Jonathan Nieder wrote:
>
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>>  __git_compute_merge_strategies ()
>>>  {
>>> -	: ${__git_merge_strategies=$(__git_list_merge_strategies)}
>>> +	__git_merge_strategies=$(__git_list_merge_strategies)
>> Wouldn't
>>
>> 	: ${__gms:=$(command)}
>>
>> run command only until it gives a non-empty string?
>
> Yes. :)

Why not do this for all of the lists and not just the merge strategies?

[PATCH v3] Speed up bash completion loading

[Jonathan Nieder]

Since git is not used in each and every interactive xterm, it
seems best to load completion support with cold caches and then
load each needed thing lazily.  This has most of the speed
advantage of pre-generating everything at build time, without the
complication of figuring out at build time what commands will be
available at run time.

On this slow laptop, this decreases the time to load
git-completion.bash from about 500 ms to about 175 ms.

Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
Cc: Stephen Boyd <bebarino@gmail.com>
Cc: SZEDER Gábor <szeder@ira.uka.de>
Cc: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I do not know whether it is kosher to carry over an ack like this.
The interdiff is small, for what it’s worth:

	$ git branch jn/faster-completion-startup ee41299
	$ git diff jn/faster-completion-startup
	diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
	index bd22802..f67254d 100755
	--- a/contrib/completion/git-completion.bash
	+++ b/contrib/completion/git-completion.bash
	@@ -330,7 +330,7 @@ __git_list_merge_strategies ()
		}'
	 }
	 
	-unset __git_merge_strategies
	+__git_merge_strategies=
	 # 'git merge -s help' (and thus detection of the merge strategy
	 # list) fails, unfortunately, if run outside of any git working
	 # tree.  __git_merge_strategies is set to the empty string in
	@@ -501,10 +501,10 @@ __git_list_all_commands ()
		done
	 }
	 
	-unset __git_all_commands
	+__git_all_commands=
	 __git_compute_all_commands ()
	 {
	-	: ${__git_all_commands=$(__git_list_all_commands)}
	+	: ${__git_all_commands:=$(__git_list_all_commands)}
	 }
	 
	 __git_list_porcelain_commands ()
	@@ -593,11 +593,11 @@ __git_list_porcelain_commands ()
		done
	 }
	 
	-unset __git_porcelain_commands
	+__git_porcelain_commands=
	 __git_compute_porcelain_commands ()
	 {
		__git_compute_all_commands
	-	: ${__git_porcelain_commands=$(__git_list_porcelain_commands)}
	+	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
	 }
	 
	 __git_aliases ()

Please let me know if I have commited a faux pas.

Stephen Boyd wrote:
>> Junio C Hamano wrote:

>>> Wouldn't
>>>
>>>	: ${__gms:=$(command)}
>>>
>>> run command only until it gives a non-empty string?
> 
> Why not do this for all of the lists and not just the merge strategies?

I generally find set-if-unset a little more intuitive.  But some users
might install and try out git completion before realizing the need to
install git, and in this case set-if-empty gives better behavior.

Thanks.

 contrib/completion/git-completion.bash |   76 +++++++++++++++++---------------
 1 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e3ddecc..1223a07 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -324,12 +318,8 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
+__git_list_merge_strategies ()
 {
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
 	git merge -s help 2>&1 |
 	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
 		s/\.$//
@@ -339,8 +329,17 @@ __git_merge_strategies ()
 		p
 	}'
 }
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
+
+__git_merge_strategies=
+# 'git merge -s help' (and thus detection of the merge strategy
+# list) fails, unfortunately, if run outside of any git working
+# tree.  __git_merge_strategies is set to the empty string in
+# that case, and the detection will be repeated the next time it
+# is needed.
+__git_compute_merge_strategies ()
+{
+	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+}
 
 __git_complete_file ()
 {
@@ -474,27 +473,24 @@ __git_complete_remote_or_refspec ()
 
 __git_complete_strategy ()
 {
+	__git_compute_merge_strategies
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategies"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategies" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
+__git_list_all_commands ()
 {
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
 	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
 	do
@@ -504,17 +500,18 @@ __git_all_commands ()
 		esac
 	done
 }
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
-__git_porcelain_commands ()
+__git_all_commands=
+__git_compute_all_commands ()
+{
+	: ${__git_all_commands:=$(__git_list_all_commands)}
+}
+
+__git_list_porcelain_commands ()
 {
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
 	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
+	__git_compute_all_commands
+	for i in "help" $__git_all_commands
 	do
 		case $i in
 		*--*)             : helper pattern;;
@@ -595,8 +592,13 @@ __git_porcelain_commands ()
 		esac
 	done
 }
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
+
+__git_porcelain_commands=
+__git_compute_porcelain_commands ()
+{
+	__git_compute_all_commands
+	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+}
 
 __git_aliases ()
 {
@@ -1088,7 +1090,8 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__git_compute_all_commands
+	__gitcomp "$__git_all_commands
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1444,7 +1447,8 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__git_compute_merge_strategies
+		__gitcomp "$__git_merge_strategies"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1545,7 +1549,8 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__git_compute_all_commands
+		__gitcomp "$__git_all_commands" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2142,7 +2147,8 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __git_compute_porcelain_commands
+		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.2

Re: [PATCH v3] Speed up bash completion loading

[Shawn O. Pearce]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Since git is not used in each and every interactive xterm, it
> seems best to load completion support with cold caches and then
> load each needed thing lazily.  This has most of the speed
> advantage of pre-generating everything at build time, without the
> complication of figuring out at build time what commands will be
> available at run time.
> 
> On this slow laptop, this decreases the time to load
> git-completion.bash from about 500 ms to about 175 ms.
> 
> Suggested-by: Kirill Smelkov <kirr@mns.spb.ru>

Yup, still...

> Acked-by: Shawn O. Pearce <spearce@spearce.org>

:-)

> I do not know whether it is kosher to carry over an ack like this.
> The interdiff is small, for what it???s worth:

Usually you leave it in if all you've done is address minor reviewer
comments and they had actually supplied an Acked-by line for the
prior version.

Its also usually fine to leave it like this, because there is a
good chance that the reviewer will agree with the new version and
it saves Junio from needing to insert the line himself later.

But it would be bad form to leave an Acked-by in if there was a major
rewrite.  E.g. this particular fix has gone through some really major
rework to reach this point, keeping an Acked-by from the very first
"speedup loading" patch (which IIRC computed them at build time) into
this one would be quite unfriendly.
 
-- 
Shawn.