Re: [PATCH] bash completion: Add completion for 'git help'

Thanks for an Ack, but personally I do not think the completion of "all
commands" is worth it.

I've been busy with day-job for the past few days, and haven't had chance
to push things out today, but FYI here is what I already have on 'master'
privately, relative to what have already been pushed out.

  Documentation: clarify how to disable elements in core.whitespace (Junio C Hamano)
  Makefile: fix shell quoting (Junio C Hamano)
  tests: propagate $(TAR) down from the toplevel Makefile (Junio C Hamano)
  index-pack.c: correctly initialize appended objects (Björn Steinbrink)
  send-email: find body-encoding correctly (Peter Valdemar Mørch)
  document that git-tag can tag more than heads (Jonathan Nieder)
  perl/Makefile: update NO_PERL_MAKEMAKER section (Brandon Casey)
  bash: offer only paths after '--' for 'git checkout' (SZEDER Gábor)
  checkout: mention '--' in the docs (SZEDER Gábor)
  git-checkout: improve error messages, detect ambiguities. (Pierre Habouzit)
  update test case to protect am --skip behaviour (Olivier Marin)
  Teach fsck and prune about the new location of temporary objects (Brandon Casey)
  git-checkout: fix command line parsing. (Pierre Habouzit)

At this point immediately before -rc1, I am giving much higher precedence
to real fixes than clean-ups, "use parse-opt", or new features.  Please do
not get alarmed if your non-fix patches are left unresponded for a while.

BTW, has anybody taken a look at this one?

  Subject: BUG: fetch incorrect interpretation of globing patterns in refspecs
  Date: Thu, 24 Jul 2008 09:07:21 +0200
  Message-ID: [off-list ref]

If not, I think I probably need to take a look at this, reproducing and
possibly fixing, before applying non-fix patches.

On Fri, Jul 25, 2008 at 02:02:15PM -0700, Junio C Hamano wrote:

BTW, has anybody taken a look at this one?

  Subject: BUG: fetch incorrect interpretation of globing patterns in refspecs
  Date: Thu, 24 Jul 2008 09:07:21 +0200
  Message-ID: [off-list ref]

If not, I think I probably need to take a look at this, reproducing and
possibly fixing, before applying non-fix patches.

I have been meaning to look at it for days, so I finally took a peek.  I
was able to reproduce the problem easily. I think it is (almost) as
simple as the patch below. In the refspec parsing, we already require
globs to come after '/', so this is the analagous check during match.

Unfortunately, this breaks t1020 (something about failing to clone HEAD
it looks like, so probably it is some boundary case for matching just
"*"). I don't have time to look further, and I will be out of touch
until probably Sunday evening, so hopefully somebody else can run with
it.

---

diff --git a/remote.c b/remote.c
index 0d6020b..3ae0431 100644
--- a/remote.c
+++ b/remote.c

@@ -1108,7 +1108,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 	for (ref = remote_refs; ref; ref = ref->next) {
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
-		if (!prefixcmp(ref->name, refspec->src)) {
+		if (!prefixcmp(ref->name, refspec->src)
+		     && ref->name[remote_prefix_len] == '/') {
 			const char *match;
 			struct ref *cpy = copy_ref(ref);
 			match = ref->name + remote_prefix_len;

On Sat, Jul 26, 2008 at 04:24:05AM -0400, Jeff King wrote:

quoted

If not, I think I probably need to take a look at this, reproducing and
possibly fixing, before applying non-fix patches.

I have been meaning to look at it for days, so I finally took a peek.  I
was able to reproduce the problem easily. I think it is (almost) as
simple as the patch below. In the refspec parsing, we already require
globs to come after '/', so this is the analagous check during match.

Also, while I have your attention, Junio, here is another bug fix
that should go into 1.6.0. I posted the patch as a "how about this" deep
in a thread and got no response (which means no complaints, right?).

-- >8 --
init: handle empty "template" parameter

If a user passes "--template=", then our template parameter
is blank. Unfortunately, copy_templates() assumes it has at
least one character, and does all sorts of bad things like
reading from template[-1] and then proceeding to link all of
'/' into the .git directory.

This patch just checks for that condition in copy_templates
and aborts. As a side effect, this means that --template=
now has the meaning "don't copy any templates."
---
I don't really care about the "side effect" behavior, but it seems
reasonable. The other obvious option is to simply die(). Certainly
either is better than the current bug.

 builtin-init-db.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..baf0d09 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c

@@ -117,6 +117,8 @@ static void copy_templates(const char *template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
+	if (!template_dir[0])
+		return;
 	strcpy(template_path, template_dir);
 	template_len = strlen(template_path);
 	if (template_path[template_len-1] != '/') {

-- 
1.6.0.rc0.233.gb3fd2

Hi,

On Sat, 26 Jul 2008, Jeff King wrote:

Also, while I have your attention, Junio, here is another bug fix
that should go into 1.6.0. I posted the patch as a "how about this" deep
in a thread and got no response (which means no complaints, right?).

Again it is in a thread...

This patch just checks for that condition in copy_templates
and aborts. As a side effect, this means that --template=
now has the meaning "don't copy any templates."

I deem this patch obviously correct, and your reasoning as to what an 
empty parameter should mean makes sense.

Ciao,
Dscho

On Sat, Jul 26, 2008 at 03:13:26PM +0200, Johannes Schindelin wrote:

quoted

Also, while I have your attention, Junio, here is another bug fix
that should go into 1.6.0. I posted the patch as a "how about this" deep
in a thread and got no response (which means no complaints, right?).

Again it is in a thread...

Heh. Point taken.

My meaning was "you, Junio, did not see this because it was in another
thread, so I am pointing it out to you" but the irony of the rest of the
statement was lost on me during the original writing.

I deem this patch obviously correct, and your reasoning as to what an 
empty parameter should mean makes sense.

Thank you for reviewing, anyway. ;)

Here is a re-post with a test for the new behavior added in (and an
extra test just to make sure --template works at all. It does get used
by test-lib, so we were implicitly checking it to some degree, but it's
entirely possible that it could fail and the tests wouldn't necessarily
notice (e.g., if it accidentally used an installed set of templates
instead)).

-- >8 --
init: handle empty "template" parameter

If a user passes "--template=", then our template parameter
is blank. Unfortunately, copy_templates() assumes it has at
least one character, and does all sorts of bad things like
reading from template[-1] and then proceeding to link all of
'/' into the .git directory.

This patch just checks for that condition in copy_templates
and aborts. As a side effect, this means that --template=
now has the meaning "don't copy any templates."
---
 builtin-init-db.c |    2 ++
 t/t0001-init.sh   |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..baf0d09 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c

@@ -117,6 +117,8 @@ static void copy_templates(const char *template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
+	if (!template_dir[0])
+		return;
 	strcpy(template_path, template_dir);
 	template_len = strlen(template_path);
 	if (template_path[template_len-1] != '/') {

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 2a38d98..620da5b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh

@@ -141,4 +141,30 @@ test_expect_success 'reinit' '
 	test_cmp again/empty again/err2
 '
 
+test_expect_success 'init with --template' '
+	mkdir template-source &&
+	echo content >template-source/file &&
+	(
+		mkdir template-custom &&
+		cd template-custom &&
+		git init --template=../template-source
+	) &&
+	test_cmp template-source/file template-custom/.git/file
+'
+
+test_expect_success 'init with --template (blank)' '
+	(
+		mkdir template-plain &&
+		cd template-plain &&
+		git init
+	) &&
+	test -f template-plain/.git/info/exclude &&
+	(
+		mkdir template-blank &&
+		cd template-blank &&
+		git init --template=
+	) &&
+	! test -f template-blank/.git/info/exclude
+'
+
 test_done

-- 
1.6.0.rc1.155.gd3310