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

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).

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

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

On Wednesday 11 November 2009, Junio C Hamano wrote:

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

Heya,

On Wed, Nov 11, 2009 at 10:34, Junio C Hamano <junio@pobox.com> wrote:

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.

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?

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.

On Wed, 11 Nov 2009, Sverre Rabbelier 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.

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>

Heya,

On Wed, Nov 11, 2009 at 19:45, Daniel Barkalow <barkalow@iabervon.org> wrote:

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?

On Sun, 15 Nov 2009, Sverre Rabbelier wrote:

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*

Hi,

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?

Jonathan Nieder 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?

Stephen Boyd wrote:

[...]

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

Jonathan Nieder 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.

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.

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:

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(-)

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:

Here’s the real patch.  Sorry about that.

Jonathan

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

Jonathan Nieder wrote:

s/__git_all_commands/\$__git_all_commands/

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

Good night, Jonathan

Jonathan Nieder 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 :-)

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----

Stephen Boyd wrote:

Clever. :)

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(-)

Stephen Boyd wrote:

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.

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(-)

Hi,

On Sat, Nov 14, 2009 at 05:01:41AM -0600, Jonathan Nieder 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.

Best, Gábor

Hi Gábor,

SZEDER Gábor wrote:

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(-)

On Sat, 2009-11-14 at 05:01 -0600, Jonathan Nieder 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.

Stephen Boyd wrote:

Right, that would require support from run_builtin().

Jonathan Nieder <jrnieder@gmail.com> writes:

Wouldn't

	: ${__gms:=$(command)}

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

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:

Yes. :)

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

Jonathan Nieder <jrnieder@gmail.com> wrote:

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

Jonathan Nieder wrote:

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

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:

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(-)

Jonathan Nieder <jrnieder@gmail.com> wrote:

Yup, still...

:-)

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.
 

The 11/11/09, Junio C Hamano 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.

  % git --version
  git version 1.6.5.2.352.g8b8744

I can provide more test or bissect it if wanted.

On Wed, 11 Nov 2009, Nicolas Sebrecht wrote:

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

On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre 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?

On Wed, 11 Nov 2009, Petr Baudis wrote:

No, it doesn't happen anymore.

What was the git-daemon version before?

Nicolas

The 11/11/09, Nicolas Pitre wrote:

I couldn't reproduce it, me neither.

On Wed, Nov 11, 2009 at 04:19:44PM -0500, Nicolas Pitre wrote:

1.5.6.5, the default version in debian lenny

On Wed, 11 Nov 2009, Petr Baudis wrote:

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

Nicolas

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:

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.

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

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...